Fix IndexOutOfRangeException bug of CA5391. #2857

LLLXXXCCC · 2019-09-18T12:41:50Z

…ng if only look at Controller-derived classes.

…LLLXXXCCC/roslyn-analyzers into UseAutoValidateAntiforgeryToken

LLLXXXCCC · 2019-09-18T12:42:20Z

mavasani · 2019-09-18T12:46:11Z

@LLLXXXCCC minor suggestion - if your PR closes a specific issue, then you can add a Fixes #%issue_id% comment in the PR description and this will ensure that when you merge the PR, the mentioned issue will automatically get closed.

docs/Analyzer Configuration.md

src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs

mavasani · 2019-09-18T13:15:33Z

src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs

@@ -340,7 +350,8 @@ void FindAllTheSpecifiedCalleeMethods(ISymbol methodSymbol, HashSet<ISymbol> vis

                        foreach (var child in callingMethods.Keys)
                        {
-                            if (onAuthorizationAsyncMethodSymbols.Contains(child))
+                            if (child is IMethodSymbol childMethodSymbol &&
+                                onAuthorizationAsyncMethodSymbols.ContainsKey(childMethodSymbol))


Do we know where the IndexOutOfRangeException was being thrown? I presume it is in the indexer access below results[methodSymbol]? If so, I am not sure how this PR would fix that exception.

@LLLXXXCCC based on your guess of the exception is a thread safety issue, I've come up with an unreliable repro (get the rule to trigger when there are multiple controllers in the project).

https://github.com/dotpaul/repro2844

Replace the Microsoft.NetCore.Analyzers.dll with your own build (maybe with Debugger.Launch() somewhere so you can catch the exceptions), and try building / rebuilding multiple times.

Also, don't remember if I got IndexOutOfRangeExceptions, but definitely ran into some others like:

warning AD0001: Analyzer 'Microsoft.NetCore.Analyzers.Security.UseAutoValidateAntiforgeryToken' threw an exception of type 'System.ArgumentException' with message 'Destination array was not long enough. Check destIndex and length, and the array's lower bounds.'.

warning AD0001: Analyzer 'Microsoft.NetCore.Analyzers.Security.UseAutoValidateAntiforgeryToken' threw an exception of type 'System.NullReferenceException' with message 'Object reference not set to an instance of an object.'.

I think these might all be likely due to use of non-concurrent data structure in this analyzer.

@LLLXXXCCC were you able to repro some exceptions being thrown, and verify your changes don't have exceptions?

Would probably be a good idea to see if a unit test with multiple controllers can repro (and should have one anyway).

Yes, I can reproduce the two warnings you mentioned and the IndexOutOfRangeException. For now, the fix works well.

Please add a unit test that contains multiple (like 4 or 5) controllers and produces diagnostics, so that we have a test committed that reproduces the original problem.

mavasani

I'd let @dotpaul review the overall PR, but am interested in knowing which change fixes the IndexOutOfRangeException.

src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs

docs/Analyzer Configuration.md

src/Utilities/Compiler/Options/EditorConfigOptionNames.cs

dotpaul · 2019-09-19T22:06:48Z

src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs

@@ -215,9 +224,10 @@ public override void Initialize(AnalysisContext context)
                    var derivedControllerTypeSymbol = (INamedTypeSymbol)symbolAnalysisContext.Symbol;
                    var baseTypes = derivedControllerTypeSymbol.GetBaseTypes();

-                    // An subtype of `Microsoft.AspNetCore.Mvc.Controller` or `Microsoft.AspNetCore.Mvc.ControllerBase`).
+                    // An subtype of `Microsoft.AspNetCore.Mvc.Controller`, which indicates that cookie-based authentication is used and thus CSRF is a concern.


which indicates that cookie-based authentication is used and thus CSRF is a concern. [](start = 76, length = 84)

which probably indicates views are used and maybe cookie-based authentication is used and thus CSRF is a concern.

(Can't be sure. 🙂)

Co-Authored-By: Genevieve Warren <gewarren@microsoft.com>

…LLLXXXCCC/roslyn-analyzers into UseAutoValidateAntiforgeryToken

docs/Analyzer Configuration.md

dotpaul

🚢

LingxiaChen added 2 commits September 18, 2019 20:39

Fix the IndexOutOfRangeException warning and add a option for configi…

53bfa2f

…ng if only look at Controller-derived classes.

Merge branch 'UseAutoValidateAntiforgeryToken' of https://github.com/…

3dfaf04

…LLLXXXCCC/roslyn-analyzers into UseAutoValidateAntiforgeryToken