review comments

Author: Stephan Brandauer

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, _)
       )
     )
   }