dotnet / roslyn-analyzers

MIT License
1.58k stars 463 forks source link

Remaining work for Security analyzers to respect `excluded_symbol_names` option #2706

Open mavasani opened 5 years ago

mavasani commented 5 years ago

See https://github.com/dotnet/roslyn-analyzers/pull/2699#discussion_r307550346.

mavasani commented 5 years ago

Would be good to also take care of https://github.com/dotnet/roslyn-analyzers/pull/2687#discussion_r307794032

Evangelink commented 3 years ago

As far as I can see this is handled. I'll let @mavasani or @dotpaul confirm and close the ticket.

dotpaul commented 3 years ago

Thanks @Evangelink for the ping. Still need to figure out what the behavior should be and implement that, if needed.

Evangelink commented 3 years ago

Except if I am missing something here but the ticket and the comment are talking about the excluded_symbol_names options which are handled here https://github.com/dotnet/roslyn-analyzers/blob/master/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Security/DoNotUseInsecureDeserializerJsonNetWithoutBinder.cs#L132-L141

dotpaul commented 3 years ago

It's currently handled by skipping dataflow analysis when both rules disable a symbol. This issue was opened to figure out what to do if one rule disables the symbol for dataflow analysis, and the other rule leaves the symbol enabled for dataflow analysis.

I suppose I can say no one's complained so far, so the current implementation is good enough. :smile:

Evangelink commented 3 years ago

Ohhh got it! I saw the TODO in the code but I thought this was one TODO missing an associated work item. My bad!

Evangelink commented 3 years ago

Would it be possible to say that we store the result of the IsConfiguredToSkipAnalysis option in a variable and at the time where we report we do a match?

foreach (KeyValuePair<(Location Location, IMethodSymbol? Method), HazardousUsageEvaluationResult> kvp
                                    in allResults)
{
    DiagnosticDescriptor descriptor;
    switch (kvp.Value)
    {
        case HazardousUsageEvaluationResult.Flagged:
            if (skipDefinitelyInsecureSerializer)
            {
                continue;
            }
            else
            {
                descriptor = DefinitelyInsecureSerializer;
            }
            break;

        case HazardousUsageEvaluationResult.MaybeFlagged:
            if (skipMaybeInsecureSerializer)
            {
                continue;
            }
            else
            {
                descriptor = MaybeInsecureSerializer;
            }
            break;

        default:
            Debug.Fail($"Unhandled result value {kvp.Value}");
            continue;
    }

    compilationAnalysisContext.ReportDiagnostic(
        Diagnostic.Create(
            descriptor,
            kvp.Key.Location));
}

WDYT?