refactor(security): remove proxy URL signing and the page token#788
Open
harlan-zw wants to merge 1 commit into
Open
refactor(security): remove proxy URL signing and the page token#788harlan-zw wants to merge 1 commit into
harlan-zw wants to merge 1 commit into
Conversation
The proxy page token was embedded per-request in the SSR payload, which made the payload non-deterministic (breaking response etag hashing, issue #783), broke under response caching and SSG, and was weak protection anyway since it is scrapeable from the payload. Signing also could not protect the one endpoint that mattered: the Google Maps proxy URL is emitted by a client component, so it cannot be signed without either shipping node:crypto to the browser or exposing a signing oracle. Remove the proxy URL signing subsystem entirely: - Delete the page-token plugin/composable, sign.ts, withSigning, sign-constants, and the proxy secret resolution + .env auto-generation. - Drop the `security` config option and the generate-secret CLI command. - Proxy/embed endpoints rely on their existing upstream-domain allowlist; the Google Maps proxy keeps its API key server-side and is cache-shielded. - useScriptProxyUrl / buildProxyUrl emit plain URLs; the SSR payload is now deterministic. Resolves #783
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
6 tasks
commit: |
📝 WalkthroughWalkthroughThis PR removes the HMAC URL signing and per-request proxy token mechanisms from Nuxt Scripts, replacing them with a simpler allowlist-based endpoint security model. Changes include deleting signing utilities and CLI commands, removing Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/unit/embed-rewriters.test.ts (1)
93-100:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the Bluesky avatar rewrite assertion strict for unsigned-URL regressions.
This currently uses
toContain, so a URL with legacy params could still pass. Please assert the exact unsigned URL (or explicitly assert absence of_pt,_ts,sig).Proposed test tightening
it('rewrites the author avatar', () => { const post = { author: { avatar: 'https://cdn.bsky.app/img/avatar.jpg' }, } rewriteBlueskyPostImages(post, BSKY_PATH) - expect(post.author.avatar).toContain(encodeURIComponent('https://cdn.bsky.app/img/avatar.jpg')) - expect(post.author.avatar).toContain(BSKY_PATH) + expect(post.author.avatar).toBe( + `${BSKY_PATH}?url=${encodeURIComponent('https://cdn.bsky.app/img/avatar.jpg')}`, + ) })🤖 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/unit/embed-rewriters.test.ts` around lines 93 - 100, The test for avatar rewriting is too loose—update the assertion in the test that calls rewriteBlueskyPostImages (using BSKY_PATH) so it verifies the exact unsigned URL returned for post.author.avatar (not using toContain), or alternatively assert that the avatar string does not include legacy signing query params `_pt`, `_ts`, or `sig`; reference post.author.avatar and the rewriteBlueskyPostImages helper to locate where to change the expectation and ensure the result matches the precise unsigned format.packages/script/src/runtime/server/google-maps-geocode-proxy.ts (1)
15-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIgnore the retired signing params here for rollout compatibility.
After removing
withSigning, stale pages/client bundles can still send_pt,_ts, andsig. Letting those flow intosafeQuerycreates distinct cache keys for the same geocode lookup and can cause avoidable paid misses right after deploy.💡 Suggested patch
- const { key: _clientKey, ...safeQuery } = query + const { + key: _clientKey, + _pt: _legacyPageToken, + _ts: _legacyTimestamp, + sig: _legacySignature, + ...safeQuery + } = query🤖 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/server/google-maps-geocode-proxy.ts` around lines 15 - 48, The proxy is inadvertently including retired signing params (_pt, _ts, sig) in safeQuery which creates distinct cache keys and causes paid misses; update the query extraction around getQuery/event so that, in addition to removing key (currently destructured as key: _clientKey), you also remove _pt, _ts, and sig (e.g., destructure { key: _clientKey, _pt, _ts, sig, ...safeQuery } = query) before calling withQuery and cachedGeocodeFetch (references: getQuery, safeQuery, withQuery, cachedGeocodeFetch) so legacy params are stripped from the geocodeUrl/cache key.
🧹 Nitpick comments (1)
packages/script/src/registry.ts (1)
685-688: Document or apply throttling for the unsigned Google Maps routes.These two handlers now proxy billable Google APIs with a server-side key and no request-level gate. If this move to allowlist-only is intentional, please make sure the release notes/docs point users at Nitro
routeRulesthrottling for these paths; otherwise they can be hotlinked to drain quota.🤖 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/registry.ts` around lines 685 - 688, The new serverHandlers exposing '/_scripts/proxy/google-static-maps' and '/_scripts/proxy/google-maps-geocode' are unthrottled billable endpoints; add request-level throttling or document required Nitro routeRules for these routes. Either implement a server-side rate limiter middleware in the handlers referenced ('./runtime/server/google-static-maps-proxy' and './runtime/server/google-maps-geocode-proxy') or add Nitro routeRules for the two routes with strict rate/limit settings and include a comment linking to the docs; ensure the chosen approach rejects or 429s requests when limits are exceeded and reference the serverHandlers entries so reviewers can verify the change.
🤖 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/server/google-static-maps-proxy.ts`:
- Around line 11-13: The cache-key generation currently only strips the 'key'
param via the STRIP_PARAMS set; update STRIP_PARAMS to also include the legacy
signing parameters '_pt', '_ts', and 'sig' so those are removed from the proxied
URL before creating the cache key (look for the STRIP_PARAMS constant in
google-static-maps-proxy.ts and add the three legacy names to it).
In `@test/unit/proxy-url.test.ts`:
- Around line 52-55: The test for buildProxyUrl should avoid using the same
object instance twice; change the spec so it calls buildProxyUrl with two
separate but equivalent query objects (e.g., const query1 = { a: '1', b:
['x','y'] } and const query2 = { a: '1', b: ['x','y'] }) and assert the returned
URLs are equal, and optionally assert query1 still equals its original shape
after the calls to guard against in-place mutation by buildProxyUrl; locate and
update the test around the existing it('is deterministic: same inputs produce
identical URLs') that currently reuses the same query variable.
---
Outside diff comments:
In `@packages/script/src/runtime/server/google-maps-geocode-proxy.ts`:
- Around line 15-48: The proxy is inadvertently including retired signing params
(_pt, _ts, sig) in safeQuery which creates distinct cache keys and causes paid
misses; update the query extraction around getQuery/event so that, in addition
to removing key (currently destructured as key: _clientKey), you also remove
_pt, _ts, and sig (e.g., destructure { key: _clientKey, _pt, _ts, sig,
...safeQuery } = query) before calling withQuery and cachedGeocodeFetch
(references: getQuery, safeQuery, withQuery, cachedGeocodeFetch) so legacy
params are stripped from the geocodeUrl/cache key.
In `@test/unit/embed-rewriters.test.ts`:
- Around line 93-100: The test for avatar rewriting is too loose—update the
assertion in the test that calls rewriteBlueskyPostImages (using BSKY_PATH) so
it verifies the exact unsigned URL returned for post.author.avatar (not using
toContain), or alternatively assert that the avatar string does not include
legacy signing query params `_pt`, `_ts`, or `sig`; reference post.author.avatar
and the rewriteBlueskyPostImages helper to locate where to change the
expectation and ensure the result matches the precise unsigned format.
---
Nitpick comments:
In `@packages/script/src/registry.ts`:
- Around line 685-688: The new serverHandlers exposing
'/_scripts/proxy/google-static-maps' and '/_scripts/proxy/google-maps-geocode'
are unthrottled billable endpoints; add request-level throttling or document
required Nitro routeRules for these routes. Either implement a server-side rate
limiter middleware in the handlers referenced
('./runtime/server/google-static-maps-proxy' and
'./runtime/server/google-maps-geocode-proxy') or add Nitro routeRules for the
two routes with strict rate/limit settings and include a comment linking to the
docs; ensure the chosen approach rejects or 429s requests when limits are
exceeded and reference the serverHandlers entries so reviewers can verify the
change.
🪄 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: d9ed0ee7-976d-4cc4-b29a-4b20717d4b21
📒 Files selected for processing (40)
docs/content/docs/1.guides/2.first-party.mddocs/content/scripts/bluesky-embed.mddocs/content/scripts/google-maps/2.api/1b.static-map.mddocs/content/scripts/google-maps/index.mddocs/content/scripts/gravatar.mddocs/content/scripts/instagram-embed.mddocs/content/scripts/x-embed.mdpackages/script/src/cli.tspackages/script/src/module.tspackages/script/src/registry.tspackages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsStaticMap.vuepackages/script/src/runtime/composables/useScriptProxyToken.tspackages/script/src/runtime/composables/useScriptProxyUrl.tspackages/script/src/runtime/plugins/proxy-token.server.tspackages/script/src/runtime/server/bluesky-embed.tspackages/script/src/runtime/server/google-maps-geocode-proxy.tspackages/script/src/runtime/server/google-static-maps-proxy.tspackages/script/src/runtime/server/gravatar-proxy.tspackages/script/src/runtime/server/instagram-embed.tspackages/script/src/runtime/server/utils/cached-upstream.tspackages/script/src/runtime/server/utils/embed-rewriters.tspackages/script/src/runtime/server/utils/image-proxy.tspackages/script/src/runtime/server/utils/instagram-embed.tspackages/script/src/runtime/server/utils/proxy-url.tspackages/script/src/runtime/server/utils/sign-constants.tspackages/script/src/runtime/server/utils/sign.tspackages/script/src/runtime/server/utils/withSigning.tspackages/script/src/runtime/server/x-embed.tspackages/script/src/runtime/types.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/unit/embed-rewriters.test.tstest/unit/proxy-url.test.tstest/unit/resolve-proxy-secret.test.tstest/unit/sign.test.tstest/unit/use-script-proxy-url.test.tstest/unit/with-signing.test.ts
💤 Files with no reviewable changes (15)
- test/unit/sign.test.ts
- docs/content/scripts/bluesky-embed.md
- test/unit/resolve-proxy-secret.test.ts
- docs/content/scripts/google-maps/2.api/1b.static-map.md
- packages/script/src/runtime/types.ts
- packages/script/src/runtime/server/utils/withSigning.ts
- packages/script/src/runtime/server/utils/sign-constants.ts
- packages/script/src/runtime/composables/useScriptProxyToken.ts
- docs/content/scripts/instagram-embed.md
- packages/script/src/runtime/plugins/proxy-token.server.ts
- docs/content/scripts/x-embed.md
- docs/content/scripts/gravatar.md
- test/unit/with-signing.test.ts
- packages/script/src/runtime/server/utils/sign.ts
- docs/content/scripts/google-maps/index.md
Comment on lines
+11
to
+13
| // Strip the client-provided API key so the cache key is pinned to the actual | ||
| // map being requested (the real key is server-injected below). | ||
| const STRIP_PARAMS = new Set(['key']) |
There was a problem hiding this comment.
Keep stripping the legacy signing params from the cache key.
Old HTML/client bundles can still hit this route with _pt, _ts, and sig during a rolling deploy. Leaving them on the proxied URL turns one logical map request into many cache entries and needlessly burns Static Maps quota.
💡 Suggested patch
-const STRIP_PARAMS = new Set(['key'])
+const STRIP_PARAMS = new Set(['key', '_pt', '_ts', 'sig'])🤖 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/server/google-static-maps-proxy.ts` around lines
11 - 13, The cache-key generation currently only strips the 'key' param via the
STRIP_PARAMS set; update STRIP_PARAMS to also include the legacy signing
parameters '_pt', '_ts', and 'sig' so those are removed from the proxied URL
before creating the cache key (look for the STRIP_PARAMS constant in
google-static-maps-proxy.ts and add the three legacy names to it).
Comment on lines
52
to
55
| it('is deterministic: same inputs produce identical URLs', () => { | ||
| const query = { a: '1', b: ['x', 'y'] } | ||
| const a = buildProxyUrl('/api/proxy', query, SECRET) | ||
| const b = buildProxyUrl('/api/proxy', query, SECRET) | ||
| expect(a).toBe(b) | ||
| }) | ||
|
|
||
| it('different secrets produce different signatures', () => { | ||
| const query = { k: 'v' } | ||
| const a = buildProxyUrl('/api/proxy', query, SECRET) | ||
| const b = buildProxyUrl('/api/proxy', query, 'different-secret-value') | ||
| expect(a).not.toBe(b) | ||
|
|
||
| const sigA = a.match(/sig=([a-f0-9]+)$/)?.[1] | ||
| const sigB = b.match(/sig=([a-f0-9]+)$/)?.[1] | ||
| expect(sigA).toBeTruthy() | ||
| expect(sigB).toBeTruthy() | ||
| expect(sigA).not.toBe(sigB) | ||
| expect(buildProxyUrl('/api/proxy', query)).toBe(buildProxyUrl('/api/proxy', query)) | ||
| }) |
There was a problem hiding this comment.
Strengthen the determinism test to avoid same-reference blind spots.
Using the same object instance on both calls can miss mutation side effects. Use two equivalent objects (and optionally assert the original object is unchanged).
Proposed test update
it('is deterministic: same inputs produce identical URLs', () => {
- const query = { a: '1', b: ['x', 'y'] }
- expect(buildProxyUrl('/api/proxy', query)).toBe(buildProxyUrl('/api/proxy', query))
+ const queryA = { a: '1', b: ['x', 'y'] }
+ const queryB = { a: '1', b: ['x', 'y'] }
+ expect(buildProxyUrl('/api/proxy', queryA)).toBe(buildProxyUrl('/api/proxy', queryB))
+ expect(queryA).toEqual({ a: '1', b: ['x', 'y'] })
})📝 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('is deterministic: same inputs produce identical URLs', () => { | |
| const query = { a: '1', b: ['x', 'y'] } | |
| const a = buildProxyUrl('/api/proxy', query, SECRET) | |
| const b = buildProxyUrl('/api/proxy', query, SECRET) | |
| expect(a).toBe(b) | |
| }) | |
| it('different secrets produce different signatures', () => { | |
| const query = { k: 'v' } | |
| const a = buildProxyUrl('/api/proxy', query, SECRET) | |
| const b = buildProxyUrl('/api/proxy', query, 'different-secret-value') | |
| expect(a).not.toBe(b) | |
| const sigA = a.match(/sig=([a-f0-9]+)$/)?.[1] | |
| const sigB = b.match(/sig=([a-f0-9]+)$/)?.[1] | |
| expect(sigA).toBeTruthy() | |
| expect(sigB).toBeTruthy() | |
| expect(sigA).not.toBe(sigB) | |
| expect(buildProxyUrl('/api/proxy', query)).toBe(buildProxyUrl('/api/proxy', query)) | |
| }) | |
| it('is deterministic: same inputs produce identical URLs', () => { | |
| const queryA = { a: '1', b: ['x', 'y'] } | |
| const queryB = { a: '1', b: ['x', 'y'] } | |
| expect(buildProxyUrl('/api/proxy', queryA)).toBe(buildProxyUrl('/api/proxy', queryB)) | |
| expect(queryA).toEqual({ a: '1', b: ['x', 'y'] }) | |
| }) |
🤖 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/unit/proxy-url.test.ts` around lines 52 - 55, The test for buildProxyUrl
should avoid using the same object instance twice; change the spec so it calls
buildProxyUrl with two separate but equivalent query objects (e.g., const query1
= { a: '1', b: ['x','y'] } and const query2 = { a: '1', b: ['x','y'] }) and
assert the returned URLs are equal, and optionally assert query1 still equals
its original shape after the calls to guard against in-place mutation by
buildProxyUrl; locate and update the test around the existing it('is
deterministic: same inputs produce identical URLs') that currently reuses the
same query variable.
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 proxy page token was embedded per-request in the SSR payload. That made the payload non-deterministic, which breaks response
etaghashing (the original report, #783). It also broke under response caching (a cached page serves a frozen, eventually-expired token → silent 403s), under SSG, and under client-side navigation — and it was weak protection anyway, since the token sits in plaintext in the payload and is trivially scrapeable.Investigating a proper fix surfaced a deeper problem: the one endpoint where signing protects real cost — the Google Maps proxy, which injects your API key — has a client-component-emitted URL. It cannot be HMAC-signed without either shipping
node:cryptoto the browser (the build literally fails) or exposing an open signing oracle (which provides zero protection). Signing only ever worked for server-emitted URLs.So this removes the proxy URL signing subsystem entirely:
sign.ts,withSigning,sign-constants, proxy-secret resolution +.envauto-generation, thesecurityconfig option, and thegenerate-secretCLI command.useScriptProxyUrl/buildProxyUrlnow emit plain URLs. The SSR payload is deterministic.scripts.securityconfig option (secret,autoGenerateSecret) is removed.NUXT_SCRIPTS_PROXY_SECRETand thenpx @nuxt/scripts generate-secretcommand are removed.403for unsigned requests. If quota abuse of the Google Maps proxy is a concern, add rate limiting via NitrorouteRulesfor/_scripts/proxy/**(documented in the first-party guide).Verification
@nuxt/scriptsbuilds; the static-map fixture builds with nonode:cryptoin the client bundle; 487 unit tests pass;test/fixtures/issue-783confirms the static map renders a plain proxy URL and the SSR payload is byte-identical across requests.