Warn when [Authorize] is overridden by [AllowAnymous] from "farther" away (#56244)

Author: halter73

src/Framework/AspNetCoreAnalyzers/samples/WebAppSample/WebAppSample.csproj

@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk.Web">
+<Project Sdk="Microsoft.NET.Sdk.Web">
 
   <PropertyGroup>
     <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>

src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs

@@ -214,4 +214,13 @@ internal static class DiagnosticDescriptors
         DiagnosticSeverity.Info,
         isEnabledByDefault: true,
         helpLinkUri: "https://aka.ms/aspnet/analyzers");
+
+    internal static readonly DiagnosticDescriptor OverriddenAuthorizeAttribute = new(
+        "ASP0026",
+        new LocalizableResourceString(nameof(Resources.Analyzer_OverriddenAuthorizeAttribute_Title), Resources.ResourceManager, typeof(Resources)),
+        new LocalizableResourceString(nameof(Resources.Analyzer_OverriddenAuthorizeAttribute_Message), Resources.ResourceManager, typeof(Resources)),
+        "Security",
+        DiagnosticSeverity.Warning,
+        isEnabledByDefault: true,
+        helpLinkUri: "https://aka.ms/aspnet/analyzers");
 }

src/Framework/AspNetCoreAnalyzers/src/Analyzers/Mvc/DetectOverriddenAuthorizeAttribute.cs

@@ -0,0 +1,223 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Generic;
+using System.Diagnostics;
+using System.Linq;
+using Microsoft.AspNetCore.App.Analyzers.Infrastructure;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.Diagnostics;
+
+namespace Microsoft.AspNetCore.Analyzers.Mvc;
+
+using WellKnownType = WellKnownTypeData.WellKnownType;
+
+public partial class MvcAnalyzer
+{
+    /// <summary>
+    /// This tries to detect [Authorize] attributes that are unwittingly overridden by [AllowAnonymous] attributes that are "farther" away from a controller.
+    /// </summary>
+    /// <remarks>
+    /// This might report the same [Authorize] attribute multiple times if it's on a shared base type, but we'd have to disable parallelization of the
+    /// entire MvcAnalyzer to avoid that. We assume that this scenario is rare enough and that overreporting is benign enough to not warrant the performance hit.
+    /// See AuthorizeOnControllerBaseWithMultipleChildren_AllowAnonymousOnControllerBaseBaseType_HasMultipleDiagnostics.
+    /// </remarks>
+    private static void DetectOverriddenAuthorizeAttributeOnController(SymbolAnalysisContext context, WellKnownTypes wellKnownTypes,
+        INamedTypeSymbol controllerSymbol, List<AttributeInfo> authorizeAttributes, out string? allowAnonClass)
+    {
+        Debug.Assert(authorizeAttributes.Count is 0);
+
+        var isCheckingBaseType = false;
+        allowAnonClass = null;
+
+        foreach (var currentClass in controllerSymbol.GetTypeHierarchy())
+        {
+            FindAuthorizeAndAllowAnonymous(wellKnownTypes, currentClass, isCheckingBaseType, authorizeAttributes, out var foundAllowAnonymous);
+            if (foundAllowAnonymous)
+            {
+                // Anything we find after this would be farther away, so we can short circuit.
+                ReportOverriddenAuthorizeAttributeDiagnosticsIfAny(context, authorizeAttributes, currentClass.Name);
+                // Keep track of the nearest class with [AllowAnonymous] for later reporting of action-level [Authorize] attributes.
+                allowAnonClass = currentClass.Name;
+                return;
+            }
+
+            isCheckingBaseType = true;
+        }
+    }
+
+    /// <summary>
+    /// This tries to detect [Authorize] attributes that are unwittingly overridden by [AllowAnonymous] attributes that are "farther" away from a controller action.
+    /// To do so, it first searches the action method and then the controller class. It repeats this process for each virtual method the action may override and for
+    /// each base class the controller may inherit from. Since it searches for the attributes closest to the action first, it short circuits as soon as [AllowAnonymous] is found.
+    /// If it has already detected a closer [Authorize] attribute, it reports a diagnostic at the [Authorize] attribute's location indicating that it will be overridden.
+    /// </summary>
+    private static void DetectOverriddenAuthorizeAttributeOnAction(SymbolAnalysisContext context, WellKnownTypes wellKnownTypes,
+        IMethodSymbol actionSymbol, List<AttributeInfo> authorizeAttributes, string? allowAnonClass)
+    {
+        Debug.Assert(authorizeAttributes.Count is 0);
+
+        var isCheckingBaseType = false;
+        var currentMethod = actionSymbol;
+
+        foreach (var currentClass in actionSymbol.ContainingType.GetTypeHierarchy())
+        {
+            bool foundAllowAnonymous;
+
+            if (currentMethod is not null && IsSameSymbol(currentMethod.ContainingType, currentClass))
+            {
+                FindAuthorizeAndAllowAnonymous(wellKnownTypes, currentMethod, isCheckingBaseType, authorizeAttributes, out foundAllowAnonymous);
+                if (foundAllowAnonymous)
+                {
+                    // [AllowAnonymous] was found on the action method. Anything we find after this would be farther away, so we short circuit.
+                    ReportOverriddenAuthorizeAttributeDiagnosticsIfAny(context, authorizeAttributes, currentMethod.ContainingType.Name, currentMethod.Name);
+                    return;
+                }
+
+                currentMethod = currentMethod.OverriddenMethod;
+
+                // We've already checked the controller and any base classes for overridden attributes in DetectOverriddenAuthorizeAttributeOnController.
+                // If there are no more base methods, and we are not tracking any unreported [Authorize] attributes that might be overridden by a class, we're done.
+                if (currentMethod is null && (authorizeAttributes.Count is 0 || !isCheckingBaseType))
+                {
+                    if (allowAnonClass is not null)
+                    {
+                        // We don't use allowAnonClass once we start checking overrides to avoid false positives. But if we found [Authorize] directly on a non-virtual
+                        // action, we can report it without rechecking the controller or its base types for [AllowAnonymous] when given a non-null allowAnonClass.
+                        ReportOverriddenAuthorizeAttributeDiagnosticsIfAny(context, authorizeAttributes, allowAnonClass);
+                    }
+
+                    return;
+                }
+            }
+
+            // Now, we're mostly trying to detect [Authorize] on virtual actions which are not covered by allowAnonClass. Overridden [Authorize] attributes on classes
+            // have mostly been reported already, but we still need to track those too just in case there is [AllowAnonymous] on a base method farther away.
+            FindAuthorizeAndAllowAnonymous(wellKnownTypes, currentClass, isCheckingBaseType, authorizeAttributes, out foundAllowAnonymous);
+            if (foundAllowAnonymous)
+            {
+                // We are only concerned with method-level [Authorize] attributes that are overridden by the [AllowAnonymous] found on this class.
+                // Any child classes should have already been reported in DetectOverriddenAuthorizeAttributeOnController.
+                ReportOverriddenAuthorizeAttributeDiagnosticsIfAny(context, authorizeAttributes.Where(a => a.IsTargetingMethod), currentClass.Name);
+                return;
+            }
+
+            isCheckingBaseType = true;
+        }
+
+        Debug.Assert(currentMethod is null);
+    }
+
+    private static bool IsSameSymbol(ISymbol? x, ISymbol? y) => SymbolEqualityComparer.Default.Equals(x, y);
+
+    private static bool IsInheritableAttribute(WellKnownTypes wellKnownTypes, INamedTypeSymbol attribute)
+    {
+        // [AttributeUsage] is sealed but inheritable.
+        var attributeUsageAttributeType = wellKnownTypes.Get(WellKnownType.System_AttributeUsageAttribute);
+        var attributeUsage = attribute.GetAttributes(attributeUsageAttributeType, inherit: true).FirstOrDefault();
+
+        if (attributeUsage is not null)
+        {
+            foreach (var arg in attributeUsage.NamedArguments)
+            {
+                if (arg.Key == nameof(AttributeUsageAttribute.Inherited))
+                {
+                    return (bool)arg.Value.Value!;
+                }
+            }
+        }
+
+        // If [AttributeUsage] is not found or the Inherited property is not set, the default is true.
+        return true;
+    }
+
+    private static bool IsMatchingAttribute(WellKnownTypes wellKnownTypes, INamedTypeSymbol attribute,
+        INamedTypeSymbol commonAttribute, ITypeSymbol attributeInterface, bool mustBeInheritable)
+    {
+        // The "common" attribute is either [Authorize] or [AllowAnonymous] so we can skip the interface and inheritable checks.
+        if (IsSameSymbol(attribute, commonAttribute))
+        {
+            return true;
+        }
+
+        if (!attributeInterface.IsAssignableFrom(attribute))
+        {
+            return false;
+        }
+
+        return !mustBeInheritable || IsInheritableAttribute(wellKnownTypes, attribute);
+    }
+
+    private static void FindAuthorizeAndAllowAnonymous(WellKnownTypes wellKnownTypes, ISymbol symbol, bool isCheckingBaseType,
+        List<AttributeInfo> authorizeAttributes, out bool foundAllowAnonymous)
+    {
+        AttributeData? localAuthorizeAttribute = null;
+        List<AttributeData>? localAuthorizeAttributeOverflow = null;
+        foundAllowAnonymous = false;
+
+        foreach (var attribute in symbol.GetAttributes())
+        {
+            if (attribute.AttributeClass is null)
+            {
+                continue;
+            }
+
+            var authInterfaceType = wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Authorization_IAuthorizeData);
+            var authAttributeType = wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Authorization_AuthorizeAttribute);
+            if (IsMatchingAttribute(wellKnownTypes, attribute.AttributeClass, authAttributeType, authInterfaceType, isCheckingBaseType))
+            {
+                if (localAuthorizeAttribute is null)
+                {
+                    localAuthorizeAttribute = attribute;
+                }
+                else
+                {
+                    // This is ony allocated if there are multiple [Authorize] attributes on the same symbol which we assume is rare.
+                    localAuthorizeAttributeOverflow ??= [];
+                    localAuthorizeAttributeOverflow.Add(attribute);
+                }
+            }
+
+            var anonInterfaceType = wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Authorization_IAllowAnonymous);
+            var anonAttributeType = wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Authorization_AllowAnonymousAttribute);
+            if (IsMatchingAttribute(wellKnownTypes, attribute.AttributeClass, anonAttributeType, anonInterfaceType, isCheckingBaseType))
+            {
+                // If localAuthorizeAttribute is not null, [AllowAnonymous] came after [Authorize] on the same method or class. We assume
+                // this closer [AllowAnonymous] was intended to override the [Authorize] attribute which it always does regardless of order.
+                // [Authorize(...)] could still be useful for configuring the authentication scheme even if the endpoint allows anonymous requests.
+                localAuthorizeAttribute = null;
+                localAuthorizeAttributeOverflow?.Clear();
+                foundAllowAnonymous = true;
+            }
+        }
+
+        if (localAuthorizeAttribute is not null)
+        {
+            var isTargetingMethod = symbol is IMethodSymbol;
+            authorizeAttributes.Add(new(localAuthorizeAttribute, isTargetingMethod));
+            foreach (var extraAttribute in localAuthorizeAttributeOverflow ?? Enumerable.Empty<AttributeData>())
+            {
+                authorizeAttributes.Add(new(extraAttribute, isTargetingMethod));
+            }
+        }
+    }
+
+    private static void ReportOverriddenAuthorizeAttributeDiagnosticsIfAny(SymbolAnalysisContext context,
+        IEnumerable<AttributeInfo> authorizeAttributes, string allowAnonClass, string? allowAnonMethod = null)
+    {
+        string? allowAnonLocation = null;
+
+        foreach (var authorizeAttribute in authorizeAttributes)
+        {
+            if (authorizeAttribute.AttributeData.ApplicationSyntaxReference is { } syntaxReference)
+            {
+                allowAnonLocation ??= allowAnonMethod is null ? allowAnonClass : $"{allowAnonClass}.{allowAnonMethod}";
+                context.ReportDiagnostic(Diagnostic.Create(
+                    DiagnosticDescriptors.OverriddenAuthorizeAttribute,
+                    syntaxReference.GetSyntax(context.CancellationToken).GetLocation(),
+                    allowAnonLocation));
+            }
+        }
+    }
+}

src/Framework/AspNetCoreAnalyzers/src/Analyzers/Mvc/MvcAnalyzer.cs

@@ -22,7 +22,8 @@ namespace Microsoft.AspNetCore.Analyzers.Mvc;
 public partial class MvcAnalyzer : DiagnosticAnalyzer
 {
     public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(
-        DiagnosticDescriptors.AmbiguousActionRoute
+        DiagnosticDescriptors.AmbiguousActionRoute,
+        DiagnosticDescriptors.OverriddenAuthorizeAttribute
     );
 
     public override void Initialize(AnalysisContext context)
@@ -36,17 +37,34 @@ public override void Initialize(AnalysisContext context)
             var wellKnownTypes = WellKnownTypes.GetOrCreate(compilation);
             var routeUsageCache = RouteUsageCache.GetOrCreate(compilation);
 
-            var concurrentQueue = new ConcurrentQueue<List<ActionRoute>>();
+            var concurrentQueue = new ConcurrentQueue<(List<ActionRoute> ActionRoutes, List<AttributeInfo> AuthorizeAttributes)>();
 
             context.RegisterSymbolAction(context =>
             {
                 var namedTypeSymbol = (INamedTypeSymbol)context.Symbol;
+
+                // Visit Controllers
                 if (MvcDetector.IsController(namedTypeSymbol, wellKnownTypes))
                 {
                     // Pool and reuse lists for each block.
-                    if (!concurrentQueue.TryDequeue(out var actionRoutes))
+                    if (!concurrentQueue.TryDequeue(out var pooledItems))
+                    {
+                        pooledItems.ActionRoutes = [];
+                        pooledItems.AuthorizeAttributes = [];
+                    }
+
+                    DetectOverriddenAuthorizeAttributeOnController(context, wellKnownTypes, namedTypeSymbol, pooledItems.AuthorizeAttributes, out var allowAnonClass);
+                    pooledItems.AuthorizeAttributes.Clear();
+
+                    // Visit Actions
+                    foreach (var member in namedTypeSymbol.GetMembers())
                     {
-                        actionRoutes = new List<ActionRoute>();
+                        if (member is IMethodSymbol methodSymbol && MvcDetector.IsAction(methodSymbol, wellKnownTypes))
+                        {
+                            PopulateActionRoutes(context, wellKnownTypes, routeUsageCache, pooledItems.ActionRoutes, methodSymbol);
+                            DetectOverriddenAuthorizeAttributeOnAction(context, wellKnownTypes, methodSymbol, pooledItems.AuthorizeAttributes, allowAnonClass);
+                            pooledItems.AuthorizeAttributes.Clear();
+                        }
                     }
 
                     RoutePatternTree? controllerRoutePattern = null;
@@ -60,50 +78,41 @@ public override void Initialize(AnalysisContext context)
                         }
                     }
 
-                    PopulateActionRoutes(context, wellKnownTypes, routeUsageCache, namedTypeSymbol, actionRoutes);
-
-                    DetectAmbiguousActionRoutes(context, wellKnownTypes, controllerRoutePattern, actionRoutes);
+                    DetectAmbiguousActionRoutes(context, wellKnownTypes, controllerRoutePattern, pooledItems.ActionRoutes);
 
                     // Return to the pool.
-                    actionRoutes.Clear();
-                    concurrentQueue.Enqueue(actionRoutes);
+                    pooledItems.ActionRoutes.Clear();
+                    concurrentQueue.Enqueue(pooledItems);
                 }
             }, SymbolKind.NamedType);
         });
     }
 
-    private static void PopulateActionRoutes(SymbolAnalysisContext context, WellKnownTypes wellKnownTypes, RouteUsageCache routeUsageCache, INamedTypeSymbol namedTypeSymbol, List<ActionRoute> actionRoutes)
+    private static void PopulateActionRoutes(SymbolAnalysisContext context, WellKnownTypes wellKnownTypes, RouteUsageCache routeUsageCache, List<ActionRoute> actionRoutes, IMethodSymbol methodSymbol)
     {
-        foreach (var member in namedTypeSymbol.GetMembers())
+        // [Route("xxx")] attributes don't have a HTTP method and instead use the HTTP methods of other attributes.
+        // For example, [HttpGet] + [HttpPost] + [Route("xxx")] means the route "xxx" is combined with the HTTP methods.
+        var unroutedHttpMethods = GetUnroutedMethodHttpMethods(wellKnownTypes, methodSymbol);
+
+        foreach (var attribute in methodSymbol.GetAttributes())
         {
-            if (member is IMethodSymbol methodSymbol &&
-                MvcDetector.IsAction(methodSymbol, wellKnownTypes))
+            if (attribute.AttributeClass is null || !wellKnownTypes.IsType(attribute.AttributeClass, RouteAttributeTypes, out var match))
             {
-                // [Route("xxx")] attributes don't have a HTTP method and instead use the HTTP methods of other attributes.
-                // For example, [HttpGet] + [HttpPost] + [Route("xxx")] means the route "xxx" is combined with the HTTP methods.
-                var unroutedHttpMethods = GetUnroutedMethodHttpMethods(wellKnownTypes, methodSymbol);
-
-                foreach (var attribute in methodSymbol.GetAttributes())
-                {
-                    if (attribute.AttributeClass is null || !wellKnownTypes.IsType(attribute.AttributeClass, RouteAttributeTypes, out var match))
-                    {
-                        continue;
-                    }
+                continue;
+            }
 
-                    var routeUsage = GetRouteUsageModel(attribute, routeUsageCache, context.CancellationToken);
-                    if (routeUsage is null)
-                    {
-                        continue;
-                    }
+            var routeUsage = GetRouteUsageModel(attribute, routeUsageCache, context.CancellationToken);
+            if (routeUsage is null)
+            {
+                continue;
+            }
 
-                    // [Route] uses unrouted HTTP verb attributes for its HTTP methods.
-                    var methods = match.Value is WellKnownType.Microsoft_AspNetCore_Mvc_RouteAttribute
-                        ? unroutedHttpMethods
-                        : ImmutableArray.Create(GetHttpMethod(match.Value)!);
+            // [Route] uses unrouted HTTP verb attributes for its HTTP methods.
+            var methods = match.Value is WellKnownType.Microsoft_AspNetCore_Mvc_RouteAttribute
+                ? unroutedHttpMethods
+                : ImmutableArray.Create(GetHttpMethod(match.Value)!);
 
-                    actionRoutes.Add(new ActionRoute(methodSymbol, routeUsage, methods));
-                }
-            }
+            actionRoutes.Add(new ActionRoute(methodSymbol, routeUsage, methods));
         }
     }
 
@@ -179,4 +188,5 @@ private static ImmutableArray<string> GetUnroutedMethodHttpMethods(WellKnownType
     }
 
     private record struct ActionRoute(IMethodSymbol ActionSymbol, RouteUsageModel RouteUsageModel, ImmutableArray<string> HttpMethods);
+    private record struct AttributeInfo(AttributeData AttributeData, bool IsTargetingMethod);
 }