Skip to content

Feature-Policy header#142

Closed
peldax wants to merge 5 commits into
nette:masterfrom
peldax:patch-1
Closed

Feature-Policy header#142
peldax wants to merge 5 commits into
nette:masterfrom
peldax:patch-1

Conversation

@peldax

@peldax peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor
  • 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

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

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

dg commented Aug 13, 2018

Copy link
Copy Markdown
Member

Comment thread src/Bridges/HttpDI/HttpExtension.php Outdated
'frames' => 'SAMEORIGIN', // X-Frame-Options
'csp' => [], // Content-Security-Policy
'csp-report' => [], // Content-Security-Policy-Report-Only
'fp' => [], // Feature-Policy

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@peldax

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

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

dg commented Aug 13, 2018

Copy link
Copy Markdown
Member

I think that reusing is good idea.

@peldax

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

@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

dg commented Aug 13, 2018

Copy link
Copy Markdown
Member

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

@peldax

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

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

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

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

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

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

@dg

dg commented Aug 13, 2018

Copy link
Copy Markdown
Member

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

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

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

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

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

dg commented Aug 13, 2018

Copy link
Copy Markdown
Member

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

@peldax

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

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

dg commented Aug 13, 2018

Copy link
Copy Markdown
Member

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

@peldax

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

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

dg commented Aug 13, 2018

Copy link
Copy Markdown
Member

I have no better idea than check for .

@peldax

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

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

@peldax

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

@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

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

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

@peldax

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

@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

dg commented Aug 13, 2018

Copy link
Copy Markdown
Member

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

@peldax

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

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

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

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

@dg

dg commented Aug 13, 2018

Copy link
Copy Markdown
Member

Is it meaningful to have a hash in configuration?

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

@peldax

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

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

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

@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

dg commented Aug 13, 2018

Copy link
Copy Markdown
Member

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

peldax commented Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

Thanks 👍

@harmim

harmim commented Aug 14, 2018

Copy link
Copy Markdown
Contributor

Nice one guys, cheers!

@dg

dg commented Aug 28, 2018

Copy link
Copy Markdown
Member

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants