2

Below is the code I got from Microsoft page: SqlCommand

public static Int32 ExecuteNonQuery(String connectionString, String commandText, CommandType commandType, params SqlParameter[] parameters)
{
        using (SqlConnection conn = new SqlConnection(connectionString))
        {
            using (SqlCommand cmd = new SqlCommand(commandText, conn))
            {
                // There're three command types: StoredProcedure, Text, TableDirect. The TableDirect 
                // type is only for OLE DB.  
                cmd.CommandType = commandType;
                cmd.Parameters.AddRange(parameters);

                conn.Open();
                return cmd.ExecuteNonQuery();
            }
        }
}

However, VS code analysis still complains about "CA2100":

Warning CA2100 The query string passed to 'SqlCommand.SqlCommand(string, SqlConnection)' in 'FlexClaimFormRepository.ExecuteNonQuery(string, string, CommandType, params SqlParameter[])' could contain the following variables 'commandText'. If any of these variables could come from user input, consider using a stored procedure or a parameterized SQL query instead of building the query with string concatenations.

I know the exact reason why the warning is there, but anyidea on how to get rid of it? Given setting commandText inside the function is not acceptable since I want it to be a parameter.

marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
Toby D
  • 1,465
  • 1
  • 19
  • 31
  • Apologize, it's CA2100. I already updated the title. – Toby D Nov 06 '17 at 07:24
  • OK. Show us sample query. In a meanwhile, please see: [CA2100: Review SQL queries for security vulnerabilities](https://msdn.microsoft.com/en-us/library/ms182310.aspx) – Maciej Los Nov 06 '17 at 07:29
  • I don' think we need sample query here since VS code analysis will just give the same warning. – Toby D Nov 06 '17 at 07:31
  • 2
    When you are sure that the command is not coming from a user input or from another kind of "outside", you can safely suppress it. – Stefan Steinegger Nov 06 '17 at 08:06
  • @StefanSteinegger Is there anything I can do where the system needs to run SQL script from internal user(s)? – Toby D Nov 06 '17 at 08:27
  • 1
    If `commandText` is free text that can be filled with arbitrary SQL like `UPDATE Salary SET Amount = Amount * 1000 WHERE Person='Me'`, then the best thing to do is ensure this application only ever connects with a read only user. For example by adding the user to the `db_datareader` role. VS code analysis won't recognise that though. – Nick.Mc Nov 06 '17 at 10:06
  • The CA2100 description says "It is safe to suppress a warning from this rule if the command text does not contain any user input." – Andrey Belykh Nov 08 '17 at 16:42
  • @TobyD: I don't know what you mean by "internal user(s)". Is it a client-server application or are there processes running under different users? Then you have to make sure that no strings coming from the client are directly used to build a query. You are passing a connection string to ExecuteNonQuery. Where is it coming from? Can a regular user get the connection string (containing a password) and access the database directly? If yes then you cannot be safe from malicious changes anyway. – Stefan Steinegger Nov 10 '17 at 12:18

1 Answers1

3

I know this question is old, but maybe this will help someone with the same issue. I also was using a stored procedure and therefore suppressed the warning using the information from this article from Microsoft: https://learn.microsoft.com/en-us/visualstudio/code-quality/in-source-suppression-overview?view=vs-2019 .

Here's the attribute I added to my method:

[System.Diagnostics.CodeAnalysis.SuppressMessage("Security", "CA2100:Review SQL queries for security vulnerabilities", Justification = "Method already uses a Stored Procedure")]
Kevin Dykema
  • 31
  • 1
  • 3