Fix S2077 FP: Support constant query built with ternary in a formatted character string

[mgirolet-gl]

Description

S2077 is a Security Hotspot rule that raises when an SQL query is dynamically formatted. However, it doesn't detect what the query is made out of.

The "What's the risk?" and "Assess the risk" sections say:

Formatted SQL queries can be difficult to maintain, debug and can increase the risk of SQL injection when concatenating untrusted values into the query. However, this rule doesn’t detect SQL injections (unlike rule S3649), the goal is only to highlight complex/formatted queries.

Ask Yourself Whether

• Some parts of the query come from untrusted values (like user inputs).
• The query is repeated/duplicated in other parts of the code.
• The application must support different types of relational databases.

There is a risk if you answered yes to any of those questions.

However, if the formatted query is only made out of constant strings, including those decided by ternaries and functions that only use constant strings too, then there is no risk of SQL injection, untrusted user values, or anything else that'd otherwise be a good reason to raise a Security Hotspot.

Repro steps

In the first example below, we add a constant string to the query if a boolean is set to true. This poses absolutely no safety threat, since it is clear here that every character the query is made out of was written safely by the programmer, even if it the end query will be different depending of a user-provided boolean. Yet, the function raises S2077.

The function below it is functionally equivalent to the first one, and does not raise S2077, but is clearly less maintainable, repeats part of the query (which infringes the second point of the "Assess the risk" section by the way) and is overall much less practical to work with.

The second example adds a constant string to the query from a function's return value. This doesn't pose a safety threat either for the same reasons as the first exemple.

The compliant version below it on the other hand is less maintainable for the same reasons.

// Using Dapper for queries and parameters

/*****************/
/* First example */
/*****************/

// Non-compliant

List<int> GetUsersIDs(bool onlyEnabled) {
    string query = $"SELECT id FROM users {(onlyEnabled ? " WHERE enabled = 1" : "")}";

    return db.Query<int>(query).ToList(); // Raises because query is dynamically formatted
}

// Compliant

List<int> GetUsersIDs(bool onlyEnabled) {
    if(onlyEnabled) {
        return db.Query<int>("SELECT id FROM users WHERE enabled = 1").ToList(); // Compliant
    }
    else {
        return db.Query<int>("SELECT id FROM users").ToList(); // Compliant
    }
}

/******************/
/* Second example */
/******************/

// Non-compliant

string GenerateWhere(int minimumPower, DynamicParameters params) {
    params.Add("@minPower", minimumPower);

    // In real cases, there would probably be some logic to generate this string.
    // But what's important here is that no matter the logic, the string does not contain any user input and all of its possible characters were safely written by the programmer
    return " WHERE power > @minPower";
}

List<int> GetUsersIDs(int minimumPower) {
    DynamicParameters params = new();

    string query = $"SELECT id FROM users";
    if(minimumPower > 0)
        query += GenerateWhere(minimumPower, params);

    return db.Query<int>(query, params).ToList(); // Raises because query is dynamically formatted
}

// Compliant

List<int> GetUsersIDs(int minimumPower) {
    if(minimumPower > 0) {
        DynamicParameters params = new();
        params.Add("@minPower", minimumPower);
        return db.Query<int>("SELECT id FROM users WHERE power > @minPower", params).ToList(); // Compliant
    }
    else {
        return db.Query<int>("SELECT id FROM users").ToList(); // Compliant
    }
}

Expected behavior

If the dynamically formatted SQL query uses only constant strings, no Security Hotspot should be raised.

Actual behavior

A Security Hotspot is raised as soon as anything in the query is dynamically formatted, regardless of the way it is done.

Known workarounds

Marking the Security Hotspot as Safe, or duplicating the whole query, both of which are impractical.

Related information

• C#/VB.NET Plugins version: 9.27.0.93347
• Visual Studio version: 2022
• MSBuild version: 17.7.6
• Dotnet version: .NET 6.0
• SonarScanner for .NET version: 6.2.0
• Operating System: Windows/Linux

[sebastien-marichal]

Hello @mgirolet-gl,

I am not able to reproduce the issue.

Which framework are you using to execute the query? What is the type of db?

Thank you,

[mgirolet-gl]

Hello @mgirolet-gl,

I am not able to reproduce the issue.

Which framework are you using to execute the query? What is the type of db?

Thank you,

Hi,

I am using .NET 6.0 with ASP.NET.

db is a MySqlConnection, a child class of System.Data.Common.DbConnection instanciated using the MySql.Data NuGet Package.

The issue also raises with syntax like $"SELECT id FROM users {(onlyEnabled ? "WHERE enabled = 1" : "")}", if it can help reproduce it.

Thanks.

[sebastien-marichal]

MySqlConnection does not seem to have a Query method. Is it an extension method?

Could you provide me with a small project that reproduces the issue?

[mgirolet-gl]

My apologies, it seems that the Query method is indeed an extension from Dapper, which we use to pass parameters to queries.

My example didn't provide parameters to queries so I ommited Dapper's inclusion.

I will update the issue accordingly. I cannot make an example project as of now but will try to do so whenever I can unless you can reproduce the issue in the meantime.

Thank you

[sebastien-marichal]

Hello @mgirolet-gl,

I was not able to reproduce your initial error. However, when using a ternary inside a formatted string, I am able to reproduce. So far, I came down to this:

public void ConstantQuery(MySqlConnection db, bool onlyEnabled)
{
    string query = "SELECT id FROM users";
    if(onlyEnabled)
        query += " WHERE enabled = 1";

    db.Query<int>(query); // This is fine
    db.Query<int>($"SELECT id FROM users {(onlyEnabled ? "WHERE enabled = 1" : "")}"); // S2077 is raising here
}

I will add this to the backlog but only for the ternary in a formatted string. Unless you can provide a reproducer, we won't be able to fix the initial issue.

[mgirolet-gl]

I cannot reproduce the initial issue either on a separate project. Might be an oddity with my project that'll require further investigation on my side.

I will update the issue to mention the ternary inside of a formatted string since it can be consistently reproduced.