diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/AttributeWebApp/AttributePocoController.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/AttributeWebApp/AttributePocoController.cs new file mode 100644 index 000000000000..f4427cc21c48 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/AttributeWebApp/AttributePocoController.cs @@ -0,0 +1,8 @@ +using Microsoft.AspNetCore.Mvc; + +[Route("api/attribute")] +public class AttributePocoController +{ + [HttpGet] + public void Action(string input) => _ = GetType().Name + input; +} \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/AttributeWebApp/AttributeWebApp.csproj b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/AttributeWebApp/AttributeWebApp.csproj new file mode 100644 index 000000000000..bddc74e0c784 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/AttributeWebApp/AttributeWebApp.csproj @@ -0,0 +1,5 @@ + + + net10.0 + + \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/AttributeWebApp/ConventionOnlyController.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/AttributeWebApp/ConventionOnlyController.cs new file mode 100644 index 000000000000..f1222ad5f181 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/AttributeWebApp/ConventionOnlyController.cs @@ -0,0 +1,4 @@ +public class ConventionOnlyController +{ + public void Action(string input) => _ = GetType().Name + input; +} \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/AttributeWebApp/Program.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/AttributeWebApp/Program.cs new file mode 100644 index 000000000000..8d74291b16a8 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/AttributeWebApp/Program.cs @@ -0,0 +1,9 @@ +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); +builder.Services.AddControllers(); + +var app = builder.Build(); +app.MapControllers(); +app.Run(); \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/Controllers.expected b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/Controllers.expected new file mode 100644 index 000000000000..d6ee73a2694f --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/Controllers.expected @@ -0,0 +1,7 @@ +| AdminAreaFallbackController.cs | AreaFallbackController | +| AttributePocoController.cs | AttributePocoController | +| FallbackOnlyController.cs | FallbackOnlyController | +| GeneratedController.cs | GeneratedController | +| IncludedController.cs | IncludedController | +| StructuralController.cs | StructuralController | +| WebPocoController.cs | WebPocoController | diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/Controllers.ql b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/Controllers.ql new file mode 100644 index 000000000000..325f55434f5f --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/Controllers.ql @@ -0,0 +1,6 @@ +import csharp +import semmle.code.csharp.frameworks.microsoft.AspNetCore + +from MicrosoftAspNetCoreMvcController controller +where controller.fromSource() +select controller.getFile().getBaseName(), controller.getName() diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/GeneratedControllers/GeneratedController.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/GeneratedControllers/GeneratedController.cs new file mode 100644 index 000000000000..0fe26a40d27b --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/GeneratedControllers/GeneratedController.cs @@ -0,0 +1,6 @@ +namespace GeneratedControllers; + +public class GeneratedController +{ + public void Action(string input) => _ = GetType().Name + input; +} \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/GeneratedControllers/GeneratedControllers.csproj b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/GeneratedControllers/GeneratedControllers.csproj new file mode 100644 index 000000000000..10f1ac4e07e4 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/GeneratedControllers/GeneratedControllers.csproj @@ -0,0 +1,5 @@ + + + net10.0 + + \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/IncludedControllers/IncludedController.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/IncludedControllers/IncludedController.cs new file mode 100644 index 000000000000..04704dd9b064 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/IncludedControllers/IncludedController.cs @@ -0,0 +1,6 @@ +namespace IncludedControllers; + +public class IncludedController +{ + public void Action(string input) => _ = GetType().Name + input; +} \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/IncludedControllers/IncludedControllers.csproj b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/IncludedControllers/IncludedControllers.csproj new file mode 100644 index 000000000000..10f1ac4e07e4 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/IncludedControllers/IncludedControllers.csproj @@ -0,0 +1,5 @@ + + + net10.0 + + \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/AdminAreaFallbackController.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/AdminAreaFallbackController.cs new file mode 100644 index 000000000000..c64aea73ddd1 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/AdminAreaFallbackController.cs @@ -0,0 +1,9 @@ +using Microsoft.AspNetCore.Mvc; + +namespace AdminArea; + +[Area("Admin")] +public class AreaFallbackController +{ + public void Index(string input) => _ = GetType().Name + input; +} diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/FallbackOnlyController.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/FallbackOnlyController.cs new file mode 100644 index 000000000000..1716d49f2296 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/FallbackOnlyController.cs @@ -0,0 +1,4 @@ +public class FallbackOnlyController +{ + public void Index(string input) => _ = GetType().Name + input; +} diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/OtherAreaFallbackController.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/OtherAreaFallbackController.cs new file mode 100644 index 000000000000..10849896f531 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/OtherAreaFallbackController.cs @@ -0,0 +1,9 @@ +using Microsoft.AspNetCore.Mvc; + +namespace OtherArea; + +[Area("Other")] +public class AreaFallbackController +{ + public void Index(string input) => _ = GetType().Name + input; +} diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/Program.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/Program.cs new file mode 100644 index 000000000000..bb27696d84b5 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/Program.cs @@ -0,0 +1,10 @@ +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); +builder.Services.AddControllers(); + +var app = builder.Build(); +app.MapFallbackToController("Index", "FallbackOnly"); +app.MapFallbackToAreaController("admin/{*path:nonfile}", "Index", "AreaFallback", "Admin"); +app.Run(); diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/StructuralController.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/StructuralController.cs new file mode 100644 index 000000000000..dd9e5e2534b5 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/StructuralController.cs @@ -0,0 +1,6 @@ +using Microsoft.AspNetCore.Mvc; + +public class StructuralController : ControllerBase +{ + public void Action(string input) => _ = GetType().Name + input; +} \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/UnmappedPocoController.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/UnmappedPocoController.cs new file mode 100644 index 000000000000..99514ceb2c3f --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/UnmappedPocoController.cs @@ -0,0 +1,4 @@ +public class UnmappedPocoController +{ + public void Action(string input) => _ = GetType().Name + input; +} \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/UnmappedWebApp.csproj b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/UnmappedWebApp.csproj new file mode 100644 index 000000000000..bddc74e0c784 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/UnmappedWebApp/UnmappedWebApp.csproj @@ -0,0 +1,5 @@ + + + net10.0 + + \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/Unrelated/ThrottlingController.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/Unrelated/ThrottlingController.cs new file mode 100644 index 000000000000..d4277061e9fa --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/Unrelated/ThrottlingController.cs @@ -0,0 +1,4 @@ +public class ThrottlingController +{ + public void Initialize(string input) => _ = GetType().Name + input; +} \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/Unrelated/Unrelated.csproj b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/Unrelated/Unrelated.csproj new file mode 100644 index 000000000000..10f1ac4e07e4 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/Unrelated/Unrelated.csproj @@ -0,0 +1,5 @@ + + + net10.0 + + \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/WebApp/ApplicationParts.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/WebApp/ApplicationParts.cs new file mode 100644 index 000000000000..a7622ef5f9be --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/WebApp/ApplicationParts.cs @@ -0,0 +1,3 @@ +using Microsoft.AspNetCore.Mvc.ApplicationParts; + +[assembly: ApplicationPart("GeneratedControllers")] \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/WebApp/Program.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/WebApp/Program.cs new file mode 100644 index 000000000000..a58be59888d9 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/WebApp/Program.cs @@ -0,0 +1,12 @@ +using IncludedControllers; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); +builder.Services + .AddControllers() + .AddApplicationPart(typeof(IncludedController).Assembly); + +var app = builder.Build(); +app.MapControllerRoute("default", "{controller}/{action}"); +app.Run(); diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/WebApp/WebApp.csproj b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/WebApp/WebApp.csproj new file mode 100644 index 000000000000..56bc24531254 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/WebApp/WebApp.csproj @@ -0,0 +1,9 @@ + + + net10.0 + + + + + + \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/WebApp/WebPocoController.cs b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/WebApp/WebPocoController.cs new file mode 100644 index 000000000000..338ebd5abe67 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/WebApp/WebPocoController.cs @@ -0,0 +1,4 @@ +public class WebPocoController +{ + public void Action(string input) => _ = GetType().Name + input; +} \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/global.json b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/global.json new file mode 100644 index 000000000000..7307ac0a4729 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/global.json @@ -0,0 +1,5 @@ +{ + "sdk": { + "version": "10.0.201" + } +} \ No newline at end of file diff --git a/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/test.py b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/test.py new file mode 100644 index 000000000000..8e705bd10c06 --- /dev/null +++ b/csharp/ql/integration-tests/all-platforms/aspnetcore_controller_discovery/test.py @@ -0,0 +1,9 @@ +def test(codeql, csharp): + codeql.database.create( + command=[ + "dotnet build -t:Rebuild WebApp/WebApp.csproj", + "dotnet build -t:Rebuild AttributeWebApp/AttributeWebApp.csproj", + "dotnet build -t:Rebuild UnmappedWebApp/UnmappedWebApp.csproj", + "dotnet build -t:Rebuild Unrelated/Unrelated.csproj", + ] + ) diff --git a/csharp/ql/lib/change-notes/2026-07-31-aspnet-core-controller-discovery.md b/csharp/ql/lib/change-notes/2026-07-31-aspnet-core-controller-discovery.md new file mode 100644 index 000000000000..a55f736a1e84 --- /dev/null +++ b/csharp/ql/lib/change-notes/2026-07-31-aspnet-core-controller-discovery.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Improved ASP.NET Core MVC controller and action discovery to more closely match runtime behavior, including application parts, endpoint mappings, inherited actions, and controller and action exclusions. Service-injected action parameters are no longer modeled as remote input. diff --git a/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/AspNetCore.qll b/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/AspNetCore.qll index 350465052d1a..d3c4c0bf6aba 100644 --- a/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/AspNetCore.qll +++ b/csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/AspNetCore.qll @@ -2,6 +2,8 @@ import csharp import semmle.code.csharp.frameworks.Microsoft +private import semmle.code.csharp.commons.Compilation +private import semmle.code.csharp.frameworks.System /** The `Microsoft.AspNetCore` namespace. */ class MicrosoftAspNetCoreNamespace extends Namespace { @@ -189,57 +191,248 @@ class MicrosoftAspNetCoreMvcControllerBaseClass extends Class { } } -/** - * A valid ASP.NET Core controller according to: - * https://docs.microsoft.com/en-us/aspnet/core/mvc/controllers/actions?view=aspnetcore-3.1 - * https://github.com/dotnet/aspnetcore/blob/b3c93967ba508b8ef139add27132d9483c1a9eb4/src/Mvc/Mvc.Core/src/Controllers/ControllerFeatureProvider.cs#L39-L75 - */ -class MicrosoftAspNetCoreMvcController extends Class { - MicrosoftAspNetCoreMvcController() { +private predicate isPotentialMicrosoftAspNetCoreMvcController(Class controller) { + controller = + any(Class c | + ( + exists(Assembly a | + a.getName() = ["Microsoft.AspNetCore.Mvc.Core", "Microsoft.AspNetCore.Mvc.ViewFeatures"] + ) or + exists(UsingNamespaceDirective ns | + ns.getImportedNamespace() instanceof MicrosoftAspNetCoreMvcNamespace + ) + ) and + c.isPublic() and + not c instanceof Generic and + ( + c.getABaseType*() instanceof MicrosoftAspNetCoreMvcControllerBaseClass + or + c.getABaseType*().getName().matches("%Controller") + or + c.getABaseType*() + .getAnAttribute() + .getType() + .getABaseType*() + // ApiControllerAttribute is derived from ControllerAttribute + .hasFullyQualifiedName("Microsoft.AspNetCore.Mvc", "ControllerAttribute") + ) and + not c.getABaseType*().getAnAttribute() instanceof MicrosoftAspNetCoreMvcNonControllerAttribute + ) +} + +private Compilation getACompilationFor(Element element) { + result.getAFileCompiled() = element.getFile() +} + +private Assembly getAnAssemblyFor(Type type) { + result = getACompilationFor(type).getOutputAssembly() +} + +private predicate isMicrosoftAspNetCoreMvcRegistration(MethodCall call) { + call.getTarget() + .hasFullyQualifiedName("Microsoft.Extensions.DependencyInjection", + ["MvcServiceCollectionExtensions", "MvcCoreServiceCollectionExtensions"], + ["AddControllers", "AddControllersWithViews", "AddMvc", "AddMvcCore"]) +} + +private predicate isMicrosoftAspNetCoreMvcApplication(Compilation compilation) { + exists(MethodCall registration | + isMicrosoftAspNetCoreMvcRegistration(registration) and + compilation.getAFileCompiled() = registration.getFile() + ) +} + +private predicate isMicrosoftAspNetCoreMvcAddApplicationPart(MethodCall call) { + call.getTarget() + .hasFullyQualifiedName("Microsoft.Extensions.DependencyInjection", + ["MvcCoreMvcBuilderExtensions", "MvcCoreMvcCoreBuilderExtensions"], "AddApplicationPart") +} + +private predicate isMicrosoftAspNetCoreMvcApplicationPart(Compilation application, Assembly part) { + part = application.getOutputAssembly() + or + exists(AssemblyAttribute attr, StringLiteral assemblyName | + application.getAFileCompiled() = attr.getFile() and + attr.getType() + .hasFullyQualifiedName("Microsoft.AspNetCore.Mvc.ApplicationParts", + "ApplicationPartAttribute") and + assemblyName = attr.getArgument(0) and + assemblyName.getValue() = part.getName() + ) + or + exists(MethodCall addPart, PropertyAccess assemblyAccess, TypeofExpr typeOf, Type partType | + application.getAFileCompiled() = addPart.getFile() and + isMicrosoftAspNetCoreMvcAddApplicationPart(addPart) and + assemblyAccess = addPart.getArgumentForName("assembly") and + assemblyAccess.getTarget().hasName("Assembly") and + assemblyAccess.getQualifier() = typeOf and + partType = typeOf.getTypeAccess().getTarget() and + part = getAnAssemblyFor(partType) + ) +} + +private predicate isInMicrosoftAspNetCoreMvcApplication(Class controller, Compilation application) { + isMicrosoftAspNetCoreMvcApplication(application) and + isMicrosoftAspNetCoreMvcApplicationPart(application, getAnAssemblyFor(controller)) +} + +private predicate hasMicrosoftAspNetCoreMvcAttributeRoute(Class controller) { + exists(Attribute attr | ( - exists(Assembly a | - a.getName() = ["Microsoft.AspNetCore.Mvc.Core", "Microsoft.AspNetCore.Mvc.ViewFeatures"] - ) or - exists(UsingNamespaceDirective ns | - ns.getImportedNamespace() instanceof MicrosoftAspNetCoreMvcNamespace + attr = controller.getABaseType*().getAnAttribute() + or + exists(Method method | + controller.hasMember(method) and + attr = method.getOverridee*().getAnAttribute() ) ) and - this.isPublic() and - not this instanceof Generic and + attr.getType() + .getABaseType*() + .getABaseInterface*() + .hasFullyQualifiedName("Microsoft.AspNetCore.Mvc.Routing", "IRouteTemplateProvider") + ) +} + +private predicate isMicrosoftAspNetCoreMvcConventionalEndpointMapping(MethodCall call) { + call.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Builder", + "ControllerEndpointRouteBuilderExtensions", + [ + "MapAreaControllerRoute", "MapControllerRoute", "MapDefaultControllerRoute", + "MapDynamicControllerRoute" + ]) + or + call.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Builder", "MvcApplicationBuilderExtensions", + "UseMvcWithDefaultRoute") + or + call.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Builder", "MvcApplicationBuilderExtensions", + "UseMvc") and + exists(call.getArgumentForName("configureRoutes")) + or + call.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Builder", "MvcAreaRouteBuilderExtensions", + "MapAreaRoute") +} + +private predicate isMicrosoftAspNetCoreMvcAttributeEndpointMapping(MethodCall call) { + call.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Builder", + "ControllerEndpointRouteBuilderExtensions", "MapControllers") + or + call.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Builder", "MvcApplicationBuilderExtensions", + "UseMvc") and + not exists(call.getArgumentForName("configureRoutes")) +} + +private predicate isMicrosoftAspNetCoreMvcFallbackEndpointMapping(MethodCall call, Class controller) { + call.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Builder", + "ControllerEndpointRouteBuilderExtensions", + ["MapFallbackToAreaController", "MapFallbackToController"]) and + call.getArgumentForName("controller").getValue().toLowerCase() + "controller" = + controller.getName().toLowerCase() and + ( + call.getTarget().hasName("MapFallbackToController") + or + call.getTarget().hasName("MapFallbackToAreaController") and + exists(Attribute area | + area = controller.getABaseType*().getAnAttribute() and + area.getType().hasFullyQualifiedName("Microsoft.AspNetCore.Mvc", "AreaAttribute") and + area.getArgument(0).getValue().toLowerCase() = + call.getArgumentForName("area").getValue().toLowerCase() + ) + ) +} + +private predicate hasMicrosoftAspNetCoreMvcEndpointMapping(Compilation application, Class controller) { + isInMicrosoftAspNetCoreMvcApplication(controller, application) and + exists(MethodCall mapping | + application.getAFileCompiled() = mapping.getFile() and ( - this.getABaseType*() instanceof MicrosoftAspNetCoreMvcControllerBaseClass + isMicrosoftAspNetCoreMvcConventionalEndpointMapping(mapping) or - this.getABaseType*().getName().matches("%Controller") + isMicrosoftAspNetCoreMvcAttributeEndpointMapping(mapping) and + hasMicrosoftAspNetCoreMvcAttributeRoute(controller) or - this.getABaseType*() - .getAnAttribute() - .getType() - .getABaseType*() - // ApiControllerAttribute is derived from ControllerAttribute - .hasFullyQualifiedName("Microsoft.AspNetCore.Mvc", "ControllerAttribute") - ) and - not this.getABaseType*().getAnAttribute() instanceof - MicrosoftAspNetCoreMvcNonControllerAttribute + isMicrosoftAspNetCoreMvcFallbackEndpointMapping(mapping, controller) + ) + ) +} + +private predicate hasMicrosoftAspNetCoreMvcControllerIdentity(Class controller) { + controller.getABaseType*() instanceof MicrosoftAspNetCoreMvcControllerBaseClass + or + controller + .getABaseType*() + .getAnAttribute() + .getType() + .getABaseType*() + .hasFullyQualifiedName("Microsoft.AspNetCore.Mvc", "ControllerAttribute") +} + +private predicate isDefaultMicrosoftAspNetCoreMvcController(Class controller) { + controller instanceof NonNestedType and + controller.isPublic() and + not controller.isAbstract() and + not controller instanceof Generic and + ( + hasMicrosoftAspNetCoreMvcControllerIdentity(controller) + or + controller.getName().toLowerCase().matches("%controller") and + hasMicrosoftAspNetCoreMvcEndpointMapping(_, controller) + ) and + not controller.getABaseType*().getAnAttribute() instanceof + MicrosoftAspNetCoreMvcNonControllerAttribute +} + +/** A class treated as an owner of ASP.NET Core MVC controller helper methods. */ +class MicrosoftAspNetCoreMvcControllerHelperClass extends Class { + MicrosoftAspNetCoreMvcControllerHelperClass() { + isPotentialMicrosoftAspNetCoreMvcController(this) + } + + /** Gets a `Redirect*`, `Accepted*`, or `Created*` method. */ + Method getAResponseMethod() { + result = this.getAMethod() and + ( + result.getName().matches("Redirect%") + or + result.getName().matches("Accepted%") + or + result.getName().matches("Created%") + ) } +} + +/** + * An ASP.NET Core MVC controller, as described by: + * https://learn.microsoft.com/en-us/aspnet/core/mvc/controllers/actions + * https://learn.microsoft.com/en-us/aspnet/core/mvc/advanced/app-parts + * https://learn.microsoft.com/en-us/aspnet/core/mvc/controllers/routing + * https://github.com/dotnet/aspnetcore/blob/747d2cdb584079a0c7309115979f13c331fb7df7/src/Mvc/Mvc.Core/src/Controllers/ControllerFeatureProvider.cs#L20-L75 + * https://github.com/dotnet/aspnetcore/blob/747d2cdb584079a0c7309115979f13c331fb7df7/src/Mvc/Mvc.Core/src/ApplicationModels/DefaultApplicationModelProvider.cs#L410-L459 + */ +class MicrosoftAspNetCoreMvcController extends Class { + MicrosoftAspNetCoreMvcController() { isDefaultMicrosoftAspNetCoreMvcController(this) } /** Gets an action method for this controller. */ Method getAnActionMethod() { - result = this.getAMethod() and + this.hasMember(result) and result.isPublic() and not result.isStatic() and - not result.getAnAttribute() instanceof MicrosoftAspNetCoreMvcNonActionAttribute + not result.isAbstract() and + not result.getUnboundDeclaration() instanceof UnboundGenericMethod and + not result.getOverridee*().getAnAttribute() instanceof MicrosoftAspNetCoreMvcNonActionAttribute and + not result.getOverridee*().getDeclaringType() instanceof ObjectType and + not result instanceof DisposeMethod } - /** Gets a `Redirect*` method. */ + /** Gets a `Redirect*`, `Accepted*`, or `Created*` method. */ Method getARedirectMethod() { - result = this.getAMethod() and - ( - result.getName().matches("Redirect%") - or - result.getName().matches("Accepted%") - or - result.getName().matches("Created%") - ) + result = this.(MicrosoftAspNetCoreMvcControllerHelperClass).getAResponseMethod() } } diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll index bad6c990fa7b..1e53fccaaea1 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll @@ -251,7 +251,8 @@ class AspNetCoreRedirectSink extends Sink { AspNetCoreRedirectSink() { exists(MethodCall mc | mc.getTarget() = any(MicrosoftAspNetCoreHttpHttpResponse response).getRedirectMethod() or - mc.getTarget() = any(MicrosoftAspNetCoreMvcController response).getARedirectMethod() + mc.getTarget() = + any(MicrosoftAspNetCoreMvcControllerHelperClass response).getAResponseMethod() | // Response.Redirect uses 'location' parameter this.getExpr() = mc.getArgumentForName("location") diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll index 68c06a1828de..d0358f6803fc 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll @@ -306,7 +306,19 @@ class AspNetCoreActionMethodParameter extends AspNetCoreRemoteFlowSource, DataFl AspNetCoreActionMethodParameter() { exists(Parameter p | p = this.getParameter() and - p.fromSource() + p.fromSource() and + not exists(Attribute attr, ValueOrRefType attributeBase | attr = p.getAnAttribute() | + attributeBase = attr.getType().getABaseType*() and + ( + attributeBase + .getABaseInterface*() + .hasFullyQualifiedName("Microsoft.AspNetCore.Http.Metadata", "IFromServiceMetadata") + or + attributeBase + .hasFullyQualifiedName("Microsoft.Extensions.DependencyInjection", + "FromKeyedServicesAttribute") + ) + ) | p = any(MicrosoftAspNetCoreMvcController c).getAnActionMethod().getAParameter() ) diff --git a/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/AspRemoteFlowSource.cs b/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/AspRemoteFlowSource.cs index 5bc8025f231b..d7b085ef61fd 100644 --- a/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/AspRemoteFlowSource.cs +++ b/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/AspRemoteFlowSource.cs @@ -1,5 +1,6 @@ using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.DependencyInjection; using System; namespace Testing @@ -21,6 +22,27 @@ public object MyAction(ViewModel viewModel) { throw null; } + + public void BindingSources( + string ordinary, + [FromQuery] string fromQuery, + [FromBody] string fromBody, + [FromRoute] string fromRoute, + [FromHeader] string fromHeader, + [FromServices] IRequestService fromServices, + [CustomFromServices] IRequestService customFromServices, + [FromKeyedServices("cache")] IRequestService fromKeyedServices, + [CustomFromKeyedServices] IRequestService customFromKeyedServices) + { } + } + + public interface IRequestService { } + + public sealed class CustomFromServicesAttribute : FromServicesAttribute { } + + public sealed class CustomFromKeyedServicesAttribute : FromKeyedServicesAttribute + { + public CustomFromKeyedServicesAttribute() : base("custom") { } } public class Item diff --git a/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/aspRemoteFlowSource.expected b/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/aspRemoteFlowSource.expected index ef7a8f3cd784..6ba2741857b5 100644 --- a/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/aspRemoteFlowSource.expected +++ b/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/aspRemoteFlowSource.expected @@ -1,23 +1,27 @@ remoteFlowSourceMembers -| AspRemoteFlowSource.cs:10:23:10:31 | RequestId | -| AspRemoteFlowSource.cs:11:23:11:36 | RequestIdField | -| AspRemoteFlowSource.cs:28:23:28:29 | Tainted | +| AspRemoteFlowSource.cs:11:23:11:31 | RequestId | +| AspRemoteFlowSource.cs:12:23:12:36 | RequestIdField | +| AspRemoteFlowSource.cs:50:23:50:29 | Tainted | remoteFlowSources -| AspRemoteFlowSource.cs:20:42:20:50 | viewModel | -| AspRemoteFlowSource.cs:35:42:35:46 | param | -| AspRemoteFlowSource.cs:43:58:43:63 | newUrl | -| AspRemoteFlowSource.cs:44:61:44:65 | myApi | -| AspRemoteFlowSource.cs:44:75:44:79 | myUrl | -| AspRemoteFlowSource.cs:46:46:46:56 | lambdaParam | -| AspRemoteFlowSource.cs:52:65:52:76 | mapPostParam | -| AspRemoteFlowSource.cs:53:63:53:73 | mapPutParam | -| AspRemoteFlowSource.cs:54:69:54:82 | mapDeleteParam | -| AspRemoteFlowSource.cs:56:41:56:44 | item | -| AspRemoteFlowSource.cs:64:43:64:47 | param | -| AspRemoteFlowSource.cs:71:34:71:35 | id | -| AspRemoteFlowSource.cs:73:35:73:41 | command | -| AspRemoteFlowSource.cs:73:48:73:52 | count | -| AspRemoteFlowSource.cs:75:40:75:43 | data | -| AspRemoteFlowSource.cs:77:34:77:38 | value | -| AspRemoteFlowSource.cs:79:37:79:42 | itemId | -| AspRemoteFlowSource.cs:92:35:92:46 | derivedParam | +| AspRemoteFlowSource.cs:21:42:21:50 | viewModel | +| AspRemoteFlowSource.cs:27:20:27:27 | ordinary | +| AspRemoteFlowSource.cs:28:32:28:40 | fromQuery | +| AspRemoteFlowSource.cs:29:31:29:38 | fromBody | +| AspRemoteFlowSource.cs:30:32:30:40 | fromRoute | +| AspRemoteFlowSource.cs:31:33:31:42 | fromHeader | +| AspRemoteFlowSource.cs:57:42:57:46 | param | +| AspRemoteFlowSource.cs:65:58:65:63 | newUrl | +| AspRemoteFlowSource.cs:66:61:66:65 | myApi | +| AspRemoteFlowSource.cs:66:75:66:79 | myUrl | +| AspRemoteFlowSource.cs:68:46:68:56 | lambdaParam | +| AspRemoteFlowSource.cs:74:65:74:76 | mapPostParam | +| AspRemoteFlowSource.cs:75:63:75:73 | mapPutParam | +| AspRemoteFlowSource.cs:76:69:76:82 | mapDeleteParam | +| AspRemoteFlowSource.cs:78:41:78:44 | item | +| AspRemoteFlowSource.cs:93:34:93:35 | id | +| AspRemoteFlowSource.cs:95:35:95:41 | command | +| AspRemoteFlowSource.cs:95:48:95:52 | count | +| AspRemoteFlowSource.cs:97:40:97:43 | data | +| AspRemoteFlowSource.cs:99:34:99:38 | value | +| AspRemoteFlowSource.cs:101:37:101:42 | itemId | +| AspRemoteFlowSource.cs:114:35:114:46 | derivedParam | diff --git a/csharp/ql/test/library-tests/frameworks/microsoft/AspNetCore.cs b/csharp/ql/test/library-tests/frameworks/microsoft/AspNetCore.cs index bcf0c766f340..7e0c880a766e 100644 --- a/csharp/ql/test/library-tests/frameworks/microsoft/AspNetCore.cs +++ b/csharp/ql/test/library-tests/frameworks/microsoft/AspNetCore.cs @@ -1,4 +1,16 @@ using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.DependencyInjection; + +public static class MvcRegistration +{ + public static void Register(IServiceCollection services, IEndpointRouteBuilder endpoints) + { + services.AddControllers(); + endpoints.MapControllerRoute("default", "{controller}/{action}"); + } +} // has sufix "Controller" public class HomeController @@ -101,3 +113,86 @@ public string Index() return "This is Home Controller"; } } + +// has case-insensitive suffix "Controller" +public class LowerCasecontroller +{ + public void Action() { } +} + +// derives from ControllerBase, whose [Controller] attribute is inherited +public class Products : ControllerBase +{ + public void List() { } +} + +// is a nested type +public class ControllerContainer +{ + public class NestedController + { + public void Action() { } + } +} + +// only this base class has the Controller suffix +public class PlainController +{ + public void BaseAction() { } +} + +public class DerivedFromPlain : PlainController +{ + public void DerivedAction() { } +} + +// is a closed subclass of an open generic controller +public class GenericBaseController : ControllerBase +{ + public void GenericBaseAction(string input) { } +} + +public class ClosedGenericController : GenericBaseController +{ + public void ClosedAction(string input) { } +} + +[Controller] +public abstract class AbstractActionBase +{ + public abstract void AbstractAction(string input); + + public void InheritedAction(string input) { } + + [NonAction] + public virtual void InheritedNonAction(string input) { } +} + +public class ConcreteActionEndpoint : AbstractActionBase +{ + public override void AbstractAction(string input) { } + + public override void InheritedNonAction(string input) { } +} + +public class ActionCasesController : ControllerBase, System.IDisposable +{ + public void PublicAction(string input) { } + + public static void StaticAction(string input) { } + + public void GenericAction(T input) { } + + [NonAction] + public void ExplicitNonAction(string input) { } + + public override string ToString() => "action cases"; + + public void Dispose() { } + + protected void ProtectedAction(string input) { } + + internal void InternalAction(string input) { } + + private void PrivateAction(string input) { } +} diff --git a/csharp/ql/test/library-tests/frameworks/microsoft/AspNetCore.expected b/csharp/ql/test/library-tests/frameworks/microsoft/AspNetCore.expected index e9866698ccdc..444058a4add4 100644 --- a/csharp/ql/test/library-tests/frameworks/microsoft/AspNetCore.expected +++ b/csharp/ql/test/library-tests/frameworks/microsoft/AspNetCore.expected @@ -1,7 +1,12 @@ -| AspNetCore.cs:4:14:4:27 | HomeController | -| AspNetCore.cs:13:14:13:28 | HomeController1 | -| AspNetCore.cs:22:14:22:28 | HomeController2 | -| AspNetCore.cs:32:14:32:28 | HomeController3 | -| AspNetCore.cs:42:14:42:28 | HomeController4 | -| AspNetCore.cs:51:14:51:28 | HomeController5 | -| AspNetCore.cs:60:23:60:37 | HomeController6 | +| AspNetCore.cs:16:14:16:27 | HomeController | +| AspNetCore.cs:25:14:25:28 | HomeController1 | +| AspNetCore.cs:34:14:34:28 | HomeController2 | +| AspNetCore.cs:44:14:44:28 | HomeController3 | +| AspNetCore.cs:54:14:54:28 | HomeController4 | +| AspNetCore.cs:63:14:63:28 | HomeController5 | +| AspNetCore.cs:118:14:118:32 | LowerCasecontroller | +| AspNetCore.cs:124:14:124:21 | Products | +| AspNetCore.cs:139:14:139:28 | PlainController | +| AspNetCore.cs:155:14:155:36 | ClosedGenericController | +| AspNetCore.cs:171:14:171:35 | ConcreteActionEndpoint | +| AspNetCore.cs:178:14:178:34 | ActionCasesController | diff --git a/csharp/ql/test/library-tests/frameworks/microsoft/AspNetCoreActions.expected b/csharp/ql/test/library-tests/frameworks/microsoft/AspNetCoreActions.expected new file mode 100644 index 000000000000..c3222a0cbf4d --- /dev/null +++ b/csharp/ql/test/library-tests/frameworks/microsoft/AspNetCoreActions.expected @@ -0,0 +1,15 @@ +| AspNetCore.cs:16:14:16:27 | HomeController | AspNetCore.cs:18:19:18:23 | Index | +| AspNetCore.cs:25:14:25:28 | HomeController1 | AspNetCore.cs:27:19:27:23 | Index | +| AspNetCore.cs:34:14:34:28 | HomeController2 | AspNetCore.cs:36:19:36:23 | Index | +| AspNetCore.cs:44:14:44:28 | HomeController3 | AspNetCore.cs:46:19:46:23 | Index | +| AspNetCore.cs:54:14:54:28 | HomeController4 | AspNetCore.cs:56:19:56:23 | Index | +| AspNetCore.cs:63:14:63:28 | HomeController5 | AspNetCore.cs:56:19:56:23 | Index | +| AspNetCore.cs:63:14:63:28 | HomeController5 | AspNetCore.cs:65:19:65:23 | Index | +| AspNetCore.cs:118:14:118:32 | LowerCasecontroller | AspNetCore.cs:120:17:120:22 | Action | +| AspNetCore.cs:124:14:124:21 | Products | AspNetCore.cs:126:17:126:20 | List | +| AspNetCore.cs:139:14:139:28 | PlainController | AspNetCore.cs:141:17:141:26 | BaseAction | +| AspNetCore.cs:155:14:155:36 | ClosedGenericController | AspNetCore.cs:152:17:152:33 | GenericBaseAction | +| AspNetCore.cs:155:14:155:36 | ClosedGenericController | AspNetCore.cs:157:17:157:28 | ClosedAction | +| AspNetCore.cs:171:14:171:35 | ConcreteActionEndpoint | AspNetCore.cs:165:17:165:31 | InheritedAction | +| AspNetCore.cs:171:14:171:35 | ConcreteActionEndpoint | AspNetCore.cs:173:26:173:39 | AbstractAction | +| AspNetCore.cs:178:14:178:34 | ActionCasesController | AspNetCore.cs:180:17:180:28 | PublicAction | diff --git a/csharp/ql/test/library-tests/frameworks/microsoft/AspNetCoreActions.ql b/csharp/ql/test/library-tests/frameworks/microsoft/AspNetCoreActions.ql new file mode 100644 index 000000000000..f4e927cd0b07 --- /dev/null +++ b/csharp/ql/test/library-tests/frameworks/microsoft/AspNetCoreActions.ql @@ -0,0 +1,9 @@ +import csharp +import semmle.code.csharp.frameworks.microsoft.AspNetCore + +from MicrosoftAspNetCoreMvcController controller, Method action +where + controller.fromSource() and + action = controller.getAnActionMethod() and + action.fromSource() +select controller, action diff --git a/csharp/ql/test/query-tests/Security Features/CWE-089/SqlInjection.cs b/csharp/ql/test/query-tests/Security Features/CWE-089/SqlInjection.cs index e7dd2f732603..a35dddb5b662 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-089/SqlInjection.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-089/SqlInjection.cs @@ -139,4 +139,6 @@ public async Task GetUserById([FromRoute] string userId, Cancella return Ok(); } } + + public class ConcreteController : MyController { } }