fix(security): add pageToken opt-out to keep SSR payload deterministic#787
Closed
harlan-zw wants to merge 1 commit into
Closed
fix(security): add pageToken opt-out to keep SSR payload deterministic#787harlan-zw wants to merge 1 commit into
harlan-zw wants to merge 1 commit into
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThis PR adds an optional Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/script/src/runtime/plugins/proxy-token.server.ts`:
- Around line 27-28: The server reads pageTokenMaxAge into the local maxAge but
useScriptProxyToken() still uses the hardcoded PAGE_TOKEN_MAX_AGE, causing
mismatch between cookie lifetime and reuse checks; fix by passing the resolved
maxAge into useScriptProxyToken(maxAge) (or alternatively update the composable
to read runtime config/public pageTokenMaxAge), and ensure PAGE_TOKEN_MAX_AGE is
only the fallback; update the call site in proxy-token.server.ts to pass the
resolved maxAge and adjust the composable signature to accept and apply that
maxAge for cookie creation and reuse logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa7fd0f0-fca8-4f04-b6e2-90cd029384ee
📒 Files selected for processing (2)
packages/script/src/runtime/composables/useScriptProxyToken.tspackages/script/src/runtime/plugins/proxy-token.server.ts
harlan-zw
force-pushed
the
fix/proxy-token-payload-opt-out
branch
from
e96c206 to
8a8714a
Compare
harlan-zw
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/script/src/module.ts`:
- Around line 418-432: The public config type is missing the 'payload' literal
while the docs and runtime accept it; update the type for the pageToken property
(named pageToken in the exported config/interface in module.ts) to include
'payload' (e.g. change the union from boolean | 'cookie' to boolean | 'cookie' |
'payload') so user code can set security.pageToken = 'payload' without a type
error and the public API matches the JSDoc/runtime behavior.
In `@packages/script/src/runtime/plugins/proxy-token.server.ts`:
- Around line 37-41: The current early-return only checks cookie.value.ts;
change it to validate the existing token before reusing by computing expected =
generateProxyToken(secret, cookie.value.ts) and confirming cookie.value.token
=== expected (and typeof cookie.value.token === 'string') in addition to the
timestamp check; only return early from the branch if both the timestamp is
within maxAge/2 and the token matches the generated value, otherwise set
cookie.value = { token: generateProxyToken(secret, now), ts: now } to regenerate
a fresh, valid token.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d139bb3-40dd-428d-b75b-7d0bfffba108
📒 Files selected for processing (4)
docs/content/docs/1.guides/2.first-party.mdpackages/script/src/module.tspackages/script/src/runtime/composables/useScriptProxyToken.tspackages/script/src/runtime/plugins/proxy-token.server.ts
Comment on lines
+418
to
+432
| * - `true` / `'payload'` (default): the token is added to the SSR payload | ||
| * only on pages that actually use a proxy helper. Pages without one keep | ||
| * a deterministic payload. No cookie is set. A page reached purely via | ||
| * client-side navigation cannot mint a token, so dynamic client-driven | ||
| * proxy calls there fall back to unsigned (use `'cookie'` if you need | ||
| * them). | ||
| * - `'cookie'`: the token is delivered in a cookie instead of the payload. | ||
| * Every page's payload stays deterministic and the token remains | ||
| * available across client-side navigation. Adds one functional cookie. | ||
| * - `false`: no page token is issued. Client-driven proxy calls then | ||
| * require each URL to be pre-signed. | ||
| * | ||
| * @default true | ||
| */ | ||
| pageToken?: boolean | 'cookie' |
There was a problem hiding this comment.
Expose 'payload' in the public config type.
The JSDoc and runtime both treat 'payload' as a valid mode, but pageToken?: boolean | 'cookie' rejects security.pageToken = 'payload' in user config. That leaves the public API type out of sync with the documented behavior.
Suggested fix
- pageToken?: boolean | 'cookie'
+ pageToken?: boolean | 'payload' | 'cookie'📝 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.
Suggested change
| * - `true` / `'payload'` (default): the token is added to the SSR payload | |
| * only on pages that actually use a proxy helper. Pages without one keep | |
| * a deterministic payload. No cookie is set. A page reached purely via | |
| * client-side navigation cannot mint a token, so dynamic client-driven | |
| * proxy calls there fall back to unsigned (use `'cookie'` if you need | |
| * them). | |
| * - `'cookie'`: the token is delivered in a cookie instead of the payload. | |
| * Every page's payload stays deterministic and the token remains | |
| * available across client-side navigation. Adds one functional cookie. | |
| * - `false`: no page token is issued. Client-driven proxy calls then | |
| * require each URL to be pre-signed. | |
| * | |
| * @default true | |
| */ | |
| pageToken?: boolean | 'cookie' | |
| * - `true` / `'payload'` (default): the token is added to the SSR payload | |
| * only on pages that actually use a proxy helper. Pages without one keep | |
| * a deterministic payload. No cookie is set. A page reached purely via | |
| * client-side navigation cannot mint a token, so dynamic client-driven | |
| * proxy calls there fall back to unsigned (use `'cookie'` if you need | |
| * them). | |
| * - `'cookie'`: the token is delivered in a cookie instead of the payload. | |
| * Every page's payload stays deterministic and the token remains | |
| * available across client-side navigation. Adds one functional cookie. | |
| * - `false`: no page token is issued. Client-driven proxy calls then | |
| * require each URL to be pre-signed. | |
| * | |
| * `@default` true | |
| */ | |
| pageToken?: boolean | 'payload' | 'cookie' |
🤖 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/script/src/module.ts` around lines 418 - 432, The public config type
is missing the 'payload' literal while the docs and runtime accept it; update
the type for the pageToken property (named pageToken in the exported
config/interface in module.ts) to include 'payload' (e.g. change the union from
boolean | 'cookie' to boolean | 'cookie' | 'payload') so user code can set
security.pageToken = 'payload' without a type error and the public API matches
the JSDoc/runtime behavior.
8a8714a to
587a6d3
Compare
harlan-zw
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/script/src/module.ts (1)
411-423:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose
'cookie'as a validsecurity.pageTokenmode in the public type.
pageTokenis currently typed asboolean, so typed user config cannot setsecurity.pageToken = 'cookie'.Proposed fix
- pageToken?: boolean + pageToken?: boolean | 'cookie'🤖 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/script/src/module.ts` around lines 411 - 423, Change the public type for pageToken from a plain boolean to allow the string mode 'cookie' so users can set security.pageToken = 'cookie'; locate the declaration of pageToken (and any related interface where security.pageToken is defined) and change its type to a union (e.g. boolean | 'cookie'), update the JSDoc comment if needed to document the new mode and default, and ensure any type usages/exports (e.g. exported config types or interfaces referencing pageToken) are updated accordingly so the compiler accepts the 'cookie' string value.
🤖 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/script/src/runtime/plugins/proxy-token.server.ts`:
- Around line 18-24: The setup currently always stores the generated token on
nuxtApp._scriptsProxyToken (using generateProxyToken and the proxySecret from
useRuntimeConfig()['nuxt-scripts']), which ignores a 'cookie' delivery mode;
update setup to read the delivery mode from the same config (e.g.,
config['nuxt-scripts'].proxyMode or deliveryMode) and branch: if mode !==
'cookie' keep the existing nuxtApp._scriptsProxyToken behavior, but if mode ===
'cookie' set or reuse an HTTP cookie for the token (registering/using the SSR
response cookie API or a dedicated server plugin) instead of stashing on
nuxtApp; ensure token generation still uses generateProxyToken(secret, ts) and
keep ts tracking for reuse.
In `@test/e2e/issue-783-proxy-token-payload.test.ts`:
- Around line 23-29: The test "keeps the payload identical across requests for
token-free pages" can false-pass when two concurrent $fetch calls happen in the
same second; fix by making the timestamp deterministic during the test: stub
Date.now (or the app's time source) to return a fixed millisecond value before
the two $fetch<string>('/') calls (variables a and b), run the concurrent
requests, assert equality, then restore the original timer; this ensures the
payload identity check is robust and not reliant on same-second timing races.
---
Duplicate comments:
In `@packages/script/src/module.ts`:
- Around line 411-423: Change the public type for pageToken from a plain boolean
to allow the string mode 'cookie' so users can set security.pageToken =
'cookie'; locate the declaration of pageToken (and any related interface where
security.pageToken is defined) and change its type to a union (e.g. boolean |
'cookie'), update the JSDoc comment if needed to document the new mode and
default, and ensure any type usages/exports (e.g. exported config types or
interfaces referencing pageToken) are updated accordingly so the compiler
accepts the 'cookie' string value.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7efb5cb4-02ef-4793-a3df-49be767087a4
📒 Files selected for processing (10)
docs/content/docs/1.guides/2.first-party.mdpackages/script/src/module.tspackages/script/src/runtime/composables/useScriptProxyToken.tspackages/script/src/runtime/plugins/proxy-token.server.tstest/e2e/issue-783-proxy-token-payload.test.tstest/fixtures/issue-783/app.vuetest/fixtures/issue-783/nuxt.config.tstest/fixtures/issue-783/package.jsontest/fixtures/issue-783/pages/index.vuetest/fixtures/issue-783/pages/proxy.vue
✅ Files skipped from review due to trivial changes (6)
- test/fixtures/issue-783/package.json
- test/fixtures/issue-783/pages/proxy.vue
- test/fixtures/issue-783/app.vue
- test/fixtures/issue-783/pages/index.vue
- test/fixtures/issue-783/nuxt.config.ts
- docs/content/docs/1.guides/2.first-party.md
Comment on lines
18
to
24
| setup(nuxtApp) { | ||
| const secret = (useRuntimeConfig()['nuxt-scripts'] as { proxySecret?: string } | undefined)?.proxySecret | ||
| if (!secret) | ||
| return | ||
| const ts = Math.floor(Date.now() / 1000) | ||
| useScriptProxyToken().value = { | ||
| token: generateProxyToken(secret, ts), | ||
| ts, | ||
| } | ||
| nuxtApp._scriptsProxyToken = { token: generateProxyToken(secret, ts), ts } | ||
| }, |
There was a problem hiding this comment.
'cookie' delivery mode is not represented in this server plugin path.
This setup always stashes the token on nuxtApp, which keeps delivery payload-based. If 'cookie' is a supported mode, add a mode branch here (or register a dedicated plugin) to issue/reuse the token via cookie instead.
🤖 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/script/src/runtime/plugins/proxy-token.server.ts` around lines 18 -
24, The setup currently always stores the generated token on
nuxtApp._scriptsProxyToken (using generateProxyToken and the proxySecret from
useRuntimeConfig()['nuxt-scripts']), which ignores a 'cookie' delivery mode;
update setup to read the delivery mode from the same config (e.g.,
config['nuxt-scripts'].proxyMode or deliveryMode) and branch: if mode !==
'cookie' keep the existing nuxtApp._scriptsProxyToken behavior, but if mode ===
'cookie' set or reuse an HTTP cookie for the token (registering/using the SSR
response cookie API or a dedicated server plugin) instead of stashing on
nuxtApp; ensure token generation still uses generateProxyToken(secret, ts) and
keep ts tracking for reuse.
Comment on lines
+23
to
+29
| it('keeps the payload identical across requests for token-free pages', async () => { | ||
| const [a, b] = await Promise.all([ | ||
| $fetch<string>('/'), | ||
| $fetch<string>('/'), | ||
| ]) | ||
| expect(a).toBe(b) | ||
| }) |
There was a problem hiding this comment.
The determinism assertion can false-pass with same-second concurrent requests.
Because token timestamps are second-based, the two concurrent fetches may match even if per-request token injection regresses.
Proposed fix
- it('keeps the payload identical across requests for token-free pages', async () => {
- const [a, b] = await Promise.all([
- $fetch<string>('/'),
- $fetch<string>('/'),
- ])
+ it('keeps the payload identical across requests for token-free pages', async () => {
+ const a = await $fetch<string>('/')
+ await new Promise(resolve => setTimeout(resolve, 1100))
+ const b = await $fetch<string>('/')
expect(a).toBe(b)
})📝 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.
Suggested change
| it('keeps the payload identical across requests for token-free pages', async () => { | |
| const [a, b] = await Promise.all([ | |
| $fetch<string>('/'), | |
| $fetch<string>('/'), | |
| ]) | |
| expect(a).toBe(b) | |
| }) | |
| it('keeps the payload identical across requests for token-free pages', async () => { | |
| const a = await $fetch<string>('/') | |
| await new Promise(resolve => setTimeout(resolve, 1100)) | |
| const b = await $fetch<string>('/') | |
| expect(a).toBe(b) | |
| }) |
🤖 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 `@test/e2e/issue-783-proxy-token-payload.test.ts` around lines 23 - 29, The
test "keeps the payload identical across requests for token-free pages" can
false-pass when two concurrent $fetch calls happen in the same second; fix by
making the timestamp deterministic during the test: stub Date.now (or the app's
time source) to return a fixed millisecond value before the two
$fetch<string>('/') calls (variables a and b), run the concurrent requests,
assert equality, then restore the original timer; this ensures the payload
identity check is robust and not reliant on same-second timing races.
The per-request proxy page token is embedded in the SSR payload whenever a signing-required proxy handler is enabled. Being timestamp-based, it makes the payload differ on every request, which breaks response etag hashing. Add a `security.pageToken` option (default true). When false, the proxy-token server plugin is not registered, so no token is generated and the payload stays deterministic. Client-driven proxy URLs must then be pre-signed. Default behaviour is unchanged. Resolves #783
harlan-zw
force-pushed
the
fix/proxy-token-payload-opt-out
branch
from
587a6d3 to
b0919e1
Compare
harlan-zw
changed the title
Collaborator
Author
|
Superseded by #788, which removes the proxy URL signing subsystem entirely (the page token was only one symptom of a mechanism that also breaks under caching/SSG and could not protect the one endpoint that mattered). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
Resolves #783
❓ Type of change
📚 Description
The per-request proxy page token is embedded in the SSR payload whenever a signing-required proxy handler is enabled. Being timestamp-based, it makes the payload differ on every request, which breaks response
etaghashing.Adds
security.pageToken(defaulttrue). Whenfalse, the proxy-token server plugin is not registered, so no token is generated and the payload stays deterministic. Client-driven proxy URLs must then be pre-signed.Default behaviour is unchanged — this is a pure opt-out, so it carries no regression for SSG, embeds, or client-side navigation. The issue author noted a higher-level flag would be acceptable.
Verified against
test/fixtures/issue-783: withpageToken: false,useScriptProxyTokenresolves to null and the SSR payload is byte-identical across requests.