Merge pull request #9406 from JarLob/controller

Extend aspnetcore controller definition

Author: michaelnebel

csharp/ql/lib/change-notes/2022-08-24-aps-net-core-controllers.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* ASP.NET Core controller definition has been made more precise. The amount of introduced taint sources or eliminated false positives should be low though, since the most common pattern is to derive all user defined ASP.NET Core controllers from the standard Controller class, which is not affected. 

csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/AspNetCore.qll

@@ -62,6 +62,13 @@ class MicrosoftAspNetCoreMvcNonActionAttribute extends MicrosoftAspNetCoreMvcAtt
   MicrosoftAspNetCoreMvcNonActionAttribute() { this.getType().hasName("NonActionAttribute") }
 }
 
+/** A `Microsoft.AspNetCore.Mvc.NonController` attribute. */
+class MicrosoftAspNetCoreMvcNonControllerAttribute extends MicrosoftAspNetCoreMvcAttribute {
+  MicrosoftAspNetCoreMvcNonControllerAttribute() {
+    this.getType().hasName("NonControllerAttribute")
+  }
+}
+
 /** The `Microsoft.AspNetCore.Antiforgery` namespace. */
 class MicrosoftAspNetCoreAntiforgeryNamespace extends Namespace {
   MicrosoftAspNetCoreAntiforgeryNamespace() {
@@ -182,10 +189,38 @@ class MicrosoftAspNetCoreMvcControllerBaseClass extends Class {
   }
 }
 
-/** A subtype of `Microsoft.AspNetCore.Mvc.Controller` or `Microsoft.AspNetCore.Mvc.ControllerBase`. */
+/**
+ * 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() {
-    this.getABaseType*() instanceof MicrosoftAspNetCoreMvcControllerBaseClass
+    (
+      exists(Assembly a |
+        a.getName() = ["Microsoft.AspNetCore.Mvc.Core", "Microsoft.AspNetCore.Mvc.ViewFeatures"]
+      ) or
+      exists(UsingNamespaceDirective ns |
+        ns.getImportedNamespace() instanceof MicrosoftAspNetCoreMvcNamespace
+      )
+    ) and
+    this.isPublic() and
+    (not this.isAbstract() or this instanceof MicrosoftAspNetCoreMvcControllerBaseClass) and
+    not this instanceof Generic and
+    (
+      this.getABaseType*() instanceof MicrosoftAspNetCoreMvcControllerBaseClass
+      or
+      this.getABaseType*().getName().matches("%Controller")
+      or
+      this.getABaseType*()
+          .getAnAttribute()
+          .getType()
+          .getABaseType*()
+          // ApiControllerAttribute is derived from ControllerAttribute
+          .hasQualifiedName("Microsoft.AspNetCore.Mvc.ControllerAttribute")
+    ) and
+    not this.getABaseType*().getAnAttribute() instanceof
+      MicrosoftAspNetCoreMvcNonControllerAttribute
   }
 
   /** Gets an action method for this controller. */

csharp/ql/test/library-tests/frameworks/microsoft/AspNetCore.cs

@@ -0,0 +1,103 @@
+using Microsoft.AspNetCore.Mvc;
+
+// has sufix "Controller"
+public class HomeController
+{
+    public string Index()
+    {
+        return "This is Home Controller";
+    }
+}
+
+// derived from Microsoft.AspNetCore.Mvc.Controller which has suffix "Controller"
+public class HomeController1 : Controller
+{
+    public string Index()
+    {
+        return "This is Home Controller";
+    }
+}
+
+// derived from Microsoft.AspNetCore.Mvc.ControllerBase which has attribute [Microsoft.AspNetCore.Mvc.Controller]
+public class HomeController2 : ControllerBase
+{
+    public string Index()
+    {
+        return "This is Home Controller";
+    }
+}
+
+// has [ApiController] attribute
+[ApiController]
+public class HomeController3
+{
+    public string Index()
+    {
+        return "This is Home Controller";
+    }
+}
+
+// has [Controller] attribute
+[Controller]
+public class HomeController4
+{
+    public string Index()
+    {
+        return "This is Home Controller";
+    }
+}
+
+// derived from a class that is a controller
+public class HomeController5 : HomeController4
+{
+    public string Index()
+    {
+        return "This is Home Controller";
+    }
+}
+
+// is not public
+internal class NotHomeController : Controller
+{
+    public string Index()
+    {
+        return "This is Home Controller";
+    }
+}
+
+// is abstract
+public abstract class NotHomeController2 : Controller
+{
+    public string Index()
+    {
+        return "This is Home Controller";
+    }
+}
+
+// contains generic parameters
+public class NotHomeController3<T> : Controller
+{
+    public string Index()
+    {
+        return "This is Home Controller";
+    }
+}
+
+// has [NonController] attribute
+[NonController]
+public class NotHomeController4 : Controller
+{
+    public string Index()
+    {
+        return "This is Home Controller";
+    }
+}
+
+// derived from a class that has [NonController] attribute
+public class NotController : NotHomeController4
+{
+    public string Index()
+    {
+        return "This is Home Controller";
+    }
+}

csharp/ql/test/library-tests/frameworks/microsoft/AspNetCore.expected

@@ -0,0 +1,6 @@
+| 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 |

csharp/ql/test/library-tests/frameworks/microsoft/AspNetCore.ql

@@ -0,0 +1,6 @@
+import csharp
+import semmle.code.csharp.frameworks.microsoft.AspNetCore
+
+from MicrosoftAspNetCoreMvcController c
+where c.fromSource()
+select c

csharp/ql/test/library-tests/frameworks/microsoft/options

@@ -0,0 +1,2 @@
+semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resources/stubs/_frameworks/Microsoft.AspNetCore.App/Microsoft.AspNetCore.App.csproj
+semmle-extractor-options: /nostdlib /noconfig