Merge pull request github#353 from github/rc/3.3

Merge rc/3.3 into main

Author: aibaars

CONTRIBUTING.md

@@ -31,7 +31,7 @@ Follow the steps below to help other users understand what your query does, and
 
 2. **Format your code correctly**
 
-   All of the standard CodeQL queries and libraries are uniformly formatted for clarity and consistency, so we strongly recommend that all contributions follow the same formatting guidelines. If you use the CodeQL extension for Visual Studio Code, you can auto-format your query using the [Format Document command](https://help.semmle.com/codeql/codeql-for-vscode/procedures/about-codeql-for-vscode.html). For more information, see the [QL style guide](https://github.com/github/codeql/blob/main/docs/ql-style-guide.md).
+   All of the standard CodeQL queries and libraries are uniformly formatted for clarity and consistency, so we strongly recommend that all contributions follow the same formatting guidelines. If you use the CodeQL extension for Visual Studio Code, you can auto-format your query using the [Format Document command](https://code.visualstudio.com/docs/editor/codebasics#_formatting). For more information, see the [QL style guide](https://github.com/github/codeql/blob/main/docs/ql-style-guide.md).
 
 3. **Make sure your query has the correct metadata**
 

ql/docs/experimental.md

@@ -21,7 +21,7 @@ Experimental queries and libraries may not be actively maintained as the standar
 
 3. **Formatting**
 
-    - The queries and libraries must be [autoformatted](https://help.semmle.com/codeql/codeql-for-vscode/reference/editor.html#autoformatting).
+    - The queries and libraries must be [autoformatted](https://code.visualstudio.com/docs/editor/codebasics#_formatting).
 
 4. **Compilation**
 

ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll

@@ -32,7 +32,7 @@ class Node extends TNode {
    * The location spans column `startcolumn` of line `startline` to
    * column `endcolumn` of line `endline` in file `filepath`.
    * For more information, see
-   * [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
+   * [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
    */
   predicate hasLocationInfo(
     string filepath, int startline, int startcolumn, int endline, int endcolumn

ql/lib/codeql/ruby/security/XSS.qll

@@ -123,79 +123,137 @@ private module Shared {
   }
 
   /**
-   * An additional step that is preserves dataflow in the context of XSS.
+   * Holds if `call` is a method call in ERB file `erb`, targeting a method
+   * named `name`.
    */
-  predicate isAdditionalXSSFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-    // node1 is a `locals` argument to a render call...
-    exists(RenderCall call, Pair kvPair, string hashKey |
+  pragma[noinline]
+  private predicate isMethodCall(MethodCall call, string name, ErbFile erb) {
+    name = call.getMethodName() and
+    erb = call.getLocation().getFile()
+  }
+
+  /**
+   * Holds if some render call passes `value` for `hashKey` in the `locals`
+   * argument, in ERB file `erb`.
+   */
+  pragma[noinline]
+  private predicate renderCallLocals(string hashKey, Expr value, ErbFile erb) {
+    exists(RenderCall call, Pair kvPair |
       call.getLocals().getAKeyValuePair() = kvPair and
-      kvPair.getValue() = node1.asExpr().getExpr() and
-      kvPair.getKey().(StringlikeLiteral).getValueText() = hashKey and
-      // `node2` appears in the rendered template file
-      call.getTemplateFile() = node2.getLocation().getFile() and
-      (
-        // node2 is an element reference against `local_assigns`
-        exists(
-          CfgNodes::ExprNodes::ElementReferenceCfgNode refNode, DataFlow::Node argNode,
-          CfgNodes::ExprNodes::StringlikeLiteralCfgNode strNode
-        |
-          refNode = node2.asExpr() and
-          argNode.asExpr() = refNode.getArgument(0) and
-          refNode.getReceiver().getExpr().(MethodCall).getMethodName() = "local_assigns" and
-          argNode.getALocalSource() = DataFlow::exprNode(strNode) and
-          strNode.getExpr().getValueText() = hashKey
-        )
-        or
-        // ...node2 is a "method call" to a "method" with `hashKey` as its name
-        // TODO: This may be a variable read in reality that we interpret as a method call
-        exists(MethodCall varAcc |
-          varAcc = node2.asExpr().(CfgNodes::ExprNodes::MethodCallCfgNode).getExpr() and
-          varAcc.getMethodName() = hashKey
-        )
-      )
+      kvPair.getValue() = value and
+      kvPair.getKey().getValueText() = hashKey and
+      call.getTemplateFile() = erb
     )
-    or
-    // instance variables in the controller
-    exists(
-      ActionControllerActionMethod action, VariableReadAccess viewVarRead, AssignExpr ae,
-      FinalInstanceVarWrite controllerVarWrite
+  }
+
+  pragma[noinline]
+  private predicate isFlowFromLocals0(
+    CfgNodes::ExprNodes::ElementReferenceCfgNode refNode, string hashKey, ErbFile erb
+  ) {
+    exists(DataFlow::Node argNode, CfgNodes::ExprNodes::StringlikeLiteralCfgNode strNode |
+      argNode.asExpr() = refNode.getArgument(0) and
+      refNode.getReceiver().getExpr().(MethodCall).getMethodName() = "local_assigns" and
+      argNode.getALocalSource() = DataFlow::exprNode(strNode) and
+      strNode.getExpr().getValueText() = hashKey and
+      erb = refNode.getFile()
+    )
+  }
+
+  private predicate isFlowFromLocals(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(string hashKey, ErbFile erb |
+      // node1 is a `locals` argument to a render call...
+      renderCallLocals(hashKey, node1.asExpr().getExpr(), erb)
     |
-      viewVarRead = node2.asExpr().(CfgNodes::ExprNodes::VariableReadAccessCfgNode).getExpr() and
-      action.getDefaultTemplateFile() = viewVarRead.getLocation().getFile() and
+      // node2 is an element reference against `local_assigns`
+      isFlowFromLocals0(node2.asExpr(), hashKey, erb)
+      or
+      // ...node2 is a "method call" to a "method" with `hashKey` as its name
+      // TODO: This may be a variable read in reality that we interpret as a method call
+      isMethodCall(node2.asExpr().getExpr(), hashKey, erb)
+    )
+  }
+
+  /**
+   * Holds if `action` contains an assignment of `value` to an instance
+   * variable named `name`, in ERB file `erb`.
+   */
+  pragma[noinline]
+  private predicate actionAssigns(
+    ActionControllerActionMethod action, string name, Expr value, ErbFile erb
+  ) {
+    exists(AssignExpr ae, FinalInstanceVarWrite controllerVarWrite |
+      action.getDefaultTemplateFile() = erb and
+      ae.getParent+() = action and
+      ae = controllerVarWrite.getAnAssignExpr() and
+      name = controllerVarWrite.getVariable().getName() and
+      value = ae.getRightOperand()
+    )
+  }
+
+  pragma[noinline]
+  private predicate isVariableReadAccess(VariableReadAccess viewVarRead, string name, ErbFile erb) {
+    erb = viewVarRead.getLocation().getFile() and
+    viewVarRead.getVariable().getName() = name
+  }
+
+  private predicate isFlowFromControllerInstanceVariable(DataFlow::Node node1, DataFlow::Node node2) {
+    // instance variables in the controller
+    exists(ActionControllerActionMethod action, string name, ErbFile template |
       // match read to write on variable name
-      viewVarRead.getVariable().getName() = controllerVarWrite.getVariable().getName() and
+      actionAssigns(action, name, node1.asExpr().getExpr(), template) and
       // propagate taint from assignment RHS expr to variable read access in view
-      ae = controllerVarWrite.getAnAssignExpr() and
-      node1.asExpr().getExpr() = ae.getRightOperand() and
-      ae.getParent+() = action
+      isVariableReadAccess(node2.asExpr().getExpr(), name, template)
     )
-    or
+  }
+
+  /**
+   * Holds if `helperMethod` is a helper method named `name` that is associated
+   * with ERB file `erb`.
+   */
+  pragma[noinline]
+  private predicate isHelperMethod(
+    ActionControllerHelperMethod helperMethod, string name, ErbFile erb
+  ) {
+    helperMethod.getName() = name and
+    helperMethod.getControllerClass() = getAssociatedControllerClass(erb)
+  }
+
+  private predicate isFlowIntoHelperMethod(DataFlow::Node node1, DataFlow::Node node2) {
     // flow from template into controller helper method
     exists(
-      ErbFile template, ActionControllerHelperMethod helperMethod,
+      ErbFile template, ActionControllerHelperMethod helperMethod, string name,
       CfgNodes::ExprNodes::MethodCallCfgNode helperMethodCall, int argIdx
     |
-      template = node1.getLocation().getFile() and
-      helperMethod.getName() = helperMethodCall.getExpr().getMethodName() and
-      helperMethod.getControllerClass() = getAssociatedControllerClass(template) and
-      helperMethodCall.getArgument(argIdx) = node1.asExpr() and
-      helperMethod.getParameter(argIdx) = node2.asExpr().getExpr()
+      isHelperMethod(helperMethod, name, template) and
+      isMethodCall(helperMethodCall.getExpr(), name, template) and
+      helperMethodCall.getArgument(pragma[only_bind_into](argIdx)) = node1.asExpr() and
+      helperMethod.getParameter(pragma[only_bind_into](argIdx)) = node2.asExpr().getExpr()
     )
-    or
+  }
+
+  private predicate isFlowFromHelperMethod(DataFlow::Node node1, DataFlow::Node node2) {
     // flow out of controller helper method into template
-    exists(
-      ErbFile template, ActionControllerHelperMethod helperMethod,
-      CfgNodes::ExprNodes::MethodCallCfgNode helperMethodCall
-    |
-      template = node2.getLocation().getFile() and
-      helperMethod.getName() = helperMethodCall.getExpr().getMethodName() and
-      helperMethod.getControllerClass() = getAssociatedControllerClass(template) and
+    exists(ErbFile template, ActionControllerHelperMethod helperMethod, string name |
       // `node1` is an expr node that may be returned by the helper method
       exprNodeReturnedFrom(node1, helperMethod) and
       // `node2` is a call to the helper method
-      node2.asExpr() = helperMethodCall
+      isHelperMethod(helperMethod, name, template) and
+      isMethodCall(node2.asExpr().getExpr(), name, template)
     )
   }
+
+  /**
+   * An additional step that is preserves dataflow in the context of XSS.
+   */
+  predicate isAdditionalXSSFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    isFlowFromLocals(node1, node2)
+    or
+    isFlowFromControllerInstanceVariable(node1, node2)
+    or
+    isFlowIntoHelperMethod(node1, node2)
+    or
+    isFlowFromHelperMethod(node1, node2)
+  }
 }
 
 /**

ql/src/AlertSuppression.ql

@@ -63,7 +63,7 @@ class SuppressionScope extends @ruby_token_comment {
    * The location spans column `startcolumn` of line `startline` to
    * column `endcolumn` of line `endline` in file `filepath`.
    * For more information, see
-   * [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
+   * [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
    */
   predicate hasLocationInfo(
     string filepath, int startline, int startcolumn, int endline, int endcolumn

scripts/identical-files.json

@@ -9,7 +9,8 @@
     ],
     "DataFlow": [
         "codeql/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowImpl.qll",
-        "ql/lib/codeql/ruby/dataflow/internal/DataFlowImpl.qll"
+        "ql/lib/codeql/ruby/dataflow/internal/DataFlowImpl.qll",
+        "ql/lib/codeql/ruby/dataflow/internal/DataFlowImpl2.qll"
     ],
     "DataFlow2": [
         "codeql/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowImpl2.qll",
@@ -48,4 +49,4 @@
         "codeql/cpp/ql/lib/tutorial.qll",
         "ql/lib/tutorial.qll"
     ]
-}
+}