-
Notifications
You must be signed in to change notification settings - Fork 10
chore: Eliminate unnecessary CSP directives #3270
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
base: main
Are you sure you want to change the base?
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
ba1b276 to
adfec05
Compare
|
Investigated the three unresolved CSP errors in Sentry. Found that some appear to only apply to browser extensions, some apply to what was already fixed in this PR, and the third I fixed with the second commit here. Ready for review! |
| "https://cdn.amplitude.com/libs/", | ||
| "https://cdn.jsdelivr.net/", | ||
| "*.littlepay.com", | ||
| "https://code.jquery.com/jquery-3.6.0.min.js", |
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.
since we aren't using jquery in the admin login page anymore, can/should we revisit this code comment?
benefits/benefits/templates/admin/login.html
Lines 9 to 10 in c6af905
| {% comment %} Overriding instead of extending agency-base here to remove jQuery declaration, which admin/login.html includes on its own {% endcomment %} | |
| {% include "core/includes/analytics.html" with api_key=analytics.api_key uid=analytics.uid did=analytics.did %} |
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.
Good catch! I tried cross-referencing the history of that comment being added with what Django seems to be doing now and couldn't pull together a complete picture, but it's clear that we aren't adding our own jQuery anywhere in the admin, so I see no reason for this comment to remain.
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.
Updated.
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'll add in my two cents too that this comment probably was referring to how django-google-sso loads in jQuery 3.6.0 in its template, so all good to remove it.
|
alright, |
angela-tran
left a comment
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've finished reviewing this. One last note is that 912c1ae contained several changes in it and could've been easier to review if broken out into more atomic commits. But aside from that, this looks good to me!
I left a question on a minor thing with leaving behind a console.log line and plan to approve once that's resolved. Nice work!
jgravois
left a comment
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 learned a lot reading up on CSP to be able to make sense of this PR. very cool! 🏂
benefits/settings.py
Outdated
| "DIRECTIVES": { | ||
| "base-uri": [NONE], | ||
| "connect-src": [SELF, "https://api.amplitude.com/"], | ||
| "connect-src": [SELF, "https://api.amplitude.com/", "https://cdn.jsdelivr.net/"], |
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'm fine with this change, but i wonder where exactly sentry caught out bootstrap trying to call fetch() or one of its friends...
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 mentioned it in the commit's extended description, but it was requests for source maps! Presumably triggered by us looking at things with dev tools.
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.
ah, good sleuthin 🕵️
|
Thanks for the reviews! I agree that I could have made the commits a bit more atomic for easier review. I also should have generally commented more about my thought process in the PR description. I think that's partly still getting used to the process of putting up a draft PR and then later making it ready for review. I'll try to do better :) To quickly summarize how it went:
I'll update this on Monday for a final review! (Or pause until after the sprint?) |
3a15e41 to
4e7664f
Compare
5f29c9c to
c332806
Compare
|
From Slack
All those items have been addressed, and I rebased this branch on top of |
3289809 to
6a25813
Compare
script-src: code.jquery.com and style-src: unsafe-inline were both required only by the django-google-sso UI. This commit overrides the package's template and styling to eliminate the need for those.
To resolve CSP errors trying to resolve source maps for minified Bootstrap assets. See: https://sentry.calitp.org/organizations/sentry/issues/85947/?project=3
Co-authored-by: john gravois <[email protected]>
Co-authored-by: john gravois <[email protected]>
Co-authored-by: john gravois <[email protected]>
6a25813 to
5dba2e3
Compare
|
@thekaveman @lalver1 This is ready for review |


script-src: code.jquery.com and style-src: unsafe-inline were both required only by the django-google-sso UI. This PR overrides the package's template and styling to eliminate the need for those.
Closes #3257
To do before finalizing: Review CSP errors in Sentry.