Skip to content

Conversation

@nichtsam
Copy link
Contributor

@nichtsam nichtsam commented Feb 9, 2025

resolves #920

Issue:

Express Helmet applies security headers globally, which causes HTML-specific headers like Content-Security-Policy and X-Download-Options to be applied to non-HTML routes.

Solution:

This PR replaces Express Helmet with a custom implementation that better distinguishes between general security headers, HTML-specific headers, and CORS-related configurations.

The new helmet function is inspired by helmet and http-helmet.
There are two layers currently, one is the tool itself, and one is a wrapper around the tool with defined configurations.

The new helmet tool

function helmet(options: HelmetOptions): Headers
export type HelmetOptions = {
	options: SecureOptions
	html?: boolean
	cors?: boolean
}
  • The function simply returns the security headers with sensible defaults (referenced express helmet)
  • The html specific options will only get applied if the html flag is enabled.
  • Cross-Origin-Resource-Policy defaults to 'same-origin', but will default to 'cross-origin' if the cors flag is enabled.
  • I went through Content-Security-Policy MDN and tried to make the interface organized, complete, correct, and auto-completing.

The wrapper around helmet

This is the function to be used for handling the projects security headers.
I added function overloads to clearly indicate that nonce is only required for html content type.

The tool will probably be extracted into another package if this proof of concept is accepted, but I thought it'd be a good idea to have the interface reviewed here first.

Test Plan

  • Unit tests for each middleware to verify expected behavior in isolation.
  • Integration tests to ensure correct security headers in different scenarios (HTML vs. non-HTML, CORS enabled/disabled).

Checklist

  • Tests updated
  • Docs updated

Screenshots

@nichtsam nichtsam marked this pull request as draft February 9, 2025 22:26
@kentcdodds
Copy link
Member

Thank you so much for working on this! I like the direction here, and I know we have to get rid of Helmet. I'm really concerned about the amount of files added here and the complexity that everybody will take on. Feels like this should probably go into a library, and I would be happy to have it be a part of the Epic Web org if you're interested in helping me maintain it.

@nichtsam
Copy link
Contributor Author

nichtsam commented Feb 10, 2025

Thank you so much for working on this! I like the direction here, and I know we have to get rid of Helmet. I'm really concerned about the amount of files added here and the complexity that everybody will take on. Feels like this should probably go into a library, and I would be happy to have it be a part of the Epic Web org if you're interested in helping me maintain it.

Yes, definitely! I’m currently working on the library, polishing the code and preparing it for publishing on npm, but it will take some time.
Would it be okay to leave this PR as a draft and update it once the package is ready, or would you prefer to close it for now and open a new PR later?

@kentcdodds
Copy link
Member

I'll close it now and that way we don't lose track of where this is at while you work on the library. Thanks!

@kentcdodds kentcdodds closed this Feb 10, 2025
@nichtsam nichtsam deleted the pr/helmet branch February 11, 2025 20:55
@nichtsam nichtsam mentioned this pull request Feb 13, 2025
2 tasks
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.

CSP is being applied to non-HTML routes

2 participants