-
Notifications
You must be signed in to change notification settings - Fork 21.8k
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 DSL for configuring Content-Security-Policy header #31162
Conversation
cf2c4fb
to
e70f128
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely. Thanks for digging in to this. Added comments on 1. protect_content
method naming; 2. declaring the base content security policy rather than using a boolean flag to indicate whether to use :application as the default policy; and 3. separating policy definitions from policy enforcement decisions, moving report-only out to the controller.
Also, food for thought: should the base policy be applied to all HTML responses in a Rack middleware? Catching other mounted apps, static pages, error pages, etc.
self.content_security_policies = PolicyCollection.new | ||
|
||
config_accessor :default_protect_content | ||
self.default_protect_content = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot to specify the app-wide policy. Since this is the base policy for the whole app, be definitive:
# Set the base policy
config.action_controller.content_security_policy = :application
# No app-wide policy
config.action_controller.content_security_policy = nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with a single base policy so this becomes irrelevant.
@policies.fetch(name).clone | ||
rescue KeyError | ||
raise ArgumentError, "Unknown content security policy: #{name.inspect}" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dupes #fetch
impl above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Irrelevant now that collection is going away
elsif values.first | ||
@directives["sandbox"] = values | ||
else | ||
@directives.delete("sandbox") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the caller for this case look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's if you need to disable the value on a controller basis, e.g.
class LegacyPagesController < ApplicationController
content_security_policy do |p|
p.sandbox false
end
end
end | ||
|
||
def report_only!(uri:) | ||
@report_only = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would treat the policy itself and how it's enforced as separate concerns. Can hoist this (and header_name
) out to the controller and handle in set_content_security_policy
.
directives.each do |directive, sources| | ||
copy[directive] = sources.map(&:dup) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
directives.transform_values { |sources| sources.map(&:dup) }
MAPPINGS.fetch(source) | ||
else | ||
raise ArgumentError, "Unknown content security policy source mapping: #{source.inspect}" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can incorporate the existence check into the fetch:
MAPPINGS.fetch(source) do
raise ArgumentError, "Unknown content security policy source mapping: #{source.inspect}"
end
elsif sources | ||
directive | ||
else | ||
nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appears that a directive sources will never be nil
, should just be absent. Raise here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is when a directive is set to false
- there's a compact
operation that removes these nil values in header_value
:
def header_value(context = nil)
build_directives(context).compact.join("; ") + ";"
end
|
||
unless policy.empty? | ||
self.content_security_policy = policy | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe an edge case, but it could be sensible to set the header even if the policy is empty, making it clear that we've set an empty policy rather than omitting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, agreed.
end | ||
end | ||
|
||
after_action :set_content_security_policy, unless: :content_security_policy_present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we foresee a controller action providing a custom policy that should preclude the declared controller policy for that action?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unless: :content_security_policy_present?
was more about handling upgraded apps that may have set the header manually - didn't want to break them.
policy :report_only, using: :application do |p| | ||
p.report_only!(uri: "/csp-violation-report-endpoint") | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this policy collection API for central definition of multiple policies. I think in practice we'll have a single base policy that's extended in controller subclasses. If there's a set of controllers that use a stricter policy, I'd probably start by mixing in a concern that sets the strict policy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - a single base policy with overrides either in concern or a common parent controller class makes the implementation simpler and doesn't lose any flexibility.
@jeremy thanks for the review - agree that we should generate the default policy for all HTML responses so will move the policy class out to Action Dispatch and then just have the override implementation in Action Controller. |
9404f54
to
727c415
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits, but overall on API 👍
|
||
def content_security_policy_report_only(*args) | ||
options = args.extract_options! | ||
flag = args.empty? ? true : args.first.present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be simplified with some kwarg help?
def content_security_policy_report_only(report_only = true, **options)
end
if MAPPINGS.key?(source) | ||
MAPPINGS.fetch(source) | ||
else | ||
raise ArgumentError, "Unknown content security policy source mapping: #{source.inspect}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could do:
MAPPINGS.fetch(source) do
raise ArgumentError, …
end
p.style_src :self, :https, :unsafe_inline | ||
|
||
# Specify URI for violation reports | ||
# p.report_uri "/csp-violation-report-endpoint" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space after p.report_uri
.
# For further information see the following documentation | ||
# https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy | ||
|
||
Rails.application.config.content_security_policy do |p| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this config be commented out for upgrading apps? Or should it be loose enough that apps can upgrade seamlessly and get more security?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do initializers get generated for upgrading apps automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, will investigate what the preferred solution is to be once the beta ships and it's easier to test against upgrading real world apps.
727c415
to
b8f5120
Compare
b8f5120
to
456c3ff
Compare
Fix CHANGELOG for CSP PR #31162 [ci skip]
Is there a guide or any documentation on how to use this in development? For example, JavaScripts served through the asset pipeline or Webpacker (either precompiled or with the hot-reloading development server) are all HTTP and therefore left broken with the default configuration. |
@dwightwatson I thought that |
@pixeltrix That's what I thought as well, and after digging further into it that appears to be correct. The issue I'm seeing is related but has to do with Should I open another ticket to follow this issue instead? It might require the CSP configuration to be altered to allow for Webpacker assets or Webpacker might need to be configured to bundle another way.
|
@dwightwatson is it the webpacker gem that uses |
@dwightwatson I wonder whether rails/webpacker#1011 is related? Can you change |
I was just reading the same thing - yeah it looks like that configuration change fixes the issue, all working locally now. Is making that configuration the new default something that could be reconsidered? |
Possibly - will raise it in the Campfire channel. |
I'm also having this issue, using rack-livereload:
|
@crispinheneise the quick fix is to add |
This may be an edge case, or it might belong to rack-livereload - if so please ignore. In order to get CSP to work with rack-livereload I had to add if Rails.env.development?
p.script_src :self, :https, :unsafe_inline, 'http://localhost:35729'
p.connect_src 'http://localhost:35729', 'ws://localhost:35729'
else
p.script_src :self, :https
end |
@crispinheneise only issue is that change is that you'll only find out about your own app's use of inline scripts when you deploy to production 😟 |
Document in the guides the new middleware added in rails#31162 [ci skip]
Hi @pixeltrix , Thanks for implementing CSP in Rails! We are still on Rails 5.0.6 so we had to backport your code to get things in motion. I did have one question regarding the ability to set both headers: POLICY = "Content-Security-Policy".freeze
POLICY_REPORT_ONLY = "Content-Security-Policy-Report-Only".freeze Currently it seems like if Report-Only option is true, it takes precedence:- def header_name(request)
if request.content_security_policy_report_only
POLICY_REPORT_ONLY
else
POLICY
end
end The CSP documentation says that both headers can be present in the same response and could have different policies. I am wondering if this was deliberate and whether Rails has plans to honor both headers in the near future? |
A HTTP feature policy is Yet Another HTTP header for instructing the browser about which features the application intends to make use of and to lock down access to others. This is a new security mechanism that ensures that should an application become compromised or a third party attempts an unexpected action, the browser will override it and maintain the intended UX. WICG specification: https://wicg.github.io/feature-policy/ The end result is a HTTP header that looks like the following: ``` Feature-Policy: geolocation 'none'; autoplay https://example.com ``` This will prevent the browser from using geolocation and only allow autoplay on `https://example.com`. Full feature list can be found over in the WICG repository[1]. As of today Chrome and Safari have public support[2] for this functionality with Firefox working on support[3] and Edge still pending acceptance of the suggestion[4]. #### Examples Using an initializer ```rb # config/initializers/feature_policy.rb Rails.application.config.feature_policy do |f| f.geolocation :none f.camera :none f.payment "https://secure.example.com" f.fullscreen :self end ``` In a controller ```rb class SampleController < ApplicationController def index feature_policy do |f| f.geolocation "https://example.com" end end end ``` Some of you might realise that the HTTP feature policy looks pretty close to that of a Content Security Policy; and you're right. So much so that I used the Content Security Policy DSL from rails#31162 as the starting point for this change. This change *doesn't* introduce support for defining a feature policy on an iframe and this has been intentionally done to split the HTTP header and the HTML element (`iframe`) support. If this is successful, I'll look to add that on it's own. Full documentation on HTTP feature policies can be found at https://wicg.github.io/feature-policy/. Google have also published[5] a great in-depth write up of this functionality. [1]: https://github.com/WICG/feature-policy/blob/master/features.md [2]: https://www.chromestatus.com/feature/5694225681219584 [3]: https://bugzilla.mozilla.org/show_bug.cgi?id=1390801 [4]: https://wpdev.uservoice.com/forums/257854-microsoft-edge-developer/suggestions/33507907-support-feature-policy [5]: https://developers.google.com/web/updates/2018/06/feature-policy
A HTTP feature policy is Yet Another HTTP header for instructing the browser about which features the application intends to make use of and to lock down access to others. This is a new security mechanism that ensures that should an application become compromised or a third party attempts an unexpected action, the browser will override it and maintain the intended UX. WICG specification: https://wicg.github.io/feature-policy/ The end result is a HTTP header that looks like the following: ``` Feature-Policy: geolocation 'none'; autoplay https://example.com ``` This will prevent the browser from using geolocation and only allow autoplay on `https://example.com`. Full feature list can be found over in the WICG repository[1]. As of today Chrome and Safari have public support[2] for this functionality with Firefox working on support[3] and Edge still pending acceptance of the suggestion[4]. #### Examples Using an initializer ```rb # config/initializers/feature_policy.rb Rails.application.config.feature_policy do |f| f.geolocation :none f.camera :none f.payment "https://secure.example.com" f.fullscreen :self end ``` In a controller ```rb class SampleController < ApplicationController def index feature_policy do |f| f.geolocation "https://example.com" end end end ``` Some of you might realise that the HTTP feature policy looks pretty close to that of a Content Security Policy; and you're right. So much so that I used the Content Security Policy DSL from #31162 as the starting point for this change. This change *doesn't* introduce support for defining a feature policy on an iframe and this has been intentionally done to split the HTTP header and the HTML element (`iframe`) support. If this is successful, I'll look to add that on it's own. Full documentation on HTTP feature policies can be found at https://wicg.github.io/feature-policy/. Google have also published[5] a great in-depth write up of this functionality. [1]: https://github.com/WICG/feature-policy/blob/master/features.md [2]: https://www.chromestatus.com/feature/5694225681219584 [3]: https://bugzilla.mozilla.org/show_bug.cgi?id=1390801 [4]: https://wpdev.uservoice.com/forums/257854-microsoft-edge-developer/suggestions/33507907-support-feature-policy [5]: https://developers.google.com/web/updates/2018/06/feature-policy
Pushing up to get feedback on implementation
Documentation
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy
Examples
Global config in an initializer:
Per controller configuration: