feat: add geocode proxy with CSRF protection for Google Maps billing optimization#635
Open
feat: add geocode proxy with CSRF protection for Google Maps billing optimization#635
Conversation
- Server-side Places API proxy at /_scripts/google-maps-geocode-proxy - Aggressive caching (24h default) for place name → coordinate lookups - API key stays server-side, never exposed to client - ScriptGoogleMaps component auto-uses proxy when enabled - Falls back to client-side Places API when proxy unavailable Closes #83
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a googleGeocodeProxy option (enabled, cacheMaxAge) and initializes defaults alongside googleStaticMapsProxy. Module auto-enables proxies based on registry.googleMaps and populates public/private runtimeConfig with injected Maps API key when enabled. Introduces CSRF utilities and middleware, client-side token handling in the GoogleMaps component, a new geocode proxy server handler, and enhanced static-maps proxy handler. Both proxies include IP rate limiting, referer checks, input validation, server-side API key usage, and configurable caching. Documentation updated with proxy guidance. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue`:
- Around line 10-11: The import order is incorrect for linting: move the
nuxt/app imports (tryUseNuxtApp, useHead, useRuntimeConfig) before the ofetch
import ($fetch) in ScriptGoogleMaps.vue so the imports are alphabetized; update
the top of the file so the line importing tryUseNuxtApp/useHead/useRuntimeConfig
appears before the line importing $fetch.
- Around line 254-264: The proxy branch assumes mapsApi.value exists and will
throw if called before the Maps API loads; update the proxy branch that uses
geocodeProxyConfig to either await the Maps API (e.g., await mapsApi to be
initialized before creating new mapsApi.value.LatLng) or return/store a plain
{lat, lng} fallback so callers (and queryToLatLngCache) don't require the Maps
API instance; specifically, inside the geocodeProxyConfig?.enabled block check
mapsApi.value and if missing either await the loader that populates mapsApi or
cache/return the raw coordinates and convert to mapsApi.value.LatLng later (so
defineExpose'd methods can be called before the API is ready).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68da8b8c-00c7-4670-acee-df006b5f0911
📒 Files selected for processing (3)
src/module.tssrc/runtime/components/GoogleMaps/ScriptGoogleMaps.vuesrc/runtime/server/google-maps-geocode-proxy.ts
Comment on lines
+254
to
+264
| // Use server-side geocode proxy when enabled to save Places API costs | ||
| if (geocodeProxyConfig?.enabled) { | ||
| const data = await $fetch<{ lat: number, lng: number }>('/_scripts/google-maps-geocode-proxy', { | ||
| query: { input: query }, | ||
| }).catch(() => null) | ||
| if (data) { | ||
| const latLng = new mapsApi.value!.LatLng(data.lat, data.lng) | ||
| queryToLatLngCache.set(query, latLng) | ||
| return latLng | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle case where mapsApi is not yet loaded.
The proxy path assumes mapsApi.value is available, but this function is exposed via defineExpose (line 330) and could be called before the Maps API loads. The fallback path (lines 268-277) properly awaits API loading, but the proxy path doesn't.
Consider either awaiting mapsApi or returning a plain object that doesn't require the Maps API:
Option 1: Return plain coordinates (avoids Maps API dependency)
if (geocodeProxyConfig?.enabled) {
const data = await $fetch<{ lat: number, lng: number }>('/_scripts/google-maps-geocode-proxy', {
query: { input: query },
}).catch(() => null)
if (data) {
- const latLng = new mapsApi.value!.LatLng(data.lat, data.lng)
- queryToLatLngCache.set(query, latLng)
- return latLng
+ // Return LatLngLiteral - compatible with most Maps API methods
+ const latLng = { lat: data.lat, lng: data.lng }
+ queryToLatLngCache.set(query, latLng as any)
+ return latLng as any
}
}Option 2: Ensure Maps API is loaded first
if (geocodeProxyConfig?.enabled) {
const data = await $fetch<{ lat: number, lng: number }>('/_scripts/google-maps-geocode-proxy', {
query: { input: query },
}).catch(() => null)
if (data) {
+ if (!mapsApi.value) {
+ await load()
+ await new Promise<void>((resolve) => {
+ const stop = watch(mapsApi, () => { stop(); resolve() })
+ })
+ }
const latLng = new mapsApi.value!.LatLng(data.lat, data.lng)
queryToLatLngCache.set(query, latLng)
return latLng
}
}📝 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
| // Use server-side geocode proxy when enabled to save Places API costs | |
| if (geocodeProxyConfig?.enabled) { | |
| const data = await $fetch<{ lat: number, lng: number }>('/_scripts/google-maps-geocode-proxy', { | |
| query: { input: query }, | |
| }).catch(() => null) | |
| if (data) { | |
| const latLng = new mapsApi.value!.LatLng(data.lat, data.lng) | |
| queryToLatLngCache.set(query, latLng) | |
| return latLng | |
| } | |
| } | |
| // Use server-side geocode proxy when enabled to save Places API costs | |
| if (geocodeProxyConfig?.enabled) { | |
| const data = await $fetch<{ lat: number, lng: number }>('/_scripts/google-maps-geocode-proxy', { | |
| query: { input: query }, | |
| }).catch(() => null) | |
| if (data) { | |
| // Return LatLngLiteral - compatible with most Maps API methods | |
| const latLng = { lat: data.lat, lng: data.lng } | |
| queryToLatLngCache.set(query, latLng as any) | |
| return latLng as any | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue` around lines 254 -
264, The proxy branch assumes mapsApi.value exists and will throw if called
before the Maps API loads; update the proxy branch that uses geocodeProxyConfig
to either await the Maps API (e.g., await mapsApi to be initialized before
creating new mapsApi.value.LatLng) or return/store a plain {lat, lng} fallback
so callers (and queryToLatLngCache) don't require the Maps API instance;
specifically, inside the geocodeProxyConfig?.enabled block check mapsApi.value
and if missing either await the loader that populates mapsApi or cache/return
the raw coordinates and convert to mapsApi.value.LatLng later (so defineExpose'd
methods can be called before the API is ready).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/scripts/google-maps.md`:
- Line 90: The sentence claiming "Both proxies validate the request referer to
prevent external abuse and inject the API key server-side so it's never exposed
to the client." overstates behavior; update that sentence to qualify referer
protection by noting the handlers only perform referer/host header checks when
those headers are present and will not reject requests that lack a Referer
header, and keep the note that the API key is injected server-side. Edit the
sentence in the docs/content/scripts/google-maps.md entry for that line to
something like: state that proxies perform referer validation when Referer and
Host headers are provided (so requests without a Referer may still pass), and
that the API key is injected server-side to avoid exposing it to the client.
- Around line 56-57: Update the documentation text to correctly name the billed
API used by googleGeocodeProxy: it proxies the Places API's findplacefromtext
endpoint (not the Geocoding API). Replace the “Geocoding API” phrase with
“Places API (findplacefromtext)” and mention that billing/costs refer to the
Places API; reference the proxy name googleGeocodeProxy and the implementation
in src/runtime/server/google-maps-geocode-proxy.ts so readers enable the correct
Google product.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8114815b-462f-44ee-8cd9-2efc9f272747
📒 Files selected for processing (1)
docs/content/scripts/google-maps.md
docs/content/scripts/google-maps.md
Outdated
Comment on lines
+56
to
+57
| - $2 per 1000 loads for the Static Maps API - Only used when you don't provide a `placeholder` slot. **Can be cached** with `googleStaticMapsProxy`. | ||
| - $5 per 1000 loads for the Geocoding API - Only used when you don't provide a `google.maps.LatLng` object instead of a query string for the `center` prop. **Can be cached** with `googleGeocodeProxy`. |
There was a problem hiding this comment.
Name the billed API correctly.
googleGeocodeProxy does not call the Geocoding API; it proxies Places findplacefromtext. Documenting this as “Geocoding API” here can send readers to enable the wrong Google product and misread the billing surface. Please align this bullet with src/runtime/server/google-maps-geocode-proxy.ts.
Suggested wording
-- $5 per 1000 loads for the Geocoding API - Only used when you don't provide a `google.maps.LatLng` object instead of a query string for the `center` prop. **Can be cached** with `googleGeocodeProxy`.
+- $5 per 1000 requests for the Places API (Find Place From Text) - Only used when you don't provide a `google.maps.LatLng` object instead of a query string for the `center` prop. **Can be cached** with `googleGeocodeProxy`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/scripts/google-maps.md` around lines 56 - 57, Update the
documentation text to correctly name the billed API used by googleGeocodeProxy:
it proxies the Places API's findplacefromtext endpoint (not the Geocoding API).
Replace the “Geocoding API” phrase with “Places API (findplacefromtext)” and
mention that billing/costs refer to the Places API; reference the proxy name
googleGeocodeProxy and the implementation in
src/runtime/server/google-maps-geocode-proxy.ts so readers enable the correct
Google product.
docs/content/scripts/google-maps.md
Outdated
| | `googleStaticMapsProxy` | Static Maps ($2/1k) | 1 hour | Caches placeholder images, hides API key | | ||
| | `googleGeocodeProxy` | Places ($5/1k) | 24 hours | Caches place name → coordinate lookups | | ||
|
|
||
| Both proxies validate the request referer to prevent external abuse and inject the API key server-side so it's never exposed to the client. |
There was a problem hiding this comment.
Qualify the referer-protection claim.
This sentence is stronger than the implementation. Both proxy handlers only reject mismatched referers when referer and host headers are present; requests without a referer header still pass. Please soften the wording so users do not assume this fully prevents external abuse.
Suggested wording
-Both proxies validate the request referer to prevent external abuse and inject the API key server-side so it's never exposed to the client.
+Both proxies attempt same-origin referer validation when the relevant headers are present, and inject the API key server-side so it's never exposed to the client.📝 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
| Both proxies validate the request referer to prevent external abuse and inject the API key server-side so it's never exposed to the client. | |
| Both proxies attempt same-origin referer validation when the relevant headers are present, and inject the API key server-side so it's never exposed to the client. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/scripts/google-maps.md` at line 90, The sentence claiming "Both
proxies validate the request referer to prevent external abuse and inject the
API key server-side so it's never exposed to the client." overstates behavior;
update that sentence to qualify referer protection by noting the handlers only
perform referer/host header checks when those headers are present and will not
reject requests that lack a Referer header, and keep the note that the API key
is injected server-side. Edit the sentence in the
docs/content/scripts/google-maps.md entry for that line to something like: state
that proxies perform referer validation when Referer and Host headers are
provided (so requests without a Referer may still pass), and that the API key is
injected server-side to avoid exposing it to the client.
- IP-based rate limiting (30/min geocode, 60/min static maps) - Input length validation (200 char max) for geocode queries - Query parameter allowlisting for static maps proxy - Fix unprotected URL parsing in referer validation
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/runtime/server/google-static-maps-proxy.ts (1)
49-56: Rate limiting works within a single process only.The in-memory rate limiting won't function correctly across serverless deployments (e.g., Cloudflare Workers, Vercel Edge) or clustered Node.js processes. Each instance maintains its own independent counter, effectively multiplying the allowed rate by the number of instances.
Consider documenting this limitation, or for production use-cases, recommend external rate limiting (e.g., Redis, CDN-level rate limiting).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/server/google-static-maps-proxy.ts` around lines 49 - 56, The current in-memory rate limiting using getRequestIP(event, { xForwardedFor: true }) and checkRateLimit(...) only applies per-process and will not work correctly across serverless or multi-process deployments; update the implementation or documentation: either replace checkRateLimit with a distributed rate limiter backed by an external store (e.g., Redis/Upstash) or add a clear comment near the ip / checkRateLimit usage (and in README) stating this limitation and recommending CDN-level or Redis-backed rate limiting for production, ensuring createError behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/server/google-maps-geocode-proxy.ts`:
- Around line 104-109: The error message currently interpolates the raw
user-provided variable input into the thrown createError call in the geocode
check (the if block that tests data.status and data.candidates), which can leak
user data; replace the direct interpolation with a sanitized or omitted value
(e.g., use a truncated/validated version of input or a generic placeholder like
"<redacted>" or "unspecified query") before passing it into createError so logs
and responses no longer reflect full user input while keeping the existing error
condition and createError usage intact.
- Around line 10-21: The in-memory Map requestCounts used by checkRateLimit
(with RATE_LIMIT_WINDOW_MS and RATE_LIMIT_MAX) will grow without bound; extract
this logic into a shared rate-limit utility (e.g., a new rate-limit module) and
replace the local requestCounts/checkRateLimit with calls to that utility which
implements eviction: store per-key {count, resetAt}, increment/reset as now
does, and run a periodic cleanup (setInterval) to remove entries whose resetAt <
Date.now() to prevent memory leaks; ensure both google-maps-geocode-proxy and
google-static-maps-proxy call the same exported functions (e.g.,
createRateLimiter or checkAndRecord) so logic is centralized and duplicated maps
are removed.
In `@src/runtime/server/google-static-maps-proxy.ts`:
- Around line 15-26: The requestCounts map used by checkRateLimit grows without
eviction; update checkRateLimit (and the module) to remove stale entries so
memory doesn't grow unbounded: when fetching an entry in checkRateLimit, if
entry.resetAt <= Date.now() treat it as expired and replace it (or delete it)
before creating a fresh record; additionally add a periodic cleanup task (e.g.,
setInterval) that scans requestCounts and deletes entries whose resetAt is in
the past to reclaim memory; reference requestCounts, checkRateLimit,
RATE_LIMIT_WINDOW_MS and RATE_LIMIT_MAX when making these changes.
- Around line 9-13: Reformat the ALLOWED_PARAMS Set initialization so each array
item appears on its own line to satisfy the antfu/consistent-list-newline ESLint
rule: break the existing comma-separated list into one entry per line inside the
new Set([...]) expression, keep trailing commas on each line, and ensure the
opening bracket/parenthesis and closing bracket/parenthesis remain correctly
placed around the items (update the declaration named ALLOWED_PARAMS
accordingly).
---
Nitpick comments:
In `@src/runtime/server/google-static-maps-proxy.ts`:
- Around line 49-56: The current in-memory rate limiting using
getRequestIP(event, { xForwardedFor: true }) and checkRateLimit(...) only
applies per-process and will not work correctly across serverless or
multi-process deployments; update the implementation or documentation: either
replace checkRateLimit with a distributed rate limiter backed by an external
store (e.g., Redis/Upstash) or add a clear comment near the ip / checkRateLimit
usage (and in README) stating this limitation and recommending CDN-level or
Redis-backed rate limiting for production, ensuring createError behavior remains
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce917b4e-6080-424a-bff7-04e19b8c71cd
📒 Files selected for processing (2)
src/runtime/server/google-maps-geocode-proxy.tssrc/runtime/server/google-static-maps-proxy.ts
Comment on lines
+10
to
+21
| const requestCounts = new Map<string, { count: number, resetAt: number }>() | ||
|
|
||
| function checkRateLimit(ip: string): boolean { | ||
| const now = Date.now() | ||
| const entry = requestCounts.get(ip) | ||
| if (!entry || now > entry.resetAt) { | ||
| requestCounts.set(ip, { count: 1, resetAt: now + RATE_LIMIT_WINDOW_MS }) | ||
| return true | ||
| } | ||
| entry.count++ | ||
| return entry.count <= RATE_LIMIT_MAX | ||
| } |
There was a problem hiding this comment.
Memory leak: rate limit entries are never evicted (same issue as static maps proxy).
This is the same unbounded memory growth issue present in google-static-maps-proxy.ts. Consider extracting a shared rate limiting utility with periodic cleanup for both proxies.
🛡️ Proposed fix with periodic cleanup
const requestCounts = new Map<string, { count: number, resetAt: number }>()
+
+// Clean up stale entries periodically (every 5 minutes)
+setInterval(() => {
+ const now = Date.now()
+ for (const [ip, entry] of requestCounts) {
+ if (now > entry.resetAt) {
+ requestCounts.delete(ip)
+ }
+ }
+}, 5 * 60 * 1000).unref()
function checkRateLimit(ip: string): boolean {Consider extracting the rate limiting logic into a shared utility module (e.g., src/runtime/server/utils/rate-limit.ts) to eliminate duplication between both proxy handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/server/google-maps-geocode-proxy.ts` around lines 10 - 21, The
in-memory Map requestCounts used by checkRateLimit (with RATE_LIMIT_WINDOW_MS
and RATE_LIMIT_MAX) will grow without bound; extract this logic into a shared
rate-limit utility (e.g., a new rate-limit module) and replace the local
requestCounts/checkRateLimit with calls to that utility which implements
eviction: store per-key {count, resetAt}, increment/reset as now does, and run a
periodic cleanup (setInterval) to remove entries whose resetAt < Date.now() to
prevent memory leaks; ensure both google-maps-geocode-proxy and
google-static-maps-proxy call the same exported functions (e.g.,
createRateLimiter or checkAndRecord) so logic is centralized and duplicated maps
are removed.
Comment on lines
+104
to
+109
| if (data.status !== 'OK' || !data.candidates?.[0]?.geometry?.location) { | ||
| throw createError({ | ||
| statusCode: 404, | ||
| statusMessage: `No location found for "${input}"`, | ||
| }) | ||
| } |
There was a problem hiding this comment.
User input is reflected in error message—consider sanitizing.
The raw input query parameter is interpolated into the error message. While this aids debugging, it could leak user-provided data in logs or error responses. Consider truncating or omitting the input value.
🛡️ Proposed fix to avoid reflecting user input
if (data.status !== 'OK' || !data.candidates?.[0]?.geometry?.location) {
throw createError({
statusCode: 404,
- statusMessage: `No location found for "${input}"`,
+ statusMessage: 'No location found for the provided query',
})
}📝 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
| if (data.status !== 'OK' || !data.candidates?.[0]?.geometry?.location) { | |
| throw createError({ | |
| statusCode: 404, | |
| statusMessage: `No location found for "${input}"`, | |
| }) | |
| } | |
| if (data.status !== 'OK' || !data.candidates?.[0]?.geometry?.location) { | |
| throw createError({ | |
| statusCode: 404, | |
| statusMessage: 'No location found for the provided query', | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/server/google-maps-geocode-proxy.ts` around lines 104 - 109, The
error message currently interpolates the raw user-provided variable input into
the thrown createError call in the geocode check (the if block that tests
data.status and data.candidates), which can leak user data; replace the direct
interpolation with a sanitized or omitted value (e.g., use a truncated/validated
version of input or a generic placeholder like "<redacted>" or "unspecified
query") before passing it into createError so logs and responses no longer
reflect full user input while keeping the existing error condition and
createError usage intact.
Comment on lines
+15
to
+26
| const requestCounts = new Map<string, { count: number, resetAt: number }>() | ||
|
|
||
| function checkRateLimit(ip: string): boolean { | ||
| const now = Date.now() | ||
| const entry = requestCounts.get(ip) | ||
| if (!entry || now > entry.resetAt) { | ||
| requestCounts.set(ip, { count: 1, resetAt: now + RATE_LIMIT_WINDOW_MS }) | ||
| return true | ||
| } | ||
| entry.count++ | ||
| return entry.count <= RATE_LIMIT_MAX | ||
| } |
There was a problem hiding this comment.
Memory leak: rate limit entries are never evicted.
The requestCounts map grows unboundedly as new IPs are tracked. Even though entries become stale after resetAt, they remain in the map indefinitely. In long-running server deployments, this will cause gradual memory growth.
🛡️ Proposed fix with periodic cleanup
const requestCounts = new Map<string, { count: number, resetAt: number }>()
+
+// Clean up stale entries periodically (every 5 minutes)
+setInterval(() => {
+ const now = Date.now()
+ for (const [ip, entry] of requestCounts) {
+ if (now > entry.resetAt) {
+ requestCounts.delete(ip)
+ }
+ }
+}, 5 * 60 * 1000).unref()
function checkRateLimit(ip: string): boolean {📝 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
| const requestCounts = new Map<string, { count: number, resetAt: number }>() | |
| function checkRateLimit(ip: string): boolean { | |
| const now = Date.now() | |
| const entry = requestCounts.get(ip) | |
| if (!entry || now > entry.resetAt) { | |
| requestCounts.set(ip, { count: 1, resetAt: now + RATE_LIMIT_WINDOW_MS }) | |
| return true | |
| } | |
| entry.count++ | |
| return entry.count <= RATE_LIMIT_MAX | |
| } | |
| const requestCounts = new Map<string, { count: number, resetAt: number }>() | |
| // Clean up stale entries periodically (every 5 minutes) | |
| setInterval(() => { | |
| const now = Date.now() | |
| for (const [ip, entry] of requestCounts) { | |
| if (now > entry.resetAt) { | |
| requestCounts.delete(ip) | |
| } | |
| } | |
| }, 5 * 60 * 1000).unref() | |
| function checkRateLimit(ip: string): boolean { | |
| const now = Date.now() | |
| const entry = requestCounts.get(ip) | |
| if (!entry || now > entry.resetAt) { | |
| requestCounts.set(ip, { count: 1, resetAt: now + RATE_LIMIT_WINDOW_MS }) | |
| return true | |
| } | |
| entry.count++ | |
| return entry.count <= RATE_LIMIT_MAX | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/server/google-static-maps-proxy.ts` around lines 15 - 26, The
requestCounts map used by checkRateLimit grows without eviction; update
checkRateLimit (and the module) to remove stale entries so memory doesn't grow
unbounded: when fetching an entry in checkRateLimit, if entry.resetAt <=
Date.now() treat it as expired and replace it (or delete it) before creating a
fresh record; additionally add a periodic cleanup task (e.g., setInterval) that
scans requestCounts and deletes entries whose resetAt is in the past to reclaim
memory; reference requestCounts, checkRateLimit, RATE_LIMIT_WINDOW_MS and
RATE_LIMIT_MAX when making these changes.
- Double-submit cookie CSRF token for geocode proxy - Auto-enable proxies when googleMaps is in registry config - Can be explicitly disabled with `enabled: false` - Update docs to reflect auto-enable behavior
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
docs/content/scripts/google-maps.md (2)
87-87:⚠️ Potential issue | 🟠 MajorSoften the referer-protection claim to match the handler.
The proxy only compares
RefererandHostwhen those headers are present; requests withoutRefererare still accepted. Please qualify this sentence so the docs don't overstate the protection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/scripts/google-maps.md` at line 87, The docs overstate referer protection: update the sentence mentioning "referer validation" to clarify that the proxy compares Referer and Host only when those headers are present and will still accept requests that lack a Referer; reference the geocode proxy and its referer-check logic so the wording reflects that validation is conditional rather than mandatory.
56-57:⚠️ Potential issue | 🟠 MajorName the billed API correctly.
googleGeocodeProxyproxies Placesfindplacefromtext, not the Geocoding API. As written, this points readers at the wrong Google product and billing surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/scripts/google-maps.md` around lines 56 - 57, The docs incorrectly state that `googleGeocodeProxy` caches the Geocoding API; update the wording to state that `googleGeocodeProxy` proxies the Places API method `findplacefromtext` (not the Geocoding API) and adjust the billing/caching note accordingly—mention that this proxy applies when you pass a string query for `center` (vs a `google.maps.LatLng`), and reference `findplacefromtext`/Places API in place of “Geocoding API” wherever `googleGeocodeProxy` or that billing line appears.src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue (1)
269-279:⚠️ Potential issue | 🟠 MajorWait for
mapsApibefore constructingLatLng.This proxy branch still does
new mapsApi.value!.LatLng(...)before the load/wait logic at Lines 284-293, soresolveQueryToLatLang()can throw when it is called through the exposed API before Maps finishes loading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue` around lines 269 - 279, The proxy branch in resolveQueryToLatLang constructs new mapsApi.value!.LatLng(...) before ensuring the Maps API is loaded, which can throw; update resolveQueryToLatLang to await the existing maps load/wait logic (the helper that waits for mapsApi/value to be ready—reuse the same await/wait function used later in the file) before creating the LatLng, and only then cache via queryToLatLngCache and return; reference mapsApi, resolveQueryToLatLang, and queryToLatLngCache when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/module.ts`:
- Around line 357-371: The code reads mapsApiKey from
nuxt.options.runtimeConfig.public.scripts before merging registry values,
causing mapsApiKey to be undefined when the key is provided in
config.registry.googleMaps; update the logic so mapsApiKey is sourced from
config.registry.googleMaps?.apiKey (or compute mapsApiKey after merging
config.registry into nuxt.options.runtimeConfig.public.scripts) before
constructing nuxt.options.runtimeConfig['nuxt-scripts'] so that
googleStaticMapsProxy and googleGeocodeProxy receive the real API key when
staticMapsEnabled or geocodeEnabled are true; adjust the variables mapsApiKey,
staticMapsEnabled, geocodeEnabled, and the
nuxt.options.runtimeConfig['nuxt-scripts'] assignment accordingly.
In `@src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue`:
- Around line 138-151: The CSRF cookie is only set during SSR which leaves
getCsrfToken() empty on the first client-side navigation; add a client-side
bootstrap that obtains/sets the proxy CSRF token before any proxy request: on
mount of the ScriptGoogleMaps component (or during its initialization) call a
lightweight client endpoint (or an existing server helper that returns/sets the
__nuxt_scripts_proxy cookie) and await its completion so setProxyCsrfCookie
logic runs for client navigations as well; update getCsrfToken() to read the
cookie after that bootstrap and ensure functions referenced (getCsrfToken,
setProxyCsrfCookie, geocodeProxyConfig) are used so the token is present before
the first proxy request.
In `@src/runtime/server/google-maps-geocode-proxy.ts`:
- Around line 56-71: The referer/host check should require origin metadata
instead of silently skipping when absent: fetch Origin via getHeader(event,
'origin') and use it as the primary check (fall back to Referer if Origin is not
present), parse the origin/referer host in a try/catch (like refererHost) and
compare that host to the request host; if neither Origin nor Referer are
present, or if the parsed origin/referer host differs from host, throw
createError({ statusCode: 403, statusMessage: 'Invalid origin' }) to enforce
same-origin-only access (update variable names such as origin, referer,
refererHost/originHost and reuse createError).
- Around line 117-120: The code uses `publicConfig.cacheMaxAge || 86400` which
treats 0 as falsy and prevents callers from disabling caching; update the
`cacheMaxAge` assignment in `google-maps-geocode-proxy.ts` to honor 0 by using a
nullish check (e.g., `?? 86400`) or an explicit undefined check on
`publicConfig.cacheMaxAge`, then continue to call `setHeader(event,
'Cache-Control', \`public, max-age=${cacheMaxAge}, s-maxage=${cacheMaxAge}\`)`
so `setHeader` receives the intended 0 when configured.
- Around line 73-87: The code assumes query.input is a string but getQuery() may
return string[]; update the validation around query/input (referencing getQuery,
query, input, MAX_INPUT_LENGTH, createError) to explicitly reject non-string or
array values: if Array.isArray(input) or typeof input !== 'string' throw
createError with a 400 and a clear message like 'Invalid "input" query
parameter; must be a single string'; keep the existing length check for valid
strings afterwards.
In `@src/runtime/server/utils/proxy-csrf.ts`:
- Around line 1-2: Reorder the imports so the type-only import comes before
value imports: move "import type { H3Event } from 'h3'" above the line importing
createError, getCookie, getHeader, and setCookie to satisfy the
perfectionist/sort-imports rule and keep the H3Event type import separate from
the value imports (createError, getCookie, getHeader, setCookie).
---
Duplicate comments:
In `@docs/content/scripts/google-maps.md`:
- Line 87: The docs overstate referer protection: update the sentence mentioning
"referer validation" to clarify that the proxy compares Referer and Host only
when those headers are present and will still accept requests that lack a
Referer; reference the geocode proxy and its referer-check logic so the wording
reflects that validation is conditional rather than mandatory.
- Around line 56-57: The docs incorrectly state that `googleGeocodeProxy` caches
the Geocoding API; update the wording to state that `googleGeocodeProxy` proxies
the Places API method `findplacefromtext` (not the Geocoding API) and adjust the
billing/caching note accordingly—mention that this proxy applies when you pass a
string query for `center` (vs a `google.maps.LatLng`), and reference
`findplacefromtext`/Places API in place of “Geocoding API” wherever
`googleGeocodeProxy` or that billing line appears.
In `@src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue`:
- Around line 269-279: The proxy branch in resolveQueryToLatLang constructs new
mapsApi.value!.LatLng(...) before ensuring the Maps API is loaded, which can
throw; update resolveQueryToLatLang to await the existing maps load/wait logic
(the helper that waits for mapsApi/value to be ready—reuse the same await/wait
function used later in the file) before creating the LatLng, and only then cache
via queryToLatLngCache and return; reference mapsApi, resolveQueryToLatLang, and
queryToLatLngCache when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e76cccd-d3fe-47d6-a46e-679fcd54f4fe
📒 Files selected for processing (5)
docs/content/scripts/google-maps.mdsrc/module.tssrc/runtime/components/GoogleMaps/ScriptGoogleMaps.vuesrc/runtime/server/google-maps-geocode-proxy.tssrc/runtime/server/utils/proxy-csrf.ts
Comment on lines
+357
to
371
| const mapsApiKey = (nuxt.options.runtimeConfig.public.scripts as any)?.googleMaps?.apiKey | ||
| // Auto-enable Google Maps proxies when googleMaps is in the registry | ||
| const hasGoogleMaps = !!config.registry?.googleMaps | ||
| const staticMapsEnabled = config.googleStaticMapsProxy?.enabled ?? hasGoogleMaps | ||
| const geocodeEnabled = config.googleGeocodeProxy?.enabled ?? hasGoogleMaps | ||
|
|
||
| nuxt.options.runtimeConfig['nuxt-scripts'] = { | ||
| version: version!, | ||
| // Private proxy config with API key (server-side only) | ||
| googleStaticMapsProxy: config.googleStaticMapsProxy?.enabled | ||
| ? { apiKey: (nuxt.options.runtimeConfig.public.scripts as any)?.googleMaps?.apiKey } | ||
| googleStaticMapsProxy: staticMapsEnabled | ||
| ? { apiKey: mapsApiKey } | ||
| : undefined, | ||
| googleGeocodeProxy: geocodeEnabled | ||
| ? { apiKey: mapsApiKey } | ||
| : undefined, |
There was a problem hiding this comment.
Populate proxy API keys from the registry config before enabling the handlers.
Line 357 reads the key from nuxt.options.runtimeConfig.public.scripts, but Lines 388-395 only merge config.registry into that object later. With the documented config shape scripts.registry.googleMaps = { apiKey: '...' }, both proxies are auto-enabled here and their private runtime config ends up with apiKey: undefined, so the handlers 500 at runtime. Read the key from config.registry.googleMaps first, or compute this after the merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/module.ts` around lines 357 - 371, The code reads mapsApiKey from
nuxt.options.runtimeConfig.public.scripts before merging registry values,
causing mapsApiKey to be undefined when the key is provided in
config.registry.googleMaps; update the logic so mapsApiKey is sourced from
config.registry.googleMaps?.apiKey (or compute mapsApiKey after merging
config.registry into nuxt.options.runtimeConfig.public.scripts) before
constructing nuxt.options.runtimeConfig['nuxt-scripts'] so that
googleStaticMapsProxy and googleGeocodeProxy receive the real API key when
staticMapsEnabled or geocodeEnabled are true; adjust the variables mapsApiKey,
staticMapsEnabled, geocodeEnabled, and the
nuxt.options.runtimeConfig['nuxt-scripts'] assignment accordingly.
Comment on lines
+138
to
+151
| // Set CSRF cookie during SSR for geocode proxy protection | ||
| if (import.meta.server && geocodeProxyConfig?.enabled) { | ||
| const event = useRequestEvent() | ||
| if (event) { | ||
| const { setProxyCsrfCookie } = await import('../../server/utils/proxy-csrf') | ||
| setProxyCsrfCookie(event) | ||
| } | ||
| } | ||
|
|
||
| // Read CSRF token from cookie on client for geocode proxy requests | ||
| function getCsrfToken(): string { | ||
| if (import.meta.server) return '' | ||
| return document.cookie.split('; ').find(c => c.startsWith('__nuxt_scripts_proxy='))?.split('=')[1] || '' | ||
| } |
There was a problem hiding this comment.
SSR-only CSRF setup breaks the first client-side visit to a map page.
This cookie is only created during server render. If the user navigates client-side to the first page that mounts <ScriptGoogleMaps>, getCsrfToken() stays empty, the proxy request 403s, and the component falls back to the paid client-side Places lookup. You need a client-side bootstrap path for the token before the first proxy request.
🧰 Tools
🪛 ESLint
[error] 149-149: Expect newline after if
(antfu/if-newline)
🪛 GitHub Check: test
[failure] 149-149:
Expect newline after if
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue` around lines 138 -
151, The CSRF cookie is only set during SSR which leaves getCsrfToken() empty on
the first client-side navigation; add a client-side bootstrap that obtains/sets
the proxy CSRF token before any proxy request: on mount of the ScriptGoogleMaps
component (or during its initialization) call a lightweight client endpoint (or
an existing server helper that returns/sets the __nuxt_scripts_proxy cookie) and
await its completion so setProxyCsrfCookie logic runs for client navigations as
well; update getCsrfToken() to read the cookie after that bootstrap and ensure
functions referenced (getCsrfToken, setProxyCsrfCookie, geocodeProxyConfig) are
used so the token is present before the first proxy request.
Comment on lines
+56
to
+71
| // Validate referer to prevent external abuse | ||
| const referer = getHeader(event, 'referer') | ||
| const host = getHeader(event, 'host') | ||
| if (referer && host) { | ||
| let refererHost: string | undefined | ||
| try { | ||
| refererHost = new URL(referer).host | ||
| } | ||
| catch {} | ||
| if (refererHost && refererHost !== host) { | ||
| throw createError({ | ||
| statusCode: 403, | ||
| statusMessage: 'Invalid referer', | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Require origin metadata instead of skipping the check when it's absent.
This only rejects requests when both referer and host are present and differ. A scripted client can omit Referer, mint a valid CSRF cookie/header pair, and still call the proxy directly. If this endpoint is meant to stay same-origin only, reject when neither a same-origin Origin nor Referer is present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/server/google-maps-geocode-proxy.ts` around lines 56 - 71, The
referer/host check should require origin metadata instead of silently skipping
when absent: fetch Origin via getHeader(event, 'origin') and use it as the
primary check (fall back to Referer if Origin is not present), parse the
origin/referer host in a try/catch (like refererHost) and compare that host to
the request host; if neither Origin nor Referer are present, or if the parsed
origin/referer host differs from host, throw createError({ statusCode: 403,
statusMessage: 'Invalid origin' }) to enforce same-origin-only access (update
variable names such as origin, referer, refererHost/originHost and reuse
createError).
Comment on lines
+73
to
+87
| const query = getQuery(event) | ||
| const input = query.input as string | ||
|
|
||
| if (!input) { | ||
| throw createError({ | ||
| statusCode: 400, | ||
| statusMessage: 'Missing "input" query parameter', | ||
| }) | ||
| } | ||
|
|
||
| if (input.length > MAX_INPUT_LENGTH) { | ||
| throw createError({ | ||
| statusCode: 400, | ||
| statusMessage: `Input too long (max ${MAX_INPUT_LENGTH} characters)`, | ||
| }) |
There was a problem hiding this comment.
Validate that input is a single string, not just truthy.
getQuery() can return string[], but query.input as string lets that through unchecked. That can produce malformed upstream requests instead of a clean 400.
Suggested fix
const query = getQuery(event)
- const input = query.input as string
+ const input = query.input
- if (!input) {
+ if (typeof input !== 'string' || !input.trim()) {
throw createError({
statusCode: 400,
statusMessage: 'Missing "input" query parameter',
})
}📝 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
| const query = getQuery(event) | |
| const input = query.input as string | |
| if (!input) { | |
| throw createError({ | |
| statusCode: 400, | |
| statusMessage: 'Missing "input" query parameter', | |
| }) | |
| } | |
| if (input.length > MAX_INPUT_LENGTH) { | |
| throw createError({ | |
| statusCode: 400, | |
| statusMessage: `Input too long (max ${MAX_INPUT_LENGTH} characters)`, | |
| }) | |
| const query = getQuery(event) | |
| const input = query.input | |
| if (typeof input !== 'string' || !input.trim()) { | |
| throw createError({ | |
| statusCode: 400, | |
| statusMessage: 'Missing "input" query parameter', | |
| }) | |
| } | |
| if (input.length > MAX_INPUT_LENGTH) { | |
| throw createError({ | |
| statusCode: 400, | |
| statusMessage: `Input too long (max ${MAX_INPUT_LENGTH} characters)`, | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/server/google-maps-geocode-proxy.ts` around lines 73 - 87, The
code assumes query.input is a string but getQuery() may return string[]; update
the validation around query/input (referencing getQuery, query, input,
MAX_INPUT_LENGTH, createError) to explicitly reject non-string or array values:
if Array.isArray(input) or typeof input !== 'string' throw createError with a
400 and a clear message like 'Invalid "input" query parameter; must be a single
string'; keep the existing length check for valid strings afterwards.
Comment on lines
+117
to
+120
| // Cache aggressively - place names rarely change coordinates | ||
| const cacheMaxAge = publicConfig.cacheMaxAge || 86400 | ||
| setHeader(event, 'Content-Type', 'application/json') | ||
| setHeader(event, 'Cache-Control', `public, max-age=${cacheMaxAge}, s-maxage=${cacheMaxAge}`) |
There was a problem hiding this comment.
Honor cacheMaxAge: 0 instead of forcing the default.
Using || 86400 treats 0 as unset, so callers cannot disable caching even though the option is configurable.
Suggested fix
- const cacheMaxAge = publicConfig.cacheMaxAge || 86400
+ const cacheMaxAge = publicConfig.cacheMaxAge ?? 86400📝 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
| // Cache aggressively - place names rarely change coordinates | |
| const cacheMaxAge = publicConfig.cacheMaxAge || 86400 | |
| setHeader(event, 'Content-Type', 'application/json') | |
| setHeader(event, 'Cache-Control', `public, max-age=${cacheMaxAge}, s-maxage=${cacheMaxAge}`) | |
| // Cache aggressively - place names rarely change coordinates | |
| const cacheMaxAge = publicConfig.cacheMaxAge ?? 86400 | |
| setHeader(event, 'Content-Type', 'application/json') | |
| setHeader(event, 'Cache-Control', `public, max-age=${cacheMaxAge}, s-maxage=${cacheMaxAge}`) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/server/google-maps-geocode-proxy.ts` around lines 117 - 120, The
code uses `publicConfig.cacheMaxAge || 86400` which treats 0 as falsy and
prevents callers from disabling caching; update the `cacheMaxAge` assignment in
`google-maps-geocode-proxy.ts` to honor 0 by using a nullish check (e.g., `??
86400`) or an explicit undefined check on `publicConfig.cacheMaxAge`, then
continue to call `setHeader(event, 'Cache-Control', \`public,
max-age=${cacheMaxAge}, s-maxage=${cacheMaxAge}\`)` so `setHeader` receives the
intended 0 when configured.
- Move CSRF cookie setup from component to Nitro server middleware - Fix import ordering and formatting lint errors - Fix passive voice in docs
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 (1)
src/runtime/server/google-static-maps-proxy.ts (1)
39-117:⚠️ Potential issue | 🟠 MajorMissing CSRF protection inconsistent with geocode proxy.
The geocode proxy (
google-maps-geocode-proxy.ts) validates CSRF tokens viavalidateProxyCsrf(event)at line 45, but this static maps proxy does not perform any CSRF validation. This creates inconsistent security posture between the two proxy endpoints.Both proxies are auto-enabled together when
googleMapsis in the registry, so they should have consistent protection. Consider adding CSRF validation here as well, or document why it's intentionally omitted for static maps (e.g., if the endpoint only returns images and doesn't perform state-changing operations).🛡️ Proposed fix to add CSRF validation
import { useRuntimeConfig } from '#imports' import { createError, defineEventHandler, getHeader, getQuery, getRequestIP, setHeader } from 'h3' import { $fetch } from 'ofetch' import { withQuery } from 'ufo' +import { validateProxyCsrf } from './utils/proxy-csrf' // ... existing code ... export default defineEventHandler(async (event) => { const runtimeConfig = useRuntimeConfig() const publicConfig = (runtimeConfig.public['nuxt-scripts'] as any)?.googleStaticMapsProxy const privateConfig = (runtimeConfig['nuxt-scripts'] as any)?.googleStaticMapsProxy if (!publicConfig?.enabled) { throw createError({ statusCode: 404, statusMessage: 'Google Static Maps proxy is not enabled', }) } + // CSRF validation (double-submit cookie pattern) + validateProxyCsrf(event) + // Get API key from private config (server-side only, not exposed to client)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/server/google-static-maps-proxy.ts` around lines 39 - 117, Add CSRF validation to the static maps proxy by invoking validateProxyCsrf(event) inside the defineEventHandler handler (same place it’s done in google-maps-geocode-proxy.ts) after confirming the proxy is enabled and before proceeding with rate-limiting / external fetch; ensure you import or reference validateProxyCsrf and throw/let it surface errors the same way as the geocode proxy so both endpoints share the same CSRF protection behavior.
🧹 Nitpick comments (2)
src/module.ts (1)
742-747: CSRF middleware registered globally affects all requests.When
geocodeEnabledis true, the CSRF middleware is registered withmiddleware: true(line 744), causing it to run on every incoming request. This sets CSRF cookies even for routes unrelated to the Google Maps proxies.This may be intentional for simplicity, but consider whether scoping to proxy routes would reduce overhead. See related comment on
proxy-csrf.tsmiddleware.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/module.ts` around lines 742 - 747, The CSRF middleware is being registered globally when geocodeEnabled is true via addServerHandler({ middleware: true, handler: await resolvePath('./runtime/server/middleware/proxy-csrf') }), which causes CSRF cookies to be set on every request; restrict its scope to only the proxy routes by changing the registration to a non-global handler (remove middleware: true) and instead register the middleware conditionally for the proxy route pattern (e.g., only mount the proxy-csrf handler for the geocode proxy endpoints) or wrap the proxy-csrf logic to early-exit unless the request path matches the proxy route; update the code around geocodeEnabled/addServerHandler and the proxy-csrf handler to ensure it's applied only to requests targeting the Google Maps proxy paths.src/runtime/server/middleware/proxy-csrf.ts (1)
4-6: Consider scoping CSRF middleware to proxy routes only.This middleware is registered globally (
middleware: truein module.ts), sosetProxyCsrfCookieexecutes on every incoming request, not just proxy-related ones. This generates and sets CSRF tokens for all routes, even those that don't use the proxies.While this works correctly, it adds unnecessary overhead. Consider scoping the middleware to only run when the request path matches proxy routes (e.g.,
/_scripts/google-*), or document why global registration is intentional.♻️ Option: Check route before setting cookie
import { defineEventHandler } from 'h3' import { setProxyCsrfCookie } from '../utils/proxy-csrf' export default defineEventHandler((event) => { + // Only set CSRF cookie for routes that need it + const path = event.path || '' + if (path.startsWith('/_scripts/google-')) { + setProxyCsrfCookie(event) + } - setProxyCsrfCookie(event) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/server/middleware/proxy-csrf.ts` around lines 4 - 6, The CSRF cookie is being set for every request because the middleware (registered via middleware: true in module.ts) always calls setProxyCsrfCookie; update the handler defined by defineEventHandler to first inspect the incoming request path (e.g., via event.node.req.url or the framework path helper) and only call setProxyCsrfCookie when the path matches your proxy routes (for example startsWith('/_scripts/google-') or a configured proxy prefix), or if you intentionally want global behavior, add a clear comment in the file/module documenting that choice; reference the existing defineEventHandler and setProxyCsrfCookie symbols when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/server/google-static-maps-proxy.ts`:
- Around line 60-67: The current fallback to the literal 'unknown' when
getRequestIP(event, { xForwardedFor: true }) returns undefined creates a shared
rate-limit bucket; change the logic in google-static-maps-proxy.ts so you do NOT
use a global 'unknown' key: if getRequestIP(...) returns undefined, return a 400
Bad Request using createError (or alternatively derive a per-request identifier
from a session token/header and pass that to checkRateLimit), and ensure you
call checkRateLimit only with a valid, non-empty identifier. Update references
around getRequestIP, checkRateLimit, and createError accordingly so callers
either receive a 400 when IP is missing or rate-limiting is performed with a
per-request identifier.
In `@src/runtime/server/utils/proxy-csrf.ts`:
- Around line 19-25: The cookie is currently set with secure: true which
prevents it from being sent over plain HTTP (breaking local dev on
http://localhost); update the setCookie call that sets COOKIE_NAME with token so
secure is conditional (e.g., secure = process.env.NODE_ENV !== 'development' or
based on a runtime isProduction flag) to allow non-HTTPS local development while
keeping secure=true in production, and keep other options (httpOnly, sameSite,
path, maxAge) the same.
---
Outside diff comments:
In `@src/runtime/server/google-static-maps-proxy.ts`:
- Around line 39-117: Add CSRF validation to the static maps proxy by invoking
validateProxyCsrf(event) inside the defineEventHandler handler (same place it’s
done in google-maps-geocode-proxy.ts) after confirming the proxy is enabled and
before proceeding with rate-limiting / external fetch; ensure you import or
reference validateProxyCsrf and throw/let it surface errors the same way as the
geocode proxy so both endpoints share the same CSRF protection behavior.
---
Nitpick comments:
In `@src/module.ts`:
- Around line 742-747: The CSRF middleware is being registered globally when
geocodeEnabled is true via addServerHandler({ middleware: true, handler: await
resolvePath('./runtime/server/middleware/proxy-csrf') }), which causes CSRF
cookies to be set on every request; restrict its scope to only the proxy routes
by changing the registration to a non-global handler (remove middleware: true)
and instead register the middleware conditionally for the proxy route pattern
(e.g., only mount the proxy-csrf handler for the geocode proxy endpoints) or
wrap the proxy-csrf logic to early-exit unless the request path matches the
proxy route; update the code around geocodeEnabled/addServerHandler and the
proxy-csrf handler to ensure it's applied only to requests targeting the Google
Maps proxy paths.
In `@src/runtime/server/middleware/proxy-csrf.ts`:
- Around line 4-6: The CSRF cookie is being set for every request because the
middleware (registered via middleware: true in module.ts) always calls
setProxyCsrfCookie; update the handler defined by defineEventHandler to first
inspect the incoming request path (e.g., via event.node.req.url or the framework
path helper) and only call setProxyCsrfCookie when the path matches your proxy
routes (for example startsWith('/_scripts/google-') or a configured proxy
prefix), or if you intentionally want global behavior, add a clear comment in
the file/module documenting that choice; reference the existing
defineEventHandler and setProxyCsrfCookie symbols when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ef167c0-21dc-4112-b9cd-034c8f6e0a65
📒 Files selected for processing (6)
docs/content/scripts/google-maps.mdsrc/module.tssrc/runtime/components/GoogleMaps/ScriptGoogleMaps.vuesrc/runtime/server/google-static-maps-proxy.tssrc/runtime/server/middleware/proxy-csrf.tssrc/runtime/server/utils/proxy-csrf.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue
Comment on lines
+60
to
+67
| // Rate limit by IP | ||
| const ip = getRequestIP(event, { xForwardedFor: true }) || 'unknown' | ||
| if (!checkRateLimit(ip)) { | ||
| throw createError({ | ||
| statusCode: 429, | ||
| statusMessage: 'Too many static map requests', | ||
| }) | ||
| } |
There was a problem hiding this comment.
Rate limiting fallback to 'unknown' allows potential bypass.
When getRequestIP returns undefined, all such requests share the single 'unknown' key. In environments where client IPs aren't available (e.g., certain proxy configurations), legitimate users could be collectively rate-limited while attackers could also abuse this shared bucket.
Consider returning a 400 error when the IP cannot be determined, or use additional identifiers (e.g., session token) for rate limiting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/server/google-static-maps-proxy.ts` around lines 60 - 67, The
current fallback to the literal 'unknown' when getRequestIP(event, {
xForwardedFor: true }) returns undefined creates a shared rate-limit bucket;
change the logic in google-static-maps-proxy.ts so you do NOT use a global
'unknown' key: if getRequestIP(...) returns undefined, return a 400 Bad Request
using createError (or alternatively derive a per-request identifier from a
session token/header and pass that to checkRateLimit), and ensure you call
checkRateLimit only with a valid, non-empty identifier. Update references around
getRequestIP, checkRateLimit, and createError accordingly so callers either
receive a 400 when IP is missing or rate-limiting is performed with a
per-request identifier.
Comment on lines
+19
to
+25
| setCookie(event, COOKIE_NAME, token, { | ||
| httpOnly: false, // must be readable by JS for double-submit pattern | ||
| secure: true, | ||
| sameSite: 'strict', | ||
| path: '/', | ||
| maxAge: 86400, | ||
| }) |
There was a problem hiding this comment.
Cookie with secure: true may break local HTTP development.
The secure: true option prevents the cookie from being sent over non-HTTPS connections. During local development on http://localhost, this cookie won't be set, causing CSRF validation to fail.
Consider making this conditional based on the environment:
🔧 Proposed fix for development compatibility
+import { useRuntimeConfig } from '#imports'
+
// Set CSRF cookie during SSR (called from the component's server-side render)
export function setProxyCsrfCookie(event: H3Event): string {
+ const isDev = process.env.NODE_ENV === 'development'
let token = getCookie(event, COOKIE_NAME)
if (!token) {
token = generateToken()
setCookie(event, COOKIE_NAME, token, {
httpOnly: false, // must be readable by JS for double-submit pattern
- secure: true,
+ secure: !isDev,
sameSite: 'strict',
path: '/',
maxAge: 86400,
})
}
return token
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/server/utils/proxy-csrf.ts` around lines 19 - 25, The cookie is
currently set with secure: true which prevents it from being sent over plain
HTTP (breaking local dev on http://localhost); update the setCookie call that
sets COOKIE_NAME with token so secure is conditional (e.g., secure =
process.env.NODE_ENV !== 'development' or based on a runtime isProduction flag)
to allow non-HTTPS local development while keeping secure=true in production,
and keep other options (httpOnly, sameSite, path, maxAge) the same.
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 #83
❓ Type of change
📚 Description
Google Maps Places API geocoding ($5/1k requests) was uncached and exposed API keys to the client. Adds a server-side geocode proxy at
/_scripts/google-maps-geocode-proxywith aggressive HTTP caching (24h default), CSRF token protection (double-submit cookie), IP-based rate limiting, and input validation. Both proxies now auto-enable whengoogleMapsis in the registry config. Also hardens the existing static maps proxy with rate limiting and parameter allowlisting.