diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs index e48dd9445bfc..9b802047efce 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs @@ -248,4 +248,13 @@ internal static class DiagnosticDescriptors DiagnosticSeverity.Info, isEnabledByDefault: true, helpLinkUri: AnalyzersLink); + + internal static readonly DiagnosticDescriptor AttributeRoutingUsedForNonActionMethod = new( + "ASP0029", + CreateLocalizableResourceString(nameof(Resources.Analyzer_AttributeRoutingUsedForNonActionMethod_Title)), + CreateLocalizableResourceString(nameof(Resources.Analyzer_AttributeRoutingUsedForNonActionMethod_Message)), + Usage, + DiagnosticSeverity.Info, + isEnabledByDefault: true, + helpLinkUri: AnalyzersLink); } diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Mvc/MvcAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Mvc/MvcAnalyzer.cs index a7be8f52796c..eaa3b6a52157 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Mvc/MvcAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Mvc/MvcAnalyzer.cs @@ -21,10 +21,10 @@ namespace Microsoft.AspNetCore.Analyzers.Mvc; [DiagnosticAnalyzer(LanguageNames.CSharp)] public partial class MvcAnalyzer : DiagnosticAnalyzer { - public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create( + public override ImmutableArray SupportedDiagnostics { get; } = [ DiagnosticDescriptors.AmbiguousActionRoute, - DiagnosticDescriptors.OverriddenAuthorizeAttribute - ); + DiagnosticDescriptors.OverriddenAuthorizeAttribute, + DiagnosticDescriptors.AttributeRoutingUsedForNonActionMethod]; public override void Initialize(AnalysisContext context) { @@ -59,12 +59,21 @@ public override void Initialize(AnalysisContext context) // Visit Actions foreach (var member in namedTypeSymbol.GetMembers()) { - if (member is IMethodSymbol methodSymbol && MvcDetector.IsAction(methodSymbol, wellKnownTypes)) + if (member is not IMethodSymbol methodSymbol) + { + continue; + } + + if (MvcDetector.IsAction(methodSymbol, wellKnownTypes)) { PopulateActionRoutes(context, wellKnownTypes, routeUsageCache, pooledItems.ActionRoutes, methodSymbol); DetectOverriddenAuthorizeAttributeOnAction(context, wellKnownTypes, methodSymbol, pooledItems.AuthorizeAttributes, allowAnonClass); pooledItems.AuthorizeAttributes.Clear(); } + else + { + AnalyzeNonActionMethod(methodSymbol, wellKnownTypes, context); + } } RoutePatternTree? controllerRoutePattern = null; @@ -88,6 +97,17 @@ public override void Initialize(AnalysisContext context) }); } + private static void AnalyzeNonActionMethod(IMethodSymbol method, WellKnownTypes wellKnownTypes, SymbolAnalysisContext context) + { + var routeTemplateProviderType = wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Mvc_Routing_IRouteTemplateProvider); + + if (method.TryGetAttributeImplementingInterface(routeTemplateProviderType, out var attribute)) + { + var syntax = attribute.ApplicationSyntaxReference!.GetSyntax(context.CancellationToken); + context.ReportDiagnostic(Diagnostic.Create(DiagnosticDescriptors.AttributeRoutingUsedForNonActionMethod, syntax.GetLocation(), method.Name)); + } + } + private static void PopulateActionRoutes(SymbolAnalysisContext context, WellKnownTypes wellKnownTypes, RouteUsageCache routeUsageCache, List actionRoutes, IMethodSymbol methodSymbol) { // [Route("xxx")] attributes don't have a HTTP method and instead use the HTTP methods of other attributes. diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Resources.resx b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Resources.resx index 8c9397f5be64..b5cf03f04c27 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Resources.resx +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Resources.resx @@ -333,4 +333,10 @@ If the server does not specifically reject IPv6, IPAddress.IPv6Any is preferred over IPAddress.Any usage for safety and performance reasons. See https://aka.ms/aspnetcore-warnings/ASP0028 for more details. + + Attribute routing is used for a method that is not an action + + + Attribute routing is used for '{0}' method that is not an action. Remove the attribute or make it a public method that can be an action. + diff --git a/src/Framework/AspNetCoreAnalyzers/test/Extensions/CSharpAnalyzerTestExtensions.cs b/src/Framework/AspNetCoreAnalyzers/test/Extensions/CSharpAnalyzerTestExtensions.cs new file mode 100644 index 000000000000..a341d4758b9d --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/test/Extensions/CSharpAnalyzerTestExtensions.cs @@ -0,0 +1,43 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics.CodeAnalysis; +using Microsoft.AspNetCore.Analyzers.Verifiers; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Testing; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Testing; + +namespace Microsoft.AspNetCore.Analyzers; + +public static class CSharpAnalyzerTestExtensions +{ + extension(CSharpAnalyzerTest) + where TAnalyzer : DiagnosticAnalyzer, new() + where TVerifier : IVerifier, new() + { + public static CSharpAnalyzerTest Create([StringSyntax("C#-test")] string source, params ReadOnlySpan expectedDiagnostics) + { + var test = new CSharpAnalyzerTest + { + TestCode = source.ReplaceLineEndings(), + // We need to set the output type to an exe to properly + // support top-level programs in the tests. Otherwise, + // the test infra will assume we are trying to build a library. + TestState = { OutputKind = OutputKind.ConsoleApplication }, + ReferenceAssemblies = CSharpAnalyzerVerifier.GetReferenceAssemblies(), + }; + + test.ExpectedDiagnostics.AddRange(expectedDiagnostics); + return test; + } + } + + public static CSharpAnalyzerTest WithSource(this CSharpAnalyzerTest test, [StringSyntax("C#-test")] string source) + where TAnalyzer : DiagnosticAnalyzer, new() + where TVerifier : IVerifier, new() + { + test.TestState.Sources.Add(source); + return test; + } +} diff --git a/src/Framework/AspNetCoreAnalyzers/test/Mvc/DetectAttributeRoutingUsedForNonActionMethodTest.cs b/src/Framework/AspNetCoreAnalyzers/test/Mvc/DetectAttributeRoutingUsedForNonActionMethodTest.cs new file mode 100644 index 000000000000..55c7b4cd9f04 --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/test/Mvc/DetectAttributeRoutingUsedForNonActionMethodTest.cs @@ -0,0 +1,100 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.CodeAnalysis.CSharp.Testing; +using Microsoft.CodeAnalysis.Testing; + +using CSTest = Microsoft.CodeAnalysis.CSharp.Testing.CSharpAnalyzerTest< + Microsoft.AspNetCore.Analyzers.Mvc.MvcAnalyzer, + Microsoft.CodeAnalysis.Testing.DefaultVerifier>; + +namespace Microsoft.AspNetCore.Analyzers.Mvc; + +public class AttributeRoutingUsedForNonActionMethodTest +{ + private const string Program = """ + public class Program + { + public static void Main() { } + } + """; + + [Theory] + [InlineData("AcceptVerbs(\"get\")")] + [InlineData("HttpDelete")] + [InlineData("HttpGet")] + [InlineData("HttpHead")] + [InlineData("HttpOptions")] + [InlineData("HttpPatch")] + [InlineData("HttpPost")] + [InlineData("HttpPut")] + [InlineData("Route(\"test\")")] + public async Task NonPublicMethod_HasDiagnostics(string attribute) + { + // Arrange + var source = $$""" + using Microsoft.AspNetCore.Mvc; + + public class TestController : ControllerBase + { + [{|#0:{{attribute}}|}] + protected object Test1() => new(); + + [{|#1:{{attribute}}|}] + private object Test2() => new(); + + [{|#2:{{attribute}}|}] + internal object Test3() => new(); + + [{|#3:{{attribute}}|}] + protected internal object Test4() => new(); + } + """; + + var test = CSTest.Create( + source, + CreateDiagnostic("Test1", location: 0), + CreateDiagnostic("Test2", location: 1), + CreateDiagnostic("Test3", location: 2), + CreateDiagnostic("Test4", location: 3)) + .WithSource(Program); + + // Act & Assert + await test.RunAsync(); + } + + [Theory] + [InlineData("AcceptVerbs(\"get\")")] + [InlineData("HttpDelete")] + [InlineData("HttpGet")] + [InlineData("HttpHead")] + [InlineData("HttpOptions")] + [InlineData("HttpPatch")] + [InlineData("HttpPost")] + [InlineData("HttpPut")] + [InlineData("Route(\"test\")")] + public async Task MethodMarkedAsNonAction_HasDiagnostics(string attribute) + { + // Arrange + var source = $$""" + using Microsoft.AspNetCore.Mvc; + + public class TestController : ControllerBase + { + [NonAction] + [{|#0:{{attribute}}|}] + public object Test() => new(); + } + """; + + var test = CSTest.Create(source, CreateDiagnostic("Test")).WithSource(Program); + + // Act & Assert + await test.RunAsync(); + } + + private static DiagnosticResult CreateDiagnostic(string methodName, int location = 0) + { + return new DiagnosticResult(DiagnosticDescriptors.AttributeRoutingUsedForNonActionMethod).WithArguments(methodName).WithLocation(location); + } +} diff --git a/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs b/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs index cfbc023573b3..46f1e364fffb 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Immutable; +using System.Diagnostics.CodeAnalysis; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.InternalTesting; @@ -30,19 +31,9 @@ public static DiagnosticResult Diagnostic(DiagnosticDescriptor descriptor) => CSharpAnalyzerVerifier.Diagnostic(descriptor); /// - public static async Task VerifyAnalyzerAsync(string source, params DiagnosticResult[] expected) + public static async Task VerifyAnalyzerAsync([StringSyntax("C#-test")] string source, params DiagnosticResult[] expected) { - var test = new CSharpAnalyzerTest - { - TestCode = source.ReplaceLineEndings(), - // We need to set the output type to an exe to properly - // support top-level programs in the tests. Otherwise, - // the test infra will assume we are trying to build a library. - TestState = { OutputKind = OutputKind.ConsoleApplication }, - ReferenceAssemblies = GetReferenceAssemblies(), - }; - - test.ExpectedDiagnostics.AddRange(expected); + var test = CSharpAnalyzerTest.Create(source, expected); await test.RunAsync(CancellationToken.None); } diff --git a/src/Shared/RoslynUtils/WellKnownTypeData.cs b/src/Shared/RoslynUtils/WellKnownTypeData.cs index a64dd1a426e8..d05098327816 100644 --- a/src/Shared/RoslynUtils/WellKnownTypeData.cs +++ b/src/Shared/RoslynUtils/WellKnownTypeData.cs @@ -109,6 +109,7 @@ public enum WellKnownType Microsoft_AspNetCore_Mvc_SkipStatusCodePagesAttribute, Microsoft_AspNetCore_Mvc_ValidateAntiForgeryTokenAttribute, Microsoft_AspNetCore_Mvc_ModelBinding_EmptyBodyBehavior, + Microsoft_AspNetCore_Mvc_Routing_IRouteTemplateProvider, Microsoft_AspNetCore_Authorization_AllowAnonymousAttribute, Microsoft_AspNetCore_Authorization_AuthorizeAttribute, Microsoft_Extensions_DependencyInjection_PolicyServiceCollectionExtensions, @@ -233,6 +234,7 @@ public enum WellKnownType "Microsoft.AspNetCore.Mvc.SkipStatusCodePagesAttribute", "Microsoft.AspNetCore.Mvc.ValidateAntiForgeryTokenAttribute", "Microsoft.AspNetCore.Mvc.ModelBinding.EmptyBodyBehavior", + "Microsoft.AspNetCore.Mvc.Routing.IRouteTemplateProvider", "Microsoft.AspNetCore.Authorization.AllowAnonymousAttribute", "Microsoft.AspNetCore.Authorization.AuthorizeAttribute", "Microsoft.Extensions.DependencyInjection.PolicyServiceCollectionExtensions",