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

Add csp_nonce for inline script and gov_template #5325

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

spatel033
Copy link
Contributor

@spatel033 spatel033 commented Dec 11, 2024

Previously we added unsafe-inline and unsafe-eval directives for script-src header. We can remove these directives as part of ITHC after removing Hogan. Additionally to fix CSP errors added csp_nonce for request to allow govuk_frontend_jinja/template.html and inline script. More details on this card.

@spatel033 spatel033 force-pushed the update-ithc-script-src-headers branch from 674ec4d to 3b22955 Compare December 11, 2024 17:30
@spatel033 spatel033 marked this pull request as ready for review December 13, 2024 10:36
@kr8n3r
Copy link
Contributor

kr8n3r commented Dec 13, 2024

missed

Copy link
Contributor

@tombye tombye left a comment

Choose a reason for hiding this comment

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

I left a comment but also wanted to suggest this adds a test to this section of tests/conftest.py for script tags that either contain inline code or reference a file not hosted on our domain. That would both identify any current scripts without the nonce attribute and protect us from adding any in future.

Also to say this is great work and much needed. Thanks for doing it!

app/templates/admin_template.html Outdated Show resolved Hide resolved
@spatel033 spatel033 force-pushed the update-ithc-script-src-headers branch 6 times, most recently from 1480ec3 to bbeb4fb Compare December 13, 2024 14:36
@spatel033 spatel033 requested review from tombye and kr8n3r December 13, 2024 14:37
Copy link
Contributor

@tombye tombye left a comment

Choose a reason for hiding this comment

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

Sorry to be asking for changes again, I'm not sure my last review was clear enough, but I think these changes would mean we could spot and guard against all unsafe script tags.

tests/conftest.py Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
@spatel033 spatel033 force-pushed the update-ithc-script-src-headers branch 2 times, most recently from 62596af to 21d13d0 Compare December 13, 2024 16:14
Copy link
Contributor

@kr8n3r kr8n3r left a comment

Choose a reason for hiding this comment

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

based on #5325 (comment) I'm happy with this but it should wait for Tom as he requested changes

Previously we added `unsafe-inline` and `unsafe-eval` directives for `script-src` header. We can remove these  directives as part of ITHC after removing `Hogan`. Additionally to fix CSP errors added `csp_nonce` for request to allow `govuk_frontend_jinja/template.html` and inline `script`.
@spatel033 spatel033 force-pushed the update-ithc-script-src-headers branch from 21d13d0 to 0bf7ea6 Compare December 17, 2024 10:42
@spatel033 spatel033 requested a review from tombye December 17, 2024 11:03
Copy link
Contributor

@tombye tombye 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 going through all the changes requested. Looks great now 👍🏻

@spatel033 spatel033 requested a review from kr8n3r December 18, 2024 09:49
@spatel033 spatel033 merged commit f26ae96 into main Dec 19, 2024
3 checks passed
@spatel033 spatel033 deleted the update-ithc-script-src-headers branch December 19, 2024 13:41
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