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

Add Content Security Policy headers #805

Merged
merged 11 commits into from
Feb 13, 2024
Merged

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Feb 9, 2024

close #770

Some notes regarding the approach of using a middleware:

We could use meta tags in <Head> in app.js but then we can't use report-to.

We could add static CSP headers in next.config.js but then we can't enforce a strict CSP with dynamic nonces. We could use hashes but hashes are fragile.

So afaict, middleware is the only viable solution. Even though middlewares feel like third-class citizens in NextJS :/

TODO:

  • use nonces to fix usage of unsafe-inline for script-src and style-src (strict CSP)

We could also uses hashes but this can break easily:

Using hashes can be a risk approach approach. If you change anything inside the script tag (even whitespace) by, e.g., formatting your code, the hash will be different, and the script won't render.

-- https://cheatsheetseries.owasp.org/cheatsheets/Content_Security_Policy_Cheat_Sheet.html#note

So I'll try to use nonces first.

@ekzyis ekzyis marked this pull request as draft February 9, 2024 20:53
@huumn
Copy link
Member

huumn commented Feb 10, 2024

I don't know if it helps at all, but this is what I did in Outer Space. I can't remember how strict this was or if it was just better than nothing.

const ContentSecurityPolicy = process.env.NODE_ENV === 'development'
  ? `
      default-src 'self';
      connect-src 'self' webpack://* wss://*;
      script-src 'self' 'unsafe-eval';
      worker-src 'self' blob:;
      script-src-elem 'self';
      style-src 'self' 'unsafe-inline';`
  : `
      default-src 'self';
      script-src 'self' 'sha256-/6SBPqW+GW+//4nlXX6Y1nR9dWlh0gsQJ6KK71djH6A=';
      worker-src 'self' blob:;
      connect-src 'self' wss://*;
      style-src 'self' 'unsafe-inline';`

const securityHeaders = [
  {
    key: 'X-DNS-Prefetch-Control',
    value: 'on'
  },
  {
    key: 'X-XSS-Protection',
    value: '1; mode=block'
  },
  {
    key: 'X-Frame-Options',
    value: 'SAMEORIGIN'
  },
  {
    key: 'X-Content-Type-Options',
    value: 'nosniff'
  },
  {
    key: 'Referrer-Policy',
    value: 'origin-when-cross-origin'
  },
  {
    key: 'Content-Security-Policy',
    value: ContentSecurityPolicy.replace(/\s{2,}/g, ' ').trim()
  }
]

@huumn
Copy link
Member

huumn commented Feb 10, 2024

We could also put stuff into nginx if that makes more sense. Just another option!

@ekzyis
Copy link
Member Author

ekzyis commented Feb 10, 2024

I don't know if it helps at all, but this is what I did in Outer Space. I can't remember how strict this was or if it was just better than nothing.

I think since you used hashes, it is a strict CSP (self is ignored in that case).

edit: Oh, I was wrong. strict-dynamic and object-src 'none' and base-uri 'none' is required to be considered strict:

Content-Security-Policy:
script-src 'sha256-{HASHED_INLINE_SCRIPT}' 'strict-dynamic';
object-src 'none';
base-uri 'none';

-- https://web.dev/articles/strict-csp#what_is_a_strict_content_security_policy

followed that link from here

We could also put stuff into nginx if that makes more sense. Just another option!

I think I got it working with nonces now in 93d33f3. Will test more tomorrow

@ekzyis
Copy link
Member Author

ekzyis commented Feb 10, 2024

One shouldn't use X-XSS-Protection with CSP though according to MDN:

Warning: Even though this feature can protect users of older web browsers that don't yet support CSP, in some cases, XSS protection can create XSS vulnerabilities in otherwise safe websites. See the section below for more information.

Did set other nice headers like Referrer-Policy and X-Content-Type-Options in 94ea9de.

Did not add X-DNS-Prefetch-Control since not recommended for production sites by MDN.

@ekzyis
Copy link
Member Author

ekzyis commented Feb 10, 2024

CSP looks good according to https://csp-evaluator.withgoogle.com/:

2024-02-10-022629_1920x1080_scrot

Another scan from https://observatory.mozilla.org/:

2024-02-10-023116_1920x1080_scrot

2024-02-10-023214_1920x1080_scrot

Would be nice if we can disable unsafe-eval but probably buried in some library.

Going to add the HSTS header for extra points (even though we shouldn't care too much about these points haha)

@huumn
Copy link
Member

huumn commented Feb 10, 2024

Would be nice if we can disable unsafe-eval but probably buried in some library.

If you want to find it, let's remove it.

@ekzyis
Copy link
Member Author

ekzyis commented Feb 11, 2024

If you want to find it, let's remove it.

Good call, unsafe-eval is only required in development mode.

Also allowed CDN, media domain where required and fixed blocking of YouTube and twitter embeds via frame-src.

New evaluation:

2024-02-11-211441_1920x1080_scrot

2024-02-11-211456_1920x1080_scrot

2024-02-11-212211_1920x1080_scrot

Going to take a look into Subresource Integrity.

require-trusted-types is experimental and not supported by Firefox and Safari. So I consider his (very) low priority.

I am getting a lot of this errors in the server log in production mode now though:

TypeError: Cannot read properties of undefined (reading 'bind')
at NextNodeServer.handleRequestImpl (/home/ekzyis/programming/stacker.news/node_modules/next/dist/server/base-server.js:389:50)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

But app seems to run fine.

I think it's related to this: vercel/next.js#56368

@ekzyis
Copy link
Member Author

ekzyis commented Feb 13, 2024

I noticed yesterday that Brave disabled the Reporting API because of its privacy implications: brave/brave-browser#7956

And wasn't able to get it to work on Chrome. Tried to test by removing the frame-src directive. YT and Twitter embeds were then blocked but no report was sent or received. Maybe related to the browser deciding when to send reports. Setup should work like this: For reporting, you need to use the CSP directives report-uri (for backwards compatibility) and report-to with the HTTP header Report-To. To not enforce the policy, you replace the HTTP header Content-Security-Policy with Content-Security-Policy-Report-Only.

Will look more into this today. However, I also noticed that one needs to explicitly allow browser extensions (chrome-extension: scheme for example). So this is also related to privacy implications since stackers probably don't like that their browsers sends reports like this which include information about which extensions they are using (even though they wouldn't be linked to their account on our side).

Found this nice website related to problems with weird CSP reports: https://csper.io/blog/csp-report-filtering

It linked to this summary of "WTF" reports: https://github.com/nico3333fr/CSP-useful/blob/master/csp-wtf/explained.md

I would like to have CSP reports but if it's too much work (and because of the privacy implications), I think we can ship it enforced and rely on user reports etc. and I can look into this at some other point in the future (when it feels more important).

I am quite confident that the current CSP policy is not interfering with most functionality on the site. I am also quite confident that the middleware works as expected. It's not interfering with referrals (I tested) and CSP is only setting headers so I don't think there is anything to worry about the middleware.

Better middleware support from NextJS would still be nice though.

@ekzyis
Copy link
Member Author

ekzyis commented Feb 13, 2024

Regarding TypeError: Cannot read properties of undefined (reading 'bind'):

This error only happened if the dev build in the browser was trying to connect to wss://sn.ekzyis.com/_next/webpack-hmr with the server built in prod mode running.

If I refreshed the page the errors stopped showing up since the prod build does not use hot reload.

Ignoring that route in 03969f4 now.

@ekzyis ekzyis marked this pull request as ready for review February 13, 2024 18:51
@huumn huumn merged commit fc18a91 into master Feb 13, 2024
1 check passed
@ekzyis ekzyis deleted the 770-content-security-policy branch February 14, 2024 00:29
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.

Use Content Security Policy headers
2 participants