SQL Server and SqlDataReader

Question

I am trying to create a login page together with a SQL Server database but when I am trying to use the SqlDataReader, I get an error

System.Data.SqlClient.SqlException: 'Incorrect syntax near ','

I've attached my code. Thanks a lot in advance

namespace LoginAndRegistration
{
    public partial class frmLogIn : Form
    {
        public frmLogIn()
        {
            InitializeComponent();
        }

        private void button1_Click(object sender, EventArgs e)
        {
            SqlConnection con = new SqlConnection("Data Source=CONSOLE-03;Initial Catalog=db_users;Integrated Security=True");

            SqlDataAdapter adapter = new SqlDataAdapter();
            con.Open();

            SqlCommand cmd = new SqlCommand("SELECT * FROM tbl_users WHERE Username =('" + txtUsername.Text + "','" + txtPassword.Text + "')",con) ;

            SqlDataReader dr = cmd.ExecuteReader();

            if (dr.Read() == true)
            {
                new Dashboard().Show();
                this.Hide();
            }
            else
            {
                MessageBox.Show("Invalid Username or Password, Please try again", "Login Failed", MessageBoxButtons.OK, MessageBoxIcon.Error);
                txtUsername.Text = "";
                txtPassword.Text = "";
                txtUsername.Focus();
            }
            
            con.Close();
        }

        private void button2_Click(object sender, EventArgs e)
        {
            txtUsername.Text = "";
            txtPassword.Text = "";
            txtUsername.Focus();
        }

        private void checkbxShowPas_CheckedChanged(object sender, EventArgs e)
        {
            if (checkbxShowPas.Checked)
            {
                txtPassword.PasswordChar = '\0';
            }
            else
            {
                txtPassword.PasswordChar = '•';
            }
        }

        private void label6_Click(object sender, EventArgs e)
        {
            new frmRegistration().Show();
            this.Hide();
        }
    }
}

`('" + txtUsername.Text + "','" + txtPassword.Text + "')"` - looks like you want to [CONCAT](https://learn.microsoft.com/en-us/sql/t-sql/functions/concat-transact-sql?view=sql-server-ver16) this really, or attempted to just without using the function — Can O' Spam, Sep 08 '22 at 09:01
Though truthfully, you're making a concat of username and password which I assume (and hope) are separate fields in the DB and need to be checked individually — Can O' Spam, Sep 08 '22 at 09:06
And to be a pain, you are using raw user input in your DB query, this is bad, like dangerously bad. Please read about [SQL Injection](https://owasp.org/www-community/attacks/SQL_Injection) — Can O' Spam, Sep 08 '22 at 09:11
Yes I created a separate fields in the DB. I am trying only to check in the Login page if the data are present by using SqldataReader. I am stock on the error. I try different method but I am still getting this error: System.Data.SqlClient.SqlException: 'Incorrect syntax near ','.' — Jeffry Vergara, Sep 08 '22 at 10:18
You are looking at `WHERE Username = {username + password}` which just feels wrong. If you have separate fields, you want a `WHERE Username = {username} AND Password = {password}`. Be very aware of the attack vector that anyone with basic SQL knowledge can do `username = '1' and password = '0' OR 'a' = 'a'; -- ` to make it always equate to true and log in without knowing details (typing password as `0' OR 'a'='a'; -- ` in the input) — Can O' Spam, Sep 08 '22 at 10:28
**Always use parameterized sql and avoid string concatenation** to add values to sql statements. This mitigates SQL Injection vulnerabilities and ensures values are passed to the statement correctly. See [How can I add user-supplied input to an SQL statement?](https://stackoverflow.com/q/35163361/1260204), [Exploits of a Mom](https://xkcd.com/327/), and [Why do we always prefer using parameters in SQL statements?](https://stackoverflow.com/q/7505808/1260204). — Igor, Sep 08 '22 at 12:16
Never store passwords as plain text. Use a cryptographic secure hashing algorithm instead like: [pbkdf2](https://en.wikipedia.org/wiki/PBKDF2), [scrypt](https://en.wikipedia.org/wiki/Scrypt), [bcrypt](https://en.wikipedia.org/wiki/Bcrypt) and use a unique salt for each password. You store the salt and hashed password value and when the user logs in you create a hash of the input using the same hashing algorithm and the salt from the user's account. If the 2 hashes match the password is the same. — Igor, Sep 08 '22 at 12:18

score 3 · Answer 1 · edited Sep 08 '22 at 12:22

There are a lot of issues with your code:

Concatting for no reason, it seems you just want to check the columns separately.
SQL injection. You should use parameters instead.
Returning the whole row instead of just returning a 1 to confirm existence.
You don't need a SqlDataAdapter, and if you only have one row you also don't need SqlDataReader as you can use cmd.ExecuteScalar().
Plain-text passwords: always salt-and-hash your passwords.
Store your connection string in a settings file, not hard-coded.
Missing using to dispose all the objects

private void button1_Click(object sender, EventArgs e)
{
    bool isCorrect;
    try
    {
        isCorrect = LoginUser(txtUsername.Text, txtPassword.Text);
    }
    catch(Exception ex)
    {
        MessageBox.Show(ex.Message, "Login Failed", MessageBoxButtons.OK, MessageBoxIcon.Error);
    }

    if (isCorrect)
    {
        new Dashboard().Show();
        this.Hide();
    }
    else
    {
        MessageBox.Show("Invalid Username or Password, Please try again", "Login Failed", MessageBoxButtons.OK, MessageBoxIcon.Error);
        txtUsername.Text = "";
        txtPassword.Text = "";
        txtUsername.Focus();
    }
}

private bool LoginUser(string username, string password)
{
    const string query = @"
SELECT 1
FROM tbl_users
WHERE Username = @username
  AND PasswordHash = @passwordHash;
";
    using (SqlConnection con = new SqlConnection(YourConnString))
    using (SqlCommand cmd = new SqlCommand(query))
    {
        cmd.Parameters.Add("@username", SqlDbType.NVarChar, 100).Value = username;
        cmd.Parameters.Add("@passwordHash", SqlDbType.VarBinary, 128).Value = SaltAndHash(password, username);
        con.Open();
        return cmd.ExecuteScalar() == 1;
    }
}

You should also consider using async to avoid blocking the UI

private async void button1_Click(object sender, EventArgs e)
{
    bool isCorrect;
    try
    {
        isCorrect = await LoginUser(txtUsername.Text, txtPassword.Text);
    }
    catch(Exception ex)
    {
        MessageBox.Show(ex.Message, "Login Failed", MessageBoxButtons.OK, MessageBoxIcon.Error);
    }

    if (isCorrect)
    {
        new Dashboard().Show();
        this.Hide();
    }
    else
    {
        MessageBox.Show("Invalid Username or Password, Please try again", "Login Failed", MessageBoxButtons.OK, MessageBoxIcon.Error);
        txtUsername.Text = "";
        txtPassword.Text = "";
        txtUsername.Focus();
    }
}

private async Task<bool> LoginUser(string username, string password)
{
    const string query = @"
SELECT 1
FROM tbl_users
WHERE Username = @username
  AND PasswordHash = @passwordHash;
";
    using (SqlConnection con = new SqlConnection(YourConnString))
    using (SqlCommand cmd = new SqlCommand(query))
    {
        cmd.Parameters.Add("@username", SqlDbType.NVarChar, 100).Value = username;
        cmd.Parameters.Add("@passwordHash", SqlDbType.VarBinary, 128).Value = SaltAndHash(password, username);
        await con.OpenAsync();
        return (await cmd.ExecuteScalar()) == 1;
    }
}

SQL Server and SqlDataReader

1 Answers1