fix(workers): harden Vite proxy and github-raw redirect fetches - #1633
Conversation
Block private/metadata targets (and redirect pivots) on the Vite binary proxies, and fetch github-raw through the allowlisted manual redirect helper.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds SSRF protection for desktop binary proxies and tile-worker GitHub raw-file requests. It validates public URLs and DNS results, checks every redirect, limits redirects and response bodies, and routes supported proxy paths through guarded helpers. ChangesUpstream request protection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ViteProxy
participant fetchWithGuard
participant DNSResolver
participant UpstreamHTTPServer
Client->>ViteProxy: Request WMS, WFS, GPX, or raster resource
ViteProxy->>fetchWithGuard: Validate target and fetch
fetchWithGuard->>DNSResolver: Resolve and validate host
DNSResolver-->>fetchWithGuard: Return validated public address
fetchWithGuard->>UpstreamHTTPServer: Request validated URL
UpstreamHTTPServer-->>fetchWithGuard: Return response or redirect
fetchWithGuard->>fetchWithGuard: Validate redirect destination
fetchWithGuard-->>ViteProxy: Return guarded response
ViteProxy-->>Client: Return bounded proxy response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
🔍 Cloudflare PR preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@apps/geolibre-desktop/vite-proxy-guard.ts`:
- Around line 114-119: The IPv6 link-local check only blocks the fe80 prefix
instead of the full fe80::/10 range. In the guard at
apps/geolibre-desktop/vite-proxy-guard.ts lines 114-119, replace the redundant
prefix checks with a numeric first-hextet check covering 0xfe80 through 0xfebf;
in tests/edge-proxy-redirect.test.ts lines 289-295, add assertions for
http://[fe90::1] and http://[febf::1].
- Around line 36-43: Update fetchWithGuard to resolve non-literal hostnames
using dns.promises.lookup(host, { all: true }), validate every returned address
with isPrivateHost before fetching, and pin the request connection to a
validated address to prevent DNS rebinding between validation and connection.
Keep the existing IPv6 normalization and blocked-address response behavior, and
update the module documentation only if the resolution guarantee cannot be
implemented.
- Around line 126-142: Update fetchWithGuard to apply a per-hop timeout by
passing an AbortSignal.timeout(...) signal in the fetch init, while allowing an
existing caller-provided init.signal to override the default. Preserve the
manual redirect handling and ensure the timeout is applied independently to each
redirect hop.
- Around line 185-193: Update the proxy response handling around contentType and
response.arrayBuffer so it rejects advertised content-length values above
PROXY_MAX_BODY_BYTES before reading. Replace whole-body buffering with
incremental response.body chunk reads, track the running byte total, abort or
reject with the existing 502 size-limit response as soon as the total exceeds
the cap, and only construct the Buffer after the stream completes within the
limit.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 0c013cd5-b4c0-4c1c-b71c-6ddf43293233
📒 Files selected for processing (5)
apps/geolibre-desktop/vite-proxy-guard.tsapps/geolibre-desktop/vite.config.tstests/edge-proxy-redirect.test.tsworkers/tiles/src/allowlisted-fetch.tsworkers/tiles/src/index.ts
🔍 GitHub Pages PR preview
|
|
/claude-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| function isPrivateHost(host: string): boolean { | ||
| // Loopback names | ||
| if (host === "localhost" || host.endsWith(".localhost")) return true; | ||
|
|
||
| // IPv4 checks | ||
| const ipv4Parts = host.split("."); | ||
| if (ipv4Parts.length === 4 && ipv4Parts.every((p) => /^\d{1,3}$/.test(p))) { | ||
| const octets = ipv4Parts.map(Number); | ||
| if (octets.some((o) => o > 255)) return false; // not a valid IPv4, let DNS decide | ||
| return isPrivateIPv4(octets); | ||
| } | ||
|
|
||
| // IPv6 checks (bare, already stripped of brackets) | ||
| if (host.includes(":")) { | ||
| return isPrivateIPv6(host); | ||
| } | ||
|
|
||
| // DNS names like "metadata.google.internal" — block if they resolve to a | ||
| // known cloud metadata hostname pattern (the actual resolution to 169.254.* | ||
| // is caught by IPv4 checks when the caller resolves, but we block the name | ||
| // too for defence in depth). | ||
| if (host === "metadata.google.internal") return true; | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Security (medium-high confidence): the guard doesn't close DNS-rebinding SSRF for hostnames.
isPrivateHost only recognizes literal IP addresses and two hardcoded names (localhost, metadata.google.internal). Any other DNS name — e.g. attacker-controlled.example — passes both this check and assertPublicHttpUrl even if it actually resolves to 169.254.169.254 or 127.0.0.1. fetch() then performs the real DNS lookup and connects straight to that address, since Node's fetch doesn't expose a hook to pin/re-check the resolved IP against this allowlist.
This matters here specifically because the whole point of this PR is blocking access to internal/metadata targets from a proxy that takes attacker-influenceable URLs (WMS/WFS/GPX/raster layer URLs). A classic DNS-rebinding domain defeats the guard entirely, both on the initial request and on each redirect hop re-validation (line 138), because both only ever inspect the literal hostname string, never the connected-to IP.
Fully closing this requires resolving the hostname yourself (e.g. via dns.lookup) and either validating the resolved address before connecting or passing a custom lookup/dispatcher to fetch that re-checks the IP at connect time. Might be acceptable as a known limitation for a local dev-server tool, but worth calling out explicitly (in the comment/docs) rather than leaving the impression that "metadata" targets are fully blocked.
| const contentType = response.headers.get("content-type") ?? "application/octet-stream"; | ||
| const buf = await response.arrayBuffer(); | ||
| if (buf.byteLength > PROXY_MAX_BODY_BYTES) { | ||
| res.statusCode = 502; | ||
| res.setHeader("content-type", "text/plain"); | ||
| res.end("Upstream response exceeds size limit"); | ||
| return; | ||
| } | ||
| const body = Buffer.from(buf); |
There was a problem hiding this comment.
Performance/DoS (medium confidence): the body-size cap is checked after the full response is already buffered into memory.
response.arrayBuffer() reads and buffers the entire upstream body before buf.byteLength > PROXY_MAX_BODY_BYTES is checked. A malicious or misbehaving upstream serving a very large (or effectively unbounded, e.g. chunked-forever) response will exhaust memory on the dev machine before the 50 MB cap ever has a chance to reject it — the cap only prevents forwarding an oversized body to the client, not the memory blow-up from fetching it.
Consider checking content-length up front when present, and/or streaming the body while counting bytes and aborting once PROXY_MAX_BODY_BYTES is exceeded (e.g. via the response's body reader or a size-limited transform), instead of buffering unconditionally with arrayBuffer().
| if (a === 100 && b >= 64 && b <= 127) return true; // 100.64.0.0/10 CGNAT | ||
| if (a === 192 && b === 0 && octets[2] === 0) return true; // 192.0.0.0/24 IETF protocol | ||
| if (a === 198 && (b === 18 || b === 19)) return true; // 198.18.0.0/15 benchmarking | ||
| if (a === 198 && b === 51 && octets[2] === 100) return true; // 198.51.100.0/24 documentation | ||
| if (a === 203 && b === 0 && octets[2] === 113) return true; // 203.0.113.0/24 documentation |
There was a problem hiding this comment.
Quality/nit (low confidence): 192.0.2.0/24 (TEST-NET-1) is missing.
The other two IPv4 documentation ranges are blocked (198.51.100.0/24, 203.0.113.0/24), but 192.0.2.0/24 isn't. These are non-routable so real-world exploitability is low, but it's an easy asymmetry to close for completeness:
| if (a === 100 && b >= 64 && b <= 127) return true; // 100.64.0.0/10 CGNAT | |
| if (a === 192 && b === 0 && octets[2] === 0) return true; // 192.0.0.0/24 IETF protocol | |
| if (a === 198 && (b === 18 || b === 19)) return true; // 198.18.0.0/15 benchmarking | |
| if (a === 198 && b === 51 && octets[2] === 100) return true; // 198.51.100.0/24 documentation | |
| if (a === 203 && b === 0 && octets[2] === 113) return true; // 203.0.113.0/24 documentation | |
| if (a === 192 && b === 0 && octets[2] === 0) return true; // 192.0.0.0/24 IETF protocol | |
| if (a === 192 && b === 0 && octets[2] === 2) return true; // 192.0.2.0/24 documentation (TEST-NET-1) | |
| if (a === 198 && (b === 18 || b === 19)) return true; // 198.18.0.0/15 benchmarking | |
| if (a === 198 && b === 51 && octets[2] === 100) return true; // 198.51.100.0/24 documentation | |
| if (a === 203 && b === 0 && octets[2] === 113) return true; // 203.0.113.0/24 documentation |
Code reviewSecurity
Performance
Quality
CLAUDE.md
|
|
@Ayush7614 Please resolve the review comments for all the PRs. Thanks. |
Resolve and pin DNS via an undici lookup that rejects private addresses, cover the full fe80::/10 range, bound each hop with a timeout, and enforce the body limit while streaming instead of after arrayBuffer.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/geolibre-desktop/vite-proxy-guard.ts (1)
350-366: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winThe handler returns internal guard errors to the client.
Line 353 and line 364 write
err.messageinto the response body.assertResolvedPublicHostbuilds messages such asBlocked private/reserved address: internal.example → 10.0.0.5, and the dispatcher lookup builds the same form at line 206. A caller can therefore submit hostnames and read back the resolved internal addresses. Network errors from undici also carry the target address and port.This turns the SSRF guard into an internal-network disclosure oracle. Log the detail on the server and return a fixed message to the client.
🛡️ Proposed fix to stop leaking guard detail
let response: Response; try { response = await fetchWithGuard(target, { headers }); } catch (err) { + console.warn("[vite-proxy-guard] upstream fetch blocked or failed:", err); res.statusCode = 502; res.setHeader("content-type", "text/plain"); - res.end(err instanceof Error ? err.message : "Upstream fetch failed"); + res.end("Upstream fetch failed"); return; } const contentType = response.headers.get("content-type") ?? "application/octet-stream"; let body: Buffer; try { body = await readBodyWithLimit(response); } catch (err) { + console.warn("[vite-proxy-guard] upstream body rejected:", err); res.statusCode = 502; res.setHeader("content-type", "text/plain"); - res.end(err instanceof Error ? err.message : "Upstream response exceeds size limit"); + res.end("Upstream response exceeds size limit"); return; }Note: line 339 returns
urlErrfromvalidatePublicUrl. Those messages describe only the client-supplied URL, so they disclose nothing new and can stay.🤖 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 `@apps/geolibre-desktop/vite-proxy-guard.ts` around lines 350 - 366, Stop returning internal error details from the upstream fetch and read-body catch blocks in the proxy handler. Log each caught error server-side, then return fixed generic 502 messages instead of err.message; preserve the existing urlErr response from validatePublicUrl and keep distinct messages for fetch failure versus response-size failure.
🤖 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 `@apps/geolibre-desktop/vite-proxy-guard.ts`:
- Around line 154-174: Remove the redundant DNS resolution performed by
assertResolvedPublicHost in the fetchWithGuard flow, since guardedDispatcher’s
lookup is the authoritative validation and connection-pinning step. Preserve the
dispatcher’s private/reserved-address checks and clear error behavior, and leave
assertResolvedPublicHost unchanged if it remains used elsewhere.
- Around line 183-191: The lookup wrapper in lookup must force all-address
results by spreading options before all: true and verbatim: true, then remove
the hardcoded non-array fallback and consume the returned address array
directly. At apps/geolibre-desktop/vite-proxy-guard.ts#L183-L191, apply this
lookup change; at apps/geolibre-desktop/vite-proxy-guard.ts#L215-L216, preserve
chosen.family as the actual resolved family and verify the callback shape
remains accepted by undici and net.connect.
- Around line 253-268: Update fetchWithGuard so providing fetchImpl no longer
skips assertResolvedPublicHost or the guarded dispatcher; introduce an explicit
skipDnsCheck option and use it only to bypass the DNS resolution check for
tests. Preserve fetchImpl solely for fetch injection, and update the affected
edge-proxy redirect tests to pass skipDnsCheck: true.
- Around line 89-93: Update the out-of-range branch in isPrivateHost’s
IPv4-literal handling to fail closed by returning the private/rejected
classification rather than false when any octet exceeds 255. Preserve the
existing isPrivateIPv4(octets) path for valid octets and the surrounding
hostname checks.
In `@tests/edge-proxy-redirect.test.ts`:
- Around line 256-259: Update the non-default-port test around validatePublicUrl
to assert the exact rejection message for the port rule rather than merely
checking for a non-null result. Apply this to both HTTPS and HTTP cases,
preserving the 8.8.8.8 address so the test distinguishes port rejection from
private-address classification.
- Around line 341-350: Update assertResolvedPublicHost to accept an optional
resolver parameter, defaulting to the existing DNS lookup implementation, and
use it in the hostname-resolution branch. Extend the tests to inject offline
stubs covering a hostname resolving to a private address and a hostname
resolving to no addresses, while preserving the existing IP-literal behavior.
- Around line 415-429: Update the redirect test’s imports to include the
exported PROXY_MAX_REDIRECT_HOPS constant, then replace the hardcoded hops
assertion and its comment in the “caps redirect hops” test with an expectation
derived as PROXY_MAX_REDIRECT_HOPS + 1.
---
Outside diff comments:
In `@apps/geolibre-desktop/vite-proxy-guard.ts`:
- Around line 350-366: Stop returning internal error details from the upstream
fetch and read-body catch blocks in the proxy handler. Log each caught error
server-side, then return fixed generic 502 messages instead of err.message;
preserve the existing urlErr response from validatePublicUrl and keep distinct
messages for fetch failure versus response-size failure.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: dea5e663-44bb-4397-ba90-f52f8a29ef6a
📒 Files selected for processing (2)
apps/geolibre-desktop/vite-proxy-guard.tstests/edge-proxy-redirect.test.ts
Add undici as a desktop dependency so Docker production builds resolve the DNS-pinning agent, fail closed on bad IPv4 literals, stop leaking guard errors to clients, and cover DNS-resolution branches in tests.
fetchImpl no longer skips assertResolvedPublicHost, so injecting a custom fetch cannot bypass the DNS-rebinding guard.
|
cc: @giswqs |
- Move undici from dependencies to devDependencies in apps/geolibre-desktop/package.json (with a refreshed lockfile). It is reachable only from vite.config.ts and tests/edge-proxy-redirect.ts, both dev-server paths, so it should not ship as a runtime dependency.
2 participants
Summary
__geolibre_*_proxyhelpers against private/metadata targets and redirect pivots (manual hops + re-validate)./github-rawthroughfetchAllowlistedUpstreamwithraw.githubusercontent.comon the prefix allowlist.Test plan
node --import tsx --test tests/edge-proxy-redirect.test.tsnpm run devurl=is refusedSummary by CodeRabbit