Properly securing SQL statement with parameters

Question

I have two SQL statements in my C# code to retrieve some values. I know they are open to SQL injection since I'm not using parameters, but I'm not sure if I'm implementing them correctly.

(Note: each of these are in loops that are looping through rows of a data table) First example:

string sql2 = "select max(day) as day from users u join days d on d.User_ID = u.id where u.ActiveUser = 1 and u.id = " + Users["ID"].ToString();
command.CommandText = sql2;               
string dt = command.ExecuteScalar().ToString();

In the above statement, it's retrieving a datetime and assigning it to string dt. Anything with id or ID in it is a bigint.

string sql = "SELECT MAX(Day) FROM Days WHERE Project_ID IN (SELECT ID FROM Projects WHERE Parent_ID = -1 AND ID = " + row["ID"] +  ") HAVING MAX(Day) < DATEADD(dd, -730, getdate())";
command.CommandText = sql;                                  
object val = command.ExecuteScalar();

The above statement is the same as the first statement, as in it's retrieving a datetime value. Anything with id or ID is a bigint.

Here's what I came up with for the first one, am I missing anything or doing something wrong?

string sql2 = "select max(day) as day from users u join days d on d.User_ID = u.id where u.ActiveUser = 1 and u.id = @userID";
using (conn)
{
     using (SqlCommand cmd = new SqlCommand(sql2, conn))
     {
          command.Parameters.AddWithValue("@userID", drUsers["ID"]);
          conn.Open();
          dt = (DateTime)command.ExecuteScalar();
     }
}

Note: I asked a question last week on DateTime conversions and there was an issue that couldn't be resolved, so I might have to just use a string version of the datetime that is returned. Will that affect anything?

Looks OK. [Related](http://stackoverflow.com/questions/265192/get-the-generated-sql-statement-from-a-sqlcommand-object) (look at all answers) [See this explanation](http://www.csharp-station.com/Tutorial/AdoDotNet/Lesson06) — JNF, Dec 15 '15 at 13:18

Cetin Basoz · Accepted Answer · 2015-12-15T13:46:39.097

It would be like:

string sql2 = @"select max(day) as day from users u 
join days d on d.User_ID = u.id 
where u.ActiveUser = 1 and u.id = @id";

string sql = @"SELECT MAX(Day) FROM Days 
WHERE Project_ID IN 
(SELECT ID FROM Projects WHERE Parent_ID = -1 AND ID = @id)
HAVING MAX(Day) < DATEADD(dd, -730, getdate())";

DateTime dt;
using (conn)
{
  using (SqlCommand cmd = new SqlCommand(sql2, conn))
  {
    cmd.Parameters.AddWithValue("@id", (Int64)Users["ID"]);
    conn.Open();
    dt = (DateTime)cmd.ExecuteScalar();
  }
}

using (conn)
{
  using (SqlCommand cmd = new SqlCommand(sql, conn))
  {
    cmd.Parameters.AddWithValue("@id", (Int64)row["ID"]);
    conn.Open();
    dt = (DateTime)cmd.ExecuteScalar();
  }
}

However, for performance reasons, I wouldn't do this once per row in the DataTable. You can simply get all the max(day) values grouped by ID once to client side. ie:

string sql = @"select u.Id, max(day) as day 
  from users u 
  join days d on d.User_ID = u.id 
  where u.ActiveUser = 1
  group by u.id";

EDIT: I saw your edit later, and the added question about datetime. Do not convert a datetime value to a string. It is where the errors begin. If you "HAVE TO" then use only ODBC canonical format which is not affected from SQL server settings - yyyyMMdd. Other types of datetime strings would only work by chance if they ever do.

Only doing it per row because a bunch of other sql stuff (from previous coder) is there. If I did it your way though, would I just make a list of DateTimes and have it equal to w/e is returned, and loop through them then? — pfinferno, Dec 15 '15 at 13:30
You would do a single SQL call returning a list of ID, Max(Day). Then you could say convert it to say a Dictionary using simple Linq and use in your loop instead of doing multiple SQLs in the loop. In comments, I don't know how to format code (if I can) so I will post a code example as a new answer. — Cetin Basoz, Dec 15 '15 at 13:33
Alright thanks. I know it's not good practice to convert datetime to a string, but something strange was going on when I was trying to use format the datetime. I asked two questions on here regarding it the past two weeks, and everything that should work just doesn't. — pfinferno, Dec 15 '15 at 13:37

score 0 · Answer 2 · answered Dec 15 '15 at 13:45

string sql = @"select u.Id, max(day) as day 
  from users u 
  join days d on d.User_ID = u.id 
  where u.ActiveUser = 1
  group by u.id";

DataTable tbl = new DataTable();
using (conn)
{
  using (SqlCommand cmd = new SqlCommand(sql, conn))
  {
    conn.Open();
    tbl.Load( cmd.ExecuteReader() );
    conn.Close();
  }
}

var dates = tbl.AsEnumerable()
               .ToDictionary(t => t.Field<Int64>("ID"), 
                             t => t.Field<DateTime?>("Day"));

Then in your loop you could say do this:

var myId = 4;
var myDate = dates[myId];

Properly securing SQL statement with parameters

2 Answers2