-
Notifications
You must be signed in to change notification settings - Fork 254
Description
This is more of a meta issue where I'm going to drop notes before actually coming up with a documented plan.
We intend to go through this process in the not so distant future and will have lessons to learn. GitHub uses the following features:
- Tons of dynamic modifications via 1st append_content_security_policy_directives`
- No less than three separate policies in one application - standard, error pages, some framed content
- The automatic handling of calling
append_content_security_policy_directives
on a value that is configured as'none'
https://bauland42.com/ruby-on-rails-content-security-policy-csp/ does a good job of explaining both APIs.
https://medium.com/just-tech/content-security-policy-ruby-on-rails-836d0c7ba098j is a story of someone doing exactly this (and calling out one important difference.
base config
Creating your base config from the secure_headers config is mostly a find/replace. Instead of a hash, you're configuring an object with methods named after directives. They take varargs so the arrays need to be splatted.
# TODO: provide find/replace regex for automating this
# secure_headers
SecureHeaders::Configuration.default do |config|
config.csp = {
directive_name: some_array
}
end
# rails
Rails.application.config.content_security_policy do |config|
config.directive_name *some_array
end
dynamic modifications
As mentioned in https://bauland42.com/ruby-on-rails-content-security-policy-csp/, the rails API overwrites config values. In secure_headers land, we generally append sources to an existing list rather than have to redefine the entire list upon use.
def some_controller_action
the_value = request.content_security_policy.frame_src
is_now_nil =request.content_security_policy.frame_src
end
def some_controller_action_overwriting
# whatever was in frame_src was lost/overwritten and this only contains the new_sources
request.content_security_policy.frame_src new_sources
end
def some_controller_action_appending
current_config = request.content_security_policy.frame_src
request.content_security_policy.frame_src *(current_config + new_sources)
end
So the obvious workaround is to grab the current config and then re-set the value along with additions. This shim seems to generally solve the problem while backporting some functionality, notably:
- When calling
append_content_security_policy_directives
on a currently undefined directive, populate the newly defined directive with the current value ofdefault_src
- When calling
append_content_security_policy_directives
on a directive with the value ofnone
, discard thenone
value
# this is just one option. We don't use the railtie so we're calling `SecureHeaders.append_content_security_policy_directives(request, additions)`. A simple multiline regex can convert these to `append_content_security_policy_directives(additions)`. I intend to write a rubocop for this so maybe I'll get the autocorrect working.
def append_content_security_policy_directives(additions)
# TODO: add allowlist for keys in the additions hash (re-use some constant)
if GitHub.flipper[:vanilla_csp].enabled?(current_user)
# current_content_security_policy gives us a safe, duplicate copy to use.
current_policy = current_content_security_policy
additions.each do |directive, source_values|
# TODO handle other unsupported values
next if directive == :preserve_schemes
if directive == :report_uri
request.content_security_policy.report_uri source_values
else
current_value = current_policy.send(directive)
if config.nil?
config = []
config = current_policy.default_src
end
if current_value == %w('none')
request.content_security_policy.send(directive, *source_values.compact.uniq)
else
request.content_security_policy.send(directive, *(current_value + source_values).compact.uniq)
end
end
end
else
SecureHeaders.append_content_security_policy_directives(request, additions)
end
end
def some_before_filter_on_every_request
if GitHub.flipper[:vanilla_csp].enabled?(current_user)
# we don't need to disable built-in rails because it exits if a policy is set
# depending on how middleware is loaded, we could end up generating a policy that is overwritten by secure_headers
SecureHeaders.opt_out_of_header(request, :csp)
end
end
testing
By default, rails does not include middleware when running tests. If you're testing your CSP (hint: you should) then you've already done this in your test class:
def app
@app ||= Rack::Builder.new do
use SecureHeaders::Middleware
use ActionDispatch::ContentSecurityPolicy::Middleware # this is the new addition
run GitHub::Application
end
end
end
differing policies
If you're testing your policies (seriously: do it) then you won't get exact matches without some extra steps.
Rails won't dedup anything so you'll be calling uniq
if you care. Rails doesn't like nil
s in config values, secure_headers
doesn't mind and will filter them. secure_headers tries to do fancy stuff around consolidating *
or overlapping config values, rails is more literal. secure_headers omits schemes by default, rails does what you tell it to.
secure_headers generates a policy that is default_src ... everything else ... report_uri
whereas rails doesn't enforce this order.
config values that won't translate
The preserve_schemes
/ disable_nonce_backwards_compatibility
concepts do not exist in rails.
secure_headers with CSP disabled is somewhat of an edge case (for me)
I've never done it but while I was working on this, a test of ours that mucks with 1st X-Frame-Options raised an error about default_src not being configured 😆 thar be dragons.
non-html resources don't get CSP
I'll add details, but there's a situation where you need to send csp on PDF resources and you may want to for e.g. api requests.
And more
I'll keep updating this issue body as we roll out the change and I'd love to incorporate anything you've learned.