refactor(security): remove proxy URL signing and the page token#788
refactor(security): remove proxy URL signing and the page token#788harlan-zw wants to merge 7 commits 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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR removes HMAC URL signing and per-request proxy tokens from Nuxt Scripts. Proxy URLs and server handlers no longer accept or generate signing secrets, while endpoint registrations use upstream allowlisting. The security configuration, secret-generation CLI command, token composable, and signing-related runtime paths are removed. Documentation now describes allowlisting, Google Maps key handling, caching, and rate limiting. Existing tests are simplified for unsigned behavior, and a new end-to-end fixture verifies unsigned URLs and deterministic concurrent SSR output. Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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
…removal # Conflicts: # docs/content/docs/1.guides/2.first-party.md # docs/content/scripts/bluesky-embed.md # docs/content/scripts/google-maps/2.api/1b.static-map.md # packages/script/src/module.ts # packages/script/src/runtime/server/instagram-embed.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
🔗 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.