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

Feature-Policy header #142

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@peldax
Copy link
Contributor

peldax commented Aug 13, 2018

  • new feature
  • BC break? no
  • doc PR: None ATM

Přidává hlavičku Feature-Policy.
Header ještě není schválený standard, ale jediné nad čím se váhá je, jaké featury budou prohlížeče implementovat.

Syntax stejná jako Content-Security-Policy, prakticky jsem jen použil existující kod.

Hlavička Feature-Policy-Report-Only zatím neexistuje a co vím ani se o ní nepřemýšlí.

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

Možná by šlo sloučit skládání s content security policy - ten skládací cyklus je úplně stejný - ale nechtěl jsem moc měnit existující kod, takhle je jasné co to dělá.

@dg

This comment has been minimized.

@@ -26,6 +26,7 @@ class HttpExtension extends Nette\DI\CompilerExtension
'frames' => 'SAMEORIGIN', // X-Frame-Options
'csp' => [], // Content-Security-Policy
'csp-report' => [], // Content-Security-Policy-Report-Only
'fp' => [], // Feature-Policy

This comment has been minimized.

@dg

dg Aug 13, 2018

Member

'feature-policy' => [] is IMHO better

This comment has been minimized.

@peldax

peldax Aug 13, 2018

Contributor

I thought so too, but I wanted to keep the same scheme as ´csp´.

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

You thumb-uped my comment about reusing the code from csp loading, but I am not sure what it means - should I keep it separate or should I reuse the code (in some function for example)?

In my other pr #143 I modified the csp loading loop (whitelist keywords). That makes reusing out of question, what do you think?

@dg

This comment has been minimized.

Copy link
Member

dg commented Aug 13, 2018

I think that reusing is good idea.

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

@dg I added lambda function (Would you prefer member function instead?) to generate header value. I also renamed the key to feature-policy instead of fp.

I also included quote whitelist from #143 - there would have been conflicts and one PR would depend on the other - they both change same code.

@dg

This comment has been minimized.

Copy link
Member

dg commented Aug 13, 2018

Would not it be better to make list of words that must be quoted?

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

I thought about it for a while and decided otherwise because of various reasons:

  • that list would be bigger (not a huge issue)
  • hashes and nonce are also quoted

Well, now while writing this I realised that current implementation doesn't support hash whitelisting.

Content-Security-Policy: script-src 'sha256-B2yPHKaXnvFWtRChIbabYmUBFZdVfKKXHbWtWidDVF8='

Hash definition doesn't pass regex and gets inserted without quotes.

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

I'll write properties here and let you decide how should we proceed.

Quoted:

  • Nonce
  • Hash
  • majority of keywords

Not quoted:

  • URLs
  • some keywords
  • colon keywords - eg. https:, data:, blob:
@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

I would personally check for second - regex URL check, keyword whitelist check, colon at the end check - but you are the architect here :).

@dg

This comment has been minimized.

Copy link
Member

dg commented Aug 13, 2018

So probably it's best to stick with current solution and add exceptions for require-sri-for & sandbox. It'll be the easiest, what do you think?

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

I think the regex should be changed to match URLs only (to support hashes) and then either add those colon keywords to whitelist or check for colon at the end.

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

Check if value is URL, check if type is require-sri-for or sandbox, check if value has colon at the end - quote everything else.

@dg

This comment has been minimized.

Copy link
Member

dg commented Aug 13, 2018

preg_match('#allow-|style\z|script\z|.*[.:]#A', $item)

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

Wouldnt it be better to add exceptions for require-sri-for & sandbox as you suggested earlier - all possible values for those two are not qouted.

@dg

This comment has been minimized.

Copy link
Member

dg commented Aug 13, 2018

I think yes. So regexp can be simply preg_match('#[.:]#', $item), or strpbrk($item, '.:') === false

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

And how to deal with the URLs ? Format can be just example.com, schema is optional, can also contain * wildcard. I am thinking about checking for ., but it is not self documenting and is seriously counter intuitive.

@dg

This comment has been minimized.

Copy link
Member

dg commented Aug 13, 2018

I have no better idea than check for .

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

I just noticed you already have the dot in your regex, didnt notice, sorry, expect the fix in few minutes. :)

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

@dg Its up - but it seems like it broke some tests.

I am not sure what should be the outcome of multiple values in one string eg. 'self' example.com. This example passes correctly, but eg. 'self' 'unsafe-inline' dont - it gets quoted.

I am not sure why would anyone merge multiple values into one string in configuration.

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

I added * to regex, because it possible to pass only * wildcard as value to allow from all - not quoted.

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

@harmim noted there are also other directives with non-quote values: reflected-xss, plugin-types and referrer.

Reflected-xss and referrer are deprecated and droped from standard, should I include them in the whitelist?

@dg dg closed this in e038d90 Aug 13, 2018

@dg

This comment has been minimized.

Copy link
Member

dg commented Aug 13, 2018

I merged it with original regexp. It there reason to change it?

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

Not for feature policy. But for Content Security policy, there are issues with quoting - hashes are unquoted but should, style/script and some others gets quoted but shouldnt.

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

Together with @harmim we completed the list of directives, which have unquoted values - those are require-sri-for, plugin-types and sandbox.

@dg

This comment has been minimized.

Copy link
Member

dg commented Aug 13, 2018

Is it meaningful to have a hash in configuration?

require-sri-for, plugin-types and sandbox are skipped.

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

Not for me, but it is defined in standard, so it might be good idea to support it even when its meaningless.

Some simple website with eg. 5 additional files could restrict its sources by hashes, IDK...

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

@dg Are the changes going to make it into nette 2.4 ? I would welcome these features in my project (also cookies sameSite - I noticed it is implemented in some recent branch.)
I dont want to pressure you, if its demanding or depends on some other stuff I'll surely wait.

@dg

This comment has been minimized.

Copy link
Member

dg commented Aug 13, 2018

I think that https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity is much better way than hashes in CSP, so I would not add support at all.

Backported to 2.4

@peldax

This comment has been minimized.

Copy link
Contributor

peldax commented Aug 13, 2018

Thanks 👍

@harmim

This comment has been minimized.

Copy link
Contributor

harmim commented Aug 14, 2018

Nice one guys, cheers!

@dg

This comment has been minimized.

Copy link
Member

dg commented Aug 28, 2018

I have changed option to featurePolicy to be consistent with the rest of configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment