CODEWITHSUNDEEP
c#sqlsql-server
pfinferno
pfinferno

Reputation: 1945

Properly securing SQL statement with parameters

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?

Upvotes: 7

Views: 348

Answers (2)

Cetin Basoz
Cetin Basoz

Reputation: 23797

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.

Upvotes: 3

Cetin Basoz
Cetin Basoz

Reputation: 23797

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];

Upvotes: 0

Related Questions