fix(workers): stop edge proxies from following off-origin redirects - #1576
Merged
giswqs merged 4 commits intoJul 31, 2026
Merged
Conversation
The viewer worker now follows redirects only while they stay under https://geolibre.app/demo, and the tiles worker fetches allowlisted upstreams with the same host-bound redirect policy so neither path can be turned into an open proxy.
|
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 selected for processing (5)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughViewer and tiles workers now enforce HTTPS upstream allowlists, bounded manual redirects, path validation, method restrictions, credential stripping, and sanitized responses. Tiles routes use the shared fetch helper, with tests covering traversal, origin, redirect, method, and hop-limit behavior. ChangesEdge proxy security
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ViewerRequest
participant proxyViewerRequest
participant ViewerUpstream
ViewerRequest->>proxyViewerRequest: request path and method
proxyViewerRequest->>ViewerUpstream: sanitized HTTPS request
ViewerUpstream-->>proxyViewerRequest: response or redirect Location
proxyViewerRequest-->>ViewerRequest: sanitized response or 4xx/502
sequenceDiagram
participant TilesRoute
participant fetchAllowlistedUpstream
participant TilesUpstream
TilesRoute->>fetchAllowlistedUpstream: upstream URL and request options
fetchAllowlistedUpstream->>TilesUpstream: manual HTTPS fetch
TilesUpstream-->>fetchAllowlistedUpstream: response or redirect Location
fetchAllowlistedUpstream-->>TilesRoute: final response or allowlist error
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Ayush7614
force-pushed
the
harden/viewer-and-edge-proxy-safety
branch
from
July 30, 2026 16:29
f6bbdd6 to
b97ae44
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workers/tiles/src/index.ts (1)
259-281: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnhandled probe error aborts the entire latest-build lookback.
fetchAllowlistedUpstreamcan now throw (disallowed redirect, hop-limit exceeded) where the previous plainfetchwould not, since defaultfetchsilently follows redirects. Because the call at Line 272 isn't wrapped in its own try/catch, a throw for any single day's probe aborts the wholeforloop, andhandlePmtilesRange's catch turns that into a hard "No recent Protomaps build available" 502 — even if other days in the lookback window (including today) are otherwise resolvable. This turns a per-day miss into a full-feature outage.🛡️ Proposed fix: treat a failed probe as a miss, not a hard failure
for (let i = 0; i <= LATEST_MAX_LOOKBACK_DAYS; i++) { const ymd = utcYmd(now - i * 86_400_000); // A one-byte range is the cheapest existence check. ... - const probe = await fetchAllowlistedUpstream(`${PMTILES_UPSTREAM}/${ymd}.pmtiles`, { - headers: { range: "bytes=0-0" }, - }); + let probe: Response; + try { + probe = await fetchAllowlistedUpstream(`${PMTILES_UPSTREAM}/${ymd}.pmtiles`, { + headers: { range: "bytes=0-0" }, + }); + } catch (err) { + console.warn(`Protomaps build probe failed for ${ymd}: ${String(err)}`); + continue; + } if (probe.status === 206) { latestCache = { date: ymd, at: now }; return ymd; } }🤖 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 `@workers/tiles/src/index.ts` around lines 259 - 281, Wrap the per-day fetchAllowlistedUpstream call in resolveLatestBuildDate with a try/catch so errors from an individual date’s probe are treated as misses and the lookback loop continues. Preserve the existing 206 success path, allowing later dates to resolve the latest build; only throw the existing “no recent Protomaps build found” error after all dates fail.
🤖 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 `@workers/tiles/src/allowlisted-fetch.ts`:
- Around line 38-64: Extract the duplicated manual redirect-validation loop from
fetchAllowlistedUpstream into a shared helper parameterized by the allowlist
predicate, then have workers/tiles/src/allowlisted-fetch.ts lines 38-64 and
workers/viewer/src/proxy.ts lines 70-124 use it while preserving their
worker-specific request logic. In tests/edge-proxy-redirect.test.ts lines
88-127, add a tiles-side redirect-hop-cap case mirroring the viewer coverage and
verify TILES_MAX_REDIRECT_HOPS is enforced.
---
Outside diff comments:
In `@workers/tiles/src/index.ts`:
- Around line 259-281: Wrap the per-day fetchAllowlistedUpstream call in
resolveLatestBuildDate with a try/catch so errors from an individual date’s
probe are treated as misses and the lookback loop continues. Preserve the
existing 206 success path, allowing later dates to resolve the latest build;
only throw the existing “no recent Protomaps build found” error after all dates
fail.
🪄 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: bf5983d6-585b-4503-bbf2-d7f10ad5e90d
📒 Files selected for processing (5)
tests/edge-proxy-redirect.test.tsworkers/tiles/src/allowlisted-fetch.tsworkers/tiles/src/index.tsworkers/viewer/src/index.tsworkers/viewer/src/proxy.ts
Member
|
/claude-review |
Contributor
🔍 Cloudflare PR preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
|
All four inline comments posted successfully. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Contributor
🔍 GitHub Pages PR preview
|
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 `@tests/edge-proxy-redirect.test.ts`:
- Around line 35-65: Extend the “ok” branch of the viewer redirect policy test
to return set-cookie and/or set-cookie2 headers from the mock upstream response,
then assert those headers are absent from the final ok response. Keep the
existing redirect assertions unchanged and ensure the test covers
sanitizeUpstreamResponse’s cookie-stripping behavior.
In `@workers/tiles/src/allowlisted-fetch.ts`:
- Around line 7-15: Update isAllowedTilesUpstreamUrl to validate both the
allowlisted S3 hostname and the approved dataset path prefix before following
redirects; do not permit arbitrary bucket/object paths on shared
s3*.amazonaws.com hosts. Reuse the resolved DATASETS prefix for OPM datasets
where available, while preserving existing allowlist behavior for non-S3
upstream hosts.
In `@workers/viewer/src/proxy.ts`:
- Around line 96-124: Update the redirect handling in the proxy flow to treat
304 Not Modified as a non-redirect response, allowing it to pass through
sanitizeUpstreamResponse(response) like other non-redirect statuses. Keep
301–303 and 307–308 responses subject to Location validation and redirect-hop
processing.
- Around line 20-28: Update sanitizeViewerPath to reject both lowercase and
uppercase encoded slashes (%2f and %2F) alongside the existing encoded-dot and
backslash checks. Preserve the current literal .. segment validation and return
behavior; if normalization is introduced, perform it before checking traversal
segments.
🪄 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: 4841c3e8-5599-4bab-ac1f-99f4f859656e
📒 Files selected for processing (5)
tests/edge-proxy-redirect.test.tsworkers/tiles/src/allowlisted-fetch.tsworkers/tiles/src/index.tsworkers/viewer/src/index.tsworkers/viewer/src/proxy.ts
Pass through 304 responses, reject encoded-slash traversal, scope S3 redirects to known dataset prefixes, harden Protomaps probes, and expand redirect/cookie coverage.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/edge-proxy-redirect.test.ts (1)
143-178: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a tiles-side 304 regression test.
The tiles helper now explicitly passes through 304 responses, but this suite only locks that behavior down for the viewer. A future broad
3xxredirect check infetchAllowlistedUpstreamwould pass these 200/302 tests while breaking conditional revalidation again. Add a 304 mock and assert itsetagis returned.As per coding guidelines, test changes must maintain the configured coverage floors.
🤖 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 `@tests/edge-proxy-redirect.test.ts` around lines 143 - 178, Add a tiles-side regression test in the existing fetchAllowlistedUpstream test coverage that mocks a 304 response with an ETag and asserts the response remains 304 with the ETag preserved. Ensure the test exercises the tiles helper path rather than only the viewer behavior, and keep the added coverage within the configured floors.Source: Coding guidelines
workers/viewer/src/proxy.ts (1)
45-52: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winReject alternate ports in allowed redirect URLs.
isAllowedUpstreamUrl()checksparsed.hostname, so allowed redirects can include non-default ports such ashttps://geolibre.app:8443/demo/.... Cloudflare Workers ignores non-default ports in outbound fetches, which can cause the redirect path to be routed differently than intended. Returnfalsewhenparsed.portis present, and add a regression case for alternate ports.🤖 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 `@workers/viewer/src/proxy.ts` around lines 45 - 52, Update isAllowedUpstreamUrl() to reject URLs when parsed.port is non-empty, while preserving the existing HTTPS, hostname, and /demo path checks. Add a regression test covering an allowed hostname with an alternate port and assert it returns false.
🤖 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.
Outside diff comments:
In `@tests/edge-proxy-redirect.test.ts`:
- Around line 143-178: Add a tiles-side regression test in the existing
fetchAllowlistedUpstream test coverage that mocks a 304 response with an ETag
and asserts the response remains 304 with the ETag preserved. Ensure the test
exercises the tiles helper path rather than only the viewer behavior, and keep
the added coverage within the configured floors.
In `@workers/viewer/src/proxy.ts`:
- Around line 45-52: Update isAllowedUpstreamUrl() to reject URLs when
parsed.port is non-empty, while preserving the existing HTTPS, hostname, and
/demo path checks. Add a regression test covering an allowed hostname with an
alternate port and assert it returns false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aabc5faf-e0de-49eb-9f16-c15b07ac0eb7
📒 Files selected for processing (5)
tests/edge-proxy-redirect.test.tsworkers/tiles/src/allowlisted-fetch.tsworkers/tiles/src/index.tsworkers/viewer/src/index.tsworkers/viewer/src/proxy.ts
💤 Files with no reviewable changes (1)
- workers/viewer/src/index.ts
Block non-default ports on viewer redirect allowlist and lock tiles conditional-revalidation passthrough with an ETag regression test.
Ayush7614
force-pushed
the
harden/viewer-and-edge-proxy-safety
branch
from
July 30, 2026 20:44
5494045 to
67baa7e
Compare
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
2 participants
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.
Summary
https://geolibre.app/demo, refuse path traversal, and allow GET/HEAD only.Test plan
node --import tsx --test tests/edge-proxy-redirect.test.ts(6 passing)npm run test:workertypechecks all workershttps://evil.examplereturns 502 instead of proxying the bodySummary by CodeRabbit
set-cookieheaders.304 Not Modifiedpass-through (including ETag) instead of treating it as a redirect issue.