From 446c192ac35d561372d2e862a6be63572f8e0e16 Mon Sep 17 00:00:00 2001 From: Robin Daugherty Date: Tue, 15 Sep 2020 16:24:38 -0400 Subject: [PATCH 1/2] Validate content-type of internal calls --- lib/better_errors/middleware.rb | 9 +++++++++ spec/better_errors/middleware_spec.rb | 29 ++++++++++++++++++++++----- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/lib/better_errors/middleware.rb b/lib/better_errors/middleware.rb index 11036128..da7bf361 100644 --- a/lib/better_errors/middleware.rb +++ b/lib/better_errors/middleware.rb @@ -156,6 +156,8 @@ def internal_call(env, opts) body = JSON.parse(request.body.read) return invalid_csrf_token_json_response unless request.cookies[CSRF_TOKEN_COOKIE_NAME] == body['csrfToken'] + return not_acceptable_json_response unless request.content_type == 'application/json' + response = @error_page.send("do_#{opts[:method]}", body) [200, { "Content-Type" => "application/json; charset=utf-8" }, [JSON.dump(response)]] end @@ -200,5 +202,12 @@ def invalid_csrf_token_json_response "or something went wrong.", )]] end + + def not_acceptable_json_response + [406, { "Content-Type" => "application/json; charset=utf-8" }, [JSON.dump( + error: "Request not acceptable", + explanation: "The internal request did not match an acceptable content type.", + )]] + end end end diff --git a/spec/better_errors/middleware_spec.rb b/spec/better_errors/middleware_spec.rb index 44b38fd1..b35b7da2 100644 --- a/spec/better_errors/middleware_spec.rb +++ b/spec/better_errors/middleware_spec.rb @@ -356,11 +356,30 @@ def initialize(message, original_exception = nil) 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' => '', - ) + context 'when the Content-Type of the request is application/json' do + before do + request_env['CONTENT_TYPE'] = 'application/json' + end + + it 'returns JSON containing 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 Content-Type of the request is application/json' do + before do + request_env['HTTP_CONTENT_TYPE'] = 'application/json' + end + + it 'returns a JSON error' do + expect(json_body).to match( + 'error' => 'Request not acceptable', + 'explanation' => /did not match an acceptable content type/, + ) + end end end From aa073f6f07ee05894a56e3cf79710e809051b0f3 Mon Sep 17 00:00:00 2001 From: Robin Daugherty Date: Tue, 15 Sep 2020 16:33:07 -0400 Subject: [PATCH 2/2] Validate internal call method names --- lib/better_errors/middleware.rb | 16 ++- spec/better_errors/middleware_spec.rb | 148 +++++++++++++++++++++++++- 2 files changed, 158 insertions(+), 6 deletions(-) diff --git a/lib/better_errors/middleware.rb b/lib/better_errors/middleware.rb index da7bf361..aab0210d 100644 --- a/lib/better_errors/middleware.rb +++ b/lib/better_errors/middleware.rb @@ -75,7 +75,7 @@ def allow_ip?(env) def better_errors_call(env) case env["PATH_INFO"] when %r{/__better_errors/(?.+?)/(?\w+)\z} - internal_call env, $~ + internal_call(env, $~[:id], $~[:method]) when %r{/__better_errors/?\z} show_error_page env else @@ -145,9 +145,10 @@ def backtrace_frames end end - def internal_call(env, opts) + def internal_call(env, id, method) + return not_found_json_response unless %w[variables eval].include?(method) return no_errors_json_response unless @error_page - return invalid_error_json_response if opts[:id] != @error_page.id + return invalid_error_json_response if id != @error_page.id request = Rack::Request.new(env) return invalid_csrf_token_json_response unless request.cookies[CSRF_TOKEN_COOKIE_NAME] @@ -158,7 +159,7 @@ def internal_call(env, opts) return not_acceptable_json_response unless request.content_type == 'application/json' - response = @error_page.send("do_#{opts[:method]}", body) + response = @error_page.send("do_#{method}", body) [200, { "Content-Type" => "application/json; charset=utf-8" }, [JSON.dump(response)]] end @@ -203,6 +204,13 @@ def invalid_csrf_token_json_response )]] end + def not_found_json_response + [404, { "Content-Type" => "application/json; charset=utf-8" }, [JSON.dump( + error: "Not found", + explanation: "Not a recognized internal call.", + )]] + end + def not_acceptable_json_response [406, { "Content-Type" => "application/json; charset=utf-8" }, [JSON.dump( error: "Request not acceptable", diff --git a/spec/better_errors/middleware_spec.rb b/spec/better_errors/middleware_spec.rb index b35b7da2..6a6a48f6 100644 --- a/spec/better_errors/middleware_spec.rb +++ b/spec/better_errors/middleware_spec.rb @@ -294,7 +294,7 @@ def initialize(message, original_exception = nil) 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(:request_body_data) { { "index" => 0 } } let(:json_body) { JSON.parse(body) } let(:id) { 'abcdefg' } @@ -384,7 +384,131 @@ def initialize(message, original_exception = nil) end context 'when the body csrfToken does not match the CSRF token cookie' do - let(:request_body_data) { {"index": 0, "csrfToken": "csrfToken123"} } + 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 + end + + context "requesting eval for a specific frame" do + let(:env) { {} } + let(:response_env) { + app.call(request_env) + } + let(:request_env) { + Rack::MockRequest.env_for("/__better_errors/#{id}/eval", input: StringIO.new(JSON.dump(request_body_data))) + } + let(:request_body_data) { { "index" => 0, source: "do_a_thing" } } + let(:json_body) { JSON.parse(body) } + let(:id) { 'abcdefg' } + + context 'when no errors have been recorded' do + it 'returns a JSON error' do + expect(json_body).to match( + 'error' => 'No exception information available', + 'explanation' => /application has been restarted/, + ) + end + + context 'when Middleman is in use' do + let!(:middleman) { class_double("Middleman").as_stubbed_const } + it 'returns a JSON error' do + expect(json_body['explanation']) + .to match(/Middleman reloads all dependencies/) + end + end + + context 'when Shotgun is in use' do + let!(:shotgun) { class_double("Shotgun").as_stubbed_const } + + it 'returns a JSON error' do + expect(json_body['explanation']) + .to match(/The shotgun gem/) + end + + context 'when Hanami is also in use' do + let!(:hanami) { class_double("Hanami").as_stubbed_const } + it 'returns a JSON error' do + expect(json_body['explanation']) + .to match(/--no-code-reloading/) + end + end + end + end + + context 'when an error has been recorded' do + let(:error_page) { ErrorPage.new(exception, env) } + before do + app.instance_variable_set('@error_page', error_page) + end + + context 'but it does not match the request' do + it 'returns a JSON error' do + expect(json_body).to match( + 'error' => 'Session expired', + 'explanation' => /no longer available in memory/, + ) + end + end + + context 'and its ID matches the requested ID' do + let(:id) { error_page.id } + + 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 + + context 'when the Content-Type of the request is application/json' do + before do + request_env['CONTENT_TYPE'] = 'application/json' + end + + it 'returns JSON containing the eval result' do + expect(error_page).to receive(:do_eval).and_return(prompt: '#', result: "much_stuff_here") + expect(json_body).to match( + 'prompt' => '#', + 'result' => 'much_stuff_here', + ) + end + end + + context 'when the Content-Type of the request is application/json' do + before do + request_env['HTTP_CONTENT_TYPE'] = 'application/json' + end + + it 'returns a JSON error' do + expect(json_body).to match( + 'error' => 'Request not acceptable', + 'explanation' => /did not match an acceptable content type/, + ) + end + 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 @@ -408,5 +532,25 @@ def initialize(message, original_exception = nil) end end end + + context "requesting an invalid internal method" do + let(:env) { {} } + let(:response_env) { + app.call(request_env) + } + let(:request_env) { + Rack::MockRequest.env_for("/__better_errors/#{id}/invalid", input: StringIO.new(JSON.dump(request_body_data))) + } + let(:request_body_data) { { "index" => 0 } } + let(:json_body) { JSON.parse(body) } + let(:id) { 'abcdefg' } + + it 'returns a JSON error' do + expect(json_body).to match( + 'error' => 'Not found', + 'explanation' => /recognized internal call/, + ) + end + end end end