4 ms·
Here is the code in question: protected void Button1_Click(object sender, EventArgs e) { string connectionString = @"Data Source=OFFICE7-PC\SQLEX
by artmageddon 15y ago
Here is the code in question:
protected void Button1_Click(object sender, EventArgs e)
{
string connectionString = @"Data Source=OFFICE7-PC\SQLEXPRESS;Integrated Security=True";
string sqlQuery = "Select UserName From [Users].[dbo].[UsersInfo] Where UserName = ' " + TextBox1.Text + "' and Password = ' " + TextBox2.Text+"'";
using (SqlConnection connection = new SqlConnection(connectionString))
{
SqlCommand command = new SqlCommand(sqlQuery, connection);
connection.Open();
SqlDataReader reader = command.ExecuteReader();
try
{
while (reader.Read())
{
}
}
finally
{
if (reader.HasRows)
{
reader.Close();
Response.Redirect(string.Format("WebForm2.aspx?UserName={0}&Password={1}", TextBox1.Text, TextBox2.Text));
}
else
{
reader.Close();
Label1.Text = "Wrong user or password";
}
}
}
}
In my opinion, this is what needs to be changed:
-"Button1" should be given a more descriptive name. We can easily see from the query that it's getting a list of users, but would it really kill to take the time to give it a better name like "btnGetUsers?"
-The query string should REALLY be put into the web.config file, or heaven help us all at least as a private readonly at the top of the class containing this method.
-Is this query trying to get the username based on what username and password is entered? It looks like the coder is trying to do authentication based on the number of rows returned.. or at least tried to.
-The query is also VERY badly written; it's entirely prone to SQL injection.
-He gets one point for at least making use of the "using" construct to make sure the SqlConnection gets disposed of once done!
-That "try" block with the while loop is a big WTF for me. Why is it waiting? It will always time out no matter what.
-What is the point of the redirection to WebForm2? If the user gets the login right, then hooray, they're on the site, but otherwise the password is wrong. Still, what's to stop anyone from just bypassing this whole thing by just messing with the URL parameters?
- dlytle 15y agoThanks for the detailed breakout. I hope he posts the original request; it'd be interesting to see what specs prompted this response. Slightly offtopic, does anyone happen to know of a site where people can post code for feedback? Something like StackOverflow but specifically along the lines of "please help me with my sucky code"?
- zwp 15y agohttp://codereview.stackexchange.com/ http://codereview.stackexchange.com/ http://refactormycode.com/ http://refactormycode.com/
- deleted 15y ago[deleted]
- artmageddon 15y agoI tried to edit my post but HN only lets you do it so much. I may have to take back the one point given because the redirect happens in the using block. I'm not sure if the using block does its job at that point but it shouldn't be coded this way.
- breckinloggins 15y agoSome more: - I wouldn't have any logic in the button click handler at all. Punt to a Login() procedure with parameters so it can be reused. I'm just generally against having any logic in event handlers except in very simple instances - Query string needs to be parameterized instead of built ad-hoc, but that may have been what you meant - A catch would be nice. How will the maintainers ever know if there's a real problem? - Lovely URL consisting of username and password in plain-text (although maybe that was a requirement). Also, if this were a larger site and it were still required to use WebForms, I'd come up with my own simple "router" logic so as not to hard code the URL schema everywhere in the code Granted, some of these points may be a bit much for a programming exercise, but if I were applying for this job I would do these things with comments to demonstrate that I knew how to write maintainable code. The REAL sad part? NOTHING about this surprises me.
- artmageddon 15y agoYeah, I touched upon the parameterized query with the SQL injection part but should've mentioned that more specifically. Good point about the catch... I missed it because I was shocked at everything else ;)
- tensafefrogs 15y agoNEVER EVER send a password as clear text. EVER. If that was a requirement, then the submitter should have said something.
- artmageddon 15y agoJust curious, how would you go about not sending out the password in cleartext? I understand WHY but I've never really figured out how to avoid doing so* *yeah, I realize I'm asking for trouble with this :)
- ceejayoz 15y agoPOST it to an HTTPS endpoint.