From a510a7b4c0ac75fec829053372f02b473e4d1bae Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Tue, 1 Aug 2023 10:29:08 +0100
Subject: [PATCH 01/25] Add insecure direct object reference definitions  and
 factor out those from missing access control

---
 .../csharp/security/auth/ActionMethods.qll    | 107 ++++++++++++++++++
 .../InsecureDirectObjectRefrerenceQuery.qll   |  28 +++++
 ...MissingFunctionLevelAccessControlQuery.qll |  91 +--------------
 3 files changed, 136 insertions(+), 90 deletions(-)
 create mode 100644 csharp/ql/lib/semmle/code/csharp/security/auth/ActionMethods.qll
 create mode 100644 csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectRefrerenceQuery.qll

diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/ActionMethods.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/ActionMethods.qll
new file mode 100644
index 000000000000..b3d8dc2701bc
--- /dev/null
+++ b/csharp/ql/lib/semmle/code/csharp/security/auth/ActionMethods.qll
@@ -0,0 +1,107 @@
+/** Common definitions for queries checking for access control measures on action methods. */
+
+import csharp
+import semmle.code.csharp.frameworks.microsoft.AspNetCore
+import semmle.code.csharp.frameworks.system.web.UI
+
+/** A method representing an action for a web endpoint. */
+abstract class ActionMethod extends Method {
+  /**
+   * Gets a string that can indicate what this method does to determine if it should have an auth check;
+   * such as its method name, class name, or file path.
+   */
+  string getADescription() {
+    result = [this.getName(), this.getDeclaringType().getBaseClass*().getName(), this.getARoute()]
+  }
+
+  /** Holds if this method may represent a stateful action such as editing or deleting */
+  predicate isEdit() {
+    exists(string str |
+      str =
+        this.getADescription()
+            // separate camelCase words
+            .regexpReplaceAll("([a-z])([A-Z])", "$1_$2")
+            .toLowerCase() and
+      str.regexpMatch(".*(edit|delete|modify|change).*") and
+      not str.regexpMatch(".*(on_?change|changed).*")
+    )
+  }
+
+  /** Holds if this method may be intended to be restricted to admin users */
+  predicate isAdmin() {
+    this.getADescription()
+        // separate camelCase words
+        .regexpReplaceAll("([a-z])([A-Z])", "$1_$2")
+        .toLowerCase()
+        .regexpMatch(".*(admin|superuser).*")
+  }
+
+  /** Holds if this method may need an authorization check. */
+  predicate needsAuth() { this.isEdit() or this.isAdmin() }
+
+  /** Gets a callable for which if it contains an auth check, this method should be considered authenticated. */
+  Callable getAnAuthorizingCallable() { result = this }
+
+  /**
+   * Gets a possible url route that could refer to this action,
+   * which would be covered by `<location>` configurations specifying a prefix of it.
+   */
+  string getARoute() { result = this.getDeclaringType().getFile().getRelativePath() }
+}
+
+/** An action method in the MVC framework. */
+private class MvcActionMethod extends ActionMethod {
+  MvcActionMethod() { this = any(MicrosoftAspNetCoreMvcController c).getAnActionMethod() }
+}
+
+/** An action method on a subclass of `System.Web.UI.Page`. */
+private class WebFormActionMethod extends ActionMethod {
+  WebFormActionMethod() {
+    this.getDeclaringType().getBaseClass+() instanceof SystemWebUIPageClass and
+    this.getAParameter().getType().getName().matches("%EventArgs")
+  }
+
+  override Callable getAnAuthorizingCallable() {
+    result = super.getAnAuthorizingCallable()
+    or
+    result.getDeclaringType() = this.getDeclaringType() and
+    result.getName() = "Page_Load"
+  }
+
+  override string getARoute() {
+    exists(string physicalRoute | physicalRoute = super.getARoute() |
+      result = physicalRoute
+      or
+      exists(string absolutePhysical |
+        virtualRouteMapping(result, absolutePhysical) and
+        physicalRouteMatches(absolutePhysical, physicalRoute)
+      )
+    )
+  }
+}
+
+/**
+ * Holds if `virtualRoute` is a URL path
+ * that can map to the corresponding `physicalRoute` filepath
+ * through a call to `MapPageRoute`
+ */
+private predicate virtualRouteMapping(string virtualRoute, string physicalRoute) {
+  exists(MethodCall mapPageRouteCall, StringLiteral virtualLit, StringLiteral physicalLit |
+    mapPageRouteCall
+        .getTarget()
+        .hasQualifiedName("System.Web.Routing", "RouteCollection", "MapPageRoute") and
+    virtualLit = mapPageRouteCall.getArgument(1) and
+    physicalLit = mapPageRouteCall.getArgument(2) and
+    virtualLit.getValue() = virtualRoute and
+    physicalLit.getValue() = physicalRoute
+  )
+}
+
+/** Holds if the filepath `route` can refer to `actual` after expanding a '~". */
+bindingset[route, actual]
+private predicate physicalRouteMatches(string route, string actual) {
+  route = actual
+  or
+  route.charAt(0) = "~" and
+  exists(string dir | actual = dir + route.suffix(1) + ".cs")
+}
diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectRefrerenceQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectRefrerenceQuery.qll
new file mode 100644
index 000000000000..1596f98f7142
--- /dev/null
+++ b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectRefrerenceQuery.qll
@@ -0,0 +1,28 @@
+/** Definitions for the Insecure Direct Object Reference query */
+
+import csharp
+import semmle.code.csharp.dataflow.flowsources.Remote
+import ActionMethods
+
+private predicate needsChecks(ActionMethod m) { m.isEdit() and not m.isAdmin() }
+
+private predicate hasIdParameter(ActionMethod m) {
+  exists(RemoteFlowSource src | src.getEnclosingCallable() = m |
+    src.asParameter().getName().toLowerCase().matches("%id")
+    or
+    exists(StringLiteral idStr |
+      idStr.getValue().toLowerCase().matches("%id") and
+      idStr.getParent*() = src.asExpr()
+    )
+  )
+}
+
+private predicate checksUser(ActionMethod m) {
+  exists(Callable c | c.getName().toLowerCase().matches("%user%") | m.calls*(c))
+}
+
+predicate hasInsecureDirectObjectReference(ActionMethod m) {
+  needsChecks(m) and
+  hasIdParameter(m) and
+  not checksUser(m)
+}
diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll
index 7623cb6b9f75..75fd0194ec41 100644
--- a/csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll
+++ b/csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll
@@ -4,96 +4,7 @@ import csharp
 import semmle.code.csharp.frameworks.microsoft.AspNetCore
 import semmle.code.csharp.frameworks.system.web.UI
 import semmle.code.asp.WebConfig
-
-/** A method representing an action for a web endpoint. */
-abstract class ActionMethod extends Method {
-  /**
-   * Gets a string that can indicate what this method does to determine if it should have an auth check;
-   * such as its method name, class name, or file path.
-   */
-  string getADescription() {
-    result =
-      [
-        this.getName(), this.getDeclaringType().getBaseClass*().getName(),
-        this.getDeclaringType().getFile().getRelativePath()
-      ]
-  }
-
-  /** Holds if this method may need an authorization check. */
-  predicate needsAuth() {
-    this.getADescription()
-        .regexpReplaceAll("([a-z])([A-Z])", "$1_$2")
-        // separate camelCase words
-        .toLowerCase()
-        .regexpMatch(".*(edit|delete|modify|admin|superuser).*")
-  }
-
-  /** Gets a callable for which if it contains an auth check, this method should be considered authenticated. */
-  Callable getAnAuthorizingCallable() { result = this }
-
-  /**
-   * Gets a possible url route that could refer to this action,
-   * which would be covered by `<location>` configurations specifying a prefix of it.
-   */
-  string getARoute() { result = this.getDeclaringType().getFile().getRelativePath() }
-}
-
-/** An action method in the MVC framework. */
-private class MvcActionMethod extends ActionMethod {
-  MvcActionMethod() { this = any(MicrosoftAspNetCoreMvcController c).getAnActionMethod() }
-}
-
-/** An action method on a subclass of `System.Web.UI.Page`. */
-private class WebFormActionMethod extends ActionMethod {
-  WebFormActionMethod() {
-    this.getDeclaringType().getBaseClass+() instanceof SystemWebUIPageClass and
-    this.getAParameter().getType().getName().matches("%EventArgs")
-  }
-
-  override Callable getAnAuthorizingCallable() {
-    result = super.getAnAuthorizingCallable()
-    or
-    result.getDeclaringType() = this.getDeclaringType() and
-    result.getName() = "Page_Load"
-  }
-
-  override string getARoute() {
-    exists(string physicalRoute | physicalRoute = super.getARoute() |
-      result = physicalRoute
-      or
-      exists(string absolutePhysical |
-        virtualRouteMapping(result, absolutePhysical) and
-        physicalRouteMatches(absolutePhysical, physicalRoute)
-      )
-    )
-  }
-}
-
-/**
- * Holds if `virtualRoute` is a URL path
- * that can map to the corresponding `physicalRoute` filepath
- * through a call to `MapPageRoute`
- */
-private predicate virtualRouteMapping(string virtualRoute, string physicalRoute) {
-  exists(MethodCall mapPageRouteCall, StringLiteral virtualLit, StringLiteral physicalLit |
-    mapPageRouteCall
-        .getTarget()
-        .hasQualifiedName("System.Web.Routing", "RouteCollection", "MapPageRoute") and
-    virtualLit = mapPageRouteCall.getArgument(1) and
-    physicalLit = mapPageRouteCall.getArgument(2) and
-    virtualLit.getValue() = virtualRoute and
-    physicalLit.getValue() = physicalRoute
-  )
-}
-
-/** Holds if the filepath `route` can refer to `actual` after expanding a '~". */
-bindingset[route, actual]
-private predicate physicalRouteMatches(string route, string actual) {
-  route = actual
-  or
-  route.charAt(0) = "~" and
-  exists(string dir | actual = dir + route.suffix(1) + ".cs")
-}
+import ActionMethods
 
 /** An expression that indicates that some authorization/authentication check is being performed. */
 class AuthExpr extends Expr {

From 5d1289672b7b85b09ab4ef316cb3e827c4c2b2b0 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Thu, 3 Aug 2023 16:31:39 +0100
Subject: [PATCH 02/25] Add IDOR query

---
 .../InsecureDirectObjectRefrerenceQuery.qll   | 31 ++++++++++++++++---
 .../CWE-939/InsecureDirectObjectReference.ql  | 19 ++++++++++++
 2 files changed, 45 insertions(+), 5 deletions(-)
 create mode 100644 csharp/ql/src/Security Features/CWE-939/InsecureDirectObjectReference.ql

diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectRefrerenceQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectRefrerenceQuery.qll
index 1596f98f7142..c51a120ce85b 100644
--- a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectRefrerenceQuery.qll
+++ b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectRefrerenceQuery.qll
@@ -4,25 +4,46 @@ import csharp
 import semmle.code.csharp.dataflow.flowsources.Remote
 import ActionMethods
 
+/**
+ * Holds if `m` is a method that may require checks
+ * against the current user to modify a particular resource.
+ */
+// We exclude admin methods as it may be expected that an admin user should be able to modify any resource.
+// Other queries check that there are authorization checks in place for admin methods.
 private predicate needsChecks(ActionMethod m) { m.isEdit() and not m.isAdmin() }
 
+private Expr getParentExpr(Expr ex) { result = ex.getParent() }
+
+/**
+ * Holds if `m` has a parameter or access a remote flow source
+ * that may indicate that it's used as the ID for some resource
+ */
 private predicate hasIdParameter(ActionMethod m) {
   exists(RemoteFlowSource src | src.getEnclosingCallable() = m |
-    src.asParameter().getName().toLowerCase().matches("%id")
+    src.asParameter().getName().toLowerCase().matches(["%id", "%idx"])
     or
     exists(StringLiteral idStr |
-      idStr.getValue().toLowerCase().matches("%id") and
-      idStr.getParent*() = src.asExpr()
+      idStr.getValue().toLowerCase().matches(["%id", "%idx"]) and
+      getParentExpr*(src.asExpr()) = getParentExpr*(idStr)
     )
   )
 }
 
+/** Holds if `m` at some point in its call graph may make some kind of check against the current user. */
 private predicate checksUser(ActionMethod m) {
-  exists(Callable c | c.getName().toLowerCase().matches("%user%") | m.calls*(c))
+  exists(Property p | p.getName().toLowerCase().matches(["%user%", "%session%"]) |
+    m.calls*(p.getGetter())
+  )
 }
 
+/**
+ * Holds if `m` is a method that modifies a particular resource based on
+ * and ID provided by user input, but does not check anything based on the current user
+ * to determine if they should modify this resource.
+ */
 predicate hasInsecureDirectObjectReference(ActionMethod m) {
   needsChecks(m) and
   hasIdParameter(m) and
-  not checksUser(m)
+  not checksUser(m) and
+  exists(m.getBody())
 }
diff --git a/csharp/ql/src/Security Features/CWE-939/InsecureDirectObjectReference.ql b/csharp/ql/src/Security Features/CWE-939/InsecureDirectObjectReference.ql
new file mode 100644
index 000000000000..3ae3aade9ecb
--- /dev/null
+++ b/csharp/ql/src/Security Features/CWE-939/InsecureDirectObjectReference.ql	
@@ -0,0 +1,19 @@
+/**
+ * @name Insecure Direct Object Reference
+ * @description Using user input to control which object is modified without
+ *              proper authorization checks allows an attacker to modify arbitrary objects.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision medium
+ * @id cs/insecure-direct0object-reference
+ * @tags security
+ *       external/cwe/639
+ */
+
+import csharp
+import semmle.code.csharp.security.auth.InsecureDirectObjectRefrerenceQuery
+
+from ActionMethod m
+where hasInsecureDirectObjectReference(m)
+select m,
+  "This method may not verify which users should be able to access resources of the provided ID."

From 251f875304223047d8a097e4df8e7bf311a90873 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Thu, 3 Aug 2023 16:32:37 +0100
Subject: [PATCH 03/25] Fix filenme typo

---
 ...frerenceQuery.qll => InsecureDirectObjectReferenceQuery.qll} | 0
 .../Security Features/CWE-939/InsecureDirectObjectReference.ql  | 2 +-
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename csharp/ql/lib/semmle/code/csharp/security/auth/{InsecureDirectObjectRefrerenceQuery.qll => InsecureDirectObjectReferenceQuery.qll} (100%)

diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectRefrerenceQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
similarity index 100%
rename from csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectRefrerenceQuery.qll
rename to csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
diff --git a/csharp/ql/src/Security Features/CWE-939/InsecureDirectObjectReference.ql b/csharp/ql/src/Security Features/CWE-939/InsecureDirectObjectReference.ql
index 3ae3aade9ecb..70f76b8f37d2 100644
--- a/csharp/ql/src/Security Features/CWE-939/InsecureDirectObjectReference.ql	
+++ b/csharp/ql/src/Security Features/CWE-939/InsecureDirectObjectReference.ql	
@@ -11,7 +11,7 @@
  */
 
 import csharp
-import semmle.code.csharp.security.auth.InsecureDirectObjectRefrerenceQuery
+import semmle.code.csharp.security.auth.InsecureDirectObjectReferenceQuery
 
 from ActionMethod m
 where hasInsecureDirectObjectReference(m)

From 2edd73eb60770e33c35af3f5e3b520412d59c518 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Fri, 4 Aug 2023 16:01:01 +0100
Subject: [PATCH 04/25] Fix typos in filepath + metadata, add severity

---
 .../{CWE-939 => CWE-639}/InsecureDirectObjectReference.ql  | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
 rename csharp/ql/src/Security Features/{CWE-939 => CWE-639}/InsecureDirectObjectReference.ql (82%)

diff --git a/csharp/ql/src/Security Features/CWE-939/InsecureDirectObjectReference.ql b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql
similarity index 82%
rename from csharp/ql/src/Security Features/CWE-939/InsecureDirectObjectReference.ql
rename to csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql
index 70f76b8f37d2..35c8187a89c9 100644
--- a/csharp/ql/src/Security Features/CWE-939/InsecureDirectObjectReference.ql	
+++ b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql	
@@ -2,12 +2,13 @@
  * @name Insecure Direct Object Reference
  * @description Using user input to control which object is modified without
  *              proper authorization checks allows an attacker to modify arbitrary objects.
- * @kind path-problem
+ * @kind problem
  * @problem.severity error
+ * @security-severity 7.5
  * @precision medium
- * @id cs/insecure-direct0object-reference
+ * @id cs/insecure-direct-object-reference
  * @tags security
- *       external/cwe/639
+ *       external/cwe-639
  */
 
 import csharp

From 20d42dfd7d602440d41ea6db8dd814f97e82d98f Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Mon, 14 Aug 2023 21:01:19 +0100
Subject: [PATCH 05/25] Add tests for webforms case

---
 .../CWE-639/WebFormsTests/EditComment.aspx.cs | 28 +++++++++++++++++++
 .../InsecureDirectObjectReference.expected    |  1 +
 .../InsecureDirectObjectReference.qlref       |  1 +
 .../CWE-639/WebFormsTests/options             |  1 +
 csharp/ql/test/resources/stubs/System.Web.cs  |  1 +
 5 files changed, 32 insertions(+)
 create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/EditComment.aspx.cs
 create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.expected
 create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.qlref
 create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/options

diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/EditComment.aspx.cs b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/EditComment.aspx.cs
new file mode 100644
index 000000000000..974869a0af4b
--- /dev/null
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/EditComment.aspx.cs	
@@ -0,0 +1,28 @@
+using System;
+using System.Web.UI;
+
+class EditComment : System.Web.UI.Page {
+    
+    // BAD - Any user can access this method.
+    protected void btn1_Click(object sender, EventArgs e) {
+        string commentId = Request.QueryString["Id"];
+        Comment comment = getCommentById(commentId);
+        comment.Text = "xyz";
+    }
+
+    // GOOD - The user ID is verified.
+    protected void btn2_Click(object sender, EventArgs e) {
+        string commentId = Request.QueryString["Id"];
+        Comment comment = getCommentById(commentId);
+        if (comment.AuthorName == User.Identity.Name){
+            comment.Text = "xyz";
+        }
+    }
+
+    class Comment {
+        public string Text { get; set; }
+        public string AuthorName { get; }
+    }
+
+    Comment getCommentById(string id) { return null; }
+} 
\ No newline at end of file
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.expected b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.expected
new file mode 100644
index 000000000000..b1cd126cdb89
--- /dev/null
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.expected	
@@ -0,0 +1 @@
+| EditComment.aspx.cs:7:20:7:29 | btn1_Click | This method may not verify which users should be able to access resources of the provided ID. |
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.qlref b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.qlref
new file mode 100644
index 000000000000..4756d5a76a4f
--- /dev/null
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.qlref	
@@ -0,0 +1 @@
+Security Features/CWE-639/InsecureDirectObjectReference.ql
\ No newline at end of file
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/options b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/options
new file mode 100644
index 000000000000..23b1e1602633
--- /dev/null
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/options	
@@ -0,0 +1 @@
+semmle-extractor-options: /r:System.Collections.Specialized.dll ${testdir}/../../../../resources/stubs/System.Web.cs
\ No newline at end of file
diff --git a/csharp/ql/test/resources/stubs/System.Web.cs b/csharp/ql/test/resources/stubs/System.Web.cs
index c3c8a0a753fa..0f055fcc123b 100644
--- a/csharp/ql/test/resources/stubs/System.Web.cs
+++ b/csharp/ql/test/resources/stubs/System.Web.cs
@@ -82,6 +82,7 @@ public class Control
     public class Page
     {
         public System.Security.Principal.IPrincipal User { get; } 
+        public System.Web.HttpRequest Request { get; }
     }
 
     interface IPostBackDataHandler

From 009a7bfc87ccc38dcdcd37471fa74a1f5d11eb3d Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Thu, 17 Aug 2023 15:51:48 +0100
Subject: [PATCH 06/25] Add MVC tests

---
 .../CWE-639/MVCTests/CommentController.cs     | 22 +++++++++++++++++++
 .../InsecureDirectObjectReference.expected    |  1 +
 .../InsecureDirectObjectReference.qlref       |  1 +
 .../CWE-639/MVCTests/options                  |  3 +++
 4 files changed, 27 insertions(+)
 create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/CommentController.cs
 create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected
 create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.qlref
 create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/options

diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/CommentController.cs b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/CommentController.cs
new file mode 100644
index 000000000000..52d8bc4e5367
--- /dev/null
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/CommentController.cs	
@@ -0,0 +1,22 @@
+using Microsoft.AspNetCore.Mvc;
+using Microsoft.AspNetCore.Authorization;
+
+public class CommentController : Controller {
+    // BAD: Any user can access this.
+    public ActionResult Edit1(int commentId, string text) {
+        editComment(commentId, text);
+        return View();
+    }
+
+    // GOOD: The user's authorization is checked.
+    public ActionResult Edit2(int commentId, string text) {
+        if (canEditComment(commentId, User.Identity.Name)){
+            editComment(commentId, text);
+        }
+        return View();
+    }
+
+    void editComment(int commentId, string text) { }
+
+    bool canEditComment(int commentId, string userName) { return false; }
+}
\ No newline at end of file
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected
new file mode 100644
index 000000000000..7b74a0c41a6a
--- /dev/null
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected	
@@ -0,0 +1 @@
+| CommentController.cs:6:25:6:29 | Edit1 | This method may not verify which users should be able to access resources of the provided ID. |
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.qlref b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.qlref
new file mode 100644
index 000000000000..4756d5a76a4f
--- /dev/null
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.qlref	
@@ -0,0 +1 @@
+Security Features/CWE-639/InsecureDirectObjectReference.ql
\ No newline at end of file
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/options b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/options
new file mode 100644
index 000000000000..c69b0d035a69
--- /dev/null
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/options	
@@ -0,0 +1,3 @@
+semmle-extractor-options: /nostdlib /noconfig
+semmle-extractor-options: --load-sources-from-project:${testdir}/../../../../resources/stubs/_frameworks/Microsoft.NETCore.App/Microsoft.NETCore.App.csproj
+semmle-extractor-options: --load-sources-from-project:${testdir}/../../../../resources/stubs/_frameworks/Microsoft.AspNetCore.App/Microsoft.AspNetCore.App.csproj
\ No newline at end of file

From f8b1b3843852544d1fbae25e44bde3ab42e6a42f Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Thu, 17 Aug 2023 17:22:43 +0100
Subject: [PATCH 07/25] Update alert message and make user checks more precise

---
 .../security/auth/InsecureDirectObjectReferenceQuery.qll   | 7 +++++--
 .../CWE-639/InsecureDirectObjectReference.ql               | 2 +-
 .../MVCTests/InsecureDirectObjectReference.expected        | 2 +-
 .../WebFormsTests/InsecureDirectObjectReference.expected   | 2 +-
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
index c51a120ce85b..462af455af36 100644
--- a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
+++ b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
@@ -22,6 +22,7 @@ private predicate hasIdParameter(ActionMethod m) {
   exists(RemoteFlowSource src | src.getEnclosingCallable() = m |
     src.asParameter().getName().toLowerCase().matches(["%id", "%idx"])
     or
+    // handle cases like `Request.QueryString["Id"]`
     exists(StringLiteral idStr |
       idStr.getValue().toLowerCase().matches(["%id", "%idx"]) and
       getParentExpr*(src.asExpr()) = getParentExpr*(idStr)
@@ -31,8 +32,10 @@ private predicate hasIdParameter(ActionMethod m) {
 
 /** Holds if `m` at some point in its call graph may make some kind of check against the current user. */
 private predicate checksUser(ActionMethod m) {
-  exists(Property p | p.getName().toLowerCase().matches(["%user%", "%session%"]) |
-    m.calls*(p.getGetter())
+  exists(Callable c, string name | name = c.getName().toLowerCase() |
+    name.matches(["%user%", "%session%"]) and
+    not name.matches("%get%by%") and // methods like `getUserById` or `getXByUsername` aren't likely to be referring to the current user
+    m.calls*(c)
   )
 }
 
diff --git a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql
index 35c8187a89c9..885e909f741a 100644
--- a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql	
+++ b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql	
@@ -17,4 +17,4 @@ import semmle.code.csharp.security.auth.InsecureDirectObjectReferenceQuery
 from ActionMethod m
 where hasInsecureDirectObjectReference(m)
 select m,
-  "This method may not verify which users should be able to access resources of the provided ID."
+  "This method may be missing authorization checks for which users can access the resource of the provided ID."
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected
index 7b74a0c41a6a..30a52e475f46 100644
--- a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected	
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected	
@@ -1 +1 @@
-| CommentController.cs:6:25:6:29 | Edit1 | This method may not verify which users should be able to access resources of the provided ID. |
+| CommentController.cs:6:25:6:29 | Edit1 | This method may be missing authorization checks for which users can access the resource of the provided ID. |
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.expected b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.expected
index b1cd126cdb89..8cb9e542f311 100644
--- a/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.expected	
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.expected	
@@ -1 +1 @@
-| EditComment.aspx.cs:7:20:7:29 | btn1_Click | This method may not verify which users should be able to access resources of the provided ID. |
+| EditComment.aspx.cs:7:20:7:29 | btn1_Click | This method may be missing authorization checks for which users can access the resource of the provided ID. |

From 3e6750ba4cb580abf193eab827bee145f10803f8 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Mon, 21 Aug 2023 10:23:27 +0100
Subject: [PATCH 08/25] Add documentation

---
 .../InsecureDirectObjectReference.qhelp       | 30 +++++++++++++++++++
 .../CWE-639/WebFormsExample.cs                | 15 ++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp
 create mode 100644 csharp/ql/src/Security Features/CWE-639/WebFormsExample.cs

diff --git a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp
new file mode 100644
index 000000000000..fc31d45267d6
--- /dev/null
+++ b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp	
@@ -0,0 +1,30 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>When an action method accepts an ID parameter used to control which resource (e.g. a comment, a user profile, etc) 
+is being accessed/modified, checks should me made to ensure that the current user is authorized to access that resource. 
+Otherwise, an attacker could access an arbitrary resource by modifying the ID parameter.</p>
+
+</overview>
+<recommendation>
+<p>
+Ensure that the current user is authorized to access the resource of the provided ID.
+</p>
+
+</recommendation>
+<example>
+<p>In the following example, in the case marked BAD, there is no authorization check, so any user is able to edit any comment.
+In the case marked GOOD, there is a check that the current usr matches the author of the comment.</p>
+<sample src="WebFormsExample.cs" />
+
+
+</example>
+<references>
+
+  <li>OWASP - <a href="https://wiki.owasp.org/index.php/Top_10_2013-A4-Insecure_Direct_Object_References">Insecure Direct Object Refrences</a>.</li>
+  <li>OWASP - <a href="https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/05-Authorization_Testing/04-Testing_for_Insecure_Direct_Object_References">Testing for Insecure Direct Object References</a>.</li>
+
+</references>
+</qhelp>
\ No newline at end of file
diff --git a/csharp/ql/src/Security Features/CWE-639/WebFormsExample.cs b/csharp/ql/src/Security Features/CWE-639/WebFormsExample.cs
new file mode 100644
index 000000000000..48f870db451a
--- /dev/null
+++ b/csharp/ql/src/Security Features/CWE-639/WebFormsExample.cs	
@@ -0,0 +1,15 @@
+    // BAD - Any user can access this method.
+    protected void btn1_Click(object sender, EventArgs e) {
+        string commentId = Request.QueryString["Id"];
+        Comment comment = getCommentById(commentId);
+        comment.Body = inputCommentBody.Text;
+    }
+
+    // GOOD - The user ID is verified.
+    protected void btn2_Click(object sender, EventArgs e) {
+        string commentId = Request.QueryString["Id"];
+        Comment comment = getCommentById(commentId);
+        if (comment.AuthorName == User.Identity.Name){
+            comment.Body = inputCommentBody.Text;
+        }
+    }
\ No newline at end of file

From 4967fe0b7772ef8291b280f48768a00b338a79a4 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Mon, 21 Aug 2023 10:31:07 +0100
Subject: [PATCH 09/25] Add change note + update query ID

---
 .../CWE-639/InsecureDirectObjectReference.ql                  | 2 +-
 .../2023-08-21-insecure-direct-object-reference.md            | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)
 create mode 100644 csharp/ql/src/change-notes/2023-08-21-insecure-direct-object-reference.md

diff --git a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql
index 885e909f741a..85b6b56f7bc9 100644
--- a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql	
+++ b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql	
@@ -6,7 +6,7 @@
  * @problem.severity error
  * @security-severity 7.5
  * @precision medium
- * @id cs/insecure-direct-object-reference
+ * @id cs/web/insecure-direct-object-reference
  * @tags security
  *       external/cwe-639
  */
diff --git a/csharp/ql/src/change-notes/2023-08-21-insecure-direct-object-reference.md b/csharp/ql/src/change-notes/2023-08-21-insecure-direct-object-reference.md
new file mode 100644
index 000000000000..edbb11347390
--- /dev/null
+++ b/csharp/ql/src/change-notes/2023-08-21-insecure-direct-object-reference.md
@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `cs/web/insecure-direct-object-reference`, to find instances of missing authorization checks for resources selected by an ID parameter.
\ No newline at end of file

From 9f25c71ca679dd352476d10398a9b7a33061a532 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Thu, 24 Aug 2023 11:02:21 +0100
Subject: [PATCH 10/25] Apply minor reveiw suggstions

---
 .../security/auth/InsecureDirectObjectReferenceQuery.qll      | 4 ++--
 .../CWE-639/InsecureDirectObjectReference.qhelp               | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
index 462af455af36..3d2db41a83ca 100644
--- a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
+++ b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
@@ -12,7 +12,7 @@ import ActionMethods
 // Other queries check that there are authorization checks in place for admin methods.
 private predicate needsChecks(ActionMethod m) { m.isEdit() and not m.isAdmin() }
 
-private Expr getParentExpr(Expr ex) { result = ex.getParent() }
+private Expr getParentExpr(Expr ex) { result.getAChildExpr() = ex }
 
 /**
  * Holds if `m` has a parameter or access a remote flow source
@@ -41,7 +41,7 @@ private predicate checksUser(ActionMethod m) {
 
 /**
  * Holds if `m` is a method that modifies a particular resource based on
- * and ID provided by user input, but does not check anything based on the current user
+ * an ID provided by user input, but does not check anything based on the current user
  * to determine if they should modify this resource.
  */
 predicate hasInsecureDirectObjectReference(ActionMethod m) {
diff --git a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp
index fc31d45267d6..8e82ac6f1bb6 100644
--- a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp	
+++ b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp	
@@ -5,7 +5,7 @@
 <overview>
 <p>When an action method accepts an ID parameter used to control which resource (e.g. a comment, a user profile, etc) 
 is being accessed/modified, checks should me made to ensure that the current user is authorized to access that resource. 
-Otherwise, an attacker could access an arbitrary resource by modifying the ID parameter.</p>
+Otherwise, an attacker could access an arbitrary resource by guessing the ID parameter.</p>
 
 </overview>
 <recommendation>

From 86abd338e5cd62de4e1e1a0c2ffebb64e62db283 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Thu, 24 Aug 2023 11:06:04 +0100
Subject: [PATCH 11/25] Update test options

---
 .../Security Features/CWE-639/WebFormsTests/options           | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/options b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/options
index 23b1e1602633..a5d7077ef37a 100644
--- a/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/options	
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/options	
@@ -1 +1,3 @@
-semmle-extractor-options: /r:System.Collections.Specialized.dll ${testdir}/../../../../resources/stubs/System.Web.cs
\ No newline at end of file
+semmle-extractor-options: /nostdlib /noconfig
+semmle-extractor-options: --load-sources-from-project:${testdir}/../../../../resources/stubs/_frameworks/Microsoft.NETCore.App/Microsoft.NETCore.App.csproj
+semmle-extractor-options: ${testdir}/../../../../resources/stubs/System.Web.cs
\ No newline at end of file

From a022893f0fc635580fa0dfee360d0412c4949eb2 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Fri, 25 Aug 2023 10:48:40 +0100
Subject: [PATCH 12/25] Add additional example to qhelp + additional resource

---
 .../InsecureDirectObjectReference.qhelp       |  3 ++
 .../Security Features/CWE-639/MVCExample.cs   | 35 +++++++++++++++++++
 2 files changed, 38 insertions(+)
 create mode 100644 csharp/ql/src/Security Features/CWE-639/MVCExample.cs

diff --git a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp
index 8e82ac6f1bb6..23af52944c4c 100644
--- a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp	
+++ b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp	
@@ -18,6 +18,8 @@ Ensure that the current user is authorized to access the resource of the provide
 <p>In the following example, in the case marked BAD, there is no authorization check, so any user is able to edit any comment.
 In the case marked GOOD, there is a check that the current usr matches the author of the comment.</p>
 <sample src="WebFormsExample.cs" />
+<p>The following example shows a similar case for the ASP.NET Core framweork.</p>
+<sample src="MVCExample.cs" />
 
 
 </example>
@@ -25,6 +27,7 @@ In the case marked GOOD, there is a check that the current usr matches the autho
 
   <li>OWASP - <a href="https://wiki.owasp.org/index.php/Top_10_2013-A4-Insecure_Direct_Object_References">Insecure Direct Object Refrences</a>.</li>
   <li>OWASP - <a href="https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/05-Authorization_Testing/04-Testing_for_Insecure_Direct_Object_References">Testing for Insecure Direct Object References</a>.</li>
+  <li>Microsoft Learn = <a href="https://learn.microsoft.com/en-us/aspnet/core/security/authorization/resourcebased?view=aspnetcore-7.0">Resource-based authorization in ASP.NET Core</a>.</li>
 
 </references>
 </qhelp>
\ No newline at end of file
diff --git a/csharp/ql/src/Security Features/CWE-639/MVCExample.cs b/csharp/ql/src/Security Features/CWE-639/MVCExample.cs
new file mode 100644
index 000000000000..36e692309cba
--- /dev/null
+++ b/csharp/ql/src/Security Features/CWE-639/MVCExample.cs	
@@ -0,0 +1,35 @@
+public class CommentController : Controller {
+    private readonly IAuthorizationService _authorizationService;
+    private readonly IDocumentRepository _commentRepository;
+
+    public CommentController(IAuthorizationService authorizationService,
+                              ICommentRepository commentRepository)
+    {
+        _authorizationService = authorizationService;
+        _commentRepository = commentRepository;
+    }
+
+    // BAD: Any user can access this.
+    public async Task<IActionResult> Edit1(int commentId, string text) {
+        Comment comment = _commentRepository.Find(commentId);
+        
+        comment.Text = text;
+
+        return View();
+    }
+
+    // GOOD: An authorization check is made.
+    public async Task<IActionResult> Edit2(int commentId, string text) {
+        Comment comment = _commentRepository.Find(commentId);
+        
+        var authResult = await _authorizationService.AuthorizeAsync(User, Comment, "EditPolicy");
+
+        if (authResult.Succeeded) {
+            comment.Text = text;
+            return View();
+        }
+        else {
+            return ForbidResult();
+        }
+    }
+}
\ No newline at end of file

From 0a27da08d6801f5f83012c3e0faf03cc6f638740 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Fri, 25 Aug 2023 11:05:00 +0100
Subject: [PATCH 13/25] Minor changes from review suggestions to shared logic
 between this and missing access control Use case insensitive regex, factor
 out page load to improve possible bad joins make needsAuth not a member
 predicate

---
 .../csharp/security/auth/ActionMethods.qll    | 22 +++++++++----------
 ...MissingFunctionLevelAccessControlQuery.qll |  7 ++++--
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/ActionMethods.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/ActionMethods.qll
index b3d8dc2701bc..81ba6c70a06b 100644
--- a/csharp/ql/lib/semmle/code/csharp/security/auth/ActionMethods.qll
+++ b/csharp/ql/lib/semmle/code/csharp/security/auth/ActionMethods.qll
@@ -20,10 +20,9 @@ abstract class ActionMethod extends Method {
       str =
         this.getADescription()
             // separate camelCase words
-            .regexpReplaceAll("([a-z])([A-Z])", "$1_$2")
-            .toLowerCase() and
-      str.regexpMatch(".*(edit|delete|modify|change).*") and
-      not str.regexpMatch(".*(on_?change|changed).*")
+            .regexpReplaceAll("([a-z])([A-Z])", "$1_$2") and
+      str.regexpMatch("(?i).*(edit|delete|modify|change).*") and
+      not str.regexpMatch("(?i).*(on_?change|changed).*")
     )
   }
 
@@ -32,13 +31,9 @@ abstract class ActionMethod extends Method {
     this.getADescription()
         // separate camelCase words
         .regexpReplaceAll("([a-z])([A-Z])", "$1_$2")
-        .toLowerCase()
-        .regexpMatch(".*(admin|superuser).*")
+        .regexpMatch("(?i).*(admin|superuser).*")
   }
 
-  /** Holds if this method may need an authorization check. */
-  predicate needsAuth() { this.isEdit() or this.isAdmin() }
-
   /** Gets a callable for which if it contains an auth check, this method should be considered authenticated. */
   Callable getAnAuthorizingCallable() { result = this }
 
@@ -64,8 +59,7 @@ private class WebFormActionMethod extends ActionMethod {
   override Callable getAnAuthorizingCallable() {
     result = super.getAnAuthorizingCallable()
     or
-    result.getDeclaringType() = this.getDeclaringType() and
-    result.getName() = "Page_Load"
+    pageLoad(result, this.getDeclaringType())
   }
 
   override string getARoute() {
@@ -80,6 +74,12 @@ private class WebFormActionMethod extends ActionMethod {
   }
 }
 
+pragma[nomagic]
+private predicate pageLoad(Callable c, Type decl) {
+  c.getName() = "Page_Load" and
+  decl = c.getDeclaringType()
+}
+
 /**
  * Holds if `virtualRoute` is a URL path
  * that can map to the corresponding `physicalRoute` filepath
diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll
index 75fd0194ec41..79a39f093ee1 100644
--- a/csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll
+++ b/csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll
@@ -6,6 +6,9 @@ import semmle.code.csharp.frameworks.system.web.UI
 import semmle.code.asp.WebConfig
 import ActionMethods
 
+/** Holds if the method `m` may need an authorization check. */
+predicate needsAuth(ActionMethod m) { m.isEdit() or m.isAdmin() }
+
 /** An expression that indicates that some authorization/authentication check is being performed. */
 class AuthExpr extends Expr {
   AuthExpr() {
@@ -25,7 +28,7 @@ class AuthExpr extends Expr {
 
 /** Holds if `m` is a method that should have an auth check, and does indeed have one. */
 predicate hasAuthViaCode(ActionMethod m) {
-  m.needsAuth() and
+  needsAuth(m) and
   exists(Callable caller, AuthExpr auth |
     m.getAnAuthorizingCallable().calls*(caller) and
     auth.getEnclosingCallable() = caller
@@ -86,7 +89,7 @@ predicate hasAuthViaAttribute(ActionMethod m) {
 
 /** Holds if `m` is a method that should have an auth check, but is missing it. */
 predicate missingAuth(ActionMethod m) {
-  m.needsAuth() and
+  needsAuth(m) and
   not hasAuthViaCode(m) and
   not hasAuthViaXml(m) and
   not hasAuthViaAttribute(m) and

From ac450505457ad3fcee6a1c217db01ed5e7a0bc45 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Mon, 11 Sep 2023 15:16:29 +0100
Subject: [PATCH 14/25] Add checks for authorization attributes

---
 .../InsecureDirectObjectReferenceQuery.qll    | 33 ++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
index 3d2db41a83ca..05bb4ac92458 100644
--- a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
+++ b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
@@ -39,6 +39,36 @@ private predicate checksUser(ActionMethod m) {
   )
 }
 
+/** Holds if `m`, its containing class, or a parent class has an attribute that extends `AuthorizeAttribute` */
+private predicate hasAuthorizeAttribute(ActionMethod m) {
+  exists(Attribute attr |
+    attr.getType()
+        .getABaseType*()
+        .hasQualifiedName("Microsoft.AspNetCore.Authorization", "AuthorizeAttribute")
+  |
+    attr = m.getAnAttribute() or
+    attr = m.getDeclaringType().getABaseType*().getAnAttribute()
+  )
+}
+
+/** Holds if `m`, its containing class, or a parent class has an attribute that extends `AllowAnonymousAttribute` */
+private predicate hasAllowAnonymousAttribute(ActionMethod m) {
+  exists(Attribute attr |
+    attr.getType()
+        .getABaseType*()
+        .hasQualifiedName("Microsoft.AspNetCore.Authorization", "AllowAnonymousAttribute")
+  |
+    attr = m.getAnAttribute() or
+    attr = m.getDeclaringType().getABaseType*().getAnAttribute()
+  )
+}
+
+/** Hols if `m` is authorized via an `Authorize` attribute */
+private predicate isAuthorizedViaAttribute(ActionMethod m) {
+  hasAuthorizeAttribute(m) and
+  not hasAllowAnonymousAttribute(m)
+}
+
 /**
  * Holds if `m` is a method that modifies a particular resource based on
  * an ID provided by user input, but does not check anything based on the current user
@@ -48,5 +78,6 @@ predicate hasInsecureDirectObjectReference(ActionMethod m) {
   needsChecks(m) and
   hasIdParameter(m) and
   not checksUser(m) and
-  exists(m.getBody())
+  not isAuthorizedViaAttribute(m) and
+  exists(m.getBody().getAChildStmt())
 }

From 6a95ed64ff9621f4861f86d9200712a235a88196 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Wed, 13 Sep 2023 16:05:01 +0100
Subject: [PATCH 15/25] Add test cases for authorization from attributes

---
 .../CWE-639/MVCTests/CommentController.cs     | 15 ++++++++++++++
 .../InsecureDirectObjectReference.expected    |  2 ++
 .../CWE-639/MVCTests/ProfileController.cs     | 20 +++++++++++++++++++
 3 files changed, 37 insertions(+)
 create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/ProfileController.cs

diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/CommentController.cs b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/CommentController.cs
index 52d8bc4e5367..891e8374c1cc 100644
--- a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/CommentController.cs	
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/CommentController.cs	
@@ -16,6 +16,21 @@ public ActionResult Edit2(int commentId, string text) {
         return View();
     }
 
+    // GOOD: The Authorize attribute is used
+    [Authorize]
+    public ActionResult Edit3(int commentId, string text) {
+        editComment(commentId, text);
+        return View();
+    }
+
+    // BAD: The AllowAnonymous attribute overrides the Authorize attribute
+    [Authorize]
+    [AllowAnonymous]
+    public ActionResult Edit4(int commentId, string text) {
+        editComment(commentId, text);
+        return View();
+    }
+
     void editComment(int commentId, string text) { }
 
     bool canEditComment(int commentId, string userName) { return false; }
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected
index 30a52e475f46..2c6bc2b10592 100644
--- a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected	
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected	
@@ -1 +1,3 @@
 | CommentController.cs:6:25:6:29 | Edit1 | This method may be missing authorization checks for which users can access the resource of the provided ID. |
+| CommentController.cs:29:25:29:29 | Edit4 | This method may be missing authorization checks for which users can access the resource of the provided ID. |
+| ProfileController.cs:14:25:14:29 | Edit2 | This method may be missing authorization checks for which users can access the resource of the provided ID. |
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/ProfileController.cs b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/ProfileController.cs
new file mode 100644
index 000000000000..43eb93194797
--- /dev/null
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/ProfileController.cs	
@@ -0,0 +1,20 @@
+using Microsoft.AspNetCore.Mvc;
+using Microsoft.AspNetCore.Authorization;
+
+[Authorize]
+public class ProfileController : Controller {
+    // GOOD: The Authorize attribute of the class restricts access to this method.
+    public ActionResult Edit1(int profileId, string text) {
+        editProfileName(profileId, text);
+        return View();
+    }
+
+    // BAD: The AllowAnonymous attribute therides the Authorize attribute on the class.
+    [AllowAnonymous]
+    public ActionResult Edit2(int profileId, string text) {
+        editProfileName(profileId, text);
+        return View();
+    }
+
+    void editProfileName(int profileId, string text) { }
+}
\ No newline at end of file

From a2dce6be1487a3b3e99bcf39a8bb177c992324f4 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Thu, 14 Sep 2023 16:15:45 +0100
Subject: [PATCH 16/25] Check for authorize attributes in more namespaces and
 on overridden methods

---
 .../auth/InsecureDirectObjectReferenceQuery.qll    | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
index 05bb4ac92458..a081be92cf6e 100644
--- a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
+++ b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
@@ -44,9 +44,11 @@ private predicate hasAuthorizeAttribute(ActionMethod m) {
   exists(Attribute attr |
     attr.getType()
         .getABaseType*()
-        .hasQualifiedName("Microsoft.AspNetCore.Authorization", "AuthorizeAttribute")
+        .hasQualifiedName([
+            "Microsoft.AspNetCore.Authorization", "System.Web.Mvc", "System.Web.Http"
+          ], "AuthorizeAttribute")
   |
-    attr = m.getAnAttribute() or
+    attr = m.getOverridee*().getAnAttribute() or
     attr = m.getDeclaringType().getABaseType*().getAnAttribute()
   )
 }
@@ -56,14 +58,16 @@ private predicate hasAllowAnonymousAttribute(ActionMethod m) {
   exists(Attribute attr |
     attr.getType()
         .getABaseType*()
-        .hasQualifiedName("Microsoft.AspNetCore.Authorization", "AllowAnonymousAttribute")
+        .hasQualifiedName([
+            "Microsoft.AspNetCore.Authorization", "System.Web.Mvc", "System.Web.Http"
+          ], "AllowAnonymousAttribute")
   |
-    attr = m.getAnAttribute() or
+    attr = m.getOverridee*().getAnAttribute() or
     attr = m.getDeclaringType().getABaseType*().getAnAttribute()
   )
 }
 
-/** Hols if `m` is authorized via an `Authorize` attribute */
+/** Holds if `m` is authorized via an `Authorize` attribute */
 private predicate isAuthorizedViaAttribute(ActionMethod m) {
   hasAuthorizeAttribute(m) and
   not hasAllowAnonymousAttribute(m)

From 6d704be7d2bdc5a18829432a835e221f60957d5b Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Thu, 14 Sep 2023 17:35:53 +0100
Subject: [PATCH 17/25] Rewrite checks for index expressions in terms of
 dataflow

---
 .../security/auth/InsecureDirectObjectReferenceQuery.qll | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
index a081be92cf6e..b2731ebdb5df 100644
--- a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
+++ b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
@@ -2,6 +2,8 @@
 
 import csharp
 import semmle.code.csharp.dataflow.flowsources.Remote
+import DataFlow as DF
+import TaintTracking as TT
 import ActionMethods
 
 /**
@@ -12,8 +14,6 @@ import ActionMethods
 // Other queries check that there are authorization checks in place for admin methods.
 private predicate needsChecks(ActionMethod m) { m.isEdit() and not m.isAdmin() }
 
-private Expr getParentExpr(Expr ex) { result.getAChildExpr() = ex }
-
 /**
  * Holds if `m` has a parameter or access a remote flow source
  * that may indicate that it's used as the ID for some resource
@@ -23,9 +23,10 @@ private predicate hasIdParameter(ActionMethod m) {
     src.asParameter().getName().toLowerCase().matches(["%id", "%idx"])
     or
     // handle cases like `Request.QueryString["Id"]`
-    exists(StringLiteral idStr |
+    exists(StringLiteral idStr, IndexerCall idx |
       idStr.getValue().toLowerCase().matches(["%id", "%idx"]) and
-      getParentExpr*(src.asExpr()) = getParentExpr*(idStr)
+      TT::localTaint(src, DataFlow::exprNode(idx.getQualifier())) and
+      DF::localExprFlow(idStr, idx.getArgument(0))
     )
   )
 }

From 68ad5b7c003deb64c2886efd3385dc9b63cd73da Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Fri, 15 Sep 2023 16:35:29 +0100
Subject: [PATCH 18/25] Restrict logic for checking for id parameters on index
 expressions for performance

---
 .../security/auth/InsecureDirectObjectReferenceQuery.qll       | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
index b2731ebdb5df..20a0de568a9a 100644
--- a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
+++ b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
@@ -2,7 +2,6 @@
 
 import csharp
 import semmle.code.csharp.dataflow.flowsources.Remote
-import DataFlow as DF
 import TaintTracking as TT
 import ActionMethods
 
@@ -26,7 +25,7 @@ private predicate hasIdParameter(ActionMethod m) {
     exists(StringLiteral idStr, IndexerCall idx |
       idStr.getValue().toLowerCase().matches(["%id", "%idx"]) and
       TT::localTaint(src, DataFlow::exprNode(idx.getQualifier())) and
-      DF::localExprFlow(idStr, idx.getArgument(0))
+      idStr = idx.getArgument(0)
     )
   )
 }

From eb2f5898bd12b7e9ebe733a2f4635de7b4ee4b85 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Fri, 15 Sep 2023 16:39:51 +0100
Subject: [PATCH 19/25] Fix typos

---
 .../CWE-639/InsecureDirectObjectReference.qhelp                 | 2 +-
 .../Security Features/CWE-639/MVCTests/ProfileController.cs     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp
index 23af52944c4c..8cc9357fd3f6 100644
--- a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp	
+++ b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp	
@@ -18,7 +18,7 @@ Ensure that the current user is authorized to access the resource of the provide
 <p>In the following example, in the case marked BAD, there is no authorization check, so any user is able to edit any comment.
 In the case marked GOOD, there is a check that the current usr matches the author of the comment.</p>
 <sample src="WebFormsExample.cs" />
-<p>The following example shows a similar case for the ASP.NET Core framweork.</p>
+<p>The following example shows a similar case for the ASP.NET Core framework.</p>
 <sample src="MVCExample.cs" />
 
 
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/ProfileController.cs b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/ProfileController.cs
index 43eb93194797..a41c32db6411 100644
--- a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/ProfileController.cs	
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/ProfileController.cs	
@@ -9,7 +9,7 @@ public ActionResult Edit1(int profileId, string text) {
         return View();
     }
 
-    // BAD: The AllowAnonymous attribute therides the Authorize attribute on the class.
+    // BAD: The AllowAnonymous attribute overrides the Authorize attribute on the class.
     [AllowAnonymous]
     public ActionResult Edit2(int profileId, string text) {
         editProfileName(profileId, text);

From 868836e747868c7f776ad237e781037813305418 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Fri, 15 Sep 2023 16:40:12 +0100
Subject: [PATCH 20/25] Update severity

---
 .../Security Features/CWE-639/InsecureDirectObjectReference.ql  | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql
index 85b6b56f7bc9..06129ec88ec7 100644
--- a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql	
+++ b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql	
@@ -3,7 +3,7 @@
  * @description Using user input to control which object is modified without
  *              proper authorization checks allows an attacker to modify arbitrary objects.
  * @kind problem
- * @problem.severity error
+ * @problem.severity warning
  * @security-severity 7.5
  * @precision medium
  * @id cs/web/insecure-direct-object-reference

From 475fe3a2a5c6b29c9b095fd05eb70f9f309d4032 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Mon, 18 Sep 2023 14:35:41 +0100
Subject: [PATCH 21/25] Attempt to improve performance in checksUser

---
 .../InsecureDirectObjectReferenceQuery.qll    | 21 ++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
index 20a0de568a9a..d96c29ecb193 100644
--- a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
+++ b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
@@ -2,6 +2,7 @@
 
 import csharp
 import semmle.code.csharp.dataflow.flowsources.Remote
+import DataFlow as DF
 import TaintTracking as TT
 import ActionMethods
 
@@ -25,20 +26,30 @@ private predicate hasIdParameter(ActionMethod m) {
     exists(StringLiteral idStr, IndexerCall idx |
       idStr.getValue().toLowerCase().matches(["%id", "%idx"]) and
       TT::localTaint(src, DataFlow::exprNode(idx.getQualifier())) and
-      idStr = idx.getArgument(0)
+      DF::localExprFlow(idStr, idx.getArgument(0))
     )
   )
 }
 
+private predicate authorizingCallable(Callable c) {
+  exists(string name | name = c.getName().toLowerCase() |
+    name.matches(["%user%", "%session%"]) and
+    not name.matches("%get%by%") // methods like `getUserById` or `getXByUsername` aren't likely to be referring to the current user
+  )
+}
+
 /** Holds if `m` at some point in its call graph may make some kind of check against the current user. */
 private predicate checksUser(ActionMethod m) {
-  exists(Callable c, string name | name = c.getName().toLowerCase() |
-    name.matches(["%user%", "%session%"]) and
-    not name.matches("%get%by%") and // methods like `getUserById` or `getXByUsername` aren't likely to be referring to the current user
-    m.calls*(c)
+  exists(Callable c |
+    authorizingCallable(c) and
+    callsPlus(m, c)
   )
 }
 
+private predicate calls(Callable c1, Callable c2) { c1.calls(c2) }
+
+private predicate callsPlus(Callable c1, Callable c2) = fastTC(calls/2)(c1, c2)
+
 /** Holds if `m`, its containing class, or a parent class has an attribute that extends `AuthorizeAttribute` */
 private predicate hasAuthorizeAttribute(ActionMethod m) {
   exists(Attribute attr |

From 4497e22195204a36a83860b58db95c7f2bd7a22c Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Wed, 20 Sep 2023 04:10:51 +0100
Subject: [PATCH 22/25] Add an additional example and additional test cases for
 authorize attribute cases

---
 .../InsecureDirectObjectReference.qhelp       |  7 +--
 .../Security Features/CWE-639/MVCExample.cs   | 10 ++++
 .../InsecureDirectObjectReference.expected    |  3 ++
 .../CWE-639/MVCTests/MiscTestControllers.cs   | 46 +++++++++++++++++++
 4 files changed, 63 insertions(+), 3 deletions(-)
 create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/MiscTestControllers.cs

diff --git a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp
index 8cc9357fd3f6..5e8b522ac4c1 100644
--- a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp	
+++ b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp	
@@ -18,16 +18,17 @@ Ensure that the current user is authorized to access the resource of the provide
 <p>In the following example, in the case marked BAD, there is no authorization check, so any user is able to edit any comment.
 In the case marked GOOD, there is a check that the current usr matches the author of the comment.</p>
 <sample src="WebFormsExample.cs" />
-<p>The following example shows a similar case for the ASP.NET Core framework.</p>
+<p>The following example shows a similar case for the ASP.NET Core framework. In the third case, the `Authorize` attribute is used
+to restrict the method to only administrators, which are expected to be able to access arbitrary resources. 
+</p>
 <sample src="MVCExample.cs" />
 
-
 </example>
 <references>
 
   <li>OWASP - <a href="https://wiki.owasp.org/index.php/Top_10_2013-A4-Insecure_Direct_Object_References">Insecure Direct Object Refrences</a>.</li>
   <li>OWASP - <a href="https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/05-Authorization_Testing/04-Testing_for_Insecure_Direct_Object_References">Testing for Insecure Direct Object References</a>.</li>
-  <li>Microsoft Learn = <a href="https://learn.microsoft.com/en-us/aspnet/core/security/authorization/resourcebased?view=aspnetcore-7.0">Resource-based authorization in ASP.NET Core</a>.</li>
+  <li>Microsoft Learn - <a href="https://learn.microsoft.com/en-us/aspnet/core/security/authorization/resourcebased?view=aspnetcore-7.0">Resource-based authorization in ASP.NET Core</a>.</li>
 
 </references>
 </qhelp>
\ No newline at end of file
diff --git a/csharp/ql/src/Security Features/CWE-639/MVCExample.cs b/csharp/ql/src/Security Features/CWE-639/MVCExample.cs
index 36e692309cba..4137bccb395e 100644
--- a/csharp/ql/src/Security Features/CWE-639/MVCExample.cs	
+++ b/csharp/ql/src/Security Features/CWE-639/MVCExample.cs	
@@ -32,4 +32,14 @@ public async Task<IActionResult> Edit2(int commentId, string text) {
             return ForbidResult();
         }
     }
+
+    // GOOD: Only users with the `admin` role can access this method.
+    [Authorize(Roles="admin")]
+    public async Task<IActionResult> Edit3(int commentId, string text) {
+        Comment comment = _commentRepository.Find(commentId);
+        
+        comment.Text = text;
+
+        return View();
+    }
 }
\ No newline at end of file
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected
index 2c6bc2b10592..061b87dc6afe 100644
--- a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected	
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected	
@@ -1,3 +1,6 @@
 | CommentController.cs:6:25:6:29 | Edit1 | This method may be missing authorization checks for which users can access the resource of the provided ID. |
 | CommentController.cs:29:25:29:29 | Edit4 | This method may be missing authorization checks for which users can access the resource of the provided ID. |
+| MiscTestControllers.cs:26:33:26:40 | EditAnon | This method may be missing authorization checks for which users can access the resource of the provided ID. |
+| MiscTestControllers.cs:34:34:34:41 | EditAnon | This method may be missing authorization checks for which users can access the resource of the provided ID. |
+| MiscTestControllers.cs:45:25:45:29 | Edit4 | This method may be missing authorization checks for which users can access the resource of the provided ID. |
 | ProfileController.cs:14:25:14:29 | Edit2 | This method may be missing authorization checks for which users can access the resource of the provided ID. |
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/MiscTestControllers.cs b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/MiscTestControllers.cs
new file mode 100644
index 000000000000..3966d418a931
--- /dev/null
+++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/MiscTestControllers.cs	
@@ -0,0 +1,46 @@
+using Microsoft.AspNetCore.Mvc;
+using Microsoft.AspNetCore.Authorization;
+
+public class BaseController : Controller {
+    // GOOD
+    [Authorize]
+    public virtual ActionResult Edit1(int id) { return View(); }
+}
+
+class MyAuthorizeAttribute : AuthorizeAttribute { }
+class MyAllowAnonymousAttribute : AllowAnonymousAttribute { }
+
+public class AController : BaseController {
+    // GOOD - Authorize is inherited from overridden method
+    public override ActionResult Edit1(int id) { return View(); }
+
+    // GOOD - A subclass of Authorize is used
+    [MyAuthorize]
+    public ActionResult Edit2(int id) { return View(); }
+}
+
+[Authorize]
+public class BaseAuthController : Controller {
+    // BAD - A subclass of AllowAnonymous is used
+    [MyAllowAnonymous]
+    public virtual ActionResult EditAnon(int id) { return View(); }
+}
+
+public class BController : BaseAuthController {
+    // GOOD - Authorize is inherited from parent class
+    public ActionResult Edit3(int id) { return View(); }
+
+    // BAD - MyAllowAnonymous is inherited from overridden method
+    public override ActionResult EditAnon(int id) { return View(); }
+}
+
+[AllowAnonymous]
+public class BaseAnonController : Controller {
+
+}
+
+public class CController : BaseAnonController {
+    // BAD - AllowAnonymous is inherited from base class and overrides Authorize
+    [Authorize]
+    public ActionResult Edit4(int id) { return View(); }
+}
\ No newline at end of file

From df5fcc92e7953b90fbd2c51e6de4ee74df92b81e Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Mon, 25 Sep 2023 10:13:56 +0100
Subject: [PATCH 23/25] Apply suggestions from docs review

Co-authored-by: Sam Browning <106113886+sabrowning1@users.noreply.github.com>
---
 .../InsecureDirectObjectReference.qhelp        | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp
index 5e8b522ac4c1..ff69ea97d9e3 100644
--- a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp	
+++ b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp	
@@ -3,9 +3,7 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p>When an action method accepts an ID parameter used to control which resource (e.g. a comment, a user profile, etc) 
-is being accessed/modified, checks should me made to ensure that the current user is authorized to access that resource. 
-Otherwise, an attacker could access an arbitrary resource by guessing the ID parameter.</p>
+<p>Resources like comments or user profiles can be accessed and modified through an action method. To find a specific resource, the action method accepts an ID parameter that determines which resource to access. If the methods do not check that the current user is authorized to access the specified resource, an attacker can access a resource by guessing the ID parameter.</p>
 
 </overview>
 <recommendation>
@@ -15,20 +13,18 @@ Ensure that the current user is authorized to access the resource of the provide
 
 </recommendation>
 <example>
-<p>In the following example, in the case marked BAD, there is no authorization check, so any user is able to edit any comment.
-In the case marked GOOD, there is a check that the current usr matches the author of the comment.</p>
+<p>In the following example, in the "BAD" case, there is no authorization check, so any user can edit any comment for which they guess the ID parameter.
+The "GOOD" case includes a check that the current user matches the author of the comment, preventing unauthorized access.</p>
 <sample src="WebFormsExample.cs" />
-<p>The following example shows a similar case for the ASP.NET Core framework. In the third case, the `Authorize` attribute is used
-to restrict the method to only administrators, which are expected to be able to access arbitrary resources. 
-</p>
+<p>The following example shows a similar scenario for the ASP.NET Core framework. As above, the "BAD" case provides an example with no authorization check, and the first "GOOD" case provides an example with a check that the current user authored the specified comment. Additionally, in the second "GOOD" case, the `Authorize` attribute is used to restrict the method to administrators, who are expected to be able to access arbitrary resources.</p>
 <sample src="MVCExample.cs" />
 
 </example>
 <references>
 
-  <li>OWASP - <a href="https://wiki.owasp.org/index.php/Top_10_2013-A4-Insecure_Direct_Object_References">Insecure Direct Object Refrences</a>.</li>
-  <li>OWASP - <a href="https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/05-Authorization_Testing/04-Testing_for_Insecure_Direct_Object_References">Testing for Insecure Direct Object References</a>.</li>
-  <li>Microsoft Learn - <a href="https://learn.microsoft.com/en-us/aspnet/core/security/authorization/resourcebased?view=aspnetcore-7.0">Resource-based authorization in ASP.NET Core</a>.</li>
+  <li>OWASP: <a href="https://wiki.owasp.org/index.php/Top_10_2013-A4-Insecure_Direct_Object_References">Insecure Direct Object Refrences</a>.</li>
+  <li>OWASP: <a href="https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/05-Authorization_Testing/04-Testing_for_Insecure_Direct_Object_References">Testing for Insecure Direct Object References</a>.</li>
+  <li>Microsoft Learn: <a href="https://learn.microsoft.com/en-us/aspnet/core/security/authorization/resourcebased?view=aspnetcore-7.0">Resource-based authorization in ASP.NET Core</a>.</li>
 
 </references>
 </qhelp>
\ No newline at end of file

From 3efbbb3645509977bace91bb0f8f90b92b65266f Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Mon, 25 Sep 2023 15:44:40 +0100
Subject: [PATCH 24/25] Elaborate 'guess' to 'guess or determine'

---
 .../CWE-639/InsecureDirectObjectReference.qhelp               | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp
index ff69ea97d9e3..aa0af4a69f5e 100644
--- a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp	
+++ b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp	
@@ -3,7 +3,7 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p>Resources like comments or user profiles can be accessed and modified through an action method. To find a specific resource, the action method accepts an ID parameter that determines which resource to access. If the methods do not check that the current user is authorized to access the specified resource, an attacker can access a resource by guessing the ID parameter.</p>
+<p>Resources like comments or user profiles can be accessed and modified through an action method. To target a certain resource, the action method accepts an ID parameter pointing to that specific resource. If the methods do not check that the current user is authorized to access the specified resource, an attacker can access a resource by guessing or otherwise determining the linked ID parameter.</p>
 
 </overview>
 <recommendation>
@@ -13,7 +13,7 @@ Ensure that the current user is authorized to access the resource of the provide
 
 </recommendation>
 <example>
-<p>In the following example, in the "BAD" case, there is no authorization check, so any user can edit any comment for which they guess the ID parameter.
+<p>In the following example, in the "BAD" case, there is no authorization check, so any user can edit any comment for which they guess or determine the ID parameter.
 The "GOOD" case includes a check that the current user matches the author of the comment, preventing unauthorized access.</p>
 <sample src="WebFormsExample.cs" />
 <p>The following example shows a similar scenario for the ASP.NET Core framework. As above, the "BAD" case provides an example with no authorization check, and the first "GOOD" case provides an example with a check that the current user authored the specified comment. Additionally, in the second "GOOD" case, the `Authorize` attribute is used to restrict the method to administrators, who are expected to be able to access arbitrary resources.</p>

From d7c1be40d9508ddac3e214c34a09986d1becf921 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Mon, 25 Sep 2023 15:47:05 +0100
Subject: [PATCH 25/25] Fix codescanning alert by tweaking imported modules

---
 .../security/auth/InsecureDirectObjectReferenceQuery.qll    | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
index d96c29ecb193..6325c4ff3b37 100644
--- a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
+++ b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll
@@ -2,8 +2,6 @@
 
 import csharp
 import semmle.code.csharp.dataflow.flowsources.Remote
-import DataFlow as DF
-import TaintTracking as TT
 import ActionMethods
 
 /**
@@ -25,8 +23,8 @@ private predicate hasIdParameter(ActionMethod m) {
     // handle cases like `Request.QueryString["Id"]`
     exists(StringLiteral idStr, IndexerCall idx |
       idStr.getValue().toLowerCase().matches(["%id", "%idx"]) and
-      TT::localTaint(src, DataFlow::exprNode(idx.getQualifier())) and
-      DF::localExprFlow(idStr, idx.getArgument(0))
+      TaintTracking::localTaint(src, DataFlow::exprNode(idx.getQualifier())) and
+      DataFlow::localExprFlow(idStr, idx.getArgument(0))
     )
   )
 }