0

I keep getting the following error: (C# WinForms)

"Invalid syntax near ','"

I have the following code:

    // Initialize and instantiate a new reader object.
    SqlDataReader slrr = null;
    // Send command.
    System.Data.SqlClient.SqlCommand command = new System.Data.SqlClient.SqlCommand("SELECT ActivationCode FROM CAccounts WHERE ActivationCode=" +
    _activationcode, connection);

    slrr = command.ExecuteReader();

    // read result(s) of command.
    while (slrr.Read())
    {
        if (slrr["ActivationCode"].ToString() == _activationcode.Text)
        {
            stat.Text = "It appears that these details have already been registered.";
            Properties.Settings.Default.GU = false;
            Properties.Settings.Default.Save();
        }
        else
        {
            System.Data.SqlClient.SqlCommand comm = new System.Data.SqlClient.SqlCommand(
                "INSERT INTO CAccounts (FirstName, LastName, Country, Gender, EmailAddress, ActivationCode, ClientID, IsActivated) VALUES ('" +
                _firstname.Text + "', '" + _lastname.Text + "', '" + _country.Text + "', '" + gender + "', '" +
                _email.Text + "', '" + _activationcode.Text + "', '" + _clientid.Text + "', '" + "yeh'", connection);

            comm.ExecuteNonQuery();

            stat.Text = "Product Activation succeeded.";
            Properties.Settings.Default.GU = true;
            Properties.Settings.Default.FirstName = _firstname.Text;
            Properties.Settings.Default.LastName = _lastname.Text;
            Properties.Settings.Default.Country = _country.Text;
            Properties.Settings.Default.Gender = gender;
            Properties.Settings.Default.DateOfBirth = _dateofbirth.Text;
            Properties.Settings.Default.EmailAddress = _email.Text;
            Properties.Settings.Default.ActivationID = _activationcode.Text;
            Properties.Settings.Default.ClientID = _clientid.Text;
            Properties.Settings.Default.IsActivated = true;
            Properties.Settings.Default.Save();
        }
    }
}
catch (Exception exception)
{
    // Catch the exception and throw an error.
    stat.Text = exception.Message;
}

I have absolutely no idea what I've done wrong. Can somebody please help me?

Improve this question
7
  • 1
    Can we see what the output from comm.ToString() is? However you get the populated version of the query so we can get a better idea where the syntax error(s) are coming from. Commented Aug 20, 2010 at 2:30
  • this works great till one of your users is called ' drop table Users' use SqlParameters Commented Aug 20, 2010 at 2:32
  • @Sam, interesting. I'll do something about that once I can get it to work :P Commented Aug 20, 2010 at 2:33
  • @OMG, Did you mean: slrr["ActivationCode"].ToString(), because there isn't any "comm.ToString();". Commented Aug 20, 2010 at 2:36
  • @Sam, are you referring to "little Bobby tables"? Commented Aug 20, 2010 at 2:36

3 Answers 3

Reset to default
3

Think about the line of code where you construct the INSERT command. What do you think will happen if any of the fields contains an apostrophe?

You guessed it, the statement becomes invalid.

You can solve this by using SqlCommand.Parameters. See the example on that page.

Of course, the same applies to the SELECT command near the top of your code snippet.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

0

Instead of:

"', '" + "yeh'", connection);

It should be:

"', '" + "yeh')", connection);

You forgot to close your VALUES parenthesis, that's why.

Improve this answer

4 Comments

This is still something you'll have to implement, but I agree with the other post where a field you're passing in may have a "'" in it. Check both of these out.
I am typing the fields in manually (textbox's), i am only typing in a few letters in each field. no apostrophes at all, no symbols of any kind.
Hmmm, that's concerning. Can you please provide "comm.CommandText.ToString()" before comm.ExecuteScalar();. Thanks.
Is it by any chance possible that any of those fields could be anything other than varchar or nchar? I was looking at the column names and ClientID to me seems like it might be an integer. Yeah, it may be converted to a String in text, but it doesn't necessarily means it's a string type in the database. Can you give us the table schema?
-1

If IsActivated is some sort of a Boolean value, is 'yeh' valid? I would start by building the SQL command into a string variable and printing it out. There is something going wrong in all of the concatenation. Printing out the SQL string before you pass it into the command object should make the error obvious.

I'm not a C# expert, but isn't there a better way to take a list of strings, surround them in quotes, and join the result using "," as a separator? Something like the following Python snippet makes building SQL strings a lot less error prone:

>>> s = str.join(', ', ("'{0}'".format(x) for x in ['bob', 'alex', 'guido']) )
>>> print s
'bob', 'alex', 'guido'

Then again, you are infinitely better off letting someone else construct the SQL so that you avoid little embarrassments caused by SQL injection.

Improve this answer

2 Comments

Congrats for duplicating the same SQL injection vulnerability into another language :-D
@Timwi - which is precisely why I warn against this approach altogether. If he is hell bent on building the SQL string by hand, then he should at least find a decent way to do it. The OP doesn't seem too concerned about SQL injection unfortunately.

Your Answer

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