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

Adds support for configuring HTTP Feature Policy #33439

Merged
merged 5 commits into from Jul 10, 2019

Conversation

@jacobbednarz
Copy link
Contributor

commented Jul 26, 2018

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.

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

# 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

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 a
great in-depth write up of this functionality.

@rails-bot

This comment has been minimized.

Copy link

commented Jul 26, 2018

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.

@jacobbednarz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

cc @pixeltrix as you implemented the CSP DSL and this is along the same lines.

@jacobbednarz jacobbednarz force-pushed the jacobbednarz:add-feature-policy branch Jul 26, 2018
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
@jacobbednarz jacobbednarz force-pushed the jacobbednarz:add-feature-policy branch to de928c2 Jul 26, 2018
end

feature_policy only: :sample_controller do |f|
f.gyroscope nil

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Jul 26, 2018

Author Contributor

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?

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

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.

@jeremy

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

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.

@jeremy jeremy requested a review from pixeltrix Sep 13, 2018
@jacobbednarz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2018

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.

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

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.

@jacobbednarz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2018

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.

@jeremy

This comment has been minimized.

Copy link
Member

commented Sep 14, 2018

Re. draft spec stability: w3c/webappsec-feature-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.

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

Sounds good to me them. Let's move to document this PR so we can merge before the pre release.

@jacobbednarz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2018

@rafaelfranca Sure thing! Would you like the documentation in this PR or would you link a separate one for that?

@jacobbednarz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

@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?

jacobbednarz added 2 commits Jul 9, 2019
@jacobbednarz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

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.

@jeremy
jeremy approved these changes Jul 10, 2019
Copy link
Member

left a comment

🙏🏼

# frozen_string_literal: true

module ActionController #:nodoc:
module FeaturePolicy

This comment has been minimized.

Copy link
@jeremy

jeremy Jul 10, 2019

Member

Good spot for docs here (

notwithstanding 😅). Using the changelog entry verbatim could work well!

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Jul 10, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Jul 10, 2019

Author Contributor

Addressed this in 88fb316.

@jacobbednarz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

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)

@jacobbednarz jacobbednarz force-pushed the jacobbednarz:add-feature-policy branch from a47d6a1 to cd85d93 Jul 10, 2019
@jeremy jeremy merged commit bf19b87 into rails:master Jul 10, 2019
2 checks passed
2 checks passed
buildkite/rails Build #62135 passed (8 minutes, 45 seconds)
Details
codeclimate All good!
Details
@jacobbednarz jacobbednarz deleted the jacobbednarz:add-feature-policy branch Jul 10, 2019
# f.microphone :none
# f.usb :none
# f.fullscreen :self
# f.payment :self, "https://secure-example.com"

This comment has been minimized.

Copy link
@y-yagi

y-yagi Jul 13, 2019

Member

Unlike example.com, secure-example.com is a domain that can be used normally. It should not be used as an example.

This comment has been minimized.

Copy link
@jacobbednarz

jacobbednarz Jul 14, 2019

Author Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.