0

I have an OleDbCommand for my inserts that I have tried to implement to avoid SQL injection. Before that I used simple strings for my queries and I didn't like that. Now my piece of code for inserting records looks like this:

 try
 {
    OleDbConnection rConn = new OleDbConnection(args[3]);
    rConn.Open();
    using (OleDbCommand insert = new OleDbCommand(String.Format(Globals.QUERY_INSERT_CLICK, args[4]), rConn))
    {
        insert.Parameters.Add("id", OleDbType.BigInt, 20);
        insert.Parameters.Add("email", OleDbType.VarChar, 255);
        insert.Parameters.Add("clickTime", OleDbType.Date, 20);
        insert.Parameters.Add("subscriberId", OleDbType.BigInt, 20);
        insert.Parameters.Add("link", OleDbType.VarChar, 255);
        insert.Parameters.Add("sendQueueId", OleDbType.BigInt, 20);
        insert.Parameters.Add("mailingListName", OleDbType.VarChar, 255);
        insert.Parameters.Add("newsletterId", OleDbType.BigInt, 20);
        insert.Parameters.Add("sendDate", OleDbType.Date, 20);

        insert.Parameters[0].Value = clickitem.Id;
        insert.Parameters[1].Value = clickitem.Email;
        insert.Parameters[2].Value = clickitem.ClickTime;
        insert.Parameters[3].Value = clickitem.SubscriberId;
        insert.Parameters[4].Value = clickitem.Link;
        insert.Parameters[5].Value = clickitem.SendQueueId;
        insert.Parameters[6].Value = mailingListName;
        insert.Parameters[7].Value = newsletterID;
        insert.Parameters[8].Value = sendDate;

        insert.Prepare();
        insert.ExecuteNonQuery();
    }
    rConn.Close();
}
catch (OleDbException oldbex)
{
    logger.WriteToLog("GETCLICKS", "OleDbException: " + Globals.ERROR_INSERT_CLICK + oldbex.Message);
}
catch (Exception ex)
{
    logger.WriteToLog("GETCLICKS", Globals.ERROR_INSERT_CLICK + ex.Message);
}

I have thousands of inserts and I see from my log that some of them are not correctly inserted. The exception tells me e.g. cannot convert from bigint to datetime and stuff like that. Although most of my records are inserted correctly, I want to know which of these insert queries exactly caused the error. How can I figure that out?

N.B. Before using this method I had access to my query string and I found the error instantly. Now I guess my immunity to SQL injection is causing some confusion for myself

disasterkid
  • 6,948
  • 25
  • 94
  • 179
  • In your `catch` just log out the parameters the same way you would have logged out your SQL string previously. – RB. Apr 08 '13 at 15:10
  • NB if `args[4]` can be influenced by your user, this code is still vulnerable to an SQL injection. – tomfanning Apr 08 '13 at 15:20
  • You may also find it easier to work with `OleDbParameterCollection.AddWithValue()` which does not require you to specify the parameter type or length. http://msdn.microsoft.com/en-us/library/system.data.oledb.oledbparametercollection.addwithvalue(v=vs.100).aspx – tomfanning Apr 08 '13 at 15:21

4 Answers4

1

Since you mentioned receiving different / multiple data conversion errors, my suggestion would be to improve your logging when catching the OleDbException.

You could write each of the parameters values to the log right after the initial 'GETCLICKS' log entry. This would give you a better idea as to what value coming from the user is in the incorrect format.

Brian Dishaw
  • 5,767
  • 34
  • 49
1

Standard SQL erroring won't show you the column or value that caused the error.

Simplest way is to add the SQL statement and parameter values to your logging call.

string params = string.Join(Environment.NewLine,
                            insert.Parameters
                                  .Select(p => string.Format("{0} : {1}",
                                                             p.Name, 
                                                             p.Value))
                                  .ToArray()
                            );

string message = string.Format("{0}: {1}{2}\n{3}\n{4}",
                               "OleDbException: " ,
                               Globals.ERROR_INSERT_CLICK,
                               oldbex.Message,
                               insert.CommandText,
                               params);

logger.WriteToLog("GETCLICKS", message );
D Stanley
  • 149,601
  • 11
  • 178
  • 240
0

The parameter Value property is of Object generic type. So it accepts anything you assign to it. Of course this is not a good way to handle your data. I will try to convert the value to the appropriate datatype for the parameter and avoid to send bogus values to the database. In this way you will catch the error immediately at the parameter assignement instead of when executing the insert

For example:

insert.Parameters[2].Value = Convert.ToDateTime(clickitem.ClickTime);

if this is not a valid datetime it will fail at the Convert.ToDateTime and you will notice that in your log

Steve
  • 213,761
  • 22
  • 232
  • 286
-1

difficult way , but good design . I would recommend that you create your own custom exception class by inheriting the Base Exception class.

Create a constructor which takes oledbcomndand as input parameter and there you can try to log the OldedbComamnd.CommandText by looping over the parameter collection. as shown in sample below for SQLcommand ( which is more or less same as OLedbCommand)

or Easy Way - when exception is thrown , write the OLDEBCommand.ComamndText in log.

Below is a sample i created for StoredProcExecutionException for SQL command. you can exactly replicate this for OleDbCommand. hope this helps

 public StoredProcExecutionException(string message, Exception innerException ,SqlCommand sqlCommand)
            : base(Convert.ToString(sqlCommand.CommandType ,CultureInfo.InvariantCulture)+" : "
                + Convert.ToString(sqlCommand.CommandText, CultureInfo.InvariantCulture)
                + "Failed. " + Convert.ToString(message, CultureInfo.InvariantCulture), innerException)
        {
            StringBuilder sb = new StringBuilder();

            foreach (SqlParameter param in sqlCommand.Parameters)
            {
                if (sb.Length > 0) sb.Append(",");
                sb.AppendFormat("{0}='{1}'", param.ParameterName, Convert.ToString(param.Value, CultureInfo.InvariantCulture));               
            }

            StringBuilder sbHeader = new StringBuilder();
            sbHeader.AppendLine(String.Format(CultureInfo.InvariantCulture,"{0} :{1} Failed. {2}", sqlCommand.CommandType, sqlCommand.CommandText, message));
            sbHeader.AppendFormat("Exec {0} ", sqlCommand.CommandText);

            sbHeader.Append(sb.ToString());

        }
aked
  • 5,625
  • 2
  • 28
  • 33
  • @tomfanning:Alright , sorry corrected the text .so hard way. Raise the custom exception So in that case loop over the parameter collection and log the values. i guess this should work. – aked Apr 08 '13 at 15:26