Skip to content

Commit

Permalink
Add CSRF to error page and internal calls
Browse files Browse the repository at this point in the history
  • Loading branch information
RobinDaugherty committed Sep 13, 2020
1 parent 86c19fc commit 617e65e
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 20 deletions.
2 changes: 1 addition & 1 deletion lib/better_errors/error_page.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
41 changes: 35 additions & 6 deletions lib/better_errors/middleware.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require "json"
require "ipaddr"
require "securerandom"
require "set"
require "rack"

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 ]
Expand All @@ -104,12 +110,21 @@ 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]]
headers = {
"Content-Type" => "text/#{type}; charset=utf-8",
}
response = Rack::Response.new(content, status_code, headers)

unless request.cookies[CSRF_TOKEN_COOKIE_NAME]
response.set_cookie(CSRF_TOKEN_COOKIE_NAME, value: csrf_token, httponly: true, same_site: :strict)
end

response.finish
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
Expand All @@ -133,9 +148,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
Expand Down Expand Up @@ -170,5 +191,13 @@ def invalid_error_json_response
"and the exception is no longer available in memory.",
)]]
end

def invalid_csrf_token_json_response
[200, { "Content-Type" => "text/plain; charset=utf-8" }, [JSON.dump(
error: "Invalid CSRF Token",
explanation: "The browser session might have been cleared, " +
"or something went wrong.",
)]]
end
end
end
2 changes: 2 additions & 0 deletions lib/better_errors/templates/main.erb
Original file line number Diff line number Diff line change
Expand Up @@ -800,6 +800,7 @@
(function() {

var OID = "<%= id %>";
var csrfToken = "<%= csrf_token %>";

var previousFrame = null;
var previousFrameInfo = null;
Expand All @@ -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) {
Expand Down
55 changes: 42 additions & 13 deletions spec/better_errors/middleware_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ def initialize(message, original_exception = nil)

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({})
Expand Down Expand Up @@ -229,18 +229,17 @@ 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}'),
)
app.call(request_env)
}
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(:status) { result[0] }
let(:headers) { result[1] }
let(:body) { result[2].join }
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
Expand Down Expand Up @@ -291,14 +290,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
Expand Down

0 comments on commit 617e65e

Please sign in to comment.