-
Notifications
You must be signed in to change notification settings - Fork 57
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
feat: allow configuring headers in runtime #298
feat: allow configuring headers in runtime #298
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@dargmuesli @Baroshem @harlan-zw Would love to have your feedback ! @pi0 feel free to correct this PR 😅 , i really hope this looks good 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.
Looks good at the first glance! I will try it out later ❤️
Hey, amazing work! Would you be ok if we could merge it to the 1.1.0 version that I am planning to release in December? This week, I would love to release 1.0.0 and your change would work perfectly for the first minor update after stable 1.0.0 :) |
Sure ! I guess we can also iterate on |
src/types/index.ts
Outdated
|
||
declare module 'nitropack' { | ||
interface NitroRuntimeHooks { | ||
'nuxt-security:headers': (route: string, headers: SecurityHeaders) => void |
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.
I haven't dug into how the module works but this type of augmentation generally doesn't work when users install the module.
You need to use the runtime-generated types for this sort of thing usually.
Also I'd type it as Promise<void> | void
so users can use promises.
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.
Personally I'd use a single context object instead of two separate params and I'd use a H3Event over the route
.
Not required though
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.
yes, we probably need to add a reference in a d.ts
file. I'll verify if this works later !
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.
would you prefer something like this?
'nuxt-security:headers': ({
route: string,
headers: SecurityHeaders
}) => void
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.
yes, it just makes it easier if you want to add other things into the hook context later
// if it makes sense
'nuxt-security:headers': ({
event: H3Event,
headers: SecurityHeaders
}) => void | Promise<void>
}) | ||
|
||
nitroApp.hooks.hook('request', (event) => { | ||
event.context.security = event.context.security || {} |
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.
I'd use the module name to avoid conflicts
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.
i used security because it's the module configKey
, but yeah we can change it to nuxtSecurity
!
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.
I think if ends users will use this context object it could make sense with this name, otherwise if it's just internal module name is better.
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.
I would stick to security
instead of nuxt-security
purely because this is a config key for both nuxt.config.ts file and for route rules so it would be better IMHO to keep this consistent :)
export default defineNitroPlugin((nitroApp) => { | ||
const router = createRouter() | ||
|
||
nitroApp.hooks.hook('nuxt-security:headers', (route, headersConfig) => { |
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.
I have a feeling this would be better suited if it was called on each request, otherwise, the user should just use route rules?
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.
Do you mean it would be up to us to call a hook insteading of hooking into the event ?
I also thought of this. The reason I made nuxt-security hooking into it instead of calling the hook is because we would need to await
all hooks.
So if users are doing a long async hooks, this would block their response
I don't think routeRules
are editable is it ? If not, there's another way to do runtime configuration. It is to have a security
/nuxtSecurity
property from event.context
and to change the headers based on its value in a middleware
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.
I need to dig into the code a bit deeper to answer this, think I'm missing some context. Will get back to you when I have a chance (otherwise feel free to ping me)
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.
👍 feel free to ping/call me, maybe can we try to find a better implementation together with @dargmuesli and @Baroshem
cd70d1b
to
1827273
Compare
Sorry for waiting so much for my feedback on this great PR. I am back again and could take a look at it . Do you need me to answer any particular discussion? |
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.
Few questions/suggestions
docs/content/1.documentation/2.headers/15.runtimeConfiguration.md
Outdated
Show resolved
Hide resolved
@@ -24,6 +24,7 @@ export default defineNuxtConfig({ | |||
rateLimiter: { | |||
tokensPerInterval: 10, | |||
interval: 10000 | |||
} | |||
}, | |||
runtimeHooks: true |
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.
question: I wonder if this should actually be enabled by default. I think that majority of the users will just use the nuxt.config.ts config or route rules while this runtime configuration is a bit of an edge case (at least IMO).
With these kind of configuration, I usually mark them as optional and disabled by default (to not ship unused code to other users).
So, if you want to have a runtime configuration, just set this config value security.runtimeHooks = false
and it will work for you.
WDYT?
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.
oh i didn't remove this since the removal of the runtimeHooks
option
@harlan-zw think we don't really need this option to always have a runtime configuration of headers. So should we set back runtimeHooks
in the options ?
If we set back runtimeHooks
option, i think it should be default to false. Then if true, we'll add the nitro plugin
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.
I personally would recommend to add an option and make it disabled by default to not add additional code for users that may not use it.
So add an option but make it disabled by default and make a note in the docs that in order to use this feature, set this config option to true and then, call the hook (like you have explained in the docs)
I think this delivers the functionality to the user that may need it without forcing every user of the module to have some dead code.
Having reviewed the source and looking at this PR again, I think the best solution would be to introduce a composable to resolve the security route rules. Currently, each function is doing this: // nuxt-security plugin
const { security } = getRouteRules(event) The PR to my understanding is solving the issue of users wanting to modify this Instead, using a new composable that wraps this logic while also exposing a Nitro hook should solve the issue. export async function resolveSecurityContext(e: H3Event) {
const routeRules = getRouteRules(event).security
const nitroApp = useNitroApp()
const context = { rules: { ...routeRules } } // can't remember if this is frozen, you may need to do a clone (see klona)
await nitroApp.hooks.hook('nuxt-security:context', context)
return context
} // nuxt-security plugin
const security = await resolveSecurityContext(event) The only catch is that you don't want users running slow events in this hook. So I'd update the docs with an example like so. export default defineNitroPlugin((nitroApp) => {
const myCustomSecurityRules = await $fetch('/security')
nitroApp.hooks.hook('nuxt-security:context', (context) => {
context.rules = defu(context.rules, myCustomSecurityRules)
})
}) Let me know what you think of this and if it solves the issue. Also feel free to name the composable / hook to whatever you think makes the most sense. |
@huang-julien any update from your side? I would love to plan 1.1.0 release with this feature :) |
Yup probably this weekend, i didn't had a lot of time since the end of holidays 😭 |
I think this can probably have it's own PR. I completly agree that having a composable will be better in terms of DX 👍 . The only issue i see is the hook being possibly async which can block the request, that's why it's up to the user to call the hook instead the other way in this PR. Please tell me if anything needs a change or if I forgot anything :) |
Separate PR is good with me, whatever is easy. It shouldn't be an issue as long as long as it's clear to the user that should be covered by the docs (explicit code example of what's good practice) and the fact that all server hooks are async and block the request. If you just want to get this PR over the line then please ignore my input and go ahead with what you have as it still solves the issue |
Hi @huang-julien |
Looks good from my side now :) |
Hi @vejja 👋 |
@huang-julien @harlan-zw @vejja @dargmuesli I am merging this PR and will release it as 1.1.0. Thanks for all the work! 💚 |
nonce: false, | ||
runtimeHooks: true, | ||
headers: { | ||
contentSecurityPolicy: false |
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.
@huang-julien is it required to turn off nonces and content security policy headers here?
When I keep contentSecurityPolicy
headers enabled here, the headers set in the hook are not applied it seems.
When I disable contentSecurityPolicy
headers here, the headers set in the hook are applied but without {{nonce}}
replacement. Is that intended?
@harlan-zw
await nitroApp.hooks.callHook('nuxt-security:context', context)
export default defineNitroPlugin(async(nitroApp) => {
...
}) Looks like |
resolve #258
Types of changes
Description
Hi 👋
This PR allows to configure headers configuration in runtime.
users will simply have to call
nuxt-security:headers
hook to change the configuration. The new configuration will have a higher priority than the routeRules .Strangely, auto scanned plugins have a higher priority than registered plugins. So I also created the
nuxt-security:ready
nitro hook.Checklist: