Merge remote-tracking branch 'origin/main' into polynomial_redos

.devcontainer/post_attach.sh

@@ -3,7 +3,7 @@ set -xe
 
 echo "Check installed CodeQL version"
 CURRENT_CODEQL_BIN=$(readlink -e /usr/local/bin/codeql || echo "")
-LATEST=$(gh release list --repo https://github.com/github/codeql-cli-binaries | cut -f 1 | sort --version-sort | tail -1)
+LATEST=$(gh release list --repo https://github.com/github/codeql-cli-binaries | cut -f 1 | grep -v beta | sort --version-sort | tail -1)
 
 BASE_DIR=/home/vscode/codeql-binaries
 mkdir -p "${BASE_DIR}"

.github/workflows/build.yml

@@ -75,7 +75,7 @@ jobs:
       - uses: actions/checkout@v2
       - name: Fetch CodeQL
         run: |
-          LATEST=$(gh release list --repo https://github.com/github/codeql-cli-binaries | cut -f 1 | sort --version-sort | tail -1)
+          LATEST=$(gh release list --repo https://github.com/github/codeql-cli-binaries | cut -f 1 | grep -v beta | sort --version-sort | tail -1)
           gh release download --repo https://github.com/github/codeql-cli-binaries --pattern codeql-linux64.zip "$LATEST"
           unzip -q codeql-linux64.zip
         env:
@@ -185,7 +185,7 @@ jobs:
       - name: Fetch CodeQL
         shell: bash
         run: |
-          LATEST=$(gh release list --repo https://github.com/github/codeql-cli-binaries | cut -f 1 | sort --version-sort | tail -1)
+          LATEST=$(gh release list --repo https://github.com/github/codeql-cli-binaries | cut -f 1 | grep -v beta | sort --version-sort | tail -1)
           gh release download --repo https://github.com/github/codeql-cli-binaries --pattern codeql.zip "$LATEST"
           unzip -q codeql.zip
         env:

.github/workflows/dataset_measure.yml

@@ -25,7 +25,7 @@ jobs:
 
       - name: Fetch CodeQL
         run: |
-          LATEST=$(gh release list --repo https://github.com/github/codeql-cli-binaries | cut -f 1 | sort --version-sort | tail -1)
+          LATEST=$(gh release list --repo https://github.com/github/codeql-cli-binaries | cut -f 1 | grep -v beta | sort --version-sort | tail -1)
           gh release download --repo https://github.com/github/codeql-cli-binaries --pattern codeql-linux64.zip "$LATEST"
           unzip -q codeql-linux64.zip
         env:

.github/workflows/qhelp.yml

@@ -23,7 +23,7 @@ jobs:
 
       - name: Fetch CodeQL
         run: |
-          LATEST=$(gh release list --repo https://github.com/github/codeql-cli-binaries | cut -f 1 | sort --version-sort | tail -1)
+          LATEST=$(gh release list --repo https://github.com/github/codeql-cli-binaries | cut -f 1 | grep -v beta | sort --version-sort | tail -1)
           gh release download --repo https://github.com/github/codeql-cli-binaries --pattern codeql-linux64.zip "$LATEST"
           unzip -q codeql-linux64.zip
         env:

CODE_OF_CONDUCT.md

@@ -0,0 +1,76 @@
+# Contributor Covenant Code of Conduct
+
+## Our Pledge
+
+In the interest of fostering an open and welcoming environment, we as
+contributors and maintainers pledge to make participation in our project and
+our community a harassment-free experience for everyone, regardless of age, body
+size, disability, ethnicity, sex characteristics, gender identity and expression,
+level of experience, education, socio-economic status, nationality, personal
+appearance, race, religion, or sexual identity and orientation.
+
+## Our Standards
+
+Examples of behavior that contributes to creating a positive environment
+include:
+
+* Using welcoming and inclusive language
+* Being respectful of differing viewpoints and experiences
+* Gracefully accepting constructive criticism
+* Focusing on what is best for the community
+* Showing empathy towards other community members
+
+Examples of unacceptable behavior by participants include:
+
+* The use of sexualized language or imagery and unwelcome sexual attention or
+  advances
+* Trolling, insulting/derogatory comments, and personal or political attacks
+* Public or private harassment
+* Publishing others' private information, such as a physical or electronic
+  address, without explicit permission
+* Other conduct which could reasonably be considered inappropriate in a
+  professional setting
+
+## Our Responsibilities
+
+Project maintainers are responsible for clarifying the standards of acceptable
+behavior and are expected to take appropriate and fair corrective action in
+response to any instances of unacceptable behavior.
+
+Project maintainers have the right and responsibility to remove, edit, or
+reject comments, commits, code, wiki edits, issues, and other contributions
+that are not aligned to this Code of Conduct, or to ban temporarily or
+permanently any contributor for other behaviors that they deem inappropriate,
+threatening, offensive, or harmful.
+
+## Scope
+
+This Code of Conduct applies within all project spaces, and it also applies when
+an individual is representing the project or its community in public spaces.
+Examples of representing a project or community include using an official
+project e-mail address, posting via an official social media account, or acting
+as an appointed representative at an online or offline event. Representation of
+a project may be further defined and clarified by project maintainers.
+
+## Enforcement
+
+Instances of abusive, harassing, or otherwise unacceptable behavior may be
+reported by contacting the project team at opensource@github.com. All
+complaints will be reviewed and investigated and will result in a response that
+is deemed necessary and appropriate to the circumstances. The project team is
+obligated to maintain confidentiality with regard to the reporter of an incident.
+Further details of specific enforcement policies may be posted separately.
+
+Project maintainers who do not follow or enforce the Code of Conduct in good
+faith may face temporary or permanent repercussions as determined by other
+members of the project's leadership.
+
+## Attribution
+
+This Code of Conduct is adapted from the [Contributor Covenant][homepage], version 1.4,
+available at https://www.contributor-covenant.org/version/1/4/code-of-conduct.html
+
+[homepage]: https://www.contributor-covenant.org
+
+For answers to common questions about this code of conduct, see
+https://www.contributor-covenant.org/faq

LICENSE

@@ -0,0 +1,21 @@
+MIT License
+
+Copyright (c) 2020-2021 GitHub
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in all
+copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+SOFTWARE.

generator/src/ql.rs

@@ -127,12 +127,13 @@ pub enum Expression<'a> {
     Or(Vec<Expression<'a>>),
     Equals(Box<Expression<'a>>, Box<Expression<'a>>),
     Dot(Box<Expression<'a>>, &'a str, Vec<Expression<'a>>),
-    Aggregate(
-        &'a str,
-        Vec<FormalParameter<'a>>,
-        Box<Expression<'a>>,
-        Box<Expression<'a>>,
-    ),
+    Aggregate {
+        name: &'a str,
+        vars: Vec<FormalParameter<'a>>,
+        range: Option<Box<Expression<'a>>>,
+        expr: Box<Expression<'a>>,
+        second_expr: Option<Box<Expression<'a>>>,
+    },
 }
 
 impl<'a> fmt::Display for Expression<'a> {
@@ -188,15 +189,31 @@ impl<'a> fmt::Display for Expression<'a> {
                 }
                 write!(f, ")")
             }
-            Expression::Aggregate(n, vars, range, term) => {
-                write!(f, "{}(", n)?;
-                for (index, var) in vars.iter().enumerate() {
-                    if index > 0 {
-                        write!(f, ", ")?;
+            Expression::Aggregate {
+                name,
+                vars,
+                range,
+                expr,
+                second_expr,
+            } => {
+                write!(f, "{}(", name)?;
+                if vars.len() > 0 {
+                    for (index, var) in vars.iter().enumerate() {
+                        if index > 0 {
+                            write!(f, ", ")?;
+                        }
+                        write!(f, "{}", var)?;
                     }
-                    write!(f, "{}", var)?;
+                    write!(f, " | ")?;
                 }
-                write!(f, " | {} | {})", range, term)
+                if let Some(range) = range {
+                    write!(f, "{} | ", range)?;
+                }
+                write!(f, "{}", expr)?;
+                if let Some(second_expr) = second_expr {
+                    write!(f, ", {}", second_expr)?;
+                }
+                write!(f, ")")
             }
         }
     }

generator/src/ql_gen.rs

@@ -79,6 +79,27 @@ pub fn create_ast_node_class<'a>(ast_node: &'a str, ast_node_parent: &'a str) ->
             Box::new(ql::Expression::String("???")),
         ),
     };
+    let get_primary_ql_classes = ql::Predicate {
+        qldoc: Some(
+            "Gets a comma-separated list of the names of the primary CodeQL \
+             classes to which this element belongs."
+                .to_owned(),
+        ),
+        name: "getPrimaryQlClasses",
+        overridden: false,
+        return_type: Some(ql::Type::String),
+        formal_parameters: vec![],
+        body: ql::Expression::Equals(
+            Box::new(ql::Expression::Var("result")),
+            Box::new(ql::Expression::Aggregate {
+                name: "concat",
+                vars: vec![],
+                range: None,
+                expr: Box::new(ql::Expression::Pred("getAPrimaryQlClass", vec![])),
+                second_expr: Some(Box::new(ql::Expression::String(","))),
+            }),
+        ),
+    };
     ql::Class {
         qldoc: Some(String::from("The base class for all AST nodes")),
         name: "AstNode",
@@ -92,6 +113,7 @@ pub fn create_ast_node_class<'a>(ast_node: &'a str, ast_node_parent: &'a str) ->
             get_parent_index,
             get_a_field_or_child,
             get_a_primary_ql_class,
+            get_primary_ql_classes,
         ],
     }
 }
@@ -410,15 +432,16 @@ fn create_field_getters<'a>(
                 })
                 .collect();
             (
-                ql::Expression::Aggregate(
-                    "exists",
-                    vec![ql::FormalParameter {
+                ql::Expression::Aggregate {
+                    name: "exists",
+                    vars: vec![ql::FormalParameter {
                         name: "value",
                         param_type: ql::Type::Int,
                     }],
-                    Box::new(get_value),
-                    Box::new(ql::Expression::Or(disjuncts)),
-                ),
+                    range: Some(Box::new(get_value)),
+                    expr: Box::new(ql::Expression::Or(disjuncts)),
+                    second_expr: None,
+                },
                 // Since the getter returns a string and not an AstNode, it won't be part of getAFieldOrChild:
                 None,
             )

ql/consistency-queries/AstConsistency.ql

@@ -1,17 +1,11 @@
 import codeql.ruby.AST
 import codeql.ruby.ast.internal.Synthesis
 
-private string getAPrimaryQlClass(AstNode node) {
-  result = node.getAPrimaryQlClass()
-  or
-  not exists(node.getAPrimaryQlClass()) and result = "(none)"
-}
-
 query predicate missingParent(AstNode node, string cls) {
   not exists(node.getParent()) and
   node.getLocation().getFile().getExtension() != "erb" and
   not node instanceof Toplevel and
-  cls = getAPrimaryQlClass(node)
+  cls = node.getPrimaryQlClasses()
 }
 
 pragma[noinline]
@@ -22,7 +16,7 @@ private AstNode parent(AstNode child, int desugarLevel) {
 
 query predicate multipleParents(AstNode node, AstNode parent, string cls) {
   parent = node.getParent() and
-  cls = getAPrimaryQlClass(parent) and
+  cls = parent.getPrimaryQlClasses() and
   exists(AstNode one, AstNode two, int desugarLevel |
     one = parent(node, desugarLevel) and
     two = parent(node, desugarLevel) and

ql/lib/codeql/ruby/AST.qll

@@ -32,6 +32,12 @@ class AstNode extends TAstNode {
    */
   string getAPrimaryQlClass() { result = "???" }
 
+  /**
+   * Gets a comma-separated list of the names of the primary CodeQL classes to
+   * which this element belongs.
+   */
+  final string getPrimaryQlClasses() { result = concat(this.getAPrimaryQlClass(), ",") }
+
   /** Gets the enclosing module, if any. */
   ModuleBase getEnclosingModule() {
     exists(Scope::Range s |

ql/lib/codeql/ruby/Concepts.qll

@@ -16,13 +16,9 @@ private import codeql.ruby.dataflow.RemoteFlowSources
  * Extend this class to refine existing API models. If you want to model new APIs,
  * extend `SqlExecution::Range` instead.
  */
-class SqlExecution extends DataFlow::Node {
-  SqlExecution::Range range;
-
-  SqlExecution() { this = range }
-
+class SqlExecution extends DataFlow::Node instanceof SqlExecution::Range {
   /** Gets the argument that specifies the SQL statements to be executed. */
-  DataFlow::Node getSql() { result = range.getSql() }
+  DataFlow::Node getSql() { result = super.getSql() }
 }
 
 /** Provides a class for modeling new SQL execution APIs. */
@@ -46,26 +42,23 @@ module SqlExecution {
  * Extend this class to refine existing API models. If you want to model new APIs,
  * extend `Escaping::Range` instead.
  */
-class Escaping extends DataFlow::Node {
-  Escaping::Range range;
-
+class Escaping extends DataFlow::Node instanceof Escaping::Range {
   Escaping() {
-    this = range and
     // escapes that don't have _both_ input/output defined are not valid
-    exists(range.getAnInput()) and
-    exists(range.getOutput())
+    exists(super.getAnInput()) and
+    exists(super.getOutput())
   }
 
   /** Gets an input that will be escaped. */
-  DataFlow::Node getAnInput() { result = range.getAnInput() }
+  DataFlow::Node getAnInput() { result = super.getAnInput() }
 
   /** Gets the output that contains the escaped data. */
-  DataFlow::Node getOutput() { result = range.getOutput() }
+  DataFlow::Node getOutput() { result = super.getOutput() }
 
   /**
    * Gets the context that this function escapes for, such as `html`, or `url`.
    */
-  string getKind() { result = range.getKind() }
+  string getKind() { result = super.getKind() }
 }
 
 /** Provides a class for modeling new escaping APIs. */
@@ -103,7 +96,7 @@ module Escaping {
  * `<p>{}</p>`.
  */
 class HtmlEscaping extends Escaping {
-  HtmlEscaping() { range.getKind() = Escaping::getHtmlKind() }
+  HtmlEscaping() { super.getKind() = Escaping::getHtmlKind() }
 }
 
 /** Provides classes for modeling HTTP-related APIs. */
@@ -116,29 +109,25 @@ module HTTP {
      * Extend this class to refine existing API models. If you want to model new APIs,
      * extend `RouteSetup::Range` instead.
      */
-    class RouteSetup extends DataFlow::Node {
-      RouteSetup::Range range;
-
-      RouteSetup() { this = range }
-
+    class RouteSetup extends DataFlow::Node instanceof RouteSetup::Range {
       /** Gets the URL pattern for this route, if it can be statically determined. */
-      string getUrlPattern() { result = range.getUrlPattern() }
+      string getUrlPattern() { result = super.getUrlPattern() }
 
       /**
        * Gets a function that will handle incoming requests for this route, if any.
        *
        * NOTE: This will be modified in the near future to have a `RequestHandler` result, instead of a `Method`.
        */
-      Method getARequestHandler() { result = range.getARequestHandler() }
+      Method getARequestHandler() { result = super.getARequestHandler() }
 
       /**
        * Gets a parameter that will receive parts of the url when handling incoming
        * requests for this route, if any. These automatically become a `RemoteFlowSource`.
        */
-      Parameter getARoutedParameter() { result = range.getARoutedParameter() }
+      Parameter getARoutedParameter() { result = super.getARoutedParameter() }
 
       /** Gets a string that identifies the framework used for this route setup. */
-      string getFramework() { result = range.getFramework() }
+      string getFramework() { result = super.getFramework() }
     }
 
     /** Provides a class for modeling new HTTP routing APIs. */
@@ -185,19 +174,15 @@ module HTTP {
      * Extend this class to refine existing API models. If you want to model new APIs,
      * extend `RequestHandler::Range` instead.
      */
-    class RequestHandler extends Method {
-      RequestHandler::Range range;
-
-      RequestHandler() { this = range }
-
+    class RequestHandler extends Method instanceof RequestHandler::Range {
       /**
        * Gets a parameter that could receive parts of the url when handling incoming
        * requests, if any. These automatically become a `RemoteFlowSource`.
        */
-      Parameter getARoutedParameter() { result = range.getARoutedParameter() }
+      Parameter getARoutedParameter() { result = super.getARoutedParameter() }
 
       /** Gets a string that identifies the framework used for this route setup. */
-      string getFramework() { result = range.getFramework() }
+      string getFramework() { result = super.getFramework() }
     }
 
     /** Provides a class for modeling new HTTP request handlers. */
@@ -253,16 +238,12 @@ module HTTP {
      * Extend this class to refine existing API models. If you want to model new APIs,
      * extend `HttpResponse::Range` instead.
      */
-    class HttpResponse extends DataFlow::Node {
-      HttpResponse::Range range;
-
-      HttpResponse() { this = range }
-
+    class HttpResponse extends DataFlow::Node instanceof HttpResponse::Range {
       /** Gets the data-flow node that specifies the body of this HTTP response. */
-      DataFlow::Node getBody() { result = range.getBody() }
+      DataFlow::Node getBody() { result = super.getBody() }
 
       /** Gets the mimetype of this HTTP response, if it can be statically determined. */
-      string getMimetype() { result = range.getMimetype() }
+      string getMimetype() { result = super.getMimetype() }
     }
 
     /** Provides a class for modeling new HTTP response APIs. */
@@ -308,13 +289,9 @@ module HTTP {
      * Extend this class to refine existing API models. If you want to model new APIs,
      * extend `HttpRedirectResponse::Range` instead.
      */
-    class HttpRedirectResponse extends HttpResponse {
-      override HttpRedirectResponse::Range range;
-
-      HttpRedirectResponse() { this = range }
-
+    class HttpRedirectResponse extends HttpResponse instanceof HttpRedirectResponse::Range {
       /** Gets the data-flow node that specifies the location of this HTTP redirect response. */
-      DataFlow::Node getRedirectLocation() { result = range.getRedirectLocation() }
+      DataFlow::Node getRedirectLocation() { result = super.getRedirectLocation() }
     }
 
     /** Provides a class for modeling new HTTP redirect response APIs. */

ql/lib/codeql/ruby/ast/Variable.qll

@@ -52,26 +52,20 @@ class LocalVariable extends Variable, TLocalVariable {
 }
 
 /** A global variable. */
-class GlobalVariable extends VariableReal, TGlobalVariable {
-  override GlobalVariable::Range range;
-
+class GlobalVariable extends VariableReal, TGlobalVariable instanceof GlobalVariable::Range {
   final override GlobalVariableAccess getAnAccess() { result.getVariable() = this }
 }
 
 /** An instance variable. */
-class InstanceVariable extends VariableReal, TInstanceVariable {
-  override InstanceVariable::Range range;
-
+class InstanceVariable extends VariableReal, TInstanceVariable instanceof InstanceVariable::Range {
   /** Holds is this variable is a class instance variable. */
-  final predicate isClassInstanceVariable() { range.isClassInstanceVariable() }
+  final predicate isClassInstanceVariable() { super.isClassInstanceVariable() }
 
   final override InstanceVariableAccess getAnAccess() { result.getVariable() = this }
 }
 
 /** A class variable. */
-class ClassVariable extends VariableReal, TClassVariable {
-  override ClassVariable::Range range;
-
+class ClassVariable extends VariableReal, TClassVariable instanceof ClassVariable::Range {
   final override ClassVariableAccess getAnAccess() { result.getVariable() = this }
 }
 

ql/lib/codeql/ruby/ast/internal/TreeSitter.qll

@@ -26,6 +26,9 @@ module Ruby {
 
     /** Gets the name of the primary QL class for this element. */
     string getAPrimaryQlClass() { result = "???" }
+
+    /** Gets a comma-separated list of the names of the primary CodeQL classes to which this element belongs. */
+    string getPrimaryQlClasses() { result = concat(getAPrimaryQlClass(), ",") }
   }
 
   /** A token. */
@@ -1864,6 +1867,9 @@ module Erb {
 
     /** Gets the name of the primary QL class for this element. */
     string getAPrimaryQlClass() { result = "???" }
+
+    /** Gets a comma-separated list of the names of the primary CodeQL classes to which this element belongs. */
+    string getPrimaryQlClasses() { result = concat(getAPrimaryQlClass(), ",") }
   }
 
   /** A token. */

ql/lib/codeql/ruby/ast/internal/Variable.qll

@@ -400,24 +400,22 @@ module LocalVariable {
   }
 }
 
-class VariableReal extends Variable, TVariableReal {
-  VariableReal::Range range;
+class VariableReal extends Variable, TVariableReal instanceof VariableReal::Range {
+  final override string getName() { result = VariableReal::Range.super.getName() }
 
-  VariableReal() { range = this }
+  final override Location getLocation() { result = VariableReal::Range.super.getLocation() }
 
-  final override string getName() { result = range.getName() }
-
-  final override Location getLocation() { result = range.getLocation() }
-
-  final override Scope getDeclaringScope() { toGenerated(result) = range.getDeclaringScope() }
+  final override Scope getDeclaringScope() {
+    toGenerated(result) = VariableReal::Range.super.getDeclaringScope()
+  }
 }
 
-class LocalVariableReal extends VariableReal, LocalVariable, TLocalVariableReal {
-  override LocalVariable::Range range;
-
+class LocalVariableReal extends VariableReal, LocalVariable, TLocalVariableReal instanceof LocalVariable::Range {
   final override LocalVariableAccessReal getAnAccess() { result.getVariable() = this }
 
-  final override VariableAccess getDefiningAccess() { result = range.getDefiningAccess() }
+  final override VariableAccess getDefiningAccess() {
+    result = LocalVariable::Range.super.getDefiningAccess()
+  }
 }
 
 class LocalVariableSynth extends LocalVariable, TLocalVariableSynth {

ql/lib/codeql/ruby/controlflow/ControlFlowGraph.qll

@@ -9,9 +9,7 @@ private import internal.Splitting
 private import internal.Completion
 
 /** An AST node with an associated control-flow graph. */
-class CfgScope extends Scope {
-  CfgScope() { this instanceof CfgScope::Range_ }
-
+class CfgScope extends Scope instanceof CfgScope::Range_ {
   /** Gets the CFG scope that this scope is nested under, if any. */
   final CfgScope getOuterCfgScope() {
     exists(AstNode parent |

ql/lib/codeql/ruby/frameworks/ActionController.qll

@@ -154,6 +154,25 @@ class RedirectToCall extends ActionControllerContextCall {
   }
 }
 
+/**
+ * A call to the `redirect_to` method, as an `HttpRedirectResponse`.
+ */
+class ActionControllerRedirectResponse extends HTTP::Server::HttpRedirectResponse::Range {
+  RedirectToCall redirectToCall;
+
+  ActionControllerRedirectResponse() { this.asExpr().getExpr() = redirectToCall }
+
+  override DataFlow::Node getBody() { none() }
+
+  override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
+
+  override string getMimetypeDefault() { none() }
+
+  override DataFlow::Node getRedirectLocation() {
+    result.asExpr().getExpr() = redirectToCall.getRedirectUrl()
+  }
+}
+
 /**
  * A method in an `ActionController` class that is accessible from within a
  * Rails view as a helper method. For instance, in:

ql/lib/codeql/ruby/regexp/RegExpTreeView.qll

@@ -18,10 +18,15 @@ class RegExpParent extends TRegExpParent {
 
   /**
    * Gets the name of a primary CodeQL class to which this regular
-   * expression
-   * term belongs.
+   * expression term belongs.
    */
   string getAPrimaryQlClass() { result = "RegExpParent" }
+
+  /**
+   * Gets a comma-separated list of the names of the primary CodeQL classes to
+   * which this regular expression term belongs.
+   */
+  final string getPrimaryQlClasses() { result = concat(this.getAPrimaryQlClass(), ",") }
 }
 
 class RegExpLiteral extends TRegExpLiteral, RegExpParent {

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

@@ -0,0 +1,126 @@
+/**
+ * Provides default sources, sinks and sanitizers for detecting "URL
+ * redirection" vulnerabilities, as well as extension points for adding your
+ * own.
+ */
+
+private import ruby
+private import codeql.ruby.DataFlow
+private import codeql.ruby.Concepts
+private import codeql.ruby.dataflow.RemoteFlowSources
+private import codeql.ruby.dataflow.BarrierGuards
+
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "URL redirection" vulnerabilities, as well as extension points for
+ * adding your own.
+ */
+module UrlRedirect {
+  /**
+   * A data flow source for "URL redirection" vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for "URL redirection" vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A sanitizer for "URL redirection" vulnerabilities.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A sanitizer guard for "URL redirection" vulnerabilities.
+   */
+  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
+
+  /**
+   * Additional taint steps for "URL redirection" vulnerabilities.
+   */
+  predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    taintStepViaMethodCallReturnValue(node1, node2)
+  }
+
+  /**
+   * A source of remote user input, considered as a flow source.
+   */
+  class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
+
+  /**
+   * A HTTP redirect response, considered as a flow sink.
+   */
+  class RedirectLocationAsSink extends Sink {
+    RedirectLocationAsSink() {
+      exists(HTTP::Server::HttpRedirectResponse e |
+        this = e.getRedirectLocation() and
+        // As a rough heuristic, assume that methods with these names are handlers for POST/PUT/PATCH/DELETE requests,
+        // which are not as vulnerable to URL redirection because browsers will not initiate them from clicking a link.
+        not this.getEnclosingCallable()
+            .(Method)
+            .getName()
+            .regexpMatch(".*(create|update|destroy).*")
+      )
+    }
+  }
+
+  /**
+   * A comparison with a constant string, considered as a sanitizer-guard.
+   */
+  class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }
+
+  /**
+   * Some methods will propagate taint to their return values.
+   * Here we cover a few common ones related to `ActionController::Parameters`.
+   * TODO: use ApiGraphs or something to restrict these method calls to the correct receiver, rather
+   * than matching on method name alone.
+   */
+  predicate taintStepViaMethodCallReturnValue(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(MethodCall m | m = node2.asExpr().getExpr() |
+      m.getReceiver() = node1.asExpr().getExpr() and
+      (actionControllerTaintedMethod(m) or hashTaintedMethod(m))
+    )
+  }
+
+  /**
+   * String interpolation is considered safe, provided the string is prefixed by a non-tainted value.
+   * In most cases this will prevent the tainted value from controlling e.g. the host of the URL.
+   *
+   * For example:
+   *
+   * ```ruby
+   * redirect_to "/users/#{params[:key]}" # safe
+   * redirect_to "#{params[:key]}/users"  # unsafe
+   * ```
+   *
+   * There are prefixed interpolations that are not safe, e.g.
+   *
+   * ```ruby
+   * redirect_to "foo#{params[:key]}/users" # => "foo-malicious-site.com/users"
+   * ```
+   *
+   * We currently don't catch these cases.
+   */
+  class StringInterpolationAsSanitizer extends Sanitizer {
+    StringInterpolationAsSanitizer() {
+      exists(StringlikeLiteral str, int n | str.getComponent(n) = this.asExpr().getExpr() and n > 0)
+    }
+  }
+
+  /**
+   * These methods return a new `ActionController::Parameters` or a `Hash` containing a subset of
+   * the original values. This may still contain user input, so the results are tainted.
+   * TODO: flesh this out to cover the whole API.
+   */
+  predicate actionControllerTaintedMethod(MethodCall m) {
+    m.getMethodName() in ["to_unsafe_hash", "to_unsafe_h", "permit", "require"]
+  }
+
+  /**
+   * These `Hash` methods preserve taint because they return a new hash which may still contain keys
+   * with user input.
+   * TODO: flesh this out to cover the whole API.
+   */
+  predicate hashTaintedMethod(MethodCall m) { m.getMethodName() in ["merge", "fetch"] }
+}

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

@@ -0,0 +1,34 @@
+/**
+ * Provides a taint-tracking configuration for detecting "URL redirection" vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if `Configuration` is needed,
+ * otherwise `UrlRedirectCustomizations` should be imported instead.
+ */
+
+private import ruby
+import codeql.ruby.DataFlow::DataFlow::PathGraph
+import codeql.ruby.DataFlow
+import codeql.ruby.TaintTracking
+import UrlRedirectCustomizations
+import UrlRedirectCustomizations::UrlRedirect
+
+/**
+ * A taint-tracking configuration for detecting "URL redirection" vulnerabilities.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "UrlRedirect" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof SanitizerGuard
+  }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    UrlRedirect::isAdditionalTaintStep(node1, node2)
+  }
+}

ql/src/queries/security/cwe-601/UrlRedirect.qhelp

@@ -0,0 +1,44 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Directly incorporating user input into a URL redirect request without validating the input
+can facilitate phishing attacks. In these attacks, unsuspecting users can be redirected to a
+malicious site that looks very similar to the real site they intend to visit, but which is
+controlled by the attacker.
+</p>
+</overview>
+
+<recommendation>
+<p>
+To guard against untrusted URL redirection, it is advisable to avoid putting user input
+directly into a redirect URL. Instead, maintain a list of authorized
+redirects on the server; then choose from that list based on the user input provided.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows an HTTP request parameter being used directly in a URL redirect
+without validating the input, which facilitates phishing attacks:
+</p>
+
+<sample src="examples/redirect_bad.rb"/>
+
+<p>
+One way to remedy the problem is to validate the user input against a known fixed string
+before doing the redirection:
+</p>
+
+<sample src="examples/redirect_good.rb"/>
+</example>
+
+<references>
+<li>OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html">
+  XSS Unvalidated Redirects and Forwards Cheat Sheet</a>.</li>
+<li>Rails Guides: <a href="https://guides.rubyonrails.org/security.html#redirection-and-files">
+  Redirection and Files</a>.</li>
+</references>
+
+</qhelp>

ql/src/queries/security/cwe-601/UrlRedirect.ql

@@ -0,0 +1,22 @@
+/**
+ * @name URL redirection from remote source
+ * @description URL redirection based on unvalidated user input
+ *              may cause redirection to malicious web sites.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 6.1
+ * @sub-severity low
+ * @id rb/url-redirection
+ * @tags security
+ *       external/cwe/cwe-601
+ * @precision high
+ */
+
+import ruby
+import codeql.ruby.security.UrlRedirectQuery
+import codeql.ruby.DataFlow::DataFlow::PathGraph
+
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Untrusted URL redirection due to $@.", source.getNode(),
+  "a user-provided value"

ql/src/queries/security/cwe-601/examples/redirect_bad.rb

@@ -0,0 +1,5 @@
+class HelloController < ActionController::Base
+  def hello
+    redirect_to params[:url]
+  end
+end

ql/src/queries/security/cwe-601/examples/redirect_good.rb

@@ -0,0 +1,11 @@
+class HelloController < ActionController::Base
+  VALID_REDIRECT = "http://cwe.mitre.org/data/definitions/601.html"
+
+  def hello
+    if params[:url] == VALID_REDIRECT
+      redirect_to params[:url]
+    else
+      # error
+    end
+  end
+end

ql/test/query-tests/security/cwe-601/UrlRedirect.expected

@@ -0,0 +1,25 @@
+edges
+| UrlRedirect.rb:9:17:9:22 | call to params :  | UrlRedirect.rb:9:17:9:28 | ...[...] |
+| UrlRedirect.rb:14:17:14:22 | call to params :  | UrlRedirect.rb:14:17:14:43 | call to fetch |
+| UrlRedirect.rb:19:17:19:22 | call to params :  | UrlRedirect.rb:19:17:19:37 | call to to_unsafe_hash |
+| UrlRedirect.rb:24:31:24:36 | call to params :  | UrlRedirect.rb:24:17:24:37 | call to filter_params |
+| UrlRedirect.rb:34:20:34:25 | call to params :  | UrlRedirect.rb:34:17:34:37 | "#{...}/foo" |
+nodes
+| UrlRedirect.rb:4:17:4:22 | call to params | semmle.label | call to params |
+| UrlRedirect.rb:9:17:9:22 | call to params :  | semmle.label | call to params :  |
+| UrlRedirect.rb:9:17:9:28 | ...[...] | semmle.label | ...[...] |
+| UrlRedirect.rb:14:17:14:22 | call to params :  | semmle.label | call to params :  |
+| UrlRedirect.rb:14:17:14:43 | call to fetch | semmle.label | call to fetch |
+| UrlRedirect.rb:19:17:19:22 | call to params :  | semmle.label | call to params :  |
+| UrlRedirect.rb:19:17:19:37 | call to to_unsafe_hash | semmle.label | call to to_unsafe_hash |
+| UrlRedirect.rb:24:17:24:37 | call to filter_params | semmle.label | call to filter_params |
+| UrlRedirect.rb:24:31:24:36 | call to params :  | semmle.label | call to params :  |
+| UrlRedirect.rb:34:17:34:37 | "#{...}/foo" | semmle.label | "#{...}/foo" |
+| UrlRedirect.rb:34:20:34:25 | call to params :  | semmle.label | call to params :  |
+#select
+| UrlRedirect.rb:4:17:4:22 | call to params | UrlRedirect.rb:4:17:4:22 | call to params | UrlRedirect.rb:4:17:4:22 | call to params | Untrusted URL redirection due to $@. | UrlRedirect.rb:4:17:4:22 | call to params | a user-provided value |
+| UrlRedirect.rb:9:17:9:28 | ...[...] | UrlRedirect.rb:9:17:9:22 | call to params :  | UrlRedirect.rb:9:17:9:28 | ...[...] | Untrusted URL redirection due to $@. | UrlRedirect.rb:9:17:9:22 | call to params | a user-provided value |
+| UrlRedirect.rb:14:17:14:43 | call to fetch | UrlRedirect.rb:14:17:14:22 | call to params :  | UrlRedirect.rb:14:17:14:43 | call to fetch | Untrusted URL redirection due to $@. | UrlRedirect.rb:14:17:14:22 | call to params | a user-provided value |
+| UrlRedirect.rb:19:17:19:37 | call to to_unsafe_hash | UrlRedirect.rb:19:17:19:22 | call to params :  | UrlRedirect.rb:19:17:19:37 | call to to_unsafe_hash | Untrusted URL redirection due to $@. | UrlRedirect.rb:19:17:19:22 | call to params | a user-provided value |
+| UrlRedirect.rb:24:17:24:37 | call to filter_params | UrlRedirect.rb:24:31:24:36 | call to params :  | UrlRedirect.rb:24:17:24:37 | call to filter_params | Untrusted URL redirection due to $@. | UrlRedirect.rb:24:31:24:36 | call to params | a user-provided value |
+| UrlRedirect.rb:34:17:34:37 | "#{...}/foo" | UrlRedirect.rb:34:20:34:25 | call to params :  | UrlRedirect.rb:34:17:34:37 | "#{...}/foo" | Untrusted URL redirection due to $@. | UrlRedirect.rb:34:20:34:25 | call to params | a user-provided value |

ql/test/query-tests/security/cwe-601/UrlRedirect.qlref

@@ -0,0 +1 @@
+queries/security/cwe-601/UrlRedirect.ql

ql/test/query-tests/security/cwe-601/UrlRedirect.rb

@@ -0,0 +1,59 @@
+class UsersController < ActionController::Base
+  # BAD
+  def route1
+    redirect_to params
+  end
+
+  # BAD
+  def route2
+    redirect_to params[:key]
+  end
+
+  # BAD
+  def route3
+    redirect_to params.fetch(:specific_arg)
+  end
+
+  # BAD
+  def route4
+    redirect_to params.to_unsafe_hash
+  end
+
+  # BAD
+  def route5
+    redirect_to filter_params(params)
+  end
+
+  # GOOD
+  def route6
+    redirect_to "/foo/#{params[:key]}"
+  end
+
+  # BAD
+  def route7
+    redirect_to "#{params[:key]}/foo"
+  end
+
+  # GOOD
+  def route8
+    key = params[:key]
+    if key == "foo"
+      redirect_to key
+    else
+      redirect_to "/default"
+    end
+  end
+
+  # GOOD
+  # Technically vulnerable, but we assume this is a handler for a POST request,
+  # so can't be triggered by following a link.
+  def create
+    redirect_to params[:key]
+  end
+
+  private
+
+  def filter_params(input_params)
+    input_params.permit(:key)
+  end
+end