Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remaining work for Security analyzers to respect excluded_symbol_names option #2706

Open
mavasani opened this issue Jul 26, 2019 · 7 comments
Assignees
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design Category-Security help wanted The issue is up-for-grabs, and can be claimed by commenting
Milestone

Comments

@mavasani
Copy link
Contributor

See #2699 (comment).

@mavasani
Copy link
Contributor Author

Would be good to also take care of #2687 (comment)

@Evangelink
Copy link
Member

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

@dotpaul
Copy link
Contributor

dotpaul commented Oct 1, 2020

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

@Evangelink
Copy link
Member

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
Copy link
Contributor

dotpaul commented Oct 1, 2020

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. 😄

@Evangelink
Copy link
Member

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
Copy link
Member

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?

@mavasani mavasani added this to the Unknown milestone May 6, 2021
@mavasani mavasani added the help wanted The issue is up-for-grabs, and can be claimed by commenting label May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design Category-Security help wanted The issue is up-for-grabs, and can be claimed by commenting
Projects
None yet
Development

No branches or pull requests

3 participants