Merge pull request #13483 from alexrford/rb/rack-extend-app-and-resp

Ruby: rack - model more responses and app types

Author: alexrford

ruby/ql/lib/change-notes/2023-06-23-rack-response.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* More kinds of rack applications are now recognized.
+* Rack::Response instances are now recognized as potential responses from rack applications.

ruby/ql/lib/codeql/ruby/frameworks/actiondispatch/internal/Request.qll

@@ -128,7 +128,7 @@ module Request {
     private import codeql.ruby.frameworks.Rack
 
     private class RackEnv extends Env {
-      RackEnv() { this = any(Rack::App::AppCandidate app).getEnv().getALocalUse() }
+      RackEnv() { this = any(Rack::App::RequestHandler handler).getEnv().getALocalUse() }
     }
 
     /**

ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll

@@ -2,24 +2,44 @@
  * Provides modeling for Rack applications.
  */
 
+private import codeql.ruby.AST
 private import codeql.ruby.ApiGraphs
 private import codeql.ruby.DataFlow
 private import codeql.ruby.typetracking.TypeTracker
 private import Response::Private as RP
 
-/** A method node for a method named `call`. */
-private class CallMethodNode extends DataFlow::MethodNode {
-  CallMethodNode() { this.getMethodName() = "call" }
+/**
+ * A callable node that takes a single argument and, if it has a method name,
+ * is called "call".
+ */
+private class PotentialRequestHandler extends DataFlow::CallableNode {
+  PotentialRequestHandler() {
+    this.getNumberOfParameters() = 1 and
+    (
+      this.(DataFlow::MethodNode).getMethodName() = "call"
+      or
+      not this instanceof DataFlow::MethodNode and
+      exists(DataFlow::CallNode cn | cn.getMethodName() = "run" |
+        this.(DataFlow::LocalSourceNode).flowsTo(cn.getArgument(0))
+        or
+        // TODO: `Proc.new` should automatically propagate flow from its block argument
+        any(DataFlow::CallNode proc |
+          proc = API::getTopLevelMember("Proc").getAnInstantiation() and
+          proc.getBlock() = this
+        ).(DataFlow::LocalSourceNode).flowsTo(cn.getArgument(0))
+      )
+    )
+  }
 }
 
-private DataFlow::LocalSourceNode trackRackResponse(TypeBackTracker t, CallMethodNode call) {
+private DataFlow::LocalSourceNode trackRackResponse(TypeBackTracker t, PotentialRequestHandler call) {
   t.start() and
   result = call.getAReturnNode().getALocalSource()
   or
   exists(TypeBackTracker t2 | result = trackRackResponse(t2, call).backtrack(t2, t))
 }
 
-private RP::PotentialResponseNode trackRackResponse(CallMethodNode call) {
+private RP::PotentialResponseNode trackRackResponse(PotentialRequestHandler call) {
   result = trackRackResponse(TypeBackTracker::end(), call)
 }
 
@@ -28,12 +48,13 @@ private RP::PotentialResponseNode trackRackResponse(CallMethodNode call) {
  */
 module App {
   /**
+   * DEPRECATED: Use `RequestHandler` instead.
    * A class that may be a rack application.
    * This is a class that has a `call` method that takes a single argument
    * (traditionally called `env`) and returns a rack-compatible response.
    */
-  class AppCandidate extends DataFlow::ClassNode {
-    private CallMethodNode call;
+  deprecated class AppCandidate extends DataFlow::ClassNode {
+    private RequestHandler call;
     private RP::PotentialResponseNode resp;
 
     AppCandidate() {
@@ -50,4 +71,19 @@ module App {
     /** Gets the response returned from a request to this application. */
     RP::PotentialResponseNode getResponse() { result = resp }
   }
+
+  /**
+   * A callable node that looks like it implements the rack specification.
+   */
+  class RequestHandler extends PotentialRequestHandler {
+    private RP::PotentialResponseNode resp;
+
+    RequestHandler() { resp = trackRackResponse(this) }
+
+    /** Gets the `env` parameter passed to this request handler. */
+    DataFlow::ParameterNode getEnv() { result = this.getParameter(0) }
+
+    /** Gets a response returned from this request handler. */
+    RP::PotentialResponseNode getAResponse() { result = resp }
+  }
 }

ruby/ql/lib/codeql/ruby/frameworks/rack/internal/Response.qll

@@ -13,15 +13,39 @@ private import App as A
 /** Contains implementation details for modeling `Rack::Response`. */
 module Private {
   /** A `DataFlow::Node` that may be a rack response. This is detected heuristically, if something "looks like" a rack response syntactically then we consider it to be a potential response node. */
-  class PotentialResponseNode extends DataFlow::ArrayLiteralNode {
-    // [status, headers, body]
-    PotentialResponseNode() { this.getNumberOfArguments() = 3 }
-
+  abstract class PotentialResponseNode extends DataFlow::Node {
     /** Gets the headers returned with this response. */
-    DataFlow::Node getHeaders() { result = this.getElement(1) }
+    abstract DataFlow::Node getHeaders();
 
     /** Gets the body of this response. */
-    DataFlow::Node getBody() { result = this.getElement(2) }
+    abstract DataFlow::Node getBody();
+  }
+
+  /** A rack response constructed directly using an array literal. */
+  private class PotentialArrayResponse extends PotentialResponseNode, DataFlow::ArrayLiteralNode {
+    // [status, headers, body]
+    PotentialArrayResponse() { this.getNumberOfArguments() = 3 }
+
+    override DataFlow::Node getHeaders() { result = this.getElement(1) }
+
+    override DataFlow::Node getBody() { result = this.getElement(2) }
+  }
+
+  /** A rack response constructed by calling `finish` on an instance of `Rack::Response`. */
+  private class RackResponseConstruction extends PotentialResponseNode, DataFlow::CallNode {
+    private DataFlow::CallNode responseConstruction;
+
+    // (body, status, headers)
+    RackResponseConstruction() {
+      responseConstruction =
+        API::getTopLevelMember("Rack").getMember("Response").getAnInstantiation() and
+      this = responseConstruction.getAMethodCall() and
+      this.getMethodName() = "finish"
+    }
+
+    override DataFlow::Node getHeaders() { result = responseConstruction.getArgument(2) }
+
+    override DataFlow::Node getBody() { result = responseConstruction.getArgument(0) }
   }
 }
 
@@ -54,19 +78,30 @@ module Public {
                   v.getStringlikeValue().toLowerCase() = headerName.toLowerCase()
                 ))
       )
+      or
+      // pair in a `Rack::Response.new` constructor
+      exists(DataFlow::PairNode headerPair | headerPair = headers |
+        headerPair.getKey().getConstantValue().getStringlikeValue().toLowerCase() =
+          headerName.toLowerCase() and
+        result = headerPair.getValue()
+      )
     )
   }
 
   /** A `DataFlow::Node` returned from a rack request. */
-  class ResponseNode extends Private::PotentialResponseNode, Http::Server::HttpResponse::Range {
-    ResponseNode() { this = any(A::App::AppCandidate app).getResponse() }
+  class ResponseNode extends Http::Server::HttpResponse::Range instanceof Private::PotentialResponseNode
+  {
+    ResponseNode() { this = any(A::App::RequestHandler handler).getAResponse() }
 
-    override DataFlow::Node getBody() { result = this.getElement(2) }
+    override DataFlow::Node getBody() { result = this.(Private::PotentialResponseNode).getBody() }
 
     override DataFlow::Node getMimetypeOrContentTypeArg() {
       result = getHeaderValue(this, "content-type")
     }
 
+    /** Gets the headers returned with this response. */
+    DataFlow::Node getHeaders() { result = this.(Private::PotentialResponseNode).getHeaders() }
+
     // TODO: is there a sensible value for this?
     override string getMimetypeDefault() { none() }
   }

ruby/ql/test/library-tests/frameworks/rack/Rack.expected

@@ -1,11 +1,18 @@
-rackApps
-| rack.rb:1:1:10:3 | HelloWorld | rack.rb:2:12:2:14 | env |
-| rack.rb:12:1:22:3 | Proxy | rack.rb:17:12:17:18 | the_env |
-| rack.rb:24:1:37:3 | Logger | rack.rb:30:12:30:14 | env |
-| rack.rb:39:1:45:3 | Redirector | rack.rb:40:12:40:14 | env |
-| rack.rb:59:1:75:3 | Baz | rack.rb:60:12:60:14 | env |
+rackRequestHandlers
+| rack.rb:2:3:9:5 | call | rack.rb:2:12:2:14 | env | rack.rb:8:5:8:38 | call to [] |
+| rack.rb:17:3:21:5 | call | rack.rb:17:12:17:18 | the_env | rack.rb:20:5:20:27 | call to [] |
+| rack.rb:30:3:36:5 | call | rack.rb:30:12:30:14 | env | rack.rb:35:5:35:26 | call to [] |
+| rack.rb:40:3:44:5 | call | rack.rb:40:12:40:14 | env | rack.rb:43:5:43:45 | call to [] |
+| rack.rb:60:3:62:5 | call | rack.rb:60:12:60:14 | env | rack.rb:66:7:66:22 | call to [] |
+| rack.rb:60:3:62:5 | call | rack.rb:60:12:60:14 | env | rack.rb:73:5:73:21 | call to [] |
+| rack.rb:79:3:81:5 | call | rack.rb:79:17:79:19 | env | rack.rb:93:5:93:78 | call to finish |
+| rack_apps.rb:6:3:12:5 | call | rack_apps.rb:6:12:6:14 | env | rack_apps.rb:10:12:10:34 | call to [] |
+| rack_apps.rb:16:3:18:5 | call | rack_apps.rb:16:17:16:19 | env | rack_apps.rb:17:5:17:28 | call to [] |
+| rack_apps.rb:21:14:21:50 | -> { ... } | rack_apps.rb:21:17:21:19 | env | rack_apps.rb:21:24:21:48 | call to [] |
+| rack_apps.rb:23:21:23:53 | { ... } | rack_apps.rb:23:24:23:26 | env | rack_apps.rb:23:29:23:51 | call to [] |
 rackResponseContentTypes
 | rack.rb:8:5:8:38 | call to [] | rack.rb:7:34:7:45 | "text/plain" |
 | rack.rb:20:5:20:27 | call to [] | rack.rb:19:28:19:38 | "text/html" |
 redirectResponses
 | rack.rb:43:5:43:45 | call to [] | rack.rb:42:30:42:40 | "/foo.html" |
+| rack.rb:93:5:93:78 | call to finish | rack.rb:93:60:93:70 | redirect_to |

ruby/ql/test/library-tests/frameworks/rack/Rack.ql

@@ -2,8 +2,10 @@ private import codeql.ruby.AST
 private import codeql.ruby.frameworks.Rack
 private import codeql.ruby.DataFlow
 
-query predicate rackApps(Rack::App::AppCandidate c, DataFlow::ParameterNode env) {
-  env = c.getEnv()
+query predicate rackRequestHandlers(
+  Rack::App::RequestHandler handler, DataFlow::ParameterNode env, Rack::Response::ResponseNode resp
+) {
+  env = handler.getEnv() and resp = handler.getAResponse()
 }
 
 query predicate rackResponseContentTypes(

ruby/ql/test/library-tests/frameworks/rack/rack.rb

@@ -73,3 +73,23 @@ def error
     [400, {}, "nope"]
   end
 end
+
+class Qux
+  attr_reader :env
+  def self.call(env)
+    new(env).call
+  end
+
+  def initialize(env)
+    @env = env
+  end
+
+  def call
+    do_redirect
+  end
+
+  def do_redirect
+    redirect_to = env['redirect_to']
+    Rack::Response.new(['redirecting'], 302, 'Location' => redirect_to).finish
+  end
+end

ruby/ql/test/library-tests/frameworks/rack/rack_apps.rb

@@ -0,0 +1,28 @@
+require 'rack'
+require 'rack/handler/puma'
+handler = Rack::Handler::Puma
+
+class InstanceApp
+  def call(env)
+    status = 200
+    headers = {}
+    body = ["instance app"]
+    resp = [status, headers, body]
+    resp
+  end
+end
+
+class ClassApp
+  def self.call(env)
+    [200, {}, ["class app"]]
+  end
+end
+
+lambda_app = ->(env) { [200, {}, ["lambda app"]] }
+
+proc_app = Proc.new { |env| [200, {}, ["proc app"]] }
+
+handler.run InstanceApp.new
+handler.run ClassApp
+handler.run lambda_app
+handler.run proc_app