Why does this SQL SELECT not run when it uses valid data?

Question

I'm practising with C# and SQL and I'm trying to make a simple email + password login that checks the database to match the input.

Why does this code return the expected 0 when the input is wrong, but says the input string is wrong when correct data is used?

System.Data.SqlClient.SqlCommand cmd = new System.Data.SqlClient.SqlCommand();
cmd.CommandType = System.Data.CommandType.Text;

cmd.CommandText = 
  "SELECT UserPassword, UserMail FROM Users WHERE UserPassword = '" + 
   textBox2.Text + 
  "' AND UserMail = '" + 
    textBox1.Text + '\''; 

cmd.Connection = sqlConnection1;

sqlConnection1.Open();
int correct = 0;
correct = Convert.ToInt32(cmd.ExecuteScalar());
sqlConnection1.Close();

if(correct <= 0)
{
    MessageBox.Show("Wrong input. Correct = " + 
                     Convert.ToString(correct) + 
                    "\n" + 
                     cmd.CommandText);
}

textbox1 and textbox2 are email and password inputs, respectively.

I expected the output of "email5" + "email5" to be 5, since it was the fifth row (same with other valid data), but I got the following exception:

An unhandled exception of type 'System.FormatException' occurred in mscorlib.dll

Additional information: Input string was not in a correct format.

Use parameters! Don't munge query strings with user input. And -- even more important -- don't pass unencrypted passwords around. These should be encrypted on the application side. — Gordon Linoff, Mar 26 '19 at 13:53
https://stackoverflow.com/questions/7505808/why-do-we-always-prefer-using-parameters-in-sql-statements — Morten Bork, Mar 26 '19 at 13:54
That has never been the correct way to construct SQL in NET. And it is wrong to store passwords as plaintext. Dump whatever tutorial source you are using and find another — Ňɏssa Pøngjǣrdenlarp, Mar 26 '19 at 13:56
You have more than one problem. 1- SQL Injection, the query should use parameters to avoid improper behavior and security bugs. 2- ExecuteScalar will give you an error if you try de retrieve two fields. 3- Are you trying to convert to an integer the password? Your passwords only can contain numbers? Does not seem right. — Cleptus, Mar 26 '19 at 13:57
what are trying to achive with `'" + textBox1.Text + '\'';` ? — styx, Mar 26 '19 at 13:57
Beware of injection....!, then you are selecting two columns using `SELECT UserPassword, UserMail` and you are expecting the output of "email5" + "email5" to be 5. what is the logic then? — sujith karivelil, Mar 26 '19 at 13:57
c# will not convert "email5" to 5 cos you say make it an int. — BugFinder, Mar 26 '19 at 13:59
btw https://learn.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlcommand.executescalar?view=netframework-4.7.2 states that the return of ExecuteScalar is just the first columnof the first row so in this case would only return the password — BugFinder, Mar 26 '19 at 14:03
I may have been unclear in the post, I will edit it a bit. Thanks for the suggestions in regards to the password, but I am only just starting with SQL and trying to solve basic problems, security isn't my priority now. — Zane Fray, Mar 26 '19 at 14:05

Dmitry Bychenko · Accepted Answer · 2019-03-26T14:31:41.013

2

As far as I can see you want to check if there's at least one record in Users table with given UserPassword and UserMail fields' values; if it is, the password and email user has provided via textBox2.Text and textBox1.Text are correct. If it's your case

    bool correct = false;

    //TODO: better create a connection here and not resuse existing one
    sqlConnection1.Open();

    try { 
      //DONE: wrap IDisposable into using in order to release unmanaged resources
      using (var cmd = new System.Data.SqlClient.SqlCommand()) {
        cmd.Connection = sqlConnection1; 

        //DONE: Keep sql be readable
        //DONE: Make sql be parametrized 
        //TODO: Do not store password as a plain text, but its hash
        cmd.CommandText = 
          @"SELECT 1      -- 1 we don't want to return password/eMail back
              FROM Users 
             WHERE UserPassword = @prm_Password 
               AND Upper(UserMail) = Upper(@prm_Email)"; // me@mymail.com == Me@MyMail.com

        // Simplest; more accurate choice .Add("@prm_Password", textBox2.Text, RDBMSType)
        cmd.Parameters.AddWithValue("@prm_Password", textBox2.Text);
        cmd.Parameters.AddWithValue("@prm_Email", textBox1.Text);

        using (var reader = cmd.ExecuteReader()) {
          // correct if we can read at least one record
          correct = reader.Read();
        }         
      }
    }
    finally {
      sqlConnection1.Close();
    }

    if (!correct) {
      MessageBox.Show("Wrong input... "); 
    }

edited Mar 26 '19 at 14:31

answered Mar 26 '19 at 14:01

Dmitry Bychenko

180,369
20
160
215

Depending on the database collation the call to the `Upper` would not be necesary – Cleptus Mar 26 '19 at 14:03
according to his original query, he wants to return email and password, if think it fair to guess otherwise – styx Mar 26 '19 at 14:07
@styx: I doubt if we should return `eMail` and `password` back since they are `textBox2.Text` and `textBox1.Text` – Dmitry Bychenko Mar 26 '19 at 14:08
When I try to run this, I get the following error: Connection property has not been initialized. – Zane Fray Mar 26 '19 at 14:16
@Zane Fray: I'm sorry for the typo; I've skipped `cmd.Connection = sqlConnection1; ` line – Dmitry Bychenko Mar 26 '19 at 14:17
Sorry and feel free to point out if it's frowned upon to say that, I'm new to the site, but that worked! Thanks! – Zane Fray Mar 26 '19 at 14:22

Daniel Brughera · Answer 2 · 2019-03-26T14:14:22.247

0

The error you are getting

"An unhandled exception of type 'System.FormatException' occurred in mscorlib.dll

Additional information: Input string was not in a correct format."

Is because you are selecting an string value

cmd.CommandText = "SELECT **UserPassword**, UserMail FROM Users WHERE ....

And then you are trying to convert it to Int

correct = Convert.**ToInt32**(cmd.ExecuteScalar());

ExecuteScalar Executes the query, and returns the first column of the first row in the result set returned by the query. Additional columns or rows are ignored.

So, you are trying to convert the password value to Int32, that's throwing you the error...

If you use

cmd.CommandText = "SELECT Count(*) FROM Users WHERE ....

The rest of your code will work as that condition is just checking that correct is greater than 0

sqlConnection1.Open();
int correct = 0;
correct = Convert.ToInt32(cmd.ExecuteScalar());
sqlConnection1.Close();

if(correct <= 0)

However you must be aware of the risk of SQL Injection and other bad practices, but is out of topic

edited Mar 26 '19 at 14:14

answered Mar 26 '19 at 13:57

Daniel Brughera

1,641
1
7
14

I don't think this will answers the question, do you? – sujith karivelil Mar 26 '19 at 13:58
3

A ExecuteScalar can not retrieve two values! – Cleptus Mar 26 '19 at 13:58
2

how this answer is relevant? – styx Mar 26 '19 at 13:59
@ZaneFray Ignore this answer, it is wrong in different ways – Cleptus Mar 26 '19 at 14:01
It does indeed remove the error! How can I use the return value of cmd.ExecuteScalar(), though? – Zane Fray Mar 26 '19 at 14:02
I am trying to explain why his code is wrong, my only suggestion is "change your select or change the type" – Daniel Brughera Mar 26 '19 at 14:03
@ZaneFray: what value are you expecting back from ExecuteScalar? – PaulF Mar 26 '19 at 14:03
The first row with the selected data, but it's just the fastest way I've thought of to check if the data is there at all. – Zane Fray Mar 26 '19 at 14:10
If you want the first row - then ExecuteScalar is the wrong method - this is used to return a single value - in your case it would be the first matching password (where UserMail is also a match) or null if no match. If you want the full row, then [ExecuteReader](https://learn.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlcommand.executereader?view=netframework-4.7.2) may be what you should be using. – PaulF Mar 26 '19 at 14:13
How can I use the return value of ExecuteReader? – Zane Fray Mar 26 '19 at 14:19
See Dmitry's answer - you assign the result to an instance of SqlReader, try to read the record - if successful the fields can be accessed similar to an array - see here : https://learn.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqldatareader?view=netframework-4.7.2 – PaulF Mar 26 '19 at 14:22
@bradbury9 I never said that, both fragments of code of my original post was copied directly from the answer and I was trying to explain why they were wrong – Daniel Brughera Mar 26 '19 at 14:22
Perhaps when you posted you should have answered the question. Your latest edit does address the problem tho. – Cleptus Mar 26 '19 at 14:38
@sujithkarivelil does it now?? – Daniel Brughera Mar 26 '19 at 14:41

Why does this SQL SELECT not run when it uses valid data?

2 Answers2