How not to write SQL queries in C#

Everyday , I see questions on Stack Overflow with string concatenation to form SQL queries. Not only this approach is difficult to debug, it is prone to SQL Injection .

Code like below is usually seen multiple times with respect to executing SQL queries in .Net (C#).

SqlConnection connection = new SqlConnection("ConnectionString");
string sqlQuery = "SELECT COUNT(*) FROM Users where Username = '" + txtUserName.Text + "';";
SqlCommand cmd = new SqlCommand(sqlQuery,connection);

 

The code above would simply break if the input (username) would contain character ('). This would cause invalid SQL to be created as a result of string concatenation and that statement would eventually cause exception during execution.

The other major issue is SQL injection, any attacker could pass in  malicious SQL statements in the input. An example would be (from wiki article):

a';DROP TABLE users; SELECT * FROM userinfo WHERE 't' = 't

With the above input the sql statement would look like as below that could potentially delete the users table :

SELECT COUNT(*) FROM Users where Username = 'a';DROP TABLE users; SELECT * FROM userinfo WHERE 't' = 't';

Solution

Very simple, use parameters.  One could implement methods to sanitize the input, but it is difficult to code against all the possible approaches and characters. Using parameters saves us from explicitly sanitizing inputs.

 string sqlQuery = "SELECT COUNT(*) FROM Users where Username = @username";
 using (SqlConnection connection = new SqlConnection("ConnectionString"))
 using (SqlCommand cmd = new SqlCommand(sqlQuery, connection))
 {
 cmd.Parameters.AddWithValue("@username", txtUserName.Text);
 int count = (int) cmd.ExecuteScalar();
 //......rest of the code
 }

I used AddWithValue  method above while adding parameters. It is useful as long as the data types are correctly matched, but if there is ambiguity in data types on SQL Server end and C# then use Add method and explicitly specify data type like:

cmd.Parameters.Add(new SqlParameter("@username", SqlDbType.VarChar) {Value = txtUserName.Text});

One more thing is it to enclose SqlCommand and SqlConnection objects in using statement. This will ensure proper disposal of resources (closing connection etc.)

Popular Bobby Tables 😉

Image from: https://xkcd.com/327/

 

String comparison mistakes in C#

I have seen lots of questions on Stack Overflow where String.Compare  is used to compare strings if one needs case insensitive comparison. I have even seen production code using the same method.  I guess the usage is due to most of the developers having c/c++ background and used to methods like strcmpi. 

Code like:

string str1 = "TEST";
string str2 = "test";
bool caseInsensitiveEqual = String.Compare(str1, str2, StringComparison.InvariantCultureIgnoreCase) == 0;

is often found and considered a usual practice when it comes to case insensitive string comparison.

Another way is converting both of the parameters to either upper or lower case and then doing the comparison like:

string str1 = "TEST";
string str2 = "test";
bool caseInsensitiveEqual = str1.ToUpper() == str2.ToUpper();

Both of these approaches would work for most of the time, but there are issues:

• String.Compare  is used for determining position in sort order (which string is bigger than other)
• Using ToUpper or ToLower could case incorrect results for strings in different cultures. (The Turkish İ Problem and Why You Should Care)

Solution:

Use the framework provided String.Equals group of methods.

string str1 = "TEST";
string str2 = "test";
bool caseInsensitiveEqual = String.Equals(str1, str2, StringComparison.InvariantCultureIgnoreCase);

It has overloads to specify string comparison approaches using StringComparison Enumeration