refactor(security)!: remove signing and billable Maps proxies - #788
Open
harlan-zw wants to merge 10 commits into
Open
refactor(security)!: remove signing and billable Maps proxies#788harlan-zw wants to merge 10 commits into
harlan-zw wants to merge 10 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
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
6 tasks
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 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. Google Maps uses direct browser-side services and Static Maps URLs. The security configuration, signing runtime paths, and Google Maps proxy handlers are removed. Documentation describes allowlisting, public Google Maps keys, quotas, caching, and rate limiting. Tests cover unsigned URLs and deterministic concurrent SSR output. Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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://cdn.github.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
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
harlan-zw
changed the title
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)
packages/script/src/module.ts (1)
839-845: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the obsolete URL-signing warning.
Line 844 states that URL signing is unavailable on static presets. This PR removes URL signing from the module. The warning now describes a feature that does not exist.
Proposed fix
- + 'Scripts will be bundled, but collection requests will not be proxied and URL signing will be unavailable.\n' + + 'Scripts will be bundled, but collection requests will not be proxied.\n'🤖 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 839 - 845, Remove the obsolete URL-signing sentence from the logger.warn message in the proxyStaticPresets handling block, while preserving the remaining proxy limitation and configuration guidance.packages/script/package.json (1)
4-4: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBump
@nuxt/scriptsto a breaking release version.
packages/script/package.jsonstill declares1.3.2, but this release removes public CLI/package contracts. Sincenpm publishreads the package manifest and the rootreleasescript can still bump manifests via bumpp, publish2.x.xor the repository’s equivalent breaking-release version before releasing these changes.🤖 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/package.json` at line 4, Update the version field in packages/script/package.json from 1.3.2 to the repository’s next breaking-release 2.x.x version, ensuring the manifest used by npm publish reflects the major-version bump before release.
🤖 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/components/GoogleMaps/ScriptGoogleMapsStaticMap.vue`:
- Around line 121-124: The public prop JSDoc for ScriptGoogleMapsStaticMap must
reflect the current direct Google Maps Static Maps URL behavior: remove
references to proxying and update the signature description so it no longer
claims signature is used without an API key, since generated URLs always include
key.
- Around line 215-216: Update the request parameter ordering in the Static Maps
configuration so `key: apiKey` precedes `signature: props.signature`, making
`signature` the final parameter in the unsigned request URL used for
verification.
---
Outside diff comments:
In `@packages/script/package.json`:
- Line 4: Update the version field in packages/script/package.json from 1.3.2 to
the repository’s next breaking-release 2.x.x version, ensuring the manifest used
by npm publish reflects the major-version bump before release.
In `@packages/script/src/module.ts`:
- Around line 839-845: Remove the obsolete URL-signing sentence from the
logger.warn message in the proxyStaticPresets handling block, while preserving
the remaining proxy limitation and configuration guidance.
🪄 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 Plus
Run ID: 6dc3179e-d370-4308-86d1-abf9dd45f1a1
📒 Files selected for processing (27)
docs/content/docs/1.guides/2.first-party.mddocs/content/docs/3.api/5.nuxt-config.mddocs/content/scripts/google-maps/2.api/1b.static-map.mddocs/content/scripts/google-maps/index.mdpackages/script/bin/cli.mjspackages/script/build.config.tspackages/script/package.jsonpackages/script/src/cli.tspackages/script/src/module.tspackages/script/src/registry.tspackages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vuepackages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsStaticMap.vuepackages/script/src/runtime/maplibre-styles.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/image-proxy.tspackages/script/src/runtime/server/utils/instagram-embed.tspackages/script/src/runtime/server/x-embed.tspackages/script/src/runtime/types.tstest/e2e/issue-783-proxy-token-payload.test.tstest/fixtures/issue-783/nuxt.config.tstest/unit/embed-rewriters.test.tstest/unit/google-maps-security.test.ts
💤 Files with no reviewable changes (6)
- packages/script/bin/cli.mjs
- packages/script/build.config.ts
- packages/script/src/runtime/server/google-static-maps-proxy.ts
- packages/script/src/runtime/server/google-maps-geocode-proxy.ts
- packages/script/src/cli.ts
- docs/content/docs/3.api/5.nuxt-config.md
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/script/src/runtime/server/utils/cached-upstream.ts
- packages/script/src/runtime/types.ts
- packages/script/src/runtime/server/gravatar-proxy.ts
- packages/script/src/registry.ts
- packages/script/src/runtime/server/utils/image-proxy.ts
- packages/script/src/runtime/server/x-embed.ts
- test/unit/embed-rewriters.test.ts
- packages/script/src/runtime/server/bluesky-embed.ts
- packages/script/src/runtime/server/utils/instagram-embed.ts
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
Per request proxy tokens changed every SSR payload and expired inside cached or generated HTML. Since the token was public, anyone who could read the page payload could reuse it with arbitrary proxy parameters.
Google Static Maps and geocoding were the signed routes with user billing exposure. Their URLs are created by client components, so protecting a server key would require an open signing endpoint. This PR removes those routes instead.
Static Maps now calls Google directly with the public browser key. Location queries use the client Places service. Google website restrictions, API restrictions, and quotas now enforce the billing boundary. Embed and avatar proxies remain limited to their declared upstream hosts and validated response types.
The change also removes the HMAC implementation, page token payload plugin, secret generation, signing configuration, and the package CLI that only generated signing secrets.
scripts.security,NUXT_SCRIPTS_PROXY_SECRET, and thegenerate-secretcommand.googleStaticMapsProxyand the Google Static Maps and geocoding server routes.<ScriptGoogleMapsStaticMap>sends the public registry key directly to Google.403.Remove the deleted config and environment values during migration. Restrict the Google key to approved websites and APIs, then set Google Cloud quota limits.
Verification
pnpm lintpnpm typecheckpnpm build