0
\$\begingroup\$

I am writing an update statement. I think my SQL statement is opening me up for SQL injections. Is there a better to do this?

public void UpdateQuote(string quote, string quote_id)
{
    using (IDbConnection dbConnection = Connection) {
    string query = "Update quotes set quotes = '"+quote+"', updated_at = NOW() where id_quotes = '"+quote_id+"'";
    dbConnection.Open();
    dbConnection.Execute(query, quote);
}
\$\endgroup\$
3
  • 2
    \$\begingroup\$ yes it does, google "c# sql prepared statements" \$\endgroup\$
    – JohnnyAW
    Jan 5, 2017 at 16:57
  • \$\begingroup\$ You are missing a closing } \$\endgroup\$
    – Heslacher
    Jan 5, 2017 at 17:07
  • \$\begingroup\$ Where is the Execute(string, string) coming from ? \$\endgroup\$
    – Heslacher
    Jan 5, 2017 at 17:10

2 Answers 2

9
\$\begingroup\$

You can parametize a command and execute it.

using (IDbConnection dbConnection = Connection)
{
    using (DbCommand cmd = dbConnection.CreateCommand())
    {
        cmd.CommandType = CommandType.Text;
        cmd.CommandText = "UPDATE quotes "
                        + "SET quotes = @Quote, "
                        + "updated_at = NOW() "
                        + "WHERE id_quotes = @QuoteID";

        IDbDataParameter quoteParam = cmd.CreateParameter();
        quoteParam.ParameterName = "@Quote";
        quoteParam.Value = quote;
        cmd.Parameters.Add(quoteParam);
        IDbDataParameter quoteIdParam = cmd.CreateParameter();
        quoteIdParam.ParameterName = "@QuoteID";
        quoteIdParam.Value = quote_id;
        cmd.Parameters.Add(quoteIdParam);
        cmd.ExecuteNonQuery();
    }
}

This isn't the most elegant solution, but should serve your needs.

I wasn't sure about your Connection variable, but it should be open at time of execution.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Without knowing the underlaying database system the provided parameter identifier @ may be wrong \$\endgroup\$
    – Heslacher
    Jan 5, 2017 at 17:55
-1
\$\begingroup\$

The { } don't match up. Let's start with valid syntax:

public void UpdateQuote(string quote, string quote_id)
{
    using (IDbConnection dbConnection = Connection) 
    {
        string query = "Update quotes set quotes = '" + quote + "', updated_at = NOW() where id_quotes = '" + quote_id + "'";
        dbConnection.Open();
        dbConnection.Execute(query, quote);
    }
}

Sharing a Connection like this is not a good practice. If it is used by other threads, things go bad.

dbConnection is just a reference. I am not sure if Connection is Disposed.

Yes, you are exposed to SQL injection.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.