CODEWITHSUNDEEP
c#asp.net
Adrian
Adrian

Reputation: 57

Is this Sql injection proof Asp.net code?

Can somebody tell me the pros and cons of this code? I know I can use stored procedures instead, but would it be easy to SQL inject this code considering I had a textbox where admins could input the commentid?

string commentId = a.Text;
SqlConnection con = new SqlConnection(ConfigurationManager.ConnectionStrings["ForumDatabaseConnectionString"].ConnectionString);
con.Open();
string sql = "DELETE FROM Comment WHERE Comment.commentId = @commentid";
SqlCommand cmd = new SqlCommand(sql, con);
cmd.Parameters.AddWithValue("@commentid", commentId);

cmd.CommandType = CommandType.Text;
cmd.ExecuteNonQuery();

Upvotes: 1

Views: 95

Answers (2)

Mayur Lohite
Mayur Lohite

Reputation: 100

Pros:

  • Good thing is you are using parameters for command which is sql injection safe.

Cons:

  • Not well written.
  • Not using function for CRUD. Always Use functions to do CRUD operation.
  • No Use of Using block. Always use using block, so you don't need to dispose connection & command. You don't need to manually close it.

Use following code in DataAccessLayer.

public void DeleteComment(int commentId)
{
     using (SqlConnection con = new SqlConnection(ConfigurationManager.ConnectionStrings["ForumDatabaseConnectionString"].ConnectionString))
        {
            con.Open();
            string sql = "DELETE FROM Comment WHERE Comment.commentId = @commentid";
            using (SqlCommand cmd = new SqlCommand(sql, con))
            {
                cmd.Parameters.AddWithValue("@commentid", commentId);
                cmd.CommandType = CommandType.Text;
                cmd.ExecuteNonQuery();
            }

        } 
}

You can write connection open code in separate function too.

Check this article for more detail:

https://www.codeproject.com/Articles/813965/Preventing-SQL-Injection-Attack-ASP-NET-Part-I

Upvotes: 0

Jon Bates
Jon Bates

Reputation: 3173

Yes, it looks fine, since you're using paramterized sql. However, you haven't given your table an alias, so I thing your sql should be

DELETE FROM Comment WHERE commentId = @commentid

As well as protecting you from sql injection attacks, Sql Server will know that this sql may be called again with different parameters, so can cache an efficient execution plan for it.

As an aside, you should always dispose of connections after using them.

    string commentId = a.Text;

    using (SqlConnection con = new SqlConnection(ConfigurationManager
                .ConnectionStrings["ForumDatabaseConnectionString"].ConnectionString))
    {
        con.Open();
        string sql = "DELETE FROM Comment WHERE Comment.commentId = @commentid";
        using(SqlCommand cmd = new SqlCommand(sql, con))
        {
            cmd.Parameters.AddWithValue("@commentid", commentId);

            cmd.CommandType = CommandType.Text;
            cmd.ExecuteNonQuery();
        }
    }

As you can see, there is a fair amount of code for such a simple operation. You may wish to take a look at dapper, which will remove a lot of these issues. There are many libraries to help you, which are off-topic here, but its a lightweight, popular one

Upvotes: 2

Related Questions