Friday, August 10, 2007

Please tell me the rest of the code doesn't look like this...

This is not right:

internal static bool DoesDbExist(SqlConnection conn, string database)
{
    using (SqlCommand cmd = conn.CreateCommand())
    {
        // prefer this to a where clause as this is not prone to injection attacks
        cmd.CommandText = "SELECT name FROM sys.databases";
        cmd.CommandType = CommandType.Text;

        using (SqlDataReader reader = cmd.ExecuteReader())
        {
            while (reader.Read())
            {
                string dbName = reader.GetString(0);
                if (string.Compare(dbName, database, true, CultureInfo.CurrentCulture) == 0)
                {
                    // the database already exists - return
                    return true;
                }
            }
        }
    }

    return false;
}

This is right:

internal static bool DoesDbExist(SqlConnection conn, string database)
{
    using (SqlCommand cmd = conn.CreateCommand())
    {
        cmd.CommandText = "SELECT name FROM sys.databases WHERE name=@name";
        cmd.CommandType = CommandType.Text;
        cmd.Parameters.Add(new SqlParameter("@name", database));

        using (SqlDataReader reader = cmd.ExecuteReader())
        {
            return reader.Read();
        }
    }
}

Someone please assure me that this is not how everyone else handles avoiding SQL injection.

2 comments:

Brian Dukes said...

That's pretty awesome. (Aren't SqlParameters great?)

The Software Purist (http://www.softwarepurist.com) said...

Hahaha. I was reading your blog. This is great. I've seen things like this done before and it always makes me want to cry. :P In the particular case I'm thinking of, it was in C++, but the concept is the same. In my own experience, I typically use stored procedures, which mitigates a lot of the risk you were mentioning.