-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Adds support for configuring HTTP Feature Policy #33439
Conversation
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @georgeclaghorn (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
cc @pixeltrix as you implemented the CSP DSL and this is along the same lines. |
5097856
to
68d216a
Compare
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
68d216a
to
de928c2
Compare
end | ||
|
||
feature_policy only: :sample_controller do |f| | ||
f.gyroscope 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.
I'm not sure how I feel about this API. The idea is that you define a global policy you need to be explicit about overriding it with nil
to remove it. Thoughts?
This seems to be a very unstable API that may change any time. I don't think it is mature enough to be included in a framework just yet. |
Cool! Makes me want an explicitly-experimental API status so app developers can track this as it's developed, but the clear expectation that it may change to track upstream. |
I can definitely see your concerns here @rafaelfranca and they are warranted as many of these new browser security features can go through multiple iterations before becoming usable by more than a single browser (I'm looking at your content security policy!). I really like @jeremy's idea of explicitly calling this out as potentially experimental due to the browser implementations not all solidified yet. I think that will give us a nice middle ground of adding security for those that do want to use this feature while still maintaining some sensibility in the framework itself. |
I'm ok with that. My worry is that if they rename a configuration it would require a released of the framework to rename that configuration. Having this is a gem would give more flexibility. I'm happy to keep an official Rails gem for this with the promise to integrate to the framework as soon the API is stable enough. |
Rather timely occurrence, this morning Mozilla has announced they will be shipping this to nightly - https://groups.google.com/d/msg/mozilla.dev.platform/w3elPpZlqIE/ZA5cuKOMCAAJ That just leaves Edge without support (for now) from the majority of browsers. |
Re. draft spec stability: w3c/webappsec-permissions-policy#218 (comment) I'm all for pitching larger additions like this via gems too. However, I think this particular spec, like CSP before it, is important enough to bake in as a default for new Rails 6 apps. And during prerelease is the prime time to iterate on the new-app and upgrade experience. |
Sounds good to me them. Let's move to document this PR so we can merge before the pre release. |
@rafaelfranca Sure thing! Would you like the documentation in this PR or would you link a separate one for that? |
@jeremy @rafaelfranca could either of you point me towards the documentation that you're expecting here and we can get this one over the line? |
I've gone ahead and updated this to the latest master as well as adding a CHANGELOG entry for the new DSL. Sing out if you want more documentation in other forms. |
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.
🙏🏼
# frozen_string_literal: true | ||
|
||
module ActionController #:nodoc: | ||
module FeaturePolicy |
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 for docs here (
# TODO: Documentation |
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.
Thanks for the pointer @jeremy! I wasn't sure if the intention was for code documentation or something like the Guide documentation. I'll get something added here as a start.
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.
Addressed this in 88fb316.
Let me know when/if we're ready here and I'll squash this into a single commit before merging it. (Unless whoever merges is happy squashing-when-merging using the GitHub UI and just take the initial commit) |
a47d6a1
to
cd85d93
Compare
# f.microphone :none | ||
# f.usb :none | ||
# f.fullscreen :self | ||
# f.payment :self, "https://secure-example.com" |
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.
Unlike example.com
, secure-example.com
is a domain that can be used normally. It should not be used as an example.
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 catch. While this is only an example for configuration, we should definitely avoid using real world domains and instead restrict to using using the reserved domains. I'll open a PR to address.
`speaker`, `vibrate`, and `vr` were [listed as policy-controlled features][1] around the time when rails#33439 was first written (2018-07-25). However, `vibrate` was removed in w3c/webappsec-permissions-policy@b7271ac, `vr` was changed to `xr` in w3c/webappsec-permissions-policy@bec5ce6, and `speaker` was removed in w3c/webappsec-permissions-policy@18707d3. (And `xr` was later changed to `xr-spatial-tracking`, and still only has [experimental support][2].) Therefore, this commit deprecates these permissions policy directives. [1]: https://github.com/w3c/webappsec-permissions-policy/blob/6d8bbbe738368c317d280bb866c66cedaef3391c/features.md#policy-controlled-features [2]: https://github.com/w3c/webappsec-permissions-policy/blob/432a1532c943a079351eecbc445ae4c6ed9b949c/features.md#standardized-features
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:
This will prevent the browser from using geolocation and only allow
autoplay on
https://example.com
. Full feature list can be found overin the WICG repository.
As of today Chrome and Safari have public support for this
functionality with Firefox working on support and Edge still pending
acceptance of the suggestion.
Examples
Using an initializer
In a controller
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'lllook 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 a
great in-depth write up of this functionality.