New rule S6934: You should specify the RouteAttribute when an HttpMet…

…hodAttribute is specified at an action level (#8954)

Co-authored-by: Costin Zaharia <costin.zaharia@sonarsource.com>

analyzers/rspec/cs/S6934.html

@@ -0,0 +1,108 @@
+<p>When a route template is defined through an attribute on an action method, conventional routing for that action is disabled. To maintain good
+practice, it’s recommended not to combine conventional and attribute-based routing within a single controller to avoid unpredicted behavior. As such,
+the controller should exclude itself from conventional routing by applying a <code>[Route]</code> attribute.</p>
+<h2>Why is this an issue?</h2>
+<p>In <a href="https://learn.microsoft.com/en-us/aspnet/core/mvc/overview">ASP.NET Core MVC</a>, the <a
+href="https://learn.microsoft.com/en-us/aspnet/core/mvc/controllers/routing">routing</a> middleware utilizes a series of rules and conventions to
+identify the appropriate controller and action method to handle a specific HTTP request. This process, known as <em>conventional routing</em>, is
+generally established using the <a
+href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.controllerendpointroutebuilderextensions.mapcontrollerroute"><code>MapControllerRoute</code></a>
+method. This method is typically configured in one central location for all controllers during the application setup.</p>
+<p>Conversely, <em>attribute routing</em> allows routes to be defined at the controller or action method level. It is possible to <a
+href="https://learn.microsoft.com/en-us/aspnet/core/mvc/controllers/routing#mixed-routing-attribute-routing-vs-conventional-routing">mix both
+mechanisms</a>. Although it’s permissible to employ diverse routing strategies across multiple controllers, combining both mechanisms within one
+controller can result in confusion and increased complexity, as illustrated below.</p>
+<pre>
+// Conventional mapping definition
+app.MapControllerRoute(
+    name: "default",
+    pattern: "{controller=Home}/{action=Index}/{id?}");
+
+public class PersonController
+{
+        // Conventional routing:
+        // Matches e.g. /Person/Index/123
+        public IActionResult Index(int? id) =&gt; View();
+
+        // Attribute routing:
+        // Matches e.g. /Age/Ascending (and model binds "Age" to sortBy and "Ascending" to direction)
+        // but does not match /Person/List/Age/Ascending
+        [HttpGet(template: "{sortBy}/{direction}")]
+        public IActionResult List(string sortBy, SortOrder direction) =&gt; View();
+}
+</pre>
+<h2>How to fix it in ASP.NET Core</h2>
+<p>When any of the controller actions are annotated with a <a
+href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.routing.httpmethodattribute"><code>HttpMethodAttribute</code></a> with a
+route template defined, you should specify a route template on the controller with the <a
+href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.routeattribute"><code>RouteAttribute</code></a> as well.</p>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+public class PersonController: Controller
+{
+    // Matches /Person/Index/123
+    public IActionResult Index(int? id) =&gt; View();
+
+    // Matches /Age/Ascending
+    [HttpGet(template: "{sortBy}/{direction}")] // Noncompliant: The "Index" and the "List" actions are
+                                                // reachable via different routing mechanisms and routes
+    public IActionResult List(string sortBy, SortOrder direction) =&gt; View();
+}
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="1" data-diff-type="compliant">
+[Route("[controller]/{action=Index}")]
+public class PersonController: Controller
+{
+    // Matches /Person/Index/123
+    [Route("{id?}")]
+    public IActionResult Index(int? id) =&gt; View();
+
+    // Matches Person/List/Age/Ascending
+    [HttpGet("{sortBy}/{direction}")] // Compliant: The route is relative to the controller
+    public IActionResult List(string sortBy, SortOrder direction) =&gt; View();
+}
+</pre>
+<p>There are also alternative options to prevent the mixing of conventional and attribute-based routing:</p>
+<pre>
+// Option 1. Replace the attribute-based routing with a conventional route
+app.MapControllerRoute(
+    name: "Lists",
+    pattern: "{controller}/List/{sortBy}/{direction}",
+    defaults: new { action = "List" } ); // Matches Person/List/Age/Ascending
+
+// Option 2. Use a binding, that does not depend on route templates
+public class PersonController: Controller
+{
+    // Matches Person/List?sortBy=Age&amp;direction=Ascending
+    [HttpGet] // Compliant: Parameters are bound from the query string
+    public IActionResult List(string sortBy, SortOrder direction) =&gt; View();
+}
+
+// Option 3. Use an absolute route
+public class PersonController: Controller
+{
+    // Matches Person/List/Age/Ascending
+    [HttpGet("/[controller]/[action]/{sortBy}/{direction}")] // Illustrate the expected route by starting with "/"
+    public IActionResult List(string sortBy, SortOrder direction) =&gt; View();
+}
+</pre>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/aspnet/core/mvc/overview">Overview of ASP.NET Core MVC</a> </li>
+  <li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/aspnet/core/mvc/controllers/routing">Routing to controller actions in ASP.NET
+  Core</a> </li>
+  <li> Microsoft Learn - <a
+  href="https://learn.microsoft.com/en-us/aspnet/core/mvc/controllers/routing#mixed-routing-attribute-routing-vs-conventional-routing">Mixed routing:
+  Attribute routing vs conventional routing</a> </li>
+  <li> Microsoft Learn - <a
+  href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.routing.httpmethodattribute">HttpMethodAttribute Class</a> </li>
+  <li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.routeattribute">RouteAttribute Class</a> </li>
+</ul>
+<h3>Articles &amp; blog posts</h3>
+<ul>
+  <li> Medium - <a href="https://medium.com/quick-code/routing-in-asp-net-core-c433bff3f1a4">Routing in ASP.NET Core</a> </li>
+</ul>
+

analyzers/rspec/cs/S6934.json

@@ -0,0 +1,23 @@
+{
+  "title": "A Route attribute should be added to the controller when a route template is specified at the action level",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [
+    "asp.net"
+  ],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-6934",
+  "sqKey": "S6934",
+  "scope": "Main",
+  "quickfix": "partial",
+  "code": {
+    "impacts": {
+      "MAINTAINABILITY": "HIGH"
+    },
+    "attribute": "CLEAR"
+  }
+}

analyzers/rspec/cs/Sonar_way_profile.json

@@ -315,6 +315,7 @@
     "S6800",
     "S6803",
     "S6930",
-    "S6931"
+    "S6931",
+    "S6934"
   ]
 }

analyzers/src/SonarAnalyzer.CSharp/Rules/SpecifyRouteAttribute.cs

@@ -0,0 +1,90 @@
+/*
+ * SonarAnalyzer for .NET
+ * Copyright (C) 2015-2024 SonarSource SA
+ * mailto: contact AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+using System.Collections.Concurrent;
+
+namespace SonarAnalyzer.Rules.CSharp;
+
+[DiagnosticAnalyzer(LanguageNames.CSharp)]
+public sealed class SpecifyRouteAttribute() : SonarDiagnosticAnalyzer<SyntaxKind>(DiagnosticId)
+{
+    private const string DiagnosticId = "S6934";
+
+    private static readonly ImmutableArray<KnownType> RouteTemplateAttributes = ImmutableArray.Create(
+        KnownType.Microsoft_AspNetCore_Mvc_Routing_HttpMethodAttribute,
+        KnownType.Microsoft_AspNetCore_Mvc_RouteAttribute);
+
+    protected override string MessageFormat => "Specify the RouteAttribute when an HttpMethodAttribute or RouteAttribute is specified at an action level.";
+    protected override ILanguageFacade<SyntaxKind> Language => CSharpFacade.Instance;
+
+    protected override void Initialize(SonarAnalysisContext context) =>
+        context.RegisterCompilationStartAction(compilationStart =>
+        {
+            if (!UsesAttributeRouting(compilationStart.Compilation))
+            {
+                return;
+            }
+            compilationStart.RegisterSymbolStartAction(symbolStart =>
+            {
+                if (symbolStart.Symbol.GetAttributes().Any(x => x.AttributeClass.Is(KnownType.Microsoft_AspNetCore_Mvc_RouteAttribute)))
+                {
+                    return;
+                }
+                var secondaryLocations = new ConcurrentStack<Location>();
+                symbolStart.RegisterSyntaxNodeAction(nodeContext =>
+                {
+                    var methodDeclaration = (MethodDeclarationSyntax)nodeContext.Node;
+                    if (nodeContext.SemanticModel.GetDeclaredSymbol(methodDeclaration, nodeContext.Cancel) is { } method
+                        && method.IsControllerMethod()
+                        && method.GetAttributes().Any(x => !CanBeIgnored(x.GetAttributeRouteTemplate(RouteTemplateAttributes))))
+                    {
+                        secondaryLocations.Push(methodDeclaration.Identifier.GetLocation());
+                    }
+                }, SyntaxKind.MethodDeclaration);
+                symbolStart.RegisterSymbolEndAction(symbolEnd => ReportIssues(symbolEnd, symbolStart.Symbol, secondaryLocations));
+            }, SymbolKind.NamedType);
+        });
+
+    private void ReportIssues(SonarSymbolReportingContext context, ISymbol symbol, ConcurrentStack<Location> secondaryLocations)
+    {
+        if (secondaryLocations.IsEmpty)
+        {
+            return;
+        }
+
+        foreach (var declaration in symbol.DeclaringSyntaxReferences.Select(x => x.GetSyntax()))
+        {
+            if (declaration.GetIdentifier() is { } identifier)
+            {
+                context.ReportIssue(CSharpGeneratedCodeRecognizer.Instance, Diagnostic.Create(Rule, identifier.GetLocation(), secondaryLocations));
+            }
+        }
+    }
+
+    private static bool UsesAttributeRouting(Compilation compilation) =>
+        compilation.GetTypeByMetadataName(KnownType.Microsoft_AspNetCore_Mvc_Routing_HttpMethodAttribute) is not null;
+
+    private static bool CanBeIgnored(string template) =>
+        string.IsNullOrEmpty(template)
+        // See: https://learn.microsoft.com/en-us/aspnet/core/mvc/controllers/routing#combining-attribute-routes
+        // Route templates applied to an action that begin with / or ~/ don't get combined with route templates applied to the controller.
+        || template.StartsWith("/")
+        || template.StartsWith("~/");
+}

analyzers/src/SonarAnalyzer.Common/Extensions/AttributeDataExtensions.cs

@@ -28,6 +28,12 @@ public static bool HasName(this AttributeData attribute, string name) =>
         public static bool HasAnyName(this AttributeData attribute, params string[] names) =>
             names.Any(x => attribute.HasName(x));
 
+        public static string GetAttributeRouteTemplate(this AttributeData attribute, ImmutableArray<KnownType> attributeTypes) =>
+            attribute.AttributeClass.DerivesFromAny(attributeTypes)
+            && attribute.TryGetAttributeValue<string>("template", out var template)
+                ? template
+                : null;
+
         public static bool TryGetAttributeValue<T>(this AttributeData attribute, string valueName, out T value)
         {
             // named arguments take precedence over constructor arguments of the same name. For [Attr(valueName: false, valueName = true)] "true" is returned.

analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs

@@ -72,6 +72,13 @@ public sealed partial class KnownType
         public static readonly KnownType Microsoft_AspNetCore_Mvc_ControllerAttribute = new("Microsoft.AspNetCore.Mvc.ControllerAttribute");
         public static readonly KnownType Microsoft_AspNetCore_Mvc_DisableRequestSizeLimitAttribute = new("Microsoft.AspNetCore.Mvc.DisableRequestSizeLimitAttribute");
         public static readonly KnownType Microsoft_AspNetCore_Mvc_FromServicesAttribute = new("Microsoft.AspNetCore.Mvc.FromServicesAttribute");
+        public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpDeleteAttribute = new("Microsoft.AspNetCore.Mvc.HttpDeleteAttribute");
+        public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpGetAttribute = new("Microsoft.AspNetCore.Mvc.HttpGetAttribute");
+        public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpHeadAttribute = new("Microsoft.AspNetCore.Mvc.HttpHeadAttribute");
+        public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpOptionsAttribute = new("Microsoft.AspNetCore.Mvc.HttpOptionsAttribute");
+        public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpPatchAttribute = new("Microsoft.AspNetCore.Mvc.HttpPatchAttribute");
+        public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpPostAttribute = new("Microsoft.AspNetCore.Mvc.HttpPostAttribute");
+        public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpPutAttribute = new("Microsoft.AspNetCore.Mvc.HttpPutAttribute");
         public static readonly KnownType Microsoft_AspNetCore_Mvc_IActionResult = new("Microsoft.AspNetCore.Mvc.IActionResult");
         public static readonly KnownType Microsoft_AspNetCore_Mvc_IgnoreAntiforgeryTokenAttribute = new("Microsoft.AspNetCore.Mvc.IgnoreAntiforgeryTokenAttribute");
         public static readonly KnownType Microsoft_AspNetCore_Mvc_NonActionAttribute = new("Microsoft.AspNetCore.Mvc.NonActionAttribute");

analyzers/src/SonarAnalyzer.Common/Rules/RouteTemplateShouldNotStartWithSlashBase.cs

@@ -22,15 +22,19 @@
 
 namespace SonarAnalyzer.Rules;
 
-public abstract class RouteTemplateShouldNotStartWithSlashBase<TSyntaxKind> : SonarDiagnosticAnalyzer<TSyntaxKind>
+public abstract class RouteTemplateShouldNotStartWithSlashBase<TSyntaxKind>() : SonarDiagnosticAnalyzer<TSyntaxKind>(DiagnosticId)
     where TSyntaxKind : struct
 {
     private const string DiagnosticId = "S6931";
     private const string MessageOnlyActions = "Change the paths of the actions of this controller to be relative and adapt the controller route accordingly.";
     private const string MessageActionsAndController = "Change the paths of the actions of this controller to be relative and add a controller route with the common prefix.";
 
+    private static readonly ImmutableArray<KnownType> RouteTemplateAttributes = ImmutableArray.Create(
+        KnownType.Microsoft_AspNetCore_Mvc_Routing_HttpMethodAttribute,
+        KnownType.Microsoft_AspNetCore_Mvc_RouteAttribute,
+        KnownType.System_Web_Mvc_RouteAttribute);
+
     protected override string MessageFormat => "{0}";
-    protected RouteTemplateShouldNotStartWithSlashBase() : base(DiagnosticId) { }
 
     protected override void Initialize(SonarAnalysisContext context) =>
         context.RegisterCompilationStartAction(compilationStartContext =>
@@ -91,10 +95,9 @@ SyntaxNode ControllerDeclarationSyntax(INamedTypeSymbol symbol) =>
     private static Dictionary<Location, string> RouteAttributeTemplateArguments(ImmutableArray<AttributeData> attributes)
     {
         var templates = new Dictionary<Location, string>();
-        var routeAttributes = attributes.Where(x => x.AttributeClass.IsAny(KnownType.RouteAttributes) || x.AttributeClass.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_Routing_HttpMethodAttribute));
-        foreach (var attribute in routeAttributes)
+        foreach (var attribute in attributes)
         {
-            if (attribute.TryGetAttributeValue<string>("template", out var templateParameter))
+            if (attribute.GetAttributeRouteTemplate(RouteTemplateAttributes) is { } templateParameter)
             {
                 templates.Add(attribute.ApplicationSyntaxReference.GetSyntax().GetLocation(), templateParameter);
             }

analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs

@@ -6858,7 +6858,7 @@ internal static class RuleTypeMappingCS
         ["S6931"] = "CODE_SMELL",
         // ["S6932"],
         // ["S6933"],
-        // ["S6934"],
+        ["S6934"] = "CODE_SMELL",
         // ["S6935"],
         // ["S6936"],
         // ["S6937"],

analyzers/tests/SonarAnalyzer.Test/Rules/SpecifyRouteAttributeTest.cs

@@ -0,0 +1,86 @@
+/*
+ * SonarAnalyzer for .NET
+ * Copyright (C) 2015-2024 SonarSource SA
+ * mailto: contact AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+#if NET
+
+using SonarAnalyzer.Rules.CSharp;
+
+namespace SonarAnalyzer.Test.Rules;
+
+[TestClass]
+public class SpecifyRouteAttributeTest
+{
+    private readonly VerifierBuilder builder = new VerifierBuilder<SpecifyRouteAttribute>()
+        .WithOptions(ParseOptionsHelper.FromCSharp12)
+        .AddReferences([
+            AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcCore,
+            AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcViewFeatures,
+            AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcAbstractions
+        ]);
+
+    [TestMethod]
+    public void SpecifyRouteAttribute_CSharp12() =>
+        builder.AddPaths("SpecifyRouteAttribute.CSharp12.cs").Verify();
+
+    [TestMethod]
+    public void SpecifyRouteAttribute_PartialClasses_CSharp12() =>
+        builder
+            .AddSnippet("""
+                using Microsoft.AspNetCore.Mvc;
+
+                public partial class HomeController : Controller       // Noncompliant [first]
+                {
+                    [HttpGet("Test")]
+                    public IActionResult Index() => View();            // Secondary [first, second]
+                }
+                """)
+            .AddSnippet("""
+                using Microsoft.AspNetCore.Mvc;
+
+                public partial class HomeController : Controller { }   // Noncompliant [second]
+                """)
+            .Verify();
+
+    [TestMethod]
+    public void SpecifyRouteAttribute_PartialClasses_OneGenerated_CSharp12() =>
+        builder
+            .AddSnippet("""
+                // <auto-generated/>
+                using Microsoft.AspNetCore.Mvc;
+
+                public partial class HomeController : Controller
+                {
+                    [HttpGet("Test")]
+                    public IActionResult ActionInGeneratedCode() => View();     // Secondary
+                }
+                """)
+            .AddSnippet("""
+                using Microsoft.AspNetCore.Mvc;
+
+                public partial class HomeController : Controller                // Noncompliant
+                {
+                    [HttpGet("Test")]
+                    public IActionResult Index() => View();                     // Secondary
+                }
+                """)
+            .Verify();
+}
+
+#endif