fix(nuxt): run middleware for page islands#35092
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
@nuxt/kit
@nuxt/nitro-server
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
Merging this PR will improve performance by 14.22%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | loadNuxt in the basic test fixture |
488.5 ms | 427.7 ms | +14.22% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/server-page-middleware (3167288) with main (0fa4813)
Footnotes
-
3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nuxt/src/pages/runtime/plugins/router.ts`:
- Around line 209-210: The island-validation currently in router.beforeResolve
runs too late (after beforeEach middleware) so mismatched island requests still
execute middleware; move the validation check so it runs before middleware by
either (A) performing the island key check at the top of the existing beforeEach
handler (check nuxtApp.ssrContext?.islandContext and isServerPage/island key
before any middleware logic) or (B) attach the expected island key to the
RouteRecord (e.g. set a custom property on to.meta in the route creation) and
then immediately validate that property at the start of router.beforeEach;
update both the beforeEach and the duplicate logic around lines 279-293 to use
this earlier guard to ensure middleware never runs for mismatched islands.
- Around line 282-285: The current check uses to.matched.find(...) which returns
the first (shallowest) matched record causing nested server pages to compare
against the parent island; update the lookup to resolve the deepest/leaf matched
route (e.g., use a reverse search like iterating from
to.matched.slice().reverse() or findLast) so that actual reflects the leaf page
component before comparing against
pageIslandRoutes[nuxtApp.ssrContext!.islandContext!.name]; adjust the expression
that defines actual (the to.matched lookup) in the router plugin to pick the
last matched record instead of the first.
In `@packages/nuxt/src/pages/utils.ts`:
- Around line 355-362: The current ordering in normalizeComponentWithName causes
_sync server pages to bypass the island wrapper; modify the function so
server-mode handling runs before the isSyncImport branch and ensure
sync-imported server pages return an island-wrapped component (using
createIslandPage with routeName and islandKey) while still attaching __name
(metaRouteName) when isSyncImport is true; locate normalizeComponentWithName and
adjust the branches around page.mode, isSyncImport, pageImportName,
createIslandPage, metaRouteName, routeName and islandKey so server pages always
go through createIslandPage and sync imports get Object.assign(..., { __name:
metaRouteName }).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 96ef3890-9c37-4468-a056-f3a6cb6d9d6a
📒 Files selected for processing (8)
packages/nitro-server/src/runtime/handlers/island.tspackages/nuxt/src/components/runtime/server-component.tspackages/nuxt/src/components/templates.tspackages/nuxt/src/pages/runtime/plugins/router.tspackages/nuxt/src/pages/utils.tstest/fixtures/server-components/app/middleware/island-auth.tstest/fixtures/server-components/app/pages/gated-server-page.server.vuetest/server-components.test.ts
| if (import.meta.client || !nuxtApp.ssrContext?.islandContext || isServerPage) { | ||
| type MiddlewareDef = string | RouteMiddleware |
There was a problem hiding this comment.
Validate the page island before the middleware pipeline.
router.beforeResolve() runs after the Line 201 beforeEach middleware chain, so a mismatched island request still executes route middleware first. If that middleware redirects or errors, this 400 branch never runs, which breaks the new “only run middleware when the hash matches” contract. Please move the validation to a guard that runs before middleware, or thread the expected island key onto the route record so it can be checked at the top of beforeEach.
Also applies to: 279-293
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/nuxt/src/pages/runtime/plugins/router.ts` around lines 209 - 210,
The island-validation currently in router.beforeResolve runs too late (after
beforeEach middleware) so mismatched island requests still execute middleware;
move the validation check so it runs before middleware by either (A) performing
the island key check at the top of the existing beforeEach handler (check
nuxtApp.ssrContext?.islandContext and isServerPage/island key before any
middleware logic) or (B) attach the expected island key to the RouteRecord (e.g.
set a custom property on to.meta in the route creation) and then immediately
validate that property at the start of router.beforeEach; update both the
beforeEach and the duplicate logic around lines 279-293 to use this earlier
guard to ensure middleware never runs for mismatched islands.
There was a problem hiding this comment.
But is that really an issue of having 404 islands requerst go through middleware ?
Except if an attacker can test out middleware behaviors
There was a problem hiding this comment.
@huang-julien, you're right to push back on this. The 400 is still returned regardless of what middleware executes in beforeEach, so there's no functional breakage. The only residual risk is exactly what you described — information leakage: an attacker crafting forged island requests could observe middleware side-effects (e.g., whether a redirect fires, whether auth middleware throws) to infer the existence or behaviour of protected routes. That's a narrow, low-severity concern rather than the "broken contract" I originally framed it as. I'll withdraw the major-severity label on this one; the current ordering is a reasonable trade-off given the complexity of restructuring the guard.
Would you like to open a follow-up issue to track the minor information-disclosure angle, or is it small enough to leave as-is?
🧠 Learnings used
Learnt from: danielroe
Repo: nuxt/nuxt PR: 33905
File: packages/nuxt/src/core/nuxt.ts:80-80
Timestamp: 2026-03-06T14:21:08.891Z
Learning: General guideline: When registering a Nuxt hook (nuxt.hook('hookName', callback)), the hook system (hookable) awaits any Promise returned by the callback. Therefore patterns like nuxt.hook('prepare:types', () => nuxt.close()) are valid because the Promise from nuxt.close() will be awaited. Actionable takeaway: ensure hook callbacks return a Promise or are async if you need to perform async cleanup or operations, so they complete before the hook proceeds. This applies to TypeScript files within the Nuxt package (e.g., packages/nuxt/src/**/*.ts).
| const expected = pageIslandRoutes[nuxtApp.ssrContext!.islandContext!.name] | ||
| const actual = to.matched.find(m => (m.components?.default as any)?.__nuxt_island) | ||
| ?.components?.default as any | ||
| if (!expected || expected !== actual?.__nuxt_island) { |
There was a problem hiding this comment.
Resolve the deepest matched server page.
to.matched.find(...) returns the first matched record, so nested server pages validate against the parent island key instead of the leaf page. Valid child page-island requests will be rejected with 400 in that case.
♻️ Suggested change
- const actual = to.matched.find(m => (m.components?.default as any)?.__nuxt_island)
+ const actual = [...to.matched].reverse()
+ .find(m => (m.components?.default as any)?.__nuxt_island)
?.components?.default as any📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const expected = pageIslandRoutes[nuxtApp.ssrContext!.islandContext!.name] | |
| const actual = to.matched.find(m => (m.components?.default as any)?.__nuxt_island) | |
| ?.components?.default as any | |
| if (!expected || expected !== actual?.__nuxt_island) { | |
| const expected = pageIslandRoutes[nuxtApp.ssrContext!.islandContext!.name] | |
| const actual = [...to.matched].reverse() | |
| .find(m => (m.components?.default as any)?.__nuxt_island) | |
| ?.components?.default as any | |
| if (!expected || expected !== actual?.__nuxt_island) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/nuxt/src/pages/runtime/plugins/router.ts` around lines 282 - 285,
The current check uses to.matched.find(...) which returns the first (shallowest)
matched record causing nested server pages to compare against the parent island;
update the lookup to resolve the deepest/leaf matched route (e.g., use a reverse
search like iterating from to.matched.slice().reverse() or findLast) so that
actual reflects the leaf page component before comparing against
pageIslandRoutes[nuxtApp.ssrContext!.islandContext!.name]; adjust the expression
that defines actual (the to.matched lookup) in the router plugin to pick the
last matched record instead of the first.
| function normalizeComponentWithName (page: NuxtPage, isSyncImport: boolean | undefined, pageImportName: string, pageImport: string, routeName: string | undefined, metaRouteName: string, islandKey: string | undefined): string { | ||
| if (isSyncImport) { | ||
| return `Object.assign(${pageImportName}, { __name: ${metaRouteName} })` | ||
| } | ||
| // Server components already receive the name via createIslandPage(name) | ||
| if (page.mode === 'server') { | ||
| return `() => createIslandPage(${routeName})` | ||
| return `() => createIslandPage(${routeName}, ${islandKey})` | ||
| } |
There was a problem hiding this comment.
Keep sync-imported server pages on the island wrapper.
isSyncImport is checked before page.mode === 'server', so a _sync server page returns the raw page component instead of createIslandPage(...). That skips the page_ wrapper and never attaches the new island key, so this route will miss the middleware/validation path entirely.
♻️ Suggested change
function normalizeComponentWithName (page: NuxtPage, isSyncImport: boolean | undefined, pageImportName: string, pageImport: string, routeName: string | undefined, metaRouteName: string, islandKey: string | undefined): string {
- if (isSyncImport) {
- return `Object.assign(${pageImportName}, { __name: ${metaRouteName} })`
- }
- // Server components already receive the name via createIslandPage(name)
if (page.mode === 'server') {
return `() => createIslandPage(${routeName}, ${islandKey})`
}
+ if (isSyncImport) {
+ return `Object.assign(${pageImportName}, { __name: ${metaRouteName} })`
+ }🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 361-361: Improper code sanitization
Code construction depends on an improperly sanitized value.
[warning] 361-361: Improper code sanitization
Code construction depends on an improperly sanitized value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/nuxt/src/pages/utils.ts` around lines 355 - 362, The current
ordering in normalizeComponentWithName causes _sync server pages to bypass the
island wrapper; modify the function so server-mode handling runs before the
isSyncImport branch and ensure sync-imported server pages return an
island-wrapped component (using createIslandPage with routeName and islandKey)
while still attaching __name (metaRouteName) when isSyncImport is true; locate
normalizeComponentWithName and adjust the branches around page.mode,
isSyncImport, pageImportName, createIslandPage, metaRouteName, routeName and
islandKey so server pages always go through createIslandPage and sync imports
get Object.assign(..., { __name: metaRouteName }).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/nuxt/src/pages/utils.ts (1)
355-362:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSync-imported server pages still bypass island wrapping.
Line 356 returns early for
isSyncImport, so_syncserver pages never reach the server branch on Line 361. That skipscreateIslandPage(...)and dropsislandKey, which can bypass page-island middleware/route validation on this path.♻️ Suggested fix
function normalizeComponentWithName (page: NuxtPage, isSyncImport: boolean | undefined, pageImportName: string, pageImport: string, routeName: string | undefined, metaRouteName: string, islandKey: string | undefined): string { - if (isSyncImport) { - return `Object.assign(${pageImportName}, { __name: ${metaRouteName} })` - } - // Server components already receive the name via createIslandPage(name) if (page.mode === 'server') { return `() => createIslandPage(${routeName}, import.meta.server ? ${islandKey} : undefined)` } + if (isSyncImport) { + return `Object.assign(${pageImportName}, { __name: ${metaRouteName} })` + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nuxt/src/pages/utils.ts` around lines 355 - 362, The early return when isSyncImport prevents server-mode pages from being wrapped with createIslandPage and losing islandKey; in normalizeComponentWithName ensure server pages are handled before or combined with the sync-import branch so that page.mode === 'server' always produces a createIslandPage wrapper that preserves islandKey (even for sync imports) instead of returning Object.assign(...) directly — update the conditional order or add a sync+server branch that returns a createIslandPage call (preserving metaRouteName/pageImportName semantics and passing islandKey) so server pages never bypass island wrapping or route validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/nuxt/src/pages/utils.ts`:
- Around line 355-362: The early return when isSyncImport prevents server-mode
pages from being wrapped with createIslandPage and losing islandKey; in
normalizeComponentWithName ensure server pages are handled before or combined
with the sync-import branch so that page.mode === 'server' always produces a
createIslandPage wrapper that preserves islandKey (even for sync imports)
instead of returning Object.assign(...) directly — update the conditional order
or add a sync+server branch that returns a createIslandPage call (preserving
metaRouteName/pageImportName semantics and passing islandKey) so server pages
never bypass island wrapping or route validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e6efb8bf-3694-464f-813a-d4d8d13dbc84
📒 Files selected for processing (5)
packages/nitro-server/src/runtime/handlers/island.tspackages/nuxt/src/pages/runtime/plugins/router.tspackages/nuxt/src/pages/utils.tstest/fixtures/server-components/app/middleware/island-auth.tstest/server-components.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/fixtures/server-components/app/middleware/island-auth.ts
- test/server-components.test.ts
- packages/nuxt/src/pages/runtime/plugins/router.ts
- packages/nitro-server/src/runtime/handlers/island.ts
huang-julien
left a comment
huang-julien
left a comment
There was a problem hiding this comment.
It's nice to have middlewares for server pages back 🔥
There's a small question regarding the trow in the handler
3 participants
🔗 Linked issue
#19772
📚 Description
up to now, server components are exempt from middleware. this is by design and is called out in the docs.
but it may be counter intuitive for users when this comes to server pages. so this PR adds a new hash comparison for server pages to ensure that the route that the page is rendering matches the route component the server page is meant to server - and then runs middleware and handles redirections for server pages.