Merge pull request #336 from github/improve-getTemplateFile

Improve `RenderCall#getTemplateFile` performance and accuracy

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

@@ -260,8 +260,28 @@ class ErbFile extends File {
    */
   predicate isPartial() { this.getStem().charAt(0) = "_" }
 
+  /**
+   * Gets the base template name associated with this ERB file.
+   * For instance, a file named `foo.html.erb` has a template name of `foo`.
+   * A partial template file named `_item.html.erb` has a template name of `item`.
+   */
+  string getTemplateName() { none() }
+
   /**
    * Gets the erb template contained within this file.
    */
   ErbTemplate getTemplate() { result = template }
 }
+
+private class PartialErbFile extends ErbFile {
+  PartialErbFile() { this.isPartial() }
+
+  // Drop the leading underscore
+  override string getTemplateName() { result = this.getStem().splitAt(".", 0).suffix(1) }
+}
+
+private class FullErbFile extends ErbFile {
+  FullErbFile() { not this.isPartial() }
+
+  override string getTemplateName() { result = this.getStem().splitAt(".", 0) }
+}

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

@@ -73,48 +73,30 @@ private class ActionViewParamsCall extends ActionViewContextCall, ParamsCall { }
 abstract class RenderCall extends MethodCall {
   RenderCall() { this.getMethodName() = "render" }
 
-  private string getWorkingDirectory() {
-    result = this.getLocation().getFile().getParentContainer().getAbsolutePath()
+  private Expr getTemplatePathArgument() {
+    // TODO: support other ways of specifying paths (e.g. `file`)
+    result = [this.getKeywordArgument(["partial", "template", "action"]), this.getArgument(0)]
   }
 
-  bindingset[templatePath]
-  private string templatePathPattern(string templatePath) {
-    exists(string basename, string relativeRoot |
-      // everything after the final slash, or the whole string if there is no slash
-      basename = templatePath.regexpCapture("^(?:.*/)?([^/]*)$", 1) and
-      // everything up to and including the final slash
-      relativeRoot = templatePath.regexpCapture("^(.*/)?(?:[^/]*?)$", 1)
-    |
-      (
-        // path relative to <source_prefix>/app/views/
-        result = "%/app/views/" + relativeRoot + "%" + basename + "%"
-        or
-        // relative to file containing call
-        result = this.getWorkingDirectory() + "%" + templatePath + "%"
-      )
-    )
+  private string getTemplatePathValue() { result = this.getTemplatePathArgument().getValueText() }
+
+  // everything up to and including the final slash, but ignoring any leading slash
+  private string getSubPath() {
+    result = this.getTemplatePathValue().regexpCapture("^/?(.*/)?(?:[^/]*?)$", 1)
   }
 
-  private string getTemplatePathPatterns() {
-    exists(string templatePath |
-      exists(Expr arg |
-        // TODO: support other ways of specifying paths (e.g. `file`)
-        arg = this.getKeywordArgument("partial") or
-        arg = this.getKeywordArgument("template") or
-        arg = this.getKeywordArgument("action") or
-        arg = this.getArgument(0)
-      |
-        templatePath = arg.(StringlikeLiteral).getValueText()
-      )
-    |
-      result = this.templatePathPattern(templatePath)
-    )
+  // everything after the final slash, or the whole string if there is no slash
+  private string getBaseName() {
+    result = this.getTemplatePathValue().regexpCapture("^/?(?:.*/)?([^/]*?)$", 1)
   }
 
   /**
-   * Get the template file to be rendered by this call, if any.
+   * Gets the template file to be rendered by this call, if any.
    */
-  ErbFile getTemplateFile() { result.getAbsolutePath().matches(this.getTemplatePathPatterns()) }
+  ErbFile getTemplateFile() {
+    result.getTemplateName() = this.getBaseName() and
+    result.getRelativePath().matches("%app/views/" + this.getSubPath() + "%")
+  }
 
   /**
    * Get the local variables passed as context to the renderer

ql/test/query-tests/security/cwe-079/app/views/foo/bars/show_unused.html.erb

@@ -0,0 +1,5 @@
+<%# This file is not rendered. There should be no flow into it. %>
+
+<%= display_text.html_safe %>
+<%= local_assigns[:display_text].html_safe %>
+<%= @instance_text.html_safe %>

ql/test/query-tests/security/cwe-079/app/views/foo/stores/show_unused.html.erb

@@ -0,0 +1,5 @@
+<%# This file is not rendered. There should be no flow into it. %>
+
+<%= display_text.html_safe %>
+<%= local_assigns[:display_text].html_safe %>
+<%= @instance_text.html_safe %>