Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CSRF protection to internal requests #474

Merged
merged 4 commits into from Sep 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/better_errors/error_page.rb
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
46 changes: 38 additions & 8 deletions lib/better_errors/middleware.rb
@@ -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,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
Expand All @@ -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
Expand All @@ -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
2 changes: 2 additions & 0 deletions lib/better_errors/templates/main.erb
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