-
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
Changes from 3 commits
65fc0d1
da8461a
e242ccc
7207581
6e41f10
1827273
a062371
6acf660
c58491a
10c6161
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import { defineEventHandler } from "#imports" | ||
|
||
export default defineEventHandler((event) => { | ||
return { | ||
csp: getResponseHeader(event, 'Content-Security-Policy') | ||
} | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
|
||
export default defineNitroPlugin((nitroApp) => { | ||
nitroApp.hooks.hook('nuxt-security:ready', () => { | ||
nitroApp.hooks.callHook('nuxt-security:headers', '/api/runtime-hooks' ,{ | ||
contentSecurityPolicy: { | ||
"script-src": ["'self'", "'unsafe-inline'"], | ||
} | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import { type HeaderMapper, getHeaderValueFromOptions, SECURITY_HEADER_NAMES } from "../../utils/headers" | ||
import { createRouter} from "radix3" | ||
import { defineNitroPlugin } from '#imports' | ||
|
||
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 commentThe 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 commentThe 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 So if users are doing a long async hooks, this would block their response I don't think There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
const headers: Record<string, string |false > = {} | ||
|
||
for (const [header, headerOptions] of Object.entries(headersConfig)) { | ||
headers[SECURITY_HEADER_NAMES[header]] = headerOptions === false ? false : getHeaderValueFromOptions(header as HeaderMapper, headerOptions as any) | ||
} | ||
|
||
router.insert(route, headers) | ||
}) | ||
|
||
nitroApp.hooks.hook('request', (event) => { | ||
event.context.security = event.context.security || {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. i used security because it's the module There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I would stick to |
||
event.context.security.headers = router.lookup(event.path) | ||
}) | ||
|
||
nitroApp.hooks.callHook('nuxt-security:ready') | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import { defineEventHandler, setHeader, removeResponseHeader } from '#imports' | ||
|
||
export default defineEventHandler((event) => { | ||
if(event.context.security.headers) { | ||
Object.entries(event.context.security.headers).forEach(([header, value]) => { | ||
if(value === false) { | ||
removeResponseHeader(event, header) | ||
}else { | ||
setHeader(event, header, value, ) | ||
huang-julien marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
}) | ||
} | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
import type { ModuleOptions as CsrfOptions } from 'nuxt-csurf' | ||
import type { Options as RemoveOptions } from 'unplugin-remove/types' | ||
|
||
import type { SecurityHeaders } from './headers' | ||
import type { AllowedHTTPMethods, BasicAuth, CorsOptions, RateLimiter, RequestSizeLimiter, XssValidator } from './middlewares' | ||
|
||
|
@@ -23,6 +22,10 @@ export interface ModuleOptions { | |
nonce: boolean; | ||
removeLoggers?: RemoveOptions | false; | ||
ssg?: Ssg; | ||
/** | ||
* enable runtime nitro hooks to configure some options at runtime | ||
*/ | ||
runtimeHooks: boolean; | ||
huang-julien marked this conversation as resolved.
Show resolved
Hide resolved
|
||
sri?: boolean | ||
} | ||
|
||
|
@@ -34,3 +37,20 @@ export interface NuxtSecurityRouteRules { | |
allowedMethodsRestricter?: AllowedHTTPMethods | false; | ||
nonce?: boolean; | ||
} | ||
|
||
export interface NuxtSecurityEventContext { | ||
headers: Record<string, string | string[] | number | false> | null | ||
huang-julien marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
declare module 'h3' { | ||
interface H3EventContext { | ||
security: NuxtSecurityEventContext | ||
} | ||
} | ||
|
||
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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Not required though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, we probably need to add a reference in a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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> |
||
'nuxt-security:ready': () => void | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
imports.autoImport=true |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
<template> | ||
<div> | ||
<NuxtPage /> | ||
</div> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import MyModule from '../../../src/module' | ||
|
||
export default defineNuxtConfig({ | ||
modules: [ | ||
MyModule | ||
], | ||
routeRules:{ | ||
'/test': { | ||
headers: { | ||
'x-xss-protection': '1', | ||
} | ||
} | ||
}, | ||
security: { | ||
nonce: false, | ||
runtimeHooks: true, | ||
headers: { | ||
contentSecurityPolicy: false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
} | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
"private": true, | ||
"name": "runtime-hooks", | ||
"type": "module" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
<template> | ||
<div>runtime hooks</div> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import { getResponseHeader } from "h3" | ||
|
||
export default defineEventHandler((event) => { | ||
return { | ||
csp: getResponseHeader(event, 'Content-Security-Policy') | ||
} | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
export default defineNitroPlugin((nitroApp) => { | ||
nitroApp.hooks.hook('nuxt-security:ready', () => { | ||
nitroApp.hooks.callHook('nuxt-security:headers', '/api/runtime-hooks' ,{ | ||
contentSecurityPolicy: { | ||
"script-src": ["'self'", "'unsafe-inline'", '*.azure.com'], | ||
} | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import { fileURLToPath } from 'node:url' | ||
import { describe, it, expect } from 'vitest' | ||
import { setup, fetch } from '@nuxt/test-utils' | ||
|
||
await setup({ | ||
rootDir: fileURLToPath(new URL('./fixtures/runtime-hooks', import.meta.url)) | ||
}) | ||
|
||
describe('[nuxt-security] runtime hooks', () => { | ||
it('expect csp to be set by a runtime hook', async () => { | ||
const res = await fetch('/api/runtime-hooks') | ||
expect(await res.json()).toMatchInlineSnapshot(` | ||
{ | ||
"csp": "script-src 'self' 'unsafe-inline' *.azure.com", | ||
} | ||
`) | ||
expect(res.headers.get('Content-Security-Policy')).toMatchInlineSnapshot( '"script-src \'self\' \'unsafe-inline\' *.azure.com"') | ||
}) | ||
}) |
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 pluginThere 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.