Provides a filter to add nonce attribute to inline scripts.

The idea of user-inputted inline scripts still scares me, but I don't think it's something that shouldn't be possible for adequately trusted roles.

I think that there should be some help text to indicate that the filter should only be enabled for trusted roles.

A potentially expected complement could be to enable inline style blocks? That could be a separate filter, or a single filter configurable to enable either scripts or styles as desired.

There's also a gap that enabling the filter will add a nonce to the markup, but that won't permit any scripts it processes unless the nonce is added to the header through some other means ( ✨ Add helper for safely appending nonce/hash sources Fixed will help with safely adding the nonce or 'unsafe-inline' fallback).

Thank you very much for your feedback, @gapple. I have changed the following:

* Added a message stating that only trusted roles should be allowed to use this filter.
* Added support for inline styles through a filter setting.
* Created a context cache that allows us to create a new variation for each nonce value.

I'm attaching a new patch.
Thank you!

It's really important that nonce values are not cached in page fragments - if a cached nonce is repeatably sent for markup on one section of a page, another component with an XSS vulnerability could hard code that nonce value to permit a malicious script.

I think this is the use for placeholdering - the filtered markup can be cached in the Dynamic Page Cache with the placeholder, which is only replaced later in the rendering process with the actual value. (having the nonce cached in the Full Page Cache is fine, since the nonce will change if any markup on the page changes). I made this change in my MR, and it appears to work.

Using cache context to bubble up that markup on the page needs the nonce applied in the headers feels a little hack-y, but it could work. I think the limitation for the current code is that it doesn't indicate whether scripts and/or styles need the nonce value applied to the corresponding directives - I think using cache context would require separate contexts for each.
An alternative that I think I might like the semantics of a bit more is attaching a new (empty) library (e.g. 'csp/inline-js-nonce' / 'csp/inline-css-nonce'), that a subscriber to the alter event can then check for.

(This is the second time I want Drupal core to allow additional keys in render arrays' attachments so that more info can bubble up...)

Stepping through with a debugger, I found a few things:
- The placeholder is present until the Response event, when HtmlResponseSubscriber processes attachments and does the replacement, so this method works as expected with the Dynamic Page Cache, and nonce attribute appears to receive a new value whenever the page is rendered with any changes 👍
- The placeholder replacement takes place over the entire page response. Most placeholders use a hash of the lazy_builder method and arguments, which is potentially predictable. Not an issue in most cases, where it would just allow duplicating a page fragment elsewhere on the page, but for a CSP nonce, it could allow an XSS vulnerability to use the placeholder to permit it's own script with the nonce 😱. I think adding a random string to the placeholder should be sufficient to prevent its misuse.
- Big Pipe appears to wrap the placeholder for it to do dynamic replacement via JS, which will prevent the inline script from working properly (AFAICT - the script will be blocked when loaded with the placeholder value in the nonce attribute, and I don't think it will be re-evaluated after the proper value is placed). Need to find a way to have the nonce placeholder processed through the normal core method, and not via big pipe.

I added an 8-byte random string to the placeholder key. It's only present server side and never exposed in the request, so just needs to be sufficiently long that an attempt at enumerating guesses is unlikely to escape notice.
As a property on NonceFilter, all placeholders in the same request will have the same key (which is fine). Since those page fragments can be cached with the placeholder, a page may end up with multiple placeholders (which is also fine, if just slightly more processing for the final render).

For big pipe, it looks like it differentiates between placeholders that are html elements (<filter-placeholder ... />) which will have their rendering delayed until after the main page content is flushed for JS to swap, and "attribute-safe" (filter-placeholder) that it will replace server-side in the main page content 👍.

Thank you very much for your review and fixes!
Is there any task I can assist you with?

I opened an issue against core to allow additional information in render elments' #attached property, which I would prefer over hacking a solution with libraries or cache contexts, or checking a page's #lazy_builders.

Added another MR that's experimental, because it uses the proposed changes in ✨ Allow additional keys in #attached Needs review to pass CSP information from render elements to a new alter subscriber.

So from ✨ Allow additional keys in #attached Needs review and looking more at how Big Pipe enables it's own (temporary) items in #attached, it's possible to enable additional attached data for CSP without waiting for core changes 🎉.

I think I'll split out the render element #attached from the filter+lazy_builder first (in ✨ Allow CSP to be added by render elements Needs work ), but I should focus on getting 2.0.0 stable and released before adding more features, so I don't have to manage changes on two increasingly diverging branches.

gapple → closed merge request !34