First round feedback

Author: Kwstubbs

python/ql/lib/semmle/python/Concepts.qll

@@ -1422,18 +1422,18 @@ module Http {
       /**
        * Gets the string corresponding to the middleware
        */
-      string middleware_name() { result = super.middleware_name() }
+      string getMiddlewareName() { result = super.getMiddlewareName() }
 
       /**
        * Gets the dataflow node corresponding to the allowed CORS origins
        */
-      DataFlow::Node allowed_origins() { result = super.allowed_origins() }
+      DataFlow::Node getOrigins() { result = super.getOrigins() }
 
       /**
        * Gets the boolean value corresponding to if CORS credentials is enabled
        * (`true`) or disabled (`false`) by this node.
        */
-      DataFlow::Node allowed_credentials() { result = super.allowed_credentials() }
+      DataFlow::Node getCredentialsAllowed() { result = super.getCredentialsAllowed() }
     }
 
     /** Provides a class for modeling new CORS middleware APIs. */
@@ -1447,20 +1447,20 @@ module Http {
        */
       abstract class Range extends DataFlow::Node {
         /**
-         * Gets the string corresponding to the middleware
+         * Gets the name corresponding to the middleware
          */
-        abstract string middleware_name();
+        abstract string getMiddlewareName();
 
         /**
          * Gets the boolean value corresponding to if CORS credentials is enabled
          * (`true`) or disabled (`false`) by this node.
          */
-        abstract DataFlow::Node allowed_credentials();
+        abstract DataFlow::Node getCredentialsAllowed();
 
         /**
          * Gets the strings corresponding to the origins allowed by the cors policy
          */
-        abstract DataFlow::Node allowed_origins();
+        abstract DataFlow::Node getOrigins();
       }
     }
 

python/ql/lib/semmle/python/frameworks/FastApi.qll

@@ -39,7 +39,7 @@ module FastApi {
     /**
      * Gets the string corresponding to the middleware
      */
-    string middleware_name() { result = this.getArg(0).asExpr().(Name).toString() }
+    string getMiddlewareName() { result = this.getArg(0).asExpr().(Name).getId() }
   }
 
   /**
@@ -49,18 +49,18 @@ module FastApi {
     /**
      * Gets the string corresponding to the middleware
      */
-    override string middleware_name() { result = this.getArg(0).asExpr().(Name).toString() }
+    override string getMiddlewareName() { result = this.getArg(0).asExpr().(Name).getId() }
 
     /**
      * Gets the dataflow node corresponding to the allowed CORS origins
      */
-    override DataFlow::Node allowed_origins() { result = this.getArgByName("allow_origins") }
+    override DataFlow::Node getOrigins() { result = this.getArgByName("allow_origins") }
 
     /**
      * Gets the boolean value corresponding to if CORS credentials is enabled
      * (`true`) or disabled (`false`) by this node.
      */
-    override DataFlow::Node allowed_credentials() {
+    override DataFlow::Node getCredentialsAllowed() {
       result = this.getArgByName("allow_credentials")
     }
 

python/ql/lib/semmle/python/frameworks/Starlette.qll

@@ -33,7 +33,7 @@ module Starlette {
     API::Node cls() { result = API::moduleImport("starlette").getMember("app") }
 
     /** Gets a reference to a Starlette application (an instance of `starlette.app`). */
-    API::Node instance() { result = cls().getReturn() }
+    API::Node instance() { result = cls().getAnInstance() }
   }
 
   /**
@@ -47,7 +47,7 @@ module Starlette {
     /**
      * Gets the string corresponding to the middleware
      */
-    string middleware_name() { result = this.getArg(0).asExpr().(Name).toString() }
+    string getMiddlewareName() { result = this.getArg(0).asExpr().(Name).getId() }
   }
 
   /**
@@ -57,11 +57,11 @@ module Starlette {
     /**
      * Gets the string corresponding to the middleware
      */
-    override string middleware_name() { result = this.getArg(0).asExpr().(Name).toString() }
+    override string getMiddlewareName() { result = this.getArg(0).asExpr().(Name).getId() }
 
-    override DataFlow::Node allowed_origins() { result = this.getArgByName("allow_origins") }
+    override DataFlow::Node getOrigins() { result = this.getArgByName("allow_origins") }
 
-    override DataFlow::Node allowed_credentials() {
+    override DataFlow::Node getCredentialsAllowed() {
       result = this.getArgByName("allow_credentials")
     }
 
@@ -89,32 +89,8 @@ module Starlette {
       result = ModelOutput::getATypeNode("starlette.middleware.Middleware~Subclass").getASubclass*()
     }
 
-    /**
-     * A source of instances of `starlette.middleware.Middleware`, extend this class to model new instances.
-     *
-     * This can include instantiations of the class, return values from function
-     * calls, or a special parameter that will be set when functions are called by an external
-     * library.
-     *
-     * Use the predicate `Middleware::instance()` to get references to instances of `starlette.middleware.middleware`.
-     */
-    abstract class InstanceSource extends DataFlow::LocalSourceNode { }
-
-    /** A direct instantiation of `starlette.middleware.Middleware`. */
-    class ClassInstantiation extends InstanceSource, DataFlow::CallCfgNode {
-      ClassInstantiation() { this = classRef().getACall() }
-    }
-
-    /** Gets a reference to an instance of `starlette.middleware.Middleware`. */
-    private DataFlow::TypeTrackingNode instance(DataFlow::TypeTracker t) {
-      t.start() and
-      result instanceof InstanceSource
-      or
-      exists(DataFlow::TypeTracker t2 | result = instance(t2).track(t2, t))
-    }
-
     /** Gets a reference to an instance of `starlette.middleware.Middleware`. */
-    DataFlow::Node instance() { instance(DataFlow::TypeTracker::end()).flowsTo(result) }
+    DataFlow::Node instance() { result = classRef().getACall() }
   }
 
   /**

python/ql/src/experimental/Security/CWE-942/CorsMisconfigurationMiddleware.qhelp

@@ -19,8 +19,8 @@
 	meaning that Peer B can send a request to Peer A that will include the cookies as if the request was executed by the user.
 </p>
 <p>
-	That can have dangerous effects if Peer B origin is not restricted correctly. 
-	An example of a dangerous scenario is when <code>Access-Control-Allow-Origin</code> header is set to a value gotten from the Peer B's request 
+	That can have dangerous effects if the origin of Peer B is not restricted correctly. 
+	An example of a dangerous scenario is when <code>Access-Control-Allow-Origin</code> header is set to a value obtained from the request made by Peer B 
 	(and not correctly validated), or is set to special values such as <code>*</code> or <code>null</code>. 
 	The above values can allow any Peer B to send requests to the misconfigured Peer A on behalf of the user.
 </p>

python/ql/src/experimental/Security/CWE-942/CorsMisconfigurationMiddleware.ql

@@ -6,7 +6,7 @@
  * @problem.severity warning
  * @security-severity 8.8
  * @precision high
- * @id py/insecure-cors-setting
+ * @id py/cors-misconfiguration-with-credentials
  * @tags security
  *       external/cwe/cwe-942
  */
@@ -17,23 +17,23 @@ private import semmle.python.dataflow.new.DataFlow
 
 predicate containsStar(DataFlow::Node array) {
   array.asExpr() instanceof List and
-  array.asExpr().getASubExpression().(StringLiteral).getText() = ["*", "null"]
+  array.asExpr().getASubExpression().(StringLiteral).getText() in ["*", "null"]
   or
-  array.asExpr().(StringLiteral).getText() = ["*", "null"]
+  array.asExpr().(StringLiteral).getText() in ["*", "null"]
 }
 
 predicate isCorsMiddleware(Http::Server::CorsMiddleware middleware) {
-  middleware.middleware_name().matches("CORSMiddleware")
+  middleware.getMiddlewareName() = "CORSMiddleware"
 }
 
 predicate credentialsAllowed(Http::Server::CorsMiddleware middleware) {
-  middleware.allowed_credentials().asExpr() instanceof True
+  middleware.getCredentialsAllowed().asExpr() instanceof True
 }
 
 from Http::Server::CorsMiddleware a
 where
   credentialsAllowed(a) and
-  containsStar(a.allowed_origins().getALocalSource()) and
+  containsStar(a.getOrigins().getALocalSource()) and
   isCorsMiddleware(a)
 select a,
-  "This CORS middleware uses a vulnerable configuration that leaves it open to attacks from arbitrary websites"
+  "This CORS middleware uses a vulnerable configuration that allows arbitrary websites to make authenticated cross-site requests"