freedomofpress / securethenews

An automated scanner and web dashboard for tracking TLS deployment across news organizations
https://securethe.news
GNU Affero General Public License v3.0
100 stars 29 forks source link

Adds nonce to handle inline script in regional leaderboards #292

Closed SaptakS closed 3 years ago

SaptakS commented 3 years ago

Since the inline scripts content changes based on sites in the regions for the regional leaderboards HTML, the script was being blocked by newly added CSP rules. Adding a nonce, instead of sha256 handles the issue of changing script content

SaptakS commented 3 years ago

cc: @maeve-fpf

maeve-fpf commented 3 years ago

👍🏻

chigby commented 3 years ago

Looking around the site, it looks like we are also using the pattern of

<script type="text/javascript">
    var STNsiteData = {{ sites_json|safe }};
</script>

in several places on the site, namely the home page and the "sites" index (alongside the region-specific leaderboards, which you updated). Do we need to incorporate a nonce into these script tags as well?

chigby commented 3 years ago

I'm also noticing several CSP-related errors in the site admin. I know it's not part of this PR, but are you seeing these as well? It seems like something that could be fixed by borrowing the appropriate script-src and style-src rules from one of our other sites where the CSP has been adapted to the wagtail admin.

SaptakS commented 3 years ago

Looking around the site, it looks like we are also using the pattern of

<script type="text/javascript">
    var STNsiteData = {{ sites_json|safe }};
</script>

in several places on the site, namely the home page and the "sites" index (alongside the region-specific leaderboards, which you updated). Do we need to incorporate a nonce into these script tags as well?

This is already handled using "'sha256-hk71/yNgJt0WwDMKIEPWJnms3ftGXFupYCx2GTlIB68='" in base.py. But thinking it again, I think you are right that adding nonce is better, because on adding a new site from admin, the sha hash will change.

SaptakS commented 3 years ago

I'm also noticing several CSP-related errors in the site admin. I know it's not part of this PR, but are you seeing these as well? It seems like something that could be fixed by borrowing the appropriate script-src and style-src rules from one of our other sites where the CSP has been adapted to the wagtail admin.

I think we might need to take SDO's approach and exclude CSP from admin links. There are lot of different inline scripts sadly being used for different fields. Which would ideally mean that if we modify some field or change some field (for example autocomplete, StreamField, etc.) there will be new CSP warning. I did try adding CSPs for some scripts, but opening every other edit page seems to be throwing different CSP errors.

Now either I can add unsafe-inline, or just exclude CSP from admin. I personally feel excluding CSP from /admin might be better than allowing unsafe-inline throughout the website.

@harrislapiroff @chigby @conorsch @maeve-fpf thoughts?

harrislapiroff commented 3 years ago

It makes sense to me to exclude the admin from the CSP. We should add comments upstream about issues we encountered. Can you do that @SaptakS? Here's the ticket upstream: https://github.com/wagtail/wagtail/issues/1288

SaptakS commented 3 years ago

Really not happy about this (feels like a step backwards in my head) adding unsafe-inline and removing hashes and nonce. But seems like the best solution for the moment to put unsafe-inline given excluding CSP only from a path using django CSP technically affects the entire website.

So I feel it's best now to have unsafe-inline for now and then once it is possible to get rid of unsafe-inline with hashes and nonce, we should move to that.