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

Set X-XSS-Protection to 1; mode=block. #1550

Merged
merged 1 commit into from Aug 6, 2018

Conversation

Projects
None yet
5 participants
@StevenXL
Copy link
Member

StevenXL commented Aug 3, 2018

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

@StevenXL StevenXL force-pushed the StevenXL:set-x-xss-protection branch from bbf5e97 to 4015ef2 Aug 3, 2018

@StevenXL

This comment has been minimized.

Copy link
Member Author

StevenXL commented Aug 3, 2018

Address #1543. This is a very small PR, so if this is deemed unnecessary, happy to discard.

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented Aug 6, 2018

Pinging @MaxGabriel. I haven't been following this header in general.

@StevenXL if you feel comfortable with this, you should also feel free to merge.

@MaxGabriel

This comment has been minimized.

Copy link
Member

MaxGabriel commented Aug 6, 2018

I think that this is unlikely to cause anyone any problems, and improves the situation over the browser default by making the error more visible. I'd say it's good to merge.

@StevenXL

This comment has been minimized.

Copy link
Member Author

StevenXL commented Aug 6, 2018

@MaxGabriel thanks.

@StevenXL StevenXL merged commit 3ebd8f9 into yesodweb:master Aug 6, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@StevenXL StevenXL deleted the StevenXL:set-x-xss-protection branch Aug 6, 2018

@MaxGabriel

This comment has been minimized.

Copy link
Member

MaxGabriel commented Aug 6, 2018

As a side note, there are other security headers that people should maybe add:
https://securityheaders.com/?q=alpha.mercury.co&followRedirects=on

They require more custom configuration compared to this one, so there isn't much a library would really be adding, but it would be cool if Yesod developers were encouraged to use them. Not sure the best way to do that—maybe some commented out code in the scaffolding?

@bitemyapp

This comment has been minimized.

Copy link
Contributor

bitemyapp commented Aug 6, 2018

Not sure the best way to do that—maybe some commented out code in the scaffolding?

Be a good start but I'd rather good defaults that are turned on, if possible.

screenshot from 2018-08-06 16-04-45

Here's what was missing on my relatively vanilla Yesod app that is otherwise A+ on qualys ssl labs.

@MaxGabriel

This comment has been minimized.

Copy link
Member

MaxGabriel commented Aug 6, 2018

Content-Security-Policy is a huge win for security, because you can whitelist where things like Javascript can be loaded from, and prevent XSS from harming you. This is our CSP used for a single-page app (with some additions coming from headers):

  <meta http-equiv="Content-Security-Policy" content="
    base-uri 'self';
    block-all-mixed-content;
    default-src 'self';
    connect-src
      'self'
      ws://localhost:3001
      ws://app.mercury.local
      ws://lh.mercury.co
      wss://app.mercury.local
      https://maps.googleapis.com
      http://localhost:3000
      https://api.mercury.local
      https://app.mercury.local
      https://s3.amazonaws.com/mercury-technologies-user-uploads-dev
      https://api.mixpanel.com
      https://sessions.bugsnag.com
      https://notify.bugsnag.com
      https://backend.mercury.co;
    font-src
      'self'
      https://fonts.gstatic.com;
    img-src
      https://csi.gstatic.com
      https://maps.googleapis.com
      'self';
    media-src
      'none';
    script-src
      'self'
      'sha256-srzUm2bR52Dg2g2SqWiGuo719+FqQulMWAYRYJ8hkhc='
      https://maps.googleapis.com
      https://cdn.plaid.com/link/v2/stable/link-initialize.js;
    style-src
      'self'
      https://hello.myfonts.net/count/3694b0
      https://fonts.googleapis.com
      https://maps.googleapis.com
      'unsafe-inline';
    frame-src
      https://app.mercury.local
      https://cdn.plaid.com/;
    object-src
      'none';
    form-action
      'none';
    worker-src
      'self';
  " />

I think the best default you could give is to set the default-src to self, to only allow the domain serving the Yesod app to send any javascript, css, etc. The UX downside is that developers wouldn't be able to load e.g. JQuery from cdnjs until they went to the CSP and whitelisted it. The error messages in the browser are pretty informative about this, but it would definitely trip a lot of people up. Github has a write-up of their use of it:
https://githubengineering.com/githubs-csp-journey/ Thoughts?

Referrer-Policy seems fine to enable by default. My understanding is that it improves privacy by concealing where users came from.

Feature-Policy is brand spanking new, like it was on Hacker News last week, so I don't have comments on it really. But it lets you do things like prevent the site from vibrating the phone
https://wicg.github.io/feature-policy/

@iand675

This comment has been minimized.

Copy link
Contributor

iand675 commented Oct 9, 2018

I'm a bit late to the party here, but found this issue via the changelog. It's a chrome specific feature, but would be nice to allow setting a report URI as noted here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-XSS-Protection

You can combine both block and report, so it would look something like this:

X-XSS-Protection: 1; mode=block; report=https://www.google.com/appserve/security-bugs/log/youtube
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.