From 0bd9e9f298040daa3cca0f441476a981b661548a Mon Sep 17 00:00:00 2001
From: Stephan Brandauer <kaeluka@github.com>
Date: Fri, 11 Mar 2022 10:26:54 +0100
Subject: [PATCH 1/7] add handlebars taint step

---
 .../javascript/frameworks/Handlebars.qll      | 66 +++++++++++++++++++
 .../dataflow/TaintedPathCustomizations.qll    |  2 +-
 .../CWE-022/TaintedPath/TaintedPath.expected  | 45 +++++++++++++
 .../CWE-022/TaintedPath/handlebars.js         | 32 +++++++++
 4 files changed, 144 insertions(+), 1 deletion(-)
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js

diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
index 40c52849c255..76aa79443d15 100644
--- a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
+++ b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
@@ -27,3 +27,69 @@ module Handlebars {
     SafeString() { this = any(Handlebars h).getAConstructorInvocation("SafeString") }
   }
 }
+
+/** Provides logic for taint steps for the handlebars library. */
+module TaintStep {
+  /**
+   * Gets a `SourceNode` tracked from a compilation of a Handlebars template.
+   */
+  private DataFlow::SourceNode compiledHandlebarsTemplate(DataFlow::Node originalCall) {
+    result = compiledHandlebarsTemplate(DataFlow::TypeTracker::end(), originalCall)
+  }
+
+  private DataFlow::SourceNode compiledHandlebarsTemplate(
+    DataFlow::TypeTracker t, DataFlow::Node originalCall
+  ) {
+    t.start() and
+    result = any(Handlebars::Handlebars hb).getAMethodCall(["compile", "template"]) and
+    result = originalCall
+    or
+    exists(DataFlow::TypeTracker t2 |
+      result = compiledHandlebarsTemplate(t2, originalCall).track(t2, t)
+    )
+  }
+
+  /**
+   * Holds if there's a step from `pred` to `succ` due to templating data being
+   * passed from a templating call to a registered helper via a parameter.
+   */
+  private predicate isHandlebarsArgStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(string helperName |
+      exists(DataFlow::CallNode templatingCall, DataFlow::CallNode compileCall |
+        templatingCall = compiledHandlebarsTemplate(compileCall).getACall() and
+        pred = templatingCall.getAnArgument().getALocalSource().getAPropertyWrite().getRhs() and
+        (
+          compileCall
+              .getArgument(0)
+              .mayHaveStringValue(any(string templateText |
+                  // an approximation: if the template contains the
+                  // helper name, it's probably a helper call
+                  templateText.matches("%" + helperName + "%")
+                ))
+          or
+          // When we don't have a string value, we can't be sure
+          // and will assume a step.
+          not exists(string s | compileCall.getArgument(0).mayHaveStringValue(s))
+        )
+      ) and
+      exists(DataFlow::CallNode registerHelperCall, DataFlow::FunctionNode helperFunc |
+        registerHelperCall = any(Handlebars::Handlebars hb).getAMethodCall("registerHelper") and
+        registerHelperCall.getArgument(0).mayHaveStringValue(helperName) and
+        helperFunc = registerHelperCall.getArgument(1).getAFunctionValue() and
+        helperFunc.getAParameter() = succ
+      )
+    )
+  }
+
+  /**
+   * A shared flow step from passing data to a handlebars template with
+   * helpers registered.
+   */
+  class HandlebarsStep extends DataFlow::SharedFlowStep {
+    DataFlow::CallNode templatingCall;
+
+    override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
+      isHandlebarsArgStep(node1, node2)
+    }
+  }
+}
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
index bc84ba5eb77d..c094d82163c7 100644
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
@@ -850,7 +850,7 @@ module TaintedPath {
 
   /**
    * Holds if we should include a step from `src -> dst` with labels `srclabel -> dstlabel`, and the
-   * standard taint step `src -> dst` should be suppresesd.
+   * standard taint step `src -> dst` should be suppressed.
    */
   private predicate isPosixPathStep(
     DataFlow::Node src, DataFlow::Node dst, Label::PosixPath srclabel, Label::PosixPath dstlabel
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
index 595c0f39cb9d..0a31bde88d4e 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
@@ -1541,6 +1541,25 @@ nodes
 | express.js:8:20:8:32 | req.query.bar |
 | express.js:8:20:8:32 | req.query.bar |
 | express.js:8:20:8:32 | req.query.bar |
+| handlebars.js:19:46:19:60 | req.params.path |
+| handlebars.js:19:46:19:60 | req.params.path |
+| handlebars.js:19:46:19:60 | req.params.path |
+| handlebars.js:19:46:19:60 | req.params.path |
+| handlebars.js:19:46:19:60 | req.params.path |
+| handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name |
+| handlebars.js:31:43:31:57 | req.params.name |
+| handlebars.js:31:43:31:57 | req.params.name |
+| handlebars.js:31:43:31:57 | req.params.name |
+| handlebars.js:31:43:31:57 | req.params.name |
 | normalizedPaths.js:11:7:11:27 | path |
 | normalizedPaths.js:11:7:11:27 | path |
 | normalizedPaths.js:11:7:11:27 | path |
@@ -6414,6 +6433,30 @@ edges
 | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
 | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
 | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
 | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
 | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
 | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
@@ -9844,6 +9887,8 @@ edges
 | TaintedPath.js:213:45:213:48 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:213:45:213:48 | path | This path depends on $@. | TaintedPath.js:211:24:211:30 | req.url | a user-provided value |
 | TaintedPath.js:214:35:214:38 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:214:35:214:38 | path | This path depends on $@. | TaintedPath.js:211:24:211:30 | req.url | a user-provided value |
 | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar | This path depends on $@. | express.js:8:20:8:32 | req.query.bar | a user-provided value |
+| handlebars.js:23:28:23:35 | filePath | handlebars.js:19:46:19:60 | req.params.path | handlebars.js:23:28:23:35 | filePath | This path depends on $@. | handlebars.js:19:46:19:60 | req.params.path | a user-provided value |
+| handlebars.js:23:28:23:35 | filePath | handlebars.js:31:43:31:57 | req.params.name | handlebars.js:23:28:23:35 | filePath | This path depends on $@. | handlebars.js:31:43:31:57 | req.params.name | a user-provided value |
 | normalizedPaths.js:13:19:13:22 | path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:13:19:13:22 | path | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
 | normalizedPaths.js:14:19:14:29 | './' + path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:14:19:14:29 | './' + path | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
 | normalizedPaths.js:15:19:15:38 | path + '/index.html' | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:15:19:15:38 | path + '/index.html' | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js
new file mode 100644
index 000000000000..47d77e190f92
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js
@@ -0,0 +1,32 @@
+const express = require('express');
+const hb = require("handlebars");
+const fs = require("fs");
+
+const app = express();
+
+const data = {};
+
+function init() {
+    data.compiledFileAccess = hb.compile("contents of file {{path}} are: {{catFile path}}")
+    data.compiledBenign = hb.compile("hello, {{name}}");
+    data.compiledUnknown = hb.compile(fs.readFileSync("greeting.template"));
+    hb.registerHelper("catFile", catFile);
+}
+
+init();
+
+app.get('/some/path1', function (req, res) {
+    res.send(data.compiledFileAccess({ path: req.params.path })); // NOT ALLOWED (template uses vulnerable catFile)
+});
+
+function catFile(filePath) {
+    return fs.readFileSync(filePath); // SINK (reads file)
+}
+
+app.get('/some/path2', function (req, res) {
+    res.send(data.compiledBenign({ name: req.params.name })); // ALLOWED (this template does not use catFile)
+});
+
+app.get('/some/path3', function (req, res) { 
+    res.send(data.compiledUnknown({ name: req.params.name })); // NOT ALLOWED (could be using vulnerable catFile)
+});
\ No newline at end of file

From a28e9c5b6eee43a579bc2fa68ecbd4bdd7480cf7 Mon Sep 17 00:00:00 2001
From: Stephan Brandauer <kaeluka@github.com>
Date: Thu, 24 Mar 2022 13:08:52 +0100
Subject: [PATCH 2/7] documentation for handlebars.js flow step

---
 .../semmle/javascript/frameworks/Handlebars.qll | 17 +++++++++++++++++
 .../Security/CWE-022/TaintedPath/handlebars.js  |  2 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
index 76aa79443d15..97a62031fd16 100644
--- a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
+++ b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
@@ -52,6 +52,23 @@ module TaintStep {
   /**
    * Holds if there's a step from `pred` to `succ` due to templating data being
    * passed from a templating call to a registered helper via a parameter.
+   *
+   * To establish the step, we look at the template passed to `compile`, and will
+   * only track steps from templates to helpers they actually reference.
+   *
+   * ```javascript
+   * function loudHelper(text) {
+   *   //                ^^^^ succ
+   *   return text.toUpperCase();
+   * }
+   *
+   * hb.registerHelper("loud", loudHelper);
+   *
+   * const template = hb.compile("Hello, {{loud name}}!");
+   *
+   * template({name: "user"});
+   * //              ^^^^^^ pred
+   * ```
    */
   private predicate isHandlebarsArgStep(DataFlow::Node pred, DataFlow::Node succ) {
     exists(string helperName |
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js
index 47d77e190f92..5b2648dfbd1f 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js
@@ -7,10 +7,10 @@ const app = express();
 const data = {};
 
 function init() {
+    hb.registerHelper("catFile", catFile);
     data.compiledFileAccess = hb.compile("contents of file {{path}} are: {{catFile path}}")
     data.compiledBenign = hb.compile("hello, {{name}}");
     data.compiledUnknown = hb.compile(fs.readFileSync("greeting.template"));
-    hb.registerHelper("catFile", catFile);
 }
 
 init();

From 9c3fcb6268d0478feac2231f8ef87a695a91c8a6 Mon Sep 17 00:00:00 2001
From: Stephan Brandauer <kaeluka@github.com>
Date: Mon, 28 Mar 2022 17:26:43 +0200
Subject: [PATCH 3/7] precise tracking of handlebars arguments

---
 .../javascript/frameworks/Handlebars.qll      | 106 +++++++++-----
 .../CWE-022/TaintedPath/TaintedPath.expected  | 130 ++++++++++++------
 .../CWE-022/TaintedPath/handlebars.js         |  34 ++++-
 3 files changed, 185 insertions(+), 85 deletions(-)

diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
index 97a62031fd16..777825263e7f 100644
--- a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
+++ b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
@@ -29,23 +29,66 @@ module Handlebars {
 }
 
 /** Provides logic for taint steps for the handlebars library. */
-module TaintStep {
+private module HandlebarsTaintSteps {
   /**
-   * Gets a `SourceNode` tracked from a compilation of a Handlebars template.
+   * Gets a reference to a compiled Handlebars template.
    */
-  private DataFlow::SourceNode compiledHandlebarsTemplate(DataFlow::Node originalCall) {
-    result = compiledHandlebarsTemplate(DataFlow::TypeTracker::end(), originalCall)
+  private DataFlow::SourceNode compiledTemplate(DataFlow::CallNode compileCall) {
+    result = compiledTemplate(DataFlow::TypeTracker::end(), compileCall)
   }
 
-  private DataFlow::SourceNode compiledHandlebarsTemplate(
-    DataFlow::TypeTracker t, DataFlow::Node originalCall
+  private DataFlow::SourceNode compiledTemplate(
+    DataFlow::TypeTracker t, DataFlow::CallNode compileCall
   ) {
     t.start() and
     result = any(Handlebars::Handlebars hb).getAMethodCall(["compile", "template"]) and
-    result = originalCall
+    result = compileCall
     or
-    exists(DataFlow::TypeTracker t2 |
-      result = compiledHandlebarsTemplate(t2, originalCall).track(t2, t)
+    exists(DataFlow::TypeTracker t2 | result = compiledTemplate(t2, compileCall).track(t2, t))
+  }
+
+  private predicate isHelperParam(
+    string helperName, DataFlow::FunctionNode helperFunction, DataFlow::ParameterNode param,
+    int paramIndex
+  ) {
+    exists(DataFlow::CallNode registerHelperCall |
+      registerHelperCall = any(Handlebars::Handlebars hb).getAMethodCall("registerHelper") and
+      registerHelperCall.getArgument(0).mayHaveStringValue(helperName) and
+      helperFunction = registerHelperCall.getArgument(1).getAFunctionValue() and
+      param = helperFunction.getParameter(paramIndex)
+    )
+  }
+
+  /** Holds if `call` is a block wrapped inside curly braces inside the template `templateText`. */
+  bindingset[templateText]
+  predicate templateHelperParamBlock(string templateText, string call) {
+    call = templateText.regexpFind("\\{\\{[^}]+\\}\\}", _, _).regexpReplaceAll("[{}]", "").trim() and
+    call.regexpMatch(".*\\s.*")
+  }
+
+  /**
+   * Holds for calls to helpers from handlebars templates.
+   *
+   * ```javascript
+   * hb.compile("contents of file {{path}} are: {{catFile path}} {{echo p1 p2}}");
+   * ```
+   *
+   * In the example, the predicate will hold for:
+   *
+   * * helperName="catFile", argIdx=1, arg="path"
+   * * helperName="echo", argIdx=1, arg="p1"
+   * * helperName="echo", argIdx=2, arg="p2"
+   *
+   * The initial `{{path}}` will not be considered, as it has no arguments.
+   */
+  bindingset[templateText]
+  private predicate isTemplateHelperCallArg(
+    string templateText, string helperName, int argIdx, string argVal
+  ) {
+    exists(string call | templateHelperParamBlock(templateText, call) |
+      helperName = call.regexpFind("[^\\s]+", 0, _) and
+      argIdx >= 0 and
+      argVal = call.regexpFind("[^\\s]+", argIdx + 1, _)
     )
   }
 
@@ -71,29 +114,26 @@ module TaintStep {
    * ```
    */
   private predicate isHandlebarsArgStep(DataFlow::Node pred, DataFlow::Node succ) {
-    exists(string helperName |
-      exists(DataFlow::CallNode templatingCall, DataFlow::CallNode compileCall |
-        templatingCall = compiledHandlebarsTemplate(compileCall).getACall() and
-        pred = templatingCall.getAnArgument().getALocalSource().getAPropertyWrite().getRhs() and
-        (
-          compileCall
-              .getArgument(0)
-              .mayHaveStringValue(any(string templateText |
-                  // an approximation: if the template contains the
-                  // helper name, it's probably a helper call
-                  templateText.matches("%" + helperName + "%")
-                ))
-          or
-          // When we don't have a string value, we can't be sure
-          // and will assume a step.
-          not exists(string s | compileCall.getArgument(0).mayHaveStringValue(s))
+    exists(
+      string helperName, DataFlow::CallNode templatingCall, DataFlow::CallNode compileCall,
+      DataFlow::FunctionNode helperFunction
+    |
+      templatingCall = compiledTemplate(compileCall).getACall() and
+      (
+        exists(string templateText, string paramName, int argIdx |
+          compileCall.getArgument(0).mayHaveStringValue(templateText)
+        |
+          pred =
+            templatingCall.getAnArgument().getALocalSource().getAPropertyWrite(paramName).getRhs() and
+          isTemplateHelperCallArg(templateText, helperName, argIdx, paramName) and
+          isHelperParam(helperName, helperFunction, succ, argIdx)
         )
-      ) and
-      exists(DataFlow::CallNode registerHelperCall, DataFlow::FunctionNode helperFunc |
-        registerHelperCall = any(Handlebars::Handlebars hb).getAMethodCall("registerHelper") and
-        registerHelperCall.getArgument(0).mayHaveStringValue(helperName) and
-        helperFunc = registerHelperCall.getArgument(1).getAFunctionValue() and
-        helperFunc.getAParameter() = succ
+        or
+        // When we don't have a string value, we can't be sure
+        // and will assume a step.
+        not exists(string s | compileCall.getArgument(0).mayHaveStringValue(s)) and
+        pred = templatingCall.getAnArgument().getALocalSource().getAPropertyWrite().getRhs() and
+        isHelperParam(helperName, helperFunction, succ, _)
       )
     )
   }
@@ -105,8 +145,8 @@ module TaintStep {
   class HandlebarsStep extends DataFlow::SharedFlowStep {
     DataFlow::CallNode templatingCall;
 
-    override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
-      isHandlebarsArgStep(node1, node2)
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      isHandlebarsArgStep(pred, succ)
     }
   }
 }
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
index 0a31bde88d4e..25baf9af117f 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
@@ -1541,25 +1541,39 @@ nodes
 | express.js:8:20:8:32 | req.query.bar |
 | express.js:8:20:8:32 | req.query.bar |
 | express.js:8:20:8:32 | req.query.bar |
-| handlebars.js:19:46:19:60 | req.params.path |
-| handlebars.js:19:46:19:60 | req.params.path |
-| handlebars.js:19:46:19:60 | req.params.path |
-| handlebars.js:19:46:19:60 | req.params.path |
-| handlebars.js:19:46:19:60 | req.params.path |
-| handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:23:28:23:35 | filePath |
-| handlebars.js:23:28:23:35 | filePath |
-| handlebars.js:23:28:23:35 | filePath |
-| handlebars.js:23:28:23:35 | filePath |
-| handlebars.js:23:28:23:35 | filePath |
-| handlebars.js:31:43:31:57 | req.params.name |
-| handlebars.js:31:43:31:57 | req.params.name |
-| handlebars.js:31:43:31:57 | req.params.name |
-| handlebars.js:31:43:31:57 | req.params.name |
-| handlebars.js:31:43:31:57 | req.params.name |
+| handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path |
+| handlebars.js:29:46:29:60 | req.params.path |
+| handlebars.js:29:46:29:60 | req.params.path |
+| handlebars.js:29:46:29:60 | req.params.path |
+| handlebars.js:29:46:29:60 | req.params.path |
+| handlebars.js:37:43:37:57 | req.params.name |
+| handlebars.js:37:43:37:57 | req.params.name |
+| handlebars.js:37:43:37:57 | req.params.name |
+| handlebars.js:37:43:37:57 | req.params.name |
+| handlebars.js:37:43:37:57 | req.params.name |
+| handlebars.js:43:15:43:29 | req.params.path |
+| handlebars.js:43:15:43:29 | req.params.path |
+| handlebars.js:43:15:43:29 | req.params.path |
+| handlebars.js:43:15:43:29 | req.params.path |
+| handlebars.js:43:15:43:29 | req.params.path |
 | normalizedPaths.js:11:7:11:27 | path |
 | normalizedPaths.js:11:7:11:27 | path |
 | normalizedPaths.js:11:7:11:27 | path |
@@ -6433,30 +6447,54 @@ edges
 | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
 | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
 | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar |
-| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
-| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
-| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
-| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
-| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
-| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
-| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
-| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
-| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
-| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
+| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
 | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
 | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
 | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
@@ -9887,8 +9925,10 @@ edges
 | TaintedPath.js:213:45:213:48 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:213:45:213:48 | path | This path depends on $@. | TaintedPath.js:211:24:211:30 | req.url | a user-provided value |
 | TaintedPath.js:214:35:214:38 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:214:35:214:38 | path | This path depends on $@. | TaintedPath.js:211:24:211:30 | req.url | a user-provided value |
 | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar | This path depends on $@. | express.js:8:20:8:32 | req.query.bar | a user-provided value |
-| handlebars.js:23:28:23:35 | filePath | handlebars.js:19:46:19:60 | req.params.path | handlebars.js:23:28:23:35 | filePath | This path depends on $@. | handlebars.js:19:46:19:60 | req.params.path | a user-provided value |
-| handlebars.js:23:28:23:35 | filePath | handlebars.js:31:43:31:57 | req.params.name | handlebars.js:23:28:23:35 | filePath | This path depends on $@. | handlebars.js:31:43:31:57 | req.params.name | a user-provided value |
+| handlebars.js:11:32:11:39 | filePath | handlebars.js:29:46:29:60 | req.params.path | handlebars.js:11:32:11:39 | filePath | This path depends on $@. | handlebars.js:29:46:29:60 | req.params.path | a user-provided value |
+| handlebars.js:11:32:11:39 | filePath | handlebars.js:37:43:37:57 | req.params.name | handlebars.js:11:32:11:39 | filePath | This path depends on $@. | handlebars.js:37:43:37:57 | req.params.name | a user-provided value |
+| handlebars.js:15:25:15:32 | filePath | handlebars.js:37:43:37:57 | req.params.name | handlebars.js:15:25:15:32 | filePath | This path depends on $@. | handlebars.js:37:43:37:57 | req.params.name | a user-provided value |
+| handlebars.js:15:25:15:32 | filePath | handlebars.js:43:15:43:29 | req.params.path | handlebars.js:15:25:15:32 | filePath | This path depends on $@. | handlebars.js:43:15:43:29 | req.params.path | a user-provided value |
 | normalizedPaths.js:13:19:13:22 | path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:13:19:13:22 | path | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
 | normalizedPaths.js:14:19:14:29 | './' + path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:14:19:14:29 | './' + path | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
 | normalizedPaths.js:15:19:15:38 | path + '/index.html' | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:15:19:15:38 | path + '/index.html' | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js
index 5b2648dfbd1f..f1ebc15c8d44 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js
@@ -7,10 +7,20 @@ const app = express();
 const data = {};
 
 function init() {
-    hb.registerHelper("catFile", catFile);
+    hb.registerHelper("catFile", function catFile(filePath) {
+        return fs.readFileSync(filePath); // SINK (reads file)
+    });
+    hb.registerHelper("prependToLines", function prependToLines(prefix, filePath) {
+        return fs
+          .readFileSync(filePath)
+          .split("\n")
+          .map((line) => prefix + line)
+          .join("\n");
+    });
     data.compiledFileAccess = hb.compile("contents of file {{path}} are: {{catFile path}}")
     data.compiledBenign = hb.compile("hello, {{name}}");
     data.compiledUnknown = hb.compile(fs.readFileSync("greeting.template"));
+    data.compiledMixed = hb.compile("helpers may have several args, like here: {{prependToLines prefix path}}");
 }
 
 init();
@@ -19,14 +29,24 @@ app.get('/some/path1', function (req, res) {
     res.send(data.compiledFileAccess({ path: req.params.path })); // NOT ALLOWED (template uses vulnerable catFile)
 });
 
-function catFile(filePath) {
-    return fs.readFileSync(filePath); // SINK (reads file)
-}
-
 app.get('/some/path2', function (req, res) {
     res.send(data.compiledBenign({ name: req.params.name })); // ALLOWED (this template does not use catFile)
 });
 
-app.get('/some/path3', function (req, res) { 
-    res.send(data.compiledUnknown({ name: req.params.name })); // NOT ALLOWED (could be using vulnerable catFile)
+app.get('/some/path3', function (req, res) {
+    res.send(data.compiledUnknown({ name: req.params.name })); // NOT ALLOWED (could be using a vulnerable helper)
+});
+
+app.get('/some/path4', function (req, res) {
+    res.send(data.compiledMixed({
+        prefix: ">>> ",
+        path: req.params.path // NOT ALLOWED (template uses vulnerable helper)
+    }));
+});
+
+app.get('/some/path5', function (req, res) {
+    res.send(data.compiledMixed({
+        prefix: req.params.prefix, // ALLOWED (this parameter is safe)
+        path: "data/path-5.txt"
+    }));
 });
\ No newline at end of file

From a6d2ecdc4d1b4399248060da519b8b342f8961b8 Mon Sep 17 00:00:00 2001
From: Stephan Brandauer <kaeluka@github.com>
Date: Thu, 31 Mar 2022 10:49:33 +0200
Subject: [PATCH 4/7] review comments

---
 .../javascript/frameworks/Handlebars.qll      | 42 +++++++++++++------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
index 777825263e7f..b7401da2e5ef 100644
--- a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
+++ b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
@@ -47,23 +47,39 @@ private module HandlebarsTaintSteps {
     exists(DataFlow::TypeTracker t2 | result = compiledTemplate(t2, compileCall).track(t2, t))
   }
 
-  private predicate isHelperParam(
-    string helperName, DataFlow::FunctionNode helperFunction, DataFlow::ParameterNode param,
+  /**
+   * Gets a reference to a parameter of a registered Handlebars helper.
+   * 
+   * ```javascript
+   * function loudHelper(text) { 
+   *   return text.toUpperCase();
+   * }
+   *
+   * hb.registerHelper("loud", loudHelper);
+   * ```
+   * In this example, `getRegisteredHelperParameter("loud", func, 0)` will bind `func` to
+   * the `FunctionNode` representing `function loudHelper`, and return its parameter `text`.
+   */
+  private DataFlow::ParameterNode getRegisteredHelperParam(
+    string helperName, DataFlow::FunctionNode helperFunction,
     int paramIndex
   ) {
     exists(DataFlow::CallNode registerHelperCall |
-      registerHelperCall = any(Handlebars::Handlebars hb).getAMethodCall("registerHelper") and
+      registerHelperCall = any(Handlebars::Handlebars hb).getAMemberCall("registerHelper") and
       registerHelperCall.getArgument(0).mayHaveStringValue(helperName) and
       helperFunction = registerHelperCall.getArgument(1).getAFunctionValue() and
-      param = helperFunction.getParameter(paramIndex)
+      result = helperFunction.getParameter(paramIndex)
     )
   }
 
-  /** Holds if `call` is a block wrapped inside curly braces inside the template `templateText`. */
+  /** Gets a `call` (which is a block wrapped inside curly braces inside the template) from `templateText`.
+   * 
+   * For example, `getAHelperCallFromTemplate("Hello {{loud customer}}")` will return `"loud customer"`.
+   */
   bindingset[templateText]
-  predicate templateHelperParamBlock(string templateText, string call) {
-    call = templateText.regexpFind("\\{\\{[^}]+\\}\\}", _, _).regexpReplaceAll("[{}]", "").trim() and
-    call.regexpMatch(".*\\s.*")
+  private string getAHelperCallFromTemplate(string templateText) {
+    result = templateText.regexpFind("\\{\\{[^}]+\\}\\}", _, _).regexpReplaceAll("[{}]", "").trim() and
+    result.regexpMatch(".*\\s.*")
   }
 
   /**
@@ -85,7 +101,7 @@ private module HandlebarsTaintSteps {
   private predicate isTemplateHelperCallArg(
     string templateText, string helperName, int argIdx, string argVal
   ) {
-    exists(string call | templateHelperParamBlock(templateText, call) |
+    exists(string call | call = getAHelperCallFromTemplate(templateText) |
       helperName = call.regexpFind("[^\\s]+", 0, _) and
       argIdx >= 0 and
       argVal = call.regexpFind("[^\\s]+", argIdx + 1, _)
@@ -126,14 +142,14 @@ private module HandlebarsTaintSteps {
           pred =
             templatingCall.getAnArgument().getALocalSource().getAPropertyWrite(paramName).getRhs() and
           isTemplateHelperCallArg(templateText, helperName, argIdx, paramName) and
-          isHelperParam(helperName, helperFunction, succ, argIdx)
+          succ = getRegisteredHelperParam(helperName, helperFunction, argIdx)
         )
         or
         // When we don't have a string value, we can't be sure
-        // and will assume a step.
+        // and we assume a step to all parameters of all helpers.
         not exists(string s | compileCall.getArgument(0).mayHaveStringValue(s)) and
-        pred = templatingCall.getAnArgument().getALocalSource().getAPropertyWrite().getRhs() and
-        isHelperParam(helperName, helperFunction, succ, _)
+        pred = templatingCall.getArgument(0).getALocalSource().getAPropertyWrite().getRhs() and
+        succ = getRegisteredHelperParam(helperName, helperFunction, _)
       )
     )
   }

From 8f1a3597a7bf9af78f2f2372a107d45dfbe46f2e Mon Sep 17 00:00:00 2001
From: Stephan Brandauer <kaeluka@github.com>
Date: Thu, 31 Mar 2022 11:00:33 +0200
Subject: [PATCH 5/7] autoformat

---
 .../lib/semmle/javascript/frameworks/Handlebars.qll  | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
index b7401da2e5ef..d5c771425336 100644
--- a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
+++ b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
@@ -49,9 +49,9 @@ private module HandlebarsTaintSteps {
 
   /**
    * Gets a reference to a parameter of a registered Handlebars helper.
-   * 
+   *
    * ```javascript
-   * function loudHelper(text) { 
+   * function loudHelper(text) {
    *   return text.toUpperCase();
    * }
    *
@@ -61,8 +61,7 @@ private module HandlebarsTaintSteps {
    * the `FunctionNode` representing `function loudHelper`, and return its parameter `text`.
    */
   private DataFlow::ParameterNode getRegisteredHelperParam(
-    string helperName, DataFlow::FunctionNode helperFunction,
-    int paramIndex
+    string helperName, DataFlow::FunctionNode helperFunction, int paramIndex
   ) {
     exists(DataFlow::CallNode registerHelperCall |
       registerHelperCall = any(Handlebars::Handlebars hb).getAMemberCall("registerHelper") and
@@ -72,8 +71,9 @@ private module HandlebarsTaintSteps {
     )
   }
 
-  /** Gets a `call` (which is a block wrapped inside curly braces inside the template) from `templateText`.
-   * 
+  /**
+   * Gets a `call` (which is a block wrapped inside curly braces inside the template) from `templateText`.
+   *
    * For example, `getAHelperCallFromTemplate("Hello {{loud customer}}")` will return `"loud customer"`.
    */
   bindingset[templateText]

From 2cbb25acaa4d635cb34b55e33cec3ed9b9173cd4 Mon Sep 17 00:00:00 2001
From: Stephan Brandauer <kaeluka@github.com>
Date: Thu, 31 Mar 2022 16:04:04 +0200
Subject: [PATCH 6/7] another review fix

---
 javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
index d5c771425336..aaa99c713103 100644
--- a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
+++ b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
@@ -140,7 +140,7 @@ private module HandlebarsTaintSteps {
           compileCall.getArgument(0).mayHaveStringValue(templateText)
         |
           pred =
-            templatingCall.getAnArgument().getALocalSource().getAPropertyWrite(paramName).getRhs() and
+            templatingCall.getArgument(0).getALocalSource().getAPropertyWrite(paramName).getRhs() and
           isTemplateHelperCallArg(templateText, helperName, argIdx, paramName) and
           succ = getRegisteredHelperParam(helperName, helperFunction, argIdx)
         )

From fb66ccff39302d2ea1bc40334e09b960cf0ef116 Mon Sep 17 00:00:00 2001
From: Stephan Brandauer <kaeluka@github.com>
Date: Wed, 13 Apr 2022 09:27:59 +0200
Subject: [PATCH 7/7] handlebars taint step: conservatively assume unknown
 templates have no flow to helpers

---
 .../javascript/frameworks/Handlebars.qll      | 21 +++++------------
 .../CWE-022/TaintedPath/TaintedPath.expected  | 23 -------------------
 .../CWE-022/TaintedPath/handlebars.js         |  4 ++--
 3 files changed, 8 insertions(+), 40 deletions(-)

diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
index aaa99c713103..8eb3b5d7eaed 100644
--- a/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
+++ b/javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll
@@ -135,21 +135,12 @@ private module HandlebarsTaintSteps {
       DataFlow::FunctionNode helperFunction
     |
       templatingCall = compiledTemplate(compileCall).getACall() and
-      (
-        exists(string templateText, string paramName, int argIdx |
-          compileCall.getArgument(0).mayHaveStringValue(templateText)
-        |
-          pred =
-            templatingCall.getArgument(0).getALocalSource().getAPropertyWrite(paramName).getRhs() and
-          isTemplateHelperCallArg(templateText, helperName, argIdx, paramName) and
-          succ = getRegisteredHelperParam(helperName, helperFunction, argIdx)
-        )
-        or
-        // When we don't have a string value, we can't be sure
-        // and we assume a step to all parameters of all helpers.
-        not exists(string s | compileCall.getArgument(0).mayHaveStringValue(s)) and
-        pred = templatingCall.getArgument(0).getALocalSource().getAPropertyWrite().getRhs() and
-        succ = getRegisteredHelperParam(helperName, helperFunction, _)
+      exists(string templateText, string paramName, int argIdx |
+        compileCall.getArgument(0).mayHaveStringValue(templateText)
+      |
+        pred = templatingCall.getArgument(0).getALocalSource().getAPropertyWrite(paramName).getRhs() and
+        isTemplateHelperCallArg(templateText, helperName, argIdx, paramName) and
+        succ = getRegisteredHelperParam(helperName, helperFunction, argIdx)
       )
     )
   }
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
index 25baf9af117f..e8ca5f0f5ff6 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
@@ -1564,11 +1564,6 @@ nodes
 | handlebars.js:29:46:29:60 | req.params.path |
 | handlebars.js:29:46:29:60 | req.params.path |
 | handlebars.js:29:46:29:60 | req.params.path |
-| handlebars.js:37:43:37:57 | req.params.name |
-| handlebars.js:37:43:37:57 | req.params.name |
-| handlebars.js:37:43:37:57 | req.params.name |
-| handlebars.js:37:43:37:57 | req.params.name |
-| handlebars.js:37:43:37:57 | req.params.name |
 | handlebars.js:43:15:43:29 | req.params.path |
 | handlebars.js:43:15:43:29 | req.params.path |
 | handlebars.js:43:15:43:29 | req.params.path |
@@ -6471,22 +6466,6 @@ edges
 | handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
 | handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
 | handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:10:51:10:58 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
-| handlebars.js:37:43:37:57 | req.params.name | handlebars.js:13:73:13:80 | filePath |
 | handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
 | handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
 | handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
@@ -9926,8 +9905,6 @@ edges
 | TaintedPath.js:214:35:214:38 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:214:35:214:38 | path | This path depends on $@. | TaintedPath.js:211:24:211:30 | req.url | a user-provided value |
 | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar | This path depends on $@. | express.js:8:20:8:32 | req.query.bar | a user-provided value |
 | handlebars.js:11:32:11:39 | filePath | handlebars.js:29:46:29:60 | req.params.path | handlebars.js:11:32:11:39 | filePath | This path depends on $@. | handlebars.js:29:46:29:60 | req.params.path | a user-provided value |
-| handlebars.js:11:32:11:39 | filePath | handlebars.js:37:43:37:57 | req.params.name | handlebars.js:11:32:11:39 | filePath | This path depends on $@. | handlebars.js:37:43:37:57 | req.params.name | a user-provided value |
-| handlebars.js:15:25:15:32 | filePath | handlebars.js:37:43:37:57 | req.params.name | handlebars.js:15:25:15:32 | filePath | This path depends on $@. | handlebars.js:37:43:37:57 | req.params.name | a user-provided value |
 | handlebars.js:15:25:15:32 | filePath | handlebars.js:43:15:43:29 | req.params.path | handlebars.js:15:25:15:32 | filePath | This path depends on $@. | handlebars.js:43:15:43:29 | req.params.path | a user-provided value |
 | normalizedPaths.js:13:19:13:22 | path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:13:19:13:22 | path | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
 | normalizedPaths.js:14:19:14:29 | './' + path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:14:19:14:29 | './' + path | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js
index f1ebc15c8d44..512b851592aa 100644
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js
@@ -34,7 +34,7 @@ app.get('/some/path2', function (req, res) {
 });
 
 app.get('/some/path3', function (req, res) {
-    res.send(data.compiledUnknown({ name: req.params.name })); // NOT ALLOWED (could be using a vulnerable helper)
+    res.send(data.compiledUnknown({ name: req.params.name })); // ALLOWED (could be using a vulnerable helper, but we'll assume it's ok)
 });
 
 app.get('/some/path4', function (req, res) {
@@ -49,4 +49,4 @@ app.get('/some/path5', function (req, res) {
         prefix: req.params.prefix, // ALLOWED (this parameter is safe)
         path: "data/path-5.txt"
     }));
-});
\ No newline at end of file
+});