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.

Sign up for GitHub

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

Jump to bottom

Feat: Implement strict Content Security Policy #1358

Merged
merged 6 commits into from Apr 20, 2023
Merged

Feat: Implement strict Content Security Policy #1358

merged 6 commits into from Apr 20, 2023

Conversation

thekaveman
Copy link
Member

@thekaveman thekaveman commented Apr 13, 2023 •
edited by machikoyasuda

Closes #212

Also adds a new environment variable to configure the CSP report-uri Report Directive.

Note: report-uri is being deprecated in favor of report-to, but the latter lacks full browser support at this time. See above MDN link for more.

This can be used to send CSP violation reports to Sentry, which has out of the box support for Security Policy Reporting.

Notes for reviewers

  • Test the running appcontainer, in addition to the devcontainer
  • Ensure everything works with reCAPTCHA enabled
  • Configure test Amplitude to ensure browser events (like clicking a link) are sent
  • Here's a sample of a CSP violation report in Sentry (I triggered this locally by disallowing font-src)

@thekaveman thekaveman added the security Changes to improve or maintain the availability and resilience of the app label Apr 13, 2023
@thekaveman thekaveman requested a review from a team as a code owner April 13, 2023 21:28
@github-actions github-actions bot added documentation [auto] Improvements or additions to documentation deployment-dev [auto] Changes that will trigger a deploy if merged to dev back-end Django views, sessions, middleware, models, migrations etc. front-end HTML/CSS/JavaScript and Django templates infrastructure Terraform, Azure, etc. labels Apr 13, 2023
@thekaveman
feat(settings): enforce strict Content Security Policy
6c65db8 
* disallow base-uri
* disallow object-src
* disallow unsafe-inline for script-src
* require nonce for script-src
@thekaveman
chore: implement nonce on <script> tags
9b20fa7
@thekaveman
chore(analytics): add nonce to inject <script> tag
ad9730d
@thekaveman
chore(enrollment): add nonce to payment processor script
24d954f 
use the more generic $.ajax() to download the script and apply the nonce
before execution
@thekaveman
feat(sentry): support CSP report-uri directive
2faabec 
sentry has out of the box support for receiving CSP reports
https://docs.sentry.io/product/security-policy-reporting/
@thekaveman thekaveman mentioned this pull request Apr 14, 2023
Merged
@angela-tran
Copy link
Member

👀 Finished code reviewing this earlier. Still need to run through manual testing steps listed in pull request description

@machikoyasuda
Copy link
Member

machikoyasuda commented Apr 20, 2023 •
edited

Testing

  • ReCaptcha - Tested the app with ReCaptcha in Spanish, MST x Courtesy Cards, SacRT/MST modal

image

  • Testing the appcontainer with ReCaptcha enabled:

image

machikoyasuda
machikoyasuda approved these changes Apr 20, 2023
View reviewed changes
Copy link
Member

@machikoyasuda machikoyasuda left a comment

Choose a reason for hiding this comment

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

Tested appcontainer, devcontainer, with and without ReCAPTCHA. LGTM!

angela-tran
angela-tran approved these changes Apr 20, 2023
View reviewed changes
Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

I tested the appcontainer and devcontainer

@thekaveman thekaveman merged commit a997078 into dev Apr 20, 2023
11 checks passed
@thekaveman thekaveman deleted the feat/strict-csp branch April 20, 2023 19:43
@thekaveman thekaveman mentioned this pull request Apr 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@machikoyasuda machikoyasuda machikoyasuda approved these changes

@angela-tran angela-tran angela-tran approved these changes

Assignees
No one assigned
Labels
back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev documentation [auto] Improvements or additions to documentation front-end HTML/CSS/JavaScript and Django templates infrastructure Terraform, Azure, etc. security Changes to improve or maintain the availability and resilience of the app
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider strict Content Security Policy
3 participants
@thekaveman @angela-tran @machikoyasuda