ASP.NET, C# How to Pass a StringQuery to a custom SQL Command

I was so sure this would be a duplicate of some canonical question about parameterized queries in C#, but apparently there isn't one (see this)!

You should parameterize your query - if you don't, you run the risk of a malicious piece of code injecting itself into your query. For example, if your current code could run against the database, it would be trivial to make that code do something like this:

// string id = "1 OR 1=1"
"SELECT * Products WHERE Idx_ProductId = 1 OR 1=1" // will return all product rows
// string id = "NULL; SELECT * FROM UserPasswords" - return contents of another table
// string id = "NULL; DROP TABLE Products" - uh oh
// etc....

ADO.NET provides very simple functionality to parameterize your queries, and your DBOps class most assuredly is not using it (you're passing in a built up command string). Instead you should do something like this:

public DataTable ViewProduct(string id)
{
    try
    {
        string connStr = ConfigurationManager.ConnectionStrings["MyDatabase"].ConnectionString;
        using (SqlConnection conn = new SqlConnection(connStr))
        {
            conn.Open();
            using (SqlCommand cmd = conn.CreateCommand())
            {
                // @id is very important here!
                // this should really be refactored - SELECT * is a bad idea
                // someone might add or remove a column you expect, or change the order of columns at some point
                cmd.CommandText = "SELECT * Products WHERE Idx_ProductId = @id";
                // this will properly escape/prevent malicious versions of id
                // use the correct type - if it's int, SqlDbType.Int, etc.
                cmd.Parameters.Add("@id", SqlDbType.Varchar).Value = id;
                using (SqlDataReader reader = cmd.ExecuteReader())
                {
                    DataTable vpTbl = new DataTable();
                    vpTbl.Load(reader);
                    return vpTbl;
                }
            }
        }
    }
    catch (Exception e)
    {
        // do some meaningful logging, possibly "throw;" exception - don't just return null!
        // callers won't know why null got returned - because there are no rows? because the connection couldn't be made to the database? because of something else?
    }
}

Now, if someone tries to pass "NULL; SELECT * FROM SensitiveData", it will be properly parameterized. ADO.NET/Sql Server will convert this to:

DECLARE @id VARCHAR(100) = 'NULL; SELECT * FROM SensitiveData';
SELECT * FROM PRoducts WHERE Idx_ProductId = @id;

which will return no results (unless you have a Idx_ProductId that actually is that string) instead of returning the results of the second SELECT.

Some additional reading:

• https://security.stackexchange.com/questions/25684/how-can-i-explain-sql-injection-without-technical-jargon
• Difference between Parameters.Add and Parameters.AddWithValue
• SQL injection on INSERT
• Avoiding SQL injection without parameters
• How do I create a parameterized SQL query? Why Should I? (VB.NET)
• How can I prevent SQL injection in PHP? (PHP specific, but many helpful points)
• Is there a canonical question telling people why they should use SQL parameters?