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

EB-134: Add a Content Security Policy to the website #81

Merged
merged 10 commits into from
Jan 13, 2025

Conversation

RMCrean
Copy link
Member

@RMCrean RMCrean commented Dec 20, 2024

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:

  1. In order to remove 'unsafe-inline' from the script-src line required updating each species config.json to disable google analytics. Just whitelisting google analytics webpages did not work.
  2. Trying to remove 'unsafe-inline' from style-src was unsuccessful because JBrowse relies on it. I think it is okay to leave 'unsafe-inline' in style-srcand 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).

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.
@RMCrean RMCrean requested review from kwentine and brinkdp December 20, 2024 08:42
Copy link
Collaborator

@kwentine kwentine 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 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 😅

docker/nginx-custom.conf Outdated Show resolved Hide resolved
hugo/assets/js/recaptcha.js Outdated Show resolved Hide resolved
hugo/layouts/contact/list.html Outdated Show resolved Hide resolved
hugo/layouts/partials/footer.html Outdated Show resolved Hide resolved
Served no purpose as we have a static site and we use SRI checks on external scripts anyway
See discussion in PR 81.
@RMCrean
Copy link
Member Author

RMCrean commented Jan 10, 2025

@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 docker/nginx-custom.conf (commit 9e4fb6a) but kept the rest of changes.

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).
@RMCrean RMCrean requested a review from a team as a code owner January 10, 2025 13:32
Copy link
Collaborator

@kwentine kwentine left a 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
@RMCrean RMCrean merged commit 176d9c6 into main Jan 13, 2025
4 checks passed
@RMCrean RMCrean deleted the add-csp-improve-js branch January 13, 2025 10:52
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.

2 participants