Ruby: remove public abstract classes for Action{View,Controller}

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

@@ -8,8 +8,10 @@ private import codeql.ruby.controlflow.CfgNodes
 private import codeql.ruby.DataFlow
 private import codeql.ruby.dataflow.RemoteFlowSources
 private import codeql.ruby.ApiGraphs
-private import codeql.ruby.frameworks.ActionView
 private import codeql.ruby.frameworks.ActionDispatch
+private import codeql.ruby.frameworks.ActionView
+private import codeql.ruby.frameworks.Rails
+private import codeql.ruby.frameworks.internal.Rails
 
 /**
  * A `ClassDeclaration` for a class that extends `ActionController::Base`.
@@ -119,13 +121,6 @@ private class ActionControllerContextCall extends MethodCall {
   ActionControllerControllerClass getControllerClass() { result = controllerClass }
 }
 
-/**
- * A call to the `params` method to fetch the request parameters.
- */
-abstract class ParamsCall extends MethodCall {
-  ParamsCall() { this.getMethodName() = "params" }
-}
-
 /**
  * A `RemoteFlowSource::Range` to represent accessing the
  * ActionController parameters available via the `params` method.
@@ -136,13 +131,6 @@ class ParamsSource extends Http::Server::RequestInputAccess::Range {
   override string getSourceType() { result = "ActionController::Metal#params" }
 }
 
-/**
- * A call to the `cookies` method to fetch the request parameters.
- */
-abstract class CookiesCall extends MethodCall {
-  CookiesCall() { this.getMethodName() = "cookies" }
-}
-
 /**
  * A `RemoteFlowSource::Range` to represent accessing the
  * ActionController parameters available via the `cookies` method.
@@ -154,27 +142,38 @@ class CookiesSource extends Http::Server::RequestInputAccess::Range {
 }
 
 /** A call to `cookies` from within a controller. */
-private class ActionControllerCookiesCall extends ActionControllerContextCall, CookiesCall { }
+private class ActionControllerCookiesCall extends ActionControllerContextCall, CookiesCallImpl {
+  ActionControllerCookiesCall() { this.getMethodName() = "cookies" }
+}
 
 /** A call to `params` from within a controller. */
-private class ActionControllerParamsCall extends ActionControllerContextCall, ParamsCall { }
+private class ActionControllerParamsCall extends ActionControllerContextCall, ParamsCallImpl {
+  ActionControllerParamsCall() { this.getMethodName() = "params" }
+}
 
 /** A call to `render` from within a controller. */
-private class ActionControllerRenderCall extends ActionControllerContextCall, RenderCall { }
+private class ActionControllerRenderCall extends ActionControllerContextCall, RenderCallImpl {
+  ActionControllerRenderCall() { this.getMethodName() = "render" }
+}
 
 /** A call to `render_to` from within a controller. */
-private class ActionControllerRenderToCall extends ActionControllerContextCall, RenderToCall { }
+private class ActionControllerRenderToCall extends ActionControllerContextCall, RenderToCallImpl {
+  ActionControllerRenderToCall() { this.getMethodName() = ["render_to_body", "render_to_string"] }
+}
 
 /** A call to `html_safe` from within a controller. */
-private class ActionControllerHtmlSafeCall extends HtmlSafeCall {
+private class ActionControllerHtmlSafeCall extends HtmlSafeCallImpl {
   ActionControllerHtmlSafeCall() {
+    this.getMethodName() = "html_safe" and
     this.getEnclosingModule() instanceof ActionControllerControllerClass
   }
 }
 
 /** A call to `html_escape` from within a controller. */
-private class ActionControllerHtmlEscapeCall extends HtmlEscapeCall {
+private class ActionControllerHtmlEscapeCall extends HtmlEscapeCallImpl {
   ActionControllerHtmlEscapeCall() {
+    // "h" is aliased to "html_escape" in ActiveSupport
+    this.getMethodName() = ["html_escape", "html_escape_once", "h", "sanitize"] and
     this.getEnclosingModule() instanceof ActionControllerControllerClass
   }
 }

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

@@ -8,36 +8,20 @@ private import codeql.ruby.Concepts
 private import codeql.ruby.controlflow.CfgNodes
 private import codeql.ruby.DataFlow
 private import codeql.ruby.dataflow.RemoteFlowSources
-private import ActionController
+private import codeql.ruby.frameworks.internal.Rails
+private import codeql.ruby.frameworks.Rails
 
 /**
  * Holds if this AST node is in a context where `ActionView` methods are available.
  */
-predicate inActionViewContext(AstNode n) {
+private predicate inActionViewContext(AstNode n) {
   // Within a template
   n.getLocation().getFile() instanceof ErbFile
 }
 
-/**
- * A method call on a string to mark it as HTML safe for Rails.
- * Strings marked as such will not be automatically escaped when inserted into
- * HTML.
- */
-abstract class HtmlSafeCall extends MethodCall {
-  HtmlSafeCall() { this.getMethodName() = "html_safe" }
-}
-
-// A call to `html_safe` from within a template.
-private class ActionViewHtmlSafeCall extends HtmlSafeCall {
-  ActionViewHtmlSafeCall() { inActionViewContext(this) }
-}
-
-/**
- * A call to a method named "html_escape", "html_escape_once", or "h".
- */
-abstract class HtmlEscapeCall extends MethodCall {
-  // "h" is aliased to "html_escape" in ActiveSupport
-  HtmlEscapeCall() { this.getMethodName() = ["html_escape", "html_escape_once", "h", "sanitize"] }
+/** A call to `html_safe` from within a template. */
+private class ActionViewHtmlSafeCall extends HtmlSafeCallImpl {
+  ActionViewHtmlSafeCall() { this.getMethodName() = "html_safe" and inActionViewContext(this) }
 }
 
 /**
@@ -53,12 +37,16 @@ class RailsHtmlEscaping extends Escaping::Range, DataFlow::CallNode {
   override string getKind() { result = Escaping::getHtmlKind() }
 }
 
-// A call to `html_escape` from within a template.
-private class ActionViewHtmlEscapeCall extends HtmlEscapeCall {
-  ActionViewHtmlEscapeCall() { inActionViewContext(this) }
+/** A call to `html_escape` from within a template. */
+private class ActionViewHtmlEscapeCall extends HtmlEscapeCallImpl {
+  ActionViewHtmlEscapeCall() {
+    // "h" is aliased to "html_escape" in ActiveSupport
+    this.getMethodName() = ["html_escape", "html_escape_once", "h", "sanitize"] and
+    inActionViewContext(this)
+  }
 }
 
-// A call in a context where some commonly used `ActionView` methods are available.
+/** A call in a context where some commonly used `ActionView` methods are available. */
 private class ActionViewContextCall extends MethodCall {
   ActionViewContextCall() {
     this.getReceiver() instanceof SelfVariableAccess and
@@ -76,51 +64,14 @@ class RawCall extends ActionViewContextCall {
   RawCall() { this.getMethodName() = "raw" }
 }
 
-// A call to the `params` method within the context of a template.
-private class ActionViewParamsCall extends ActionViewContextCall, ParamsCall { }
+/** A call to the `params` method within the context of a template. */
+private class ActionViewParamsCall extends ActionViewContextCall, ParamsCallImpl {
+  ActionViewParamsCall() { this.getMethodName() = "params" }
+}
 
 // A call to the `cookies` method within the context of a template.
-private class ActionViewCookiesCall extends ActionViewContextCall, CookiesCall { }
-
-/**
- * A call to a `render` method that will populate the response body with the
- * rendered content.
- */
-abstract class RenderCall extends MethodCall {
-  RenderCall() { this.getMethodName() = "render" }
-
-  private Expr getTemplatePathArgument() {
-    // TODO: support other ways of specifying paths (e.g. `file`)
-    result = [this.getKeywordArgument(["partial", "template", "action"]), this.getArgument(0)]
-  }
-
-  private string getTemplatePathValue() {
-    result = this.getTemplatePathArgument().getConstantValue().getStringlikeValue()
-  }
-
-  // everything up to and including the final slash, but ignoring any leading slash
-  private string getSubPath() {
-    result = this.getTemplatePathValue().regexpCapture("^/?(.*/)?(?:[^/]*?)$", 1)
-  }
-
-  // everything after the final slash, or the whole string if there is no slash
-  private string getBaseName() {
-    result = this.getTemplatePathValue().regexpCapture("^/?(?:.*/)?([^/]*?)$", 1)
-  }
-
-  /**
-   * Gets the template file to be rendered by this call, if any.
-   */
-  ErbFile getTemplateFile() {
-    result.getTemplateName() = this.getBaseName() and
-    result.getRelativePath().matches("%app/views/" + this.getSubPath() + "%")
-  }
-
-  /**
-   * Get the local variables passed as context to the renderer
-   */
-  HashLiteral getLocals() { result = this.getKeywordArgument("locals") }
-  // TODO: implicit renders in controller actions
+private class ActionViewCookiesCall extends ActionViewContextCall, CookiesCallImpl {
+  ActionViewCookiesCall() { this.getMethodName() = "cookies" }
 }
 
 /**
@@ -172,17 +123,14 @@ private class RenderCallAsHttpResponse extends DataFlow::CallNode, Http::Server:
 }
 
 /** A call to the `render` method within the context of a template. */
-private class ActionViewRenderCall extends RenderCall, ActionViewContextCall { }
-
-/**
- * A render call that does not automatically set the HTTP response body.
- */
-abstract class RenderToCall extends MethodCall {
-  RenderToCall() { this.getMethodName() = ["render_to_body", "render_to_string"] }
+private class ActionViewRenderCall extends ActionViewContextCall, RenderCallImpl {
+  ActionViewRenderCall() { this.getMethodName() = "render" }
 }
 
-// A call to `render_to` from within a template.
-private class ActionViewRenderToCall extends ActionViewContextCall, RenderToCall { }
+/** A call to `render_to` from within a template. */
+private class ActionViewRenderToCall extends ActionViewContextCall, RenderToCallImpl {
+  ActionViewRenderToCall() { this.getMethodName() = ["render_to_body", "render_to_string"] }
+}
 
 /**
  * A call to the ActionView `link_to` helper method.
@@ -224,16 +172,18 @@ module ActionView {
    * Action view helper methods which are XSS sinks.
    */
   module Helpers {
+    abstract private class RawHelperCallImpl extends MethodCall {
+      abstract Expr getRawArgument();
+    }
+
     /**
      * A call to an ActionView helper which renders its argument without escaping.
      * The argument should be treated as an XSS sink. In the documentation for
      * classes in this module, the vulnerable argument is named `x`.
      */
-    abstract class RawHelperCall extends MethodCall {
-      /**
-       * Get an argument which is rendered without escaping.
-       */
-      abstract Expr getRawArgument();
+    class RawHelperCall extends MethodCall instanceof RawHelperCallImpl {
+      /** Gets an argument that is rendered without escaping. */
+      Expr getRawArgument() { result = super.getRawArgument() }
     }
 
     /**
@@ -241,7 +191,7 @@ module ActionView {
      *
      * `simple_format(x, y, sanitize: false)`.
      */
-    private class SimpleFormat extends ActionViewContextCall, RawHelperCall {
+    private class SimpleFormat extends ActionViewContextCall, RawHelperCallImpl {
       SimpleFormat() {
         this.getMethodName() = "simple_format" and
         this.getKeywordArgument("sanitize").getConstantValue().isBoolean(false)
@@ -255,7 +205,7 @@ module ActionView {
      *
      * `truncate(x, escape: false)`.
      */
-    private class Truncate extends ActionViewContextCall, RawHelperCall {
+    private class Truncate extends ActionViewContextCall, RawHelperCallImpl {
       Truncate() {
         this.getMethodName() = "truncate" and
         this.getKeywordArgument("escape").getConstantValue().isBoolean(false)
@@ -269,7 +219,7 @@ module ActionView {
      *
      * `highlight(x, y, sanitize: false)`.
      */
-    private class Highlight extends ActionViewContextCall, RawHelperCall {
+    private class Highlight extends ActionViewContextCall, RawHelperCallImpl {
       Highlight() {
         this.getMethodName() = "highlight" and
         this.getKeywordArgument("sanitize").getConstantValue().isBoolean(false)
@@ -283,7 +233,7 @@ module ActionView {
      *
      * `javascript_tag(x)`.
      */
-    private class JavascriptTag extends ActionViewContextCall, RawHelperCall {
+    private class JavascriptTag extends ActionViewContextCall, RawHelperCallImpl {
       JavascriptTag() { this.getMethodName() = "javascript_tag" }
 
       override Expr getRawArgument() { result = this.getArgument(0) }
@@ -294,7 +244,7 @@ module ActionView {
      *
      * `content_tag(x, x, y, false)`.
      */
-    private class ContentTag extends ActionViewContextCall, RawHelperCall {
+    private class ContentTag extends ActionViewContextCall, RawHelperCallImpl {
       ContentTag() {
         this.getMethodName() = "content_tag" and
         this.getArgument(3).getConstantValue().isBoolean(false)
@@ -308,7 +258,7 @@ module ActionView {
      *
      * `tag(x, x, y, false)`.
      */
-    private class Tag extends ActionViewContextCall, RawHelperCall {
+    private class Tag extends ActionViewContextCall, RawHelperCallImpl {
       Tag() {
         this.getMethodName() = "tag" and
         this.getArgument(3).getConstantValue().isBoolean(false)
@@ -322,7 +272,7 @@ module ActionView {
      *
      * `tag.h1(x, escape: false)`.
      */
-    private class TagMethod extends MethodCall, RawHelperCall {
+    private class TagMethod extends MethodCall, RawHelperCallImpl {
       TagMethod() {
         inActionViewContext(this) and
         this.getReceiver().(MethodCall).getMethodName() = "tag" and

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

@@ -9,9 +9,67 @@ private import codeql.ruby.frameworks.ActionController
 private import codeql.ruby.frameworks.ActionView
 private import codeql.ruby.frameworks.ActiveRecord
 private import codeql.ruby.frameworks.ActiveStorage
+private import codeql.ruby.frameworks.internal.Rails
 private import codeql.ruby.ApiGraphs
 private import codeql.ruby.security.OpenSSL
 
+/**
+ * A method call on a string to mark it as HTML safe for Rails. Strings marked
+ * as such will not be automatically escaped when inserted into HTML.
+ */
+class HtmlSafeCall extends MethodCall instanceof HtmlSafeCallImpl { }
+
+/** A call to a Rails method to escape HTML. */
+class HtmlEscapeCall extends MethodCall instanceof HtmlEscapeCallImpl { }
+
+/** A call to fetch the request parameters in a Rails app. */
+class ParamsCall extends MethodCall instanceof ParamsCallImpl { }
+
+/** A call to fetch the request cookies in a Rails app. */
+class CookiesCall extends MethodCall instanceof CookiesCallImpl { }
+
+/**
+ * A call to a render method that will populate the response body with the
+ * rendered content.
+ */
+class RenderCall extends MethodCall instanceof RenderCallImpl {
+  private Expr getTemplatePathArgument() {
+    // TODO: support other ways of specifying paths (e.g. `file`)
+    result = [this.getKeywordArgument(["partial", "template", "action"]), this.getArgument(0)]
+  }
+
+  private string getTemplatePathValue() {
+    result = this.getTemplatePathArgument().getConstantValue().getStringlikeValue()
+  }
+
+  // everything up to and including the final slash, but ignoring any leading slash
+  private string getSubPath() {
+    result = this.getTemplatePathValue().regexpCapture("^/?(.*/)?(?:[^/]*?)$", 1)
+  }
+
+  // everything after the final slash, or the whole string if there is no slash
+  private string getBaseName() {
+    result = this.getTemplatePathValue().regexpCapture("^/?(?:.*/)?([^/]*?)$", 1)
+  }
+
+  /**
+   * Gets the template file to be rendered by this call, if any.
+   */
+  ErbFile getTemplateFile() {
+    result.getTemplateName() = this.getBaseName() and
+    result.getRelativePath().matches("%app/views/" + this.getSubPath() + "%")
+  }
+
+  /**
+   * Get the local variables passed as context to the renderer
+   */
+  HashLiteral getLocals() { result = this.getKeywordArgument("locals") }
+  // TODO: implicit renders in controller actions
+}
+
+/** A render call that does not automatically set the HTTP response body. */
+class RenderToCall extends MethodCall instanceof RenderToCallImpl { }
+
 /**
  * A reference to either `Rails::Railtie`, `Rails::Engine`, or `Rails::Application`.
  * `Engine` and `Application` extend `Railtie`, but may not have definitions present in the database.

ruby/ql/lib/codeql/ruby/frameworks/internal/Rails.qll

@@ -0,0 +1,13 @@
+private import codeql.ruby.AST
+
+abstract class HtmlSafeCallImpl extends MethodCall { }
+
+abstract class HtmlEscapeCallImpl extends MethodCall { }
+
+abstract class RenderCallImpl extends MethodCall { }
+
+abstract class RenderToCallImpl extends MethodCall { }
+
+abstract class ParamsCallImpl extends MethodCall { }
+
+abstract class CookiesCallImpl extends MethodCall { }

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

@@ -10,6 +10,7 @@ private import codeql.ruby.Concepts
 private import codeql.ruby.Frameworks
 private import codeql.ruby.frameworks.ActionController
 private import codeql.ruby.frameworks.ActionView
+private import codeql.ruby.frameworks.Rails
 private import codeql.ruby.dataflow.RemoteFlowSources
 private import codeql.ruby.dataflow.BarrierGuards
 private import codeql.ruby.dataflow.internal.DataFlowDispatch

ruby/ql/test/library-tests/frameworks/ActionController.ql

@@ -1,6 +1,6 @@
 private import codeql.ruby.AST
 private import codeql.ruby.frameworks.ActionController
-private import codeql.ruby.frameworks.ActionView
+private import codeql.ruby.frameworks.Rails
 
 query predicate actionControllerControllerClasses(ActionControllerControllerClass cls) { any() }
 

ruby/ql/test/library-tests/frameworks/ActionView.ql

@@ -1,7 +1,7 @@
 private import ruby
 private import codeql.ruby.AST
-private import codeql.ruby.frameworks.ActionController
 private import codeql.ruby.frameworks.ActionView
+private import codeql.ruby.frameworks.Rails
 private import codeql.ruby.Concepts
 
 query predicate htmlSafeCalls(HtmlSafeCall c) { any() }