search

mdn / developer-portal

The code that generates the MDN Web Docs Developer Portal.
Mozilla Public License 2.0
61 stars 38 forks source link

Try enabling CSP for the site #414

Open stevejalim opened 4 years ago

stevejalim commented 4 years ago

Historically, Wagtail didn't always play well with CSP enabled, but i'm informed by its makers that it should be OK since 2.2.

Worth a try, as it'll boost our HTTPObservatory score

stevejalim commented 4 years ago

Looking at https://github.com/wagtail/wagtail/issues/1288 there is still at least one CSP issue that blocks Wagtail from working with a strict policy

However, we actually have three sites:

Rather than use middleware to set HTTP headers, if I enable CSP via <meta> tags on the Wagtail-generated pages then both the live and static sites can benefit from strict CSP (or as strict as we can make it), without risking breaking Wagtail Admin. (We can then deal with Wagtail Admin CSP separately, depending on what Moz InfoSec team want)

@limed @escattone @peterbe I'd welcome some candid views on this 😄

peterbe commented 4 years ago

I don't know how baking works in Wagtail but if it's based on uploading rendered HTML to S3 (in Website "mode") how does it deal with custom Cache-Control headers? If Cache-Control can be managed, can you not add CSP too?

Using <meta> tags is fine too. Just make sure they appear very early in the <head>. And remember that HTTP headers trumps <meta> if you have both.

escattone commented 4 years ago

@stevejalim I think the most important target for CSP is the published site (Static pages on S3/CDN). I'm not sure it's worth worrying about CSP for the Wagtail CMS or live-site cases, since their use would be internal to Mozilla and only via the VPN. Does the published site use any inline styles or Javascript? Or can we guarantee that it won't? Of course, that would be nice so we could use a strict CSP policy. I didn't realize that CSP directives could be specified via the meta tag! That's nice. We could also specify them via HTTP headers that we provide via Lambda@Edge functions, which may be the easiest solution if @limed is already using a Lambda@Edge function for the developer-portal CDN.

stevejalim commented 4 years ago

Thanks for the feedback, guys!

@escattone I appreciate your point about the S3/CDN site vs live, and the VPN point is also a great one -- I'm checking now if requiring VPN access for the Wagtail box is a blocker for any of the contributors. If not, I'll definitely request that be set up.

Even bearing the VPN in mind, though, I think it's worth applying the same CSP rules to the live-rendered pages (which Editors will see when previewing pages via Wagtail or checking live-rendered/non-CDN ones) so that any issues (eg, embedding a video from a non-whitelisted source domain) is caught earlier, rather than only in produciton. So, I sticking with tags would be safer, even if HTTP Headers via Lambda@Edge are less work for me 😄

Will keep on thinking, but please do say if you think I'm barking up the wrong tree with what I've just said.

Cheers!

escattone commented 4 years ago

Even bearing the VPN in mind, though, I think it's worth applying the same CSP rules to the live-rendered pages (which Editors will see when previewing pages via Wagtail or checking live-rendered/non-CDN ones) so that any issues (eg, embedding a video from a non-whitelisted source domain) is caught earlier, rather than only in production. So, I sticking with tags would be safer, even if HTTP Headers via Lambda@Edge are less work for me 😄

Great point! On MDN we use django_csp which helps a bit with defining the CSP policies, but of course you know better if that would help or not here, but I also have nothing against using the meta tag approach either!

escattone commented 4 years ago

@stevejalim Scratch that comment about django_csp! Of course, using that wouldn't help with the published site, so never mind! I think your meta tag approach is brilliant! It covers all of the cases.

stevejalim commented 4 years ago

@valgrimm dusting off this ticket as I recall you agree that CSP would be good to have implemented. There is less to do now the static site is no longer in play. I'll update / add formal AC too.

stevejalim commented 4 years ago

We should be able to just use django_csp now, as long as Wagtail is also happy with its effects