Skip to content
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

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

nataliecarey
Copy link
Contributor

@nataliecarey nataliecarey commented Jun 7, 2021

Copy link
Contributor

@36degrees 36degrees left a 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!

@@ -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>
Copy link
Contributor

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…

Suggested change
<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>

Copy link
Contributor

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

Copy link
Contributor Author

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', () => {
Copy link
Contributor

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 🙌🏻

Copy link
Contributor Author

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.
@nataliecarey
Copy link
Contributor Author

Updated with the following changes:

a) scriptNonce is now called cspNonce because it's able to be used on things other than scripts. The name inlineScriptCspNonce was suggested but it can be used on linked scripts as well as other linked resources. Adding CSP gives a better description in my opinion.
b) Combined everything into one commit.
c) Used a commit message based on the suggestion from @36degrees.

If this new name is approved then the documentation alphagov/govuk-design-system#1709 and alphagov/govuk-design-system#1709 will need updating.

@36degrees 36degrees changed the title Adding Script Nonce. Allow the nonce attribute to be set on the inline script in the page template Jun 10, 2021
@36degrees 36degrees changed the title Allow the nonce attribute to be set on the inline script in the page template Allow the CSP nonce attribute to be set on the inline script in the page template Jun 10, 2021
Copy link
Member

@hannalaakso hannalaakso left a 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 👍

Copy link
Contributor

@36degrees 36degrees left a 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?

@matthewmascord
Copy link
Contributor

No need to mention me @36degrees - this was all @natcarey :-) It relates to a story we are working on in team @hmrc/platui

@36degrees 36degrees merged commit 279606d into alphagov:main Jun 11, 2021
@nataliecarey
Copy link
Contributor Author

Yes please @36degrees (no worries if I'm too late)

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.

Support nonce attribute on js-enabled script
4 participants