diff --git a/lib/better_errors/error_page.rb b/lib/better_errors/error_page.rb index 1f049f39..05a7e7bf 100644 --- a/lib/better_errors/error_page.rb +++ b/lib/better_errors/error_page.rb @@ -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). diff --git a/lib/better_errors/middleware.rb b/lib/better_errors/middleware.rb index 262cf3b2..11036128 100644 --- a/lib/better_errors/middleware.rb +++ b/lib/better_errors/middleware.rb @@ -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 diff --git a/lib/better_errors/templates/main.erb b/lib/better_errors/templates/main.erb index a945d99c..1ab68559 100644 --- a/lib/better_errors/templates/main.erb +++ b/lib/better_errors/templates/main.erb @@ -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("<", "<").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) { diff --git a/spec/better_errors/middleware_spec.rb b/spec/better_errors/middleware_spec.rb index 8a7ffa69..44b38fd1 100644 --- a/spec/better_errors/middleware_spec.rb +++ b/spec/better_errors/middleware_spec.rb @@ -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=(?[-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,7 +280,7 @@ 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 @@ -228,19 +288,15 @@ def initialize(message, original_exception = nil) 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: "") - expect(json_body).to match( - 'html' => '', - ) + 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: "") + expect(json_body).to match( + 'html' => '', + ) + 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