Skip to content

Commit

Permalink
New rule S6934: You should specify the RouteAttribute when an HttpMet…
Browse files Browse the repository at this point in the history
…hodAttribute is specified at an action level (#8954)

Co-authored-by: Costin Zaharia <costin.zaharia@sonarsource.com>
  • Loading branch information
1 parent 0400e52 commit 2d909a7
Show file tree
Hide file tree
Showing 10 changed files with 523 additions and 7 deletions.
108 changes: 108 additions & 0 deletions analyzers/rspec/cs/S6934.html
Original file line number Diff line number Diff line change
@@ -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>

23 changes: 23 additions & 0 deletions analyzers/rspec/cs/S6934.json
Original file line number Diff line number Diff line change
@@ -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"
}
}
3 changes: 2 additions & 1 deletion analyzers/rspec/cs/Sonar_way_profile.json
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@
"S6800",
"S6803",
"S6930",
"S6931"
"S6931",
"S6934"
]
}
90 changes: 90 additions & 0 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/SpecifyRouteAttribute.cs
Original file line number Diff line number Diff line change
@@ -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("~/");
}
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 7 additions & 0 deletions analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6858,7 +6858,7 @@ internal static class RuleTypeMappingCS
["S6931"] = "CODE_SMELL",
// ["S6932"],
// ["S6933"],
// ["S6934"],
["S6934"] = "CODE_SMELL",
// ["S6935"],
// ["S6936"],
// ["S6937"],
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Loading

0 comments on commit 2d909a7

Please sign in to comment.