-
Notifications
You must be signed in to change notification settings - Fork 0
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
EB-134: Add a Content Security Policy to the website #81
Conversation
Working towards a Content Security Policy (CSP) compliant website.
Work towards CSP compliance. Params for tables are now passed in the html with "data-" prefixed attributes.
Work towards CSP compliance.
Work towards CSP compliance.
- Disable JBrowse's use of google analytics for each species. Need to allow unsafe-inline to let JBrowse analytics work (i.e. can't just whitelist google analytics in the CSP). - Don't inline JS to check for config on genome browser page.
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 thorough work. It was interesting to learn about CSP using the linked resources.
How did the idea to enforce a CSP come from originally ? I am divided: while the extraction of inline code is a valuable refactoring in itself, I wonder if maintaining a list of directives in nginx,conf
is worth it. Quoting OWASP:
Even on a fully static website, which does not accept any user input, a CSP can be used to enforce the use of Subresource Integrity (SRI). This can help prevent malicious code from being loaded on the website if one of the third-party sites hosting JavaScript files (such as analytics scripts) is compromised.
It seems to imply that the main benefit of CSP for a static site like ours would be to check the integrity of scripts, rather than mitigating unlikely XSS threat by restricting subresource origins. But we actually omit the former.
I don't strongly object merging this though, but some documentation snippet should perhaps be included to avoid some head scratching the day one of us includes a new third-party script, everything working well locally, buy breaks when deployed because of a stale nginx.conf
😅
Served no purpose as we have a static site and we use SRI checks on external scripts anyway See discussion in PR 81.
@kwentine, Thanks I think you're right. We now have SRI checks on the external JS scripts anyway and I agree with you, the inclusion of a CSP does add more complexity when it would come to adding new functionality/3rd party scripts, so as it doesn't bring any extra benefits I'm also in favour of removing it. I have therefore now removed the CSP from Please let me know if you're happy with the other comments/changes. Thanks again! |
As tracking code was in the footer, it wasn't included in the genome-browser pages. Now tracking code is placed at the end of the <head> (recommended by Matomo).
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.
👍🏻 👍🏻
Simplify code by using "Automatically render the reCAPTCHA widget" instead of explicity rendering approach see docs: https://developers.google.com/recaptcha/docs/display
This PR is about adding a content security policy to the website.
This essentially involves removing inline JS and creating a whitelist inside the
docker/nginx-custom.conf
for all the external connections we expect to make.The genome browser page posed some extra challenges when creating this:
'unsafe-inline'
from thescript-src
line required updating each speciesconfig.json
to disable google analytics. Just whitelisting google analytics webpages did not work.'unsafe-inline'
fromstyle-src
was unsuccessful because JBrowse relies on it. I think it is okay to leave'unsafe-inline'
instyle-src
and we can be happy we got the rest.@brinkdp and I already spoke and he is working on including the disable google analytics param in the
config.json
for any future species. (If the param is not included the browser page still works as expected, you just see some errors if you open up the console).