Intro
In my many years of .Net coding, more than I'd like to admit sometimes, I've seen a number of mistakes over and over again by people who just don't know any better. I've got 2 in particular that I see when people hit databases with .Net code, and I figured hey, I can call these out so people know not to do it! So here they are.
Best Practice 1
Always, under all circumstances, close your connection to the database. This might sound like common sense, but it's not always as easy as just calling connection.Close(). What happens if your code throws an exception between Open() and Close()? If you don't have the Close() in a try finally and if you're not creating the connection in a using statement, you're leaving that connection open. What happens if you leave those connections hanging? My experience is with SQL Server, and with it you end up eating up all the available connections eventually, which means your queries fail because you can't open any more connections. In the case of a website it means you have to recycle the app pool to free up all those connections, which can be a bit annoying for your users. Let's see some potentially bad code in action first:
using System;
using System.Data.SqlClient;
public partial class _Default : System.Web.UI.Page
{
private void BadSqlConnection()
{
var conn = new SqlConnection("some conn string here");
conn.Open();
var cmd = new SqlCommand("select * from SomeTable", conn);
cmd.ExecuteNonQuery();
conn.Close();
}
protected void Page_Load(object sender, EventArgs e)
{
BadSqlConnection();
}
}
See the problem? If the SqlCommand cmd throws an exception of any kind, that SqlConnection stays open, eating up one of the connections in our connection pool. Bad! So what can we do to clean it up? My favorite thing is to use a "using" statement. It accomplishes the same thing a try...finally would, but with less code. Here's a sample of some good code:
private void GoodSqlConnection()
{
using (var conn = new SqlConnection("some conn string here"))
{
conn.Open();
var cmd = new SqlCommand("select * from SomeTable", conn);
cmd.ExecuteNonQuery();
}
}
Using the using statement when we create the connection ensures that the connection will be closed and freed before the method ends, regardless of whether or not any exceptions are thrown. It's conceptually the same thing as a try...finally where the Close() call is in the finally. Neato!
Best Practice 2
SQL injection attacks suck! It's a very common way for chodes to attack your data-driven website. Pretend you have a textbox on your website where people can login via userid and password. You've got 2 entries on your page and a button, one for the userid, one for the password, and the button attempts the login. Maybe your login code looks like this:
private void DoLoginBad()
{
using (var conn = new SqlConnection("some conn string here"))
{
conn.Open();
var cmd = new SqlCommand(String.Format("select * from Users where UserId = '{0}' and Password = '{1}'", UserId.Text, Password.Text), conn);
var reader = cmd.ExecuteReader();
if (reader.Read())
{
//stuff that logs the user in
}
}
}
What's bad about the above code? Think about what will happen if this value is put in the UserId textbox: "peeticus'--". Looks pretty innocuous right? Absolutely, completely, horribly wrong! Think about what this value does to the query which is sent to SQL Server; it now becomes "select * from Users where UserId = 'peeticus'--' and Password = '[blah]'
The key thing there is that our sql statement is terminated after checking the UserId because of the ending of the string value via the apostrophe, then the double-dash "--" which makes the rest of the line a comment. So, with this fairly trivial attempt somebody could log into your system as any user for which they know the userid. Believe it or not there are much worse things they can do with this tactic too, but we'll save that for another day.
How can you thwart the evildoers? Conceptually speaking you need to filter out bad characters such as apostrophes and maybe dashes, or filter in good characters if you want to be even safer. A better idea though would be to use the built-in sql parameters within .Net so that a well-tested library can do the work for you! Here's the same query from above, rewritten to be resistant to SQL Injection:
private void DoLoginGood()
{
using (var conn = new SqlConnection("some conn string here"))
{
conn.Open();
var cmd = new SqlCommand("select * from Users where UserId = @UserId and Password = @Password", conn);
cmd.Parameters.Add(new SqlParameter("@UserId", UserId.Text));
cmd.Parameters.Add(new SqlParameter("@Password", Password.Text));
var reader = cmd.ExecuteReader();
if (reader.Read())
{
//stuff
}
}
}
All you have to do is replace your concatenated values in the query with a couple parameters using @ symbols, and voila! A couple extra lines of code for a lot more peace of mind.
