-
Notifications
You must be signed in to change notification settings - Fork 334
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
Allow the CSP nonce
attribute to be set on the inline script in the page template
#2245
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, @natcarey 👍🏻
It'd be good to squash the two commits into one. We also tend to write commit messages in the present tense, and ideally it'd be good to make the commit message slightly more descriptive and include more detail about why the change is needed.
I'd suggest something like:
Allow nonce attribute to be set on inline script
Not all services are able to follow the currently recommended approach of using
hashes to allow specific inline scripts as part of their Content Security
Policy.
An alternative approach is to use a nonce, but this requires being able to set
the nonce attribute on the script itself.
Introduce a new Nunjucks variable `inlineScriptCspNonce` for the page template
to allow users to do this.
Feel free to use that as a starting point if it's helpful!
src/govuk/template.njk
Outdated
@@ -28,7 +28,7 @@ | |||
<meta property="og:image" content="{{ assetUrl | default('/assets') }}/images/govuk-opengraph-image.png"> | |||
</head> | |||
<body class="govuk-template__body {{ bodyClasses }}" {%- for attribute, value in bodyAttributes %} {{attribute}}="{{value}}"{% endfor %}> | |||
<script>document.body.className = ((document.body.className) ? document.body.className + ' js-enabled' : 'js-enabled');</script> | |||
<script{% if scriptNonce %} nonce="{{ scriptNonce }}"{% endif %}>document.body.className = ((document.body.className) ? document.body.className + ' js-enabled' : 'js-enabled');</script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's unlikely we'd need to introduce additional inline scripts, but if we did am I correct in thinking that we'd use the same nonce for all scripts, and so would still only need one variable?
Either way, I'm wondering if we can make this slightly more descriptive to help users understand what it's doing.
Possibly something like…
<script{% if scriptNonce %} nonce="{{ scriptNonce }}"{% endif %}>document.body.className = ((document.body.className) ? document.body.className + ' js-enabled' : 'js-enabled');</script> | |
<script{% if inlineScriptCspNonce %} nonce="{{ inlineScriptCspNonce }}"{% endif %}>document.body.className = ((document.body.className) ? document.body.className + ' js-enabled' : 'js-enabled');</script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct @36degrees, a single cryptographically secure random token is generated server side and included in the Content-Security-Policy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@36degrees yes, if we have more inline scripts they can use the same nonce
. I wouldn't personally call it inlineScriptCspNonce
as it can be used on remote scripts too. Adding CSP makes sense to me, should we go for scriptCspNonce
?
@@ -165,6 +165,18 @@ describe('Template', () => { | |||
// updating the hash published in https://frontend.design-system.service.gov.uk/importing-css-assets-and-javascript/#if-your-javascript-isn-t-working-properly | |||
expect('sha256-' + hash).toEqual('sha256-+6WnXIl4mbFTCARd8N3COQmT3bJJmo32N8q8ZSQAIcU=') | |||
}) | |||
it('should not have a nonce attribute by default', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding tests – these look great 🙌🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good. It's important :)
Not all services are able to follow the currently recommended approach of using hashes to allow specific inline scripts as part of their Content Security Policy. An alternative approach is to use a nonce which requires the attribute to be set on the script itself. Introduce a new Nunjucks variable `cspNonce` for the page template to allow users to do this.
7ba9f2c
to
2e40d74
Compare
Updated with the following changes: a) If this new name is approved then the documentation alphagov/govuk-design-system#1709 and alphagov/govuk-design-system#1709 will need updating. |
nonce
attribute to be set on the inline script in the page template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks @natcarey 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing.
We'll work on adding the release notes for this as part of the release process – are you happy for us to credit you
as the contributor @natcarey? Would it be appropriate to credit @matthewmascord or anyone else as well?
No need to mention me @36degrees - this was all @natcarey :-) It relates to a story we are working on in team @hmrc/platui |
Yes please @36degrees (no worries if I'm too late) |
This relates the issue Support nonce attribute on js-enabled script.