Why is concatenating SQL strings a bad idea?

Question

I have read that it's a bad idea to concatenate SQL strings like so:

cmd.CommandText = "Insert INTO workers Values (" + User.Identity.Name + "," + WorkerName.Text + "," + GetUniqueWorkerKey() + ");";

So the recommended way was:

cmd.CommandText = "Insert INTO workers Values (@Username, @WorkerName, @WorkerKey)";
cmd.Parameters.AddWithValue("@Username", User.Identity.Name);
cmd.Paramters.AddWithValue("@WorkerName", TheWorkerNameYouPassedToThisMethod);

I have been avoiding concatenating SQL strings ever since I read about it, but I never really know the rationale behind not doing so. Wouldn't the AddWithValue() method eventually do the same string concatenation behind the scene?

Maybe that method strips off special characters and convert characters to html entities to prevent sql injection, but I can do all these before concatenating my SQL and I get the same effect too, can't I? Or are there other reasons for not practising string concatenation for SQLs?

Answer 1

Short answer: building queries by concatenating strings usually allows SQL injection.

Imagine that someone tries to create a user with the name "Bob, Joe, 12345); DROP TABLE workers; --". You end up building the query "Insert INTO workers Values(Bob, Joe, 12345); DROP TABLE workers; --name, 34345236);" Bye-bye database. SQL injection can also lead to queries returning data that they shouldn't, such as password tables. Any time that you have SQL injection, just assume that you're allowing arbitrary third parties to issue arbitrary commands to your database.

The AddWithValue() approach is called "parametrized queries". It results in very similar commands being generated, but AddWithValue() takes care of any weird stuff like spaces and quotes in the parameter values that could cause your command to mean something other than what you want it to mean. Sure, you could do that escaping manually, but it can be tricky to get correct. It's much easier and safer to let the library handle it for you.

Obligatory XKCD

Note that I don't mean that the library is actually escaping the strings that you give it when you create a parameterized query. It could, but I'm not aware of any SQL libraries that take that approach -- usually, the SQL engine has special support for parameterized queries. This allows parameterized queries to be more efficient than ad-hoc queries: the SQL engine can pre-compile the SQL statement leaving only field values to be filled in at run-time.