-
-
Notifications
You must be signed in to change notification settings - Fork 49
Support function-based customisation in reply.helmet options #287
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
base: main
Are you sure you want to change the base?
Conversation
Added support for passing a function to `reply.helmet`, allowing dynamic modification of Helmet options. Updated type definitions and included tests to validate this functionality.
jean-michelet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I'm not familiar with this plugin, I'd rather not comment on the usefulness of this feature.
|
|
||
| // Helmet forward a typeof Error object so we just need to throw it as is. | ||
| function done (error) { | ||
| /* c8 ignore next */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you plz document why this is necessary, we should have very good reasons to ignore code coverage.
I see this package has a few ignore comments without any explanation, so it's not a feedback specific to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jean-michelet For whatever reason this condition isn't covered by the existing tests. I added the ignore as a work-around because I saw it was used elsewhere in the code (and it was the only way I could commit without --force). I've tried adding a test to cover it but was unsuccessful, so any help or advice is appreciated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: I’ve opened a PR to add comments:
#292
README.md
Outdated
| // you can also pass a function returning customized options | ||
| // await reply.helmet((opts) => { | ||
| // opts.frameguard = false | ||
| // return opts | ||
| // }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead, can you share an example that is only achievable with a callback rather than an object?
It would help users to understand why it is a helpful feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jean-michelet I've updated the readme with a clearer example, let me know if that's better. 06d709b
|
Thanks for the PR @Tam, i'm interested what your use case is for this? |
Then, why not using the https://github.com/fastify/fastify-helmet?tab=readme-ov-file#content-security-policy-nonce |
That would work for inline scripts, but not for external ones. For example, if I were to add a tag like this: {% js 'https://unpkg.com/[email protected]' with {
integrity: 'sha256-4gndpcgjVHnzFm3vx3UOHbzVpcGAi3eS/C5nM3aPtEc=',
crossorigin: 'anonymous',
} %}I'd want it to automatically add the domain and integrity hash to the CSP header. It's mainly a developer QoL thing. |
| style: string; | ||
| }, | ||
| helmet: (opts?: HelmetOptions) => typeof helmet | ||
| helmet: (opts?: HelmetOptions | ((opts: HelmetOptions) => HelmetOptions)) => typeof helmet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a test for the type? We use tsd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for passing a function to the reply.helmet() decorator, allowing users to modify existing helmet configuration options programmatically rather than replacing them entirely.
Key changes:
- Enhanced the
reply.helmet()decorator to accept a function that receives current options and returns modified options - Added comprehensive test coverage for the new function-based API
- Updated TypeScript type definitions to reflect the new signature
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| index.js | Updated reply.helmet to handle function parameters and added c8 ignore comment for error path |
| types/index.d.ts | Extended helmet decorator type signature to accept function parameter |
| test/global.test.js | Added test case verifying function-based options work correctly |
| README.md | Added documentation example showing how to use function-based options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let helmetConfiguration = configuration | ||
|
|
||
| if (typeof opts === 'function') { | ||
| helmetConfiguration = opts(helmetConfiguration) |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When opts is a function, the configuration is passed directly without creating a new object via Object.create(null). This inconsistency means the function could mutate the original configuration object. Consider passing a copy: helmetConfiguration = opts(Object.assign(Object.create(null), configuration))
| helmetConfiguration = opts(helmetConfiguration) | |
| helmetConfiguration = opts(Object.assign(Object.create(null), configuration)) |
| // await reply.helmet((opts) => { | ||
| // // Here we're adding a new option to a directive without having to | ||
| // // replace the entire `contentSecurityPolicy` object. | ||
| // opts.contentSecurityPolicy.directives['script-src'].push('"nonce-123abc"'); |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example assumes opts.contentSecurityPolicy and its nested properties exist, but users may receive configuration where these are undefined. Consider adding a comment noting that this is a simplified example and proper validation should be performed in production code.
Added support for passing a function to
reply.helmet, allowing dynamic modification of Helmet options. Updated type definitions and included tests to validate this functionality. The aim is to make updating the existing options easier, without needing to overwrite the entire object (i.e. when modifying a specific CSP value).Checklist
npm run testandnpm run benchmarkand the Code of conduct