Skip to content
This repository has been archived by the owner on Apr 16, 2021. It is now read-only.

!!!DO NOT MERGE!!! Add a Feature-Policy #445

Closed
wants to merge 1 commit into from

Conversation

gclssvglx
Copy link
Contributor

@gclssvglx gclssvglx commented Jul 10, 2020

!!! DO NOT MERGE THIS PR !!!

Wait until the Feature-Policy (or Permissions-Policy as it may become) has been finalised and Rails has support for it.

See here for details.

What

As part of a security review of the service, it was noted that a Feature-Policy is missing from the service.

Feature Policy allows web developers to selectively enable, disable, and modify the behaviour of certain APIs and web features in the browser. It's like CSP but instead of controlling security, it controls features! - Google Developers Article

This change adds a Feature-Policy to the service.

The Feature-Policy headers are a new(ish) addition to the overall HTTP header set, and currently un-supported in the present version of Rails used on service (v6.0.3). However, support for this will be available in in Rails v6.1 - which is expected to be released by the end of July 2020. Therefore, in order not to create a blocked story or PR the Rails version is tested prior to injecting the policy. This will automatically enable the Feature-Policy once the service has been upgraded to Rails v6.1 but silently ignored in the meantime.

Feature-Policy headers can be set to:

  • * (the feature is allowed by self and all iframe's),
  • self (the feature is only allowed from the same origin),
  • none (the feature is not allowed or blocked) or
  • origin(s) (the feature is only allowed from the specified origins).

As we are blocking iframe's (see this PR), and we currently do not use any external origin's, we only need to consider self (on or allowed) and none (off or disallowed) as viable options. Following the principle of starting with the most restrictive header setting and relax or adjust based on requirements, all Feature-Policy headers have been set to :none - except where these features are needed for assistive technologies (i.e. microphone, speaker, fullscreen and vibrate).

Links

@bevanloon bevanloon temporarily deployed to coronavirus-security-re-eu69sm July 10, 2020 07:37 Inactive
@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2020

Codecov Report

Merging #445 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #445   +/-   ##
=======================================
  Coverage   99.29%   99.29%           
=======================================
  Files          71       71           
  Lines        1972     1972           
=======================================
  Hits         1958     1958           
  Misses         14       14           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 786097f...d5c454a. Read the comment docs.

@pixeltrix
Copy link
Contributor

Interesting that you have to leave features on for assistive technologies as it seems like something that should be under the user's control and not up to the page. Is this something you've tested? There doesn't seem to be much discussion on w3c/webappsec-feature-policy that I could find.

Also looks like we're going to have to revisit this feature before we release 6.1 as it's been renamed to Permissions Policy and also some directives are not valid like vibrate. The current policy controlled feature list is available here:

https://github.com/w3c/webappsec-feature-policy/blob/master/features.md

@gclssvglx
Copy link
Contributor Author

Interesting that you have to leave features on for assistive technologies as it seems like something that should be under the user's control and not up to the page. Is this something you've tested? There doesn't seem to be much discussion on w3c/webappsec-feature-policy that I could find.

Also looks like we're going to have to revisit this feature before we release 6.1 as it's been renamed to Permissions Policy and also some directives are not valid like vibrate. The current policy controlled feature list is available here:

https://github.com/w3c/webappsec-feature-policy/blob/master/features.md

Thanks @pixeltrix 👍 No, it's not something I've tested per-see - only that the headers produced are what I'd expect. I'll have a look into Permission-Policy - I guess it's a better name than Feature-Policy - and the (now) current list. Thanks for pointing these out.

@gclssvglx
Copy link
Contributor Author

https://github.com/w3c/webappsec-feature-policy/blob/master/features.md

That's a heck of a list! Hopefully, there will be a default (none ideally), that way we can just set self the ones we want to be enabled.

@gclssvglx gclssvglx force-pushed the security-review-add-feature-policy branch from b633caa to d5de32a Compare July 14, 2020 10:40
Copy link
Contributor

@reggieb reggieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gclssvglx gclssvglx changed the title Add a Feature-Policy !!!DO NOT MERGE!!! Add a Feature-Policy Jul 14, 2020
As part of a security review of the service, it was noted that a
Feature-Policy is missing.

This change adds a Feature-Policy to the service.
@Rosa-Fox
Copy link
Contributor

Closed due to archiving this repository.

@Rosa-Fox Rosa-Fox closed this Apr 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants