SQL run time error in asp.net c#

Question

I keep getting run time SQL query errors in asp.net. I am using c#. The error always starts with Incorrect Syntax near '(some word)'. I have checked and rechecked my code for any syntactic errors but never found any.. In the code below the error is Incorrect Syntax near 'user'. Please help.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.Web.UI;
using System.Web.UI.WebControls;
using System.Data;
using System.Data.SqlClient;

public partial class LogIn : System.Web.UI.Page
{
    SqlConnection con = new SqlConnection();
    SqlCommand cmd = new SqlCommand();

    protected void Page_Load(object sender, EventArgs e)
    {
        con.ConnectionString = @"Data Source=.\SQLEXPRESS;AttachDbFilename=C:\Users        \Sony\Documents\Library\App_Data\Library.mdf;Integrated Security=True;User     Instance=True";
        cmd.Connection=con;
        con.Open();



    }
protected void  txt_user_TextChanged(object sender, EventArgs e)
{

}
protected void  txt_pass_TextChanged(object sender, EventArgs e)
{

}
protected void  btn_log_Click(object sender, EventArgs e)
{
    cmd.CommandText="select count(*) from user where Username='"+txt_user.Text+"' and     Password='"+txt_pass.Text+"'";
        int count =Convert.ToInt16(cmd.ExecuteScalar());
        if (count==1)
        {
            Response.Redirect("Home.aspx");
        }
        else
        {
            Label1.Text="Invalid Username or Password. Please try again..";
        }
}

@SLaks I set the TextMode for the password Textbox as "Password". Isn't that enough? — Dahlia George, Mar 17 '13 at 21:31
@DahliaGeorge: No; that's almost as far from being being "enough" as you can possibly get. — SLaks, Mar 17 '13 at 21:53
Anyone who can get your database (which is very easy to do through SQL injection) will have all of your users' passwords. You must _hash_ (and salt) passwords. — SLaks, Mar 17 '13 at 21:54

score 4 · Accepted Answer · edited May 23 '17 at 12:21

The reason of your error is the word user. It is a reserved keyword for SqlServer.
You need to encapsulate it with square brakets

select count(*) from [user] ....

Said that, now let's address the biggest problem of your code. Sql Injection

cmd.CommandText="select count(*) from [user] where Username=@uname " + 
                "and Password=@upass";
cmd.Parameters.AddWithValue("@uname", txt_user.Text)
cmd.Parameters.AddWithValue("@upass", txt_pass.Text);
int count =Convert.ToInt16(cmd.ExecuteScalar());
......

Using a parametrized query like this, protects your application from malicious input (see the referenced question) that could compromise (or destroy) the information stored in your database. Also you avoid problems with inputs that contains problematic characters like strings with single quotes or numeric decimal separators or date formatting difficulties.

There is another problem as I can see from your code above. Do not store the connection in global variables. There is no performance hit if you open the connection when needed and close afterwards. It is called Connection Pooling and you don't keep a valuable resource locked when you don't use it.

So to sum it all:

protected void  btn_log_Click(object sender, EventArgs e)
{

   using(SqlConnection con = new SqlConnection(@"Data Source=.\SQLEXPRESS;AttachDbFilename=" +
                                 @"C:\Users\Sony\Documents\Library\App_Data\Library.mdf;" + 
                                 @"Integrated Security=True;User Instance=True")
    {
        con.Open();
        using(SqlCommand cmd = new SqlCommand("select count(*) from [user] where "+ 
                                   "Username=@uname and Password=@upass", con)
        {
            cmd.Parameters.AddWithValue("@uname", txt_user.Text)
            cmd.Parameters.AddWithValue("@upass", txt_pass.Text);
            int count =Convert.ToInt16(cmd.ExecuteScalar());
            ......
        }
    }
}

It helped a great deal. Had no idea about Connection Pool. Thanks a ton. But what if I have to use this connection a lot many times? A Connection Class would suffice? Sorry I am too new to asp... — Dahlia George, Mar 17 '13 at 21:44
I repeat myself. Do not store globally a connection and, if you don't have a specific reason, do not worry to create a `Connection class`. Just create an istance of SqlConnection, encapsulated in a using statement, when you need it. However you could store that connection string (or read it from the web.config of your asp app) — Steve, Mar 17 '13 at 21:53

score 1 · Answer 2 · answered Mar 17 '13 at 18:42

1

The thing is, that "user" is a reserved word in SQL. Apart form the injection problem, your query should read:

select ... from [user] where

answered Mar 17 '13 at 18:42

Roman Gruber

1,411
11
16

score 1 · Answer 3 · answered Mar 17 '13 at 18:42

1

'User' is a reserved keyword in SQL server. If you have a table name 'user', you should put it in brakets within queries

select count(*) from [user] where ...

answered Mar 17 '13 at 18:42

alex

12,464
3
46
67

score 0 · Answer 4 · answered Mar 17 '13 at 20:28

I agree fully with all that has been said by Steve, Roman and Alex.

If I should add something it is that you should try to minimize the usage of ad-hoc queries, that has been used in this example. Rather you should prefer to put the most part of the sql code in sql functions and stored procedures, since that can give a hugh improvement of the performance since those queries can be compiled the first time they are executed and the times executed afterward the database can focus on just to retreive data. With ad-hoc queries the query has to be compiled every time, which can actually take some time for more complex queries.

You can then execute the Stored Procedure like this:

using(SqlCommand cmd = new SqlCommand("dbo.IsValidLogin", con)
{
    cmd.CommandType = CommandType.StoredProcedure;
    cmd.Parameters.AddWithValue("@username", txt_user.Text)
    cmd.Parameters.AddWithValue("@password", txt_pass.Text);
    var isValidLogin =Convert.ToBool(cmd.ExecuteScalar());
    ...
}

If you have a Procedure declared in the database like this:

CREATE PROC dbo.IsValidLogin
@username nvarchar(50),
@password nvarchar(50)
AS
BEGIN
  SELECT count(1) 
  FROM [user] 
  WHERE Username=@username
  AND Password=@password
END;

See a Sql Fiddle exemple here.

@mikeng actually it is a myth that SPROCs increas performance just for being used. It *might* increase performance because of less data getting sent *to* the server. But MSSQL can cache execution plans for ad-hoc queries just fine too. Parameters however are mandatory :) — Roman Gruber, Mar 17 '13 at 23:37

SQL run time error in asp.net c#

4 Answers4