Cross-Site Request Forgery in better_errors

@@ -26,7 +26,7 @@ def id
       @id ||= SecureRandom.hex(8)
     end
 
-    def render(template_name = "main")
+    def render(template_name = "main", csrf_token = nil)
       binding.eval(self.class.template(template_name).src)
     rescue => e
       # Fix the backtrace, which doesn't identify the template that failed (within Better Errors).

@@ -1,5 +1,6 @@
 require "json"
 require "ipaddr"
+require "securerandom"
 require "set"
 require "rack"
 
@@ -39,6 +40,8 @@ def self.allow_ip!(addr)
     allow_ip! "127.0.0.0/8"
     allow_ip! "::1/128" rescue nil # windows ruby doesn't have ipv6 support
 
+    CSRF_TOKEN_COOKIE_NAME = 'BetterErrors-CSRF-Token'
+
     # A new instance of BetterErrors::Middleware
     #
     # @param app      The Rack app/middleware to wrap with Better Errors
@@ -89,11 +92,14 @@ def protected_app_call(env)
     end
 
     def show_error_page(env, exception=nil)
+      request = Rack::Request.new(env)
+      csrf_token = request.cookies[CSRF_TOKEN_COOKIE_NAME] || SecureRandom.uuid
+
       type, content = if @error_page
         if text?(env)
           [ 'plain', @error_page.render('text') ]
         else
-          [ 'html', @error_page.render ]
+          [ 'html', @error_page.render('main', csrf_token) ]
         end
       else
         [ 'html', no_errors_page ]
@@ -104,12 +110,22 @@ def show_error_page(env, exception=nil)
         status_code = ActionDispatch::ExceptionWrapper.new(env, exception).status_code
       end
 
-      [status_code, { "Content-Type" => "text/#{type}; charset=utf-8" }, [content]]
+      response = Rack::Response.new(content, status_code, { "Content-Type" => "text/#{type}; charset=utf-8" })
+
+      unless request.cookies[CSRF_TOKEN_COOKIE_NAME]
+        response.set_cookie(CSRF_TOKEN_COOKIE_NAME, value: csrf_token, httponly: true, same_site: :strict)
+      end
+
+      # In older versions of Rack, the body returned here is actually a Rack::BodyProxy which seems to be a bug.
+      # (It contains status, headers and body and does not act like an array of strings.)
+      # Since we already have status code and body here, there's no need to use the ones in the Rack::Response.
+      (_status_code, headers, _body) = response.finish
+      [status_code, headers, [content]]
     end
 
     def text?(env)
       env["HTTP_X_REQUESTED_WITH"] == "XMLHttpRequest" ||
-      !env["HTTP_ACCEPT"].to_s.include?('html')
+        !env["HTTP_ACCEPT"].to_s.include?('html')
     end
 
     def log_exception
@@ -133,9 +149,15 @@ def internal_call(env, opts)
       return no_errors_json_response unless @error_page
       return invalid_error_json_response if opts[:id] != @error_page.id
 
-      env["rack.input"].rewind
-      response = @error_page.send("do_#{opts[:method]}", JSON.parse(env["rack.input"].read))
-      [200, { "Content-Type" => "text/plain; charset=utf-8" }, [JSON.dump(response)]]
+      request = Rack::Request.new(env)
+      return invalid_csrf_token_json_response unless request.cookies[CSRF_TOKEN_COOKIE_NAME]
+
+      request.body.rewind
+      body = JSON.parse(request.body.read)
+      return invalid_csrf_token_json_response unless request.cookies[CSRF_TOKEN_COOKIE_NAME] == body['csrfToken']
+
+      response = @error_page.send("do_#{opts[:method]}", body)
+      [200, { "Content-Type" => "application/json; charset=utf-8" }, [JSON.dump(response)]]
     end
 
     def no_errors_page
@@ -157,18 +179,26 @@ def no_errors_json_response
         "The application has been restarted since this page loaded, " +
           "or the framework is reloading all gems before each request "
       end
-      [200, { "Content-Type" => "text/plain; charset=utf-8" }, [JSON.dump(
+      [200, { "Content-Type" => "application/json; charset=utf-8" }, [JSON.dump(
         error: 'No exception information available',
         explanation: explanation,
       )]]
     end
 
     def invalid_error_json_response
-      [200, { "Content-Type" => "text/plain; charset=utf-8" }, [JSON.dump(
+      [200, { "Content-Type" => "application/json; charset=utf-8" }, [JSON.dump(
         error: "Session expired",
         explanation: "This page was likely opened from a previous exception, " +
           "and the exception is no longer available in memory.",
       )]]
     end
+
+    def invalid_csrf_token_json_response
+      [200, { "Content-Type" => "application/json; charset=utf-8" }, [JSON.dump(
+        error: "Invalid CSRF Token",
+        explanation: "The browser session might have been cleared, " +
+          "or something went wrong.",
+      )]]
+    end
   end
 end

@@ -800,6 +800,7 @@
 (function() {
 
     var OID = "<%= id %>";
+    var csrfToken = "<%= csrf_token %>";
 
     var previousFrame = null;
     var previousFrameInfo = null;
@@ -810,6 +811,7 @@
         var req = new XMLHttpRequest();
         req.open("POST", "//" + window.location.host + <%== uri_prefix.gsub("<", "&lt;").inspect %> + "/__better_errors/" + OID + "/" + method, true);
         req.setRequestHeader("Content-Type", "application/json");
+        opts.csrfToken = csrfToken;
         req.send(JSON.stringify(opts));
         req.onreadystatechange = function() {
             if(req.readyState == 4) {

@@ -4,9 +4,14 @@ module BetterErrors
   describe Middleware do
     let(:app) { Middleware.new(->env { ":)" }) }
     let(:exception) { RuntimeError.new("oh no :(") }
+    let(:status) { response_env[0] }
+    let(:headers) { response_env[1] }
+    let(:body) { response_env[2].join }
 
-    it "passes non-error responses through" do
-      expect(app.call({})).to eq(":)")
+    context 'when the application raises no exception' do
+      it "passes non-error responses through" do
+        expect(app.call({})).to eq(":)")
+      end
     end
 
     it "calls the internal methods" do
@@ -24,11 +29,6 @@ module BetterErrors
       app.call("PATH_INFO" => "/__better_errors/")
     end
 
-    it "shows the error page on any subfolder path" do
-      expect(app).to receive :show_error_page
-      app.call("PATH_INFO" => "/any_sub/folder/path/__better_errors/")
-    end
-
     it "doesn't show the error page to a non-local address" do
       expect(app).not_to receive :better_errors_call
       app.call("REMOTE_ADDR" => "1.2.3.4")
@@ -62,34 +62,71 @@ module BetterErrors
       expect { app.call("REMOTE_ADDR" => "0:0:0:0:0:0:0:1%0" ) }.to_not raise_error
     end
 
-    context "when requesting the /__better_errors manually" do
-      let(:app) { Middleware.new(->env { ":)" }) }
+    context "when /__better_errors is requested directly" do
+      let(:response_env) { app.call("PATH_INFO" => "/__better_errors") }
 
-      it "shows that no errors have been recorded" do
-        status, headers, body = app.call("PATH_INFO" => "/__better_errors")
-        expect(body.join).to match /No errors have been recorded yet./
-      end
+      context "when no error has been recorded since startup" do
+        it "shows that no errors have been recorded" do
+          expect(body).to match /No errors have been recorded yet./
+        end
 
-      it 'does not attempt to use ActionDispatch::ExceptionWrapper with a nil exception' do
-        ad_ew = double("ActionDispatch::ExceptionWrapper")
-        stub_const('ActionDispatch::ExceptionWrapper', ad_ew)
-        expect(ad_ew).to_not receive :new
+        it 'does not attempt to use ActionDispatch::ExceptionWrapper on the nil exception' do
+          ad_ew = double("ActionDispatch::ExceptionWrapper")
+          stub_const('ActionDispatch::ExceptionWrapper', ad_ew)
+          expect(ad_ew).to_not receive :new
+
+          response_env
+        end
+
+        context 'when requested inside a subfolder path' do
+          let(:response_env) { app.call("PATH_INFO" => "/any_sub/folder/__better_errors") }
 
-        status, headers, body = app.call("PATH_INFO" => "/__better_errors")
+          it "shows that no errors have been recorded" do
+            expect(body).to match /No errors have been recorded yet./
+          end
+        end
       end
 
-      it "shows that no errors have been recorded on any subfolder path" do
-        status, headers, body = app.call("PATH_INFO" => "/any_sub/folder/path/__better_errors")
-        expect(body.join).to match /No errors have been recorded yet./
+      context 'when an error has been recorded' do
+        let(:app) {
+          Middleware.new(->env do
+            # Only raise on the first request
+            raise exception unless @already_raised
+            @already_raised = true
+          end)
+        }
+        before do
+          app.call({})
+        end
+
+        it 'returns the information of the most recent error' do
+          expect(body).to include("oh no :(")
+        end
+
+        it 'does not attempt to use ActionDispatch::ExceptionWrapper' do
+          ad_ew = double("ActionDispatch::ExceptionWrapper")
+          stub_const('ActionDispatch::ExceptionWrapper', ad_ew)
+          expect(ad_ew).to_not receive :new
+
+          response_env
+        end
+
+        context 'when inside a subfolder path' do
+          let(:response_env) { app.call("PATH_INFO" => "/any_sub/folder/__better_errors") }
+
+          it "shows the error page on any subfolder path" do
+            expect(app).to receive :show_error_page
+            app.call("PATH_INFO" => "/any_sub/folder/path/__better_errors/")
+          end
+        end
       end
     end
 
     context "when handling an error" do
       let(:app) { Middleware.new(->env { raise exception }) }
+      let(:response_env) { app.call({}) }
 
       it "returns status 500" do
-        status, headers, body = app.call({})
-
         expect(status).to eq(500)
       end
 
@@ -109,11 +146,9 @@ module BetterErrors
         }
 
         it "shows the exception as-is" do
-          status, _, body = app.call({})
-
           expect(status).to eq(500)
-          expect(body.join).to match(/\n> Second Exception\n/)
-          expect(body.join).not_to match(/\n> First Exception\n/)
+          expect(body).to match(/\n> Second Exception\n/)
+          expect(body).not_to match(/\n> First Exception\n/)
         end
       end
 
@@ -135,11 +170,9 @@ def initialize(message, original_exception = nil)
           }
 
           it "shows the original exception instead of the last-raised one" do
-            status, _, body = app.call({})
-
             expect(status).to eq(500)
-            expect(body.join).not_to match(/Second Exception/)
-            expect(body.join).to match(/First Exception/)
+            expect(body).not_to match(/Second Exception/)
+            expect(body).to match(/First Exception/)
           end
         end
 
@@ -151,41 +184,68 @@ def initialize(message, original_exception = nil)
           }
 
           it "shows the exception as-is" do
-            status, _, body = app.call({})
-
             expect(status).to eq(500)
-            expect(body.join).to match(/The Exception/)
+            expect(body).to match(/The Exception/)
           end
         end
       end
 
       it "returns ExceptionWrapper's status_code" do
         ad_ew = double("ActionDispatch::ExceptionWrapper")
-        allow(ad_ew).to receive('new').with({}, exception) { double("ExceptionWrapper", status_code: 404) }
+        allow(ad_ew).to receive('new').with(anything, exception) { double("ExceptionWrapper", status_code: 404) }
         stub_const('ActionDispatch::ExceptionWrapper', ad_ew)
 
-        status, headers, body = app.call({})
-
         expect(status).to eq(404)
       end
 
       it "returns UTF-8 error pages" do
-        status, headers, body = app.call({})
-
         expect(headers["Content-Type"]).to match /charset=utf-8/
       end
 
-      it "returns text pages by default" do
-        status, headers, body = app.call({})
-
+      it "returns text content by default" do
         expect(headers["Content-Type"]).to match /text\/plain/
       end
 
-      it "returns HTML pages by default" do
-        # Chrome's 'Accept' header looks similar this.
-        status, headers, body = app.call("HTTP_ACCEPT" => "text/html,application/xhtml+xml;q=0.9,*/*")
+      context 'when a CSRF token cookie is not specified' do
+        it 'includes a newly-generated CSRF token cookie' do
+          expect(headers).to include(
+            'Set-Cookie' => /BetterErrors-CSRF-Token=[-a-z0-9]+; HttpOnly; SameSite=Strict/
+          )
+        end
+      end
+
+      context 'when a CSRF token cookie is specified' do
+        let(:response_env) { app.call({ 'HTTP_COOKIE' => 'BetterErrors-CSRF-Token=abc123' }) }
+
+        it 'does not set a new CSRF token cookie' do
+          expect(headers).not_to include('Set-Cookie')
+        end
+      end
+
+      context 'when the Accept header specifies HTML first' do
+        let(:response_env) { app.call("HTTP_ACCEPT" => "text/html,application/xhtml+xml;q=0.9,*/*") }
+
+        it "returns HTML content" do
+          expect(headers["Content-Type"]).to match /text\/html/
+        end
+
+        it 'includes the newly-generated CSRF token in the body of the page' do
+          matches = headers['Set-Cookie'].match(/BetterErrors-CSRF-Token=(?<tok>[-a-z0-9]+); HttpOnly; SameSite=Strict/)
+          expect(body).to include(matches[:tok])
+        end
 
-        expect(headers["Content-Type"]).to match /text\/html/
+        context 'when a CSRF token cookie is specified' do
+          let(:response_env) {
+            app.call({
+              'HTTP_COOKIE' => 'BetterErrors-CSRF-Token=csrfTokenGHI',
+              "HTTP_ACCEPT" => "text/html,application/xhtml+xml;q=0.9,*/*",
+            })
+          }
+
+          it 'includes that CSRF token in the body of the page' do
+            expect(body).to include('csrfTokenGHI')
+          end
+        end
       end
 
       context 'the logger' do
@@ -196,7 +256,7 @@ def initialize(message, original_exception = nil)
 
         it "receives the exception as a fatal message" do
           expect(logger).to receive(:fatal).with(/RuntimeError/)
-          app.call({})
+          response_env
         end
 
         context 'when Rails is being used' do
@@ -208,7 +268,7 @@ def initialize(message, original_exception = nil)
             expect(logger).to receive(:fatal) do |message|
               expect(message).to_not match(/rspec-core/)
             end
-            app.call({})
+            response_env
           end
         end
         context 'when Rails is not being used' do
@@ -220,27 +280,23 @@ def initialize(message, original_exception = nil)
             expect(logger).to receive(:fatal) do |message|
               expect(message).to match(/rspec-core/)
             end
-            app.call({})
+            response_env
           end
         end
       end
     end
 
     context "requesting the variables for a specific frame" do
       let(:env) { {} }
-      let(:result) {
-        app.call(
-          "PATH_INFO" => "/__better_errors/#{id}/#{method}",
-          # This is a POST request, and this is the body of the request.
-          "rack.input" => StringIO.new('{"index": 0}'),
-        )
+      let(:response_env) {
+        app.call(request_env)
       }
-      let(:status) { result[0] }
-      let(:headers) { result[1] }
-      let(:body) { result[2].join }
+      let(:request_env) {
+        Rack::MockRequest.env_for("/__better_errors/#{id}/variables", input: StringIO.new(JSON.dump(request_body_data)))
+      }
+      let(:request_body_data) { {"index": 0} }
       let(:json_body) { JSON.parse(body) }
       let(:id) { 'abcdefg' }
-      let(:method) { 'variables' }
 
       context 'when no errors have been recorded' do
         it 'returns a JSON error' do
@@ -291,14 +347,44 @@ def initialize(message, original_exception = nil)
           end
         end
 
-        context 'and it matches the request', :focus do
+        context 'and its ID matches the requested ID' do
           let(:id) { error_page.id }
 
-          it 'returns a JSON error' do
-            expect(error_page).to receive(:do_variables).and_return(html: "<content>")
-            expect(json_body).to match(
-              'html' => '<content>',
-            )
+          context 'when the body csrfToken matches the CSRF token cookie' do
+            let(:request_body_data) { { "index" => 0, "csrfToken" => "csrfToken123" } }
+            before do
+              request_env["HTTP_COOKIE"] = "BetterErrors-CSRF-Token=csrfToken123"
+            end
+
+            it 'returns the HTML content' do
+              expect(error_page).to receive(:do_variables).and_return(html: "<content>")
+              expect(json_body).to match(
+                'html' => '<content>',
+              )
+            end
+          end
+
+          context 'when the body csrfToken does not match the CSRF token cookie' do
+            let(:request_body_data) { {"index": 0, "csrfToken": "csrfToken123"} }
+            before do
+              request_env["HTTP_COOKIE"] = "BetterErrors-CSRF-Token=csrfToken456"
+            end
+
+            it 'returns a JSON error' do
+              expect(json_body).to match(
+                'error' => 'Invalid CSRF Token',
+                'explanation' => /session might have been cleared/,
+              )
+            end
+          end
+
+          context 'when there is no CSRF token in the request' do
+            it 'returns a JSON error' do
+              expect(json_body).to match(
+                'error' => 'Invalid CSRF Token',
+                'explanation' => /session might have been cleared/,
+              )
+            end
           end
         end
       end