Skip to content

fix(dev): answer the proxy guard's DNS lookup in the shape the connector asked for - #1726

Merged
giswqs merged 1 commit into
mainfrom
fix/dev-proxy-dns-lookup-shape
Aug 5, 2026
Merged

fix(dev): answer the proxy guard's DNS lookup in the shape the connector asked for#1726
giswqs merged 1 commit into
mainfrom
fix/dev-proxy-dns-lookup-shape

Conversation

@giswqs

@giswqs giswqs commented Aug 5, 2026

Copy link
Copy Markdown
Member

What was broken

The SSRF guard behind the dev server's __geolibre_*_proxy endpoints pins each connection to a pre-validated address through a custom undici connect.lookup. It queried DNS with all: true (correct: every candidate address must be checked before connecting) but always replied with the three-argument (err, address, family) form.

Node's net.Socket enables autoSelectFamily by default on Node 20+, so it asks for all: true and then reads addresses[0].address off the reply. Handed a string it indexes into that string and throws ERR_INVALID_IP_ADDRESS: undefined.

The visible symptom in npm run dev: every __geolibre_raster_proxy range request returned 502, so STAC / COG imagery never rendered. The browser only reported a generic fetch failure, and the real cause was buried in the terminal as [vite-proxy-guard] upstream fetch blocked or failed.

$ curl -o /dev/null -w '%{http_code}\n' -H 'Range: bytes=0-1023' \
    'http://localhost:5173/__geolibre_raster_proxy?url=https%3A%2F%2Fsentinel-cogs.s3.us-west-2.amazonaws.com%2F...%2FTCI.tif'
502          # before
206          # after

The fix

Reply in whichever shape the caller asked for. Every resolved address is still validated before either reply, so the check-then-connect rebinding window stays closed in both paths.

The lookup is now a named export (guardedLookup) with an injectable resolver, matching how assertResolvedPublicHost is already made testable.

Tests

Three cases in tests/edge-proxy-redirect.test.ts: the array reply when all: true is requested (fails against the old code), the single-address reply when it is not, and a private-address refusal in both shapes.

Verified against the live dev server: the raster proxy goes 502 -> 206 and Sentinel-2 COG tiles render again.

Summary by CodeRabbit

  • Bug Fixes

    • Improved network address validation during DNS resolution.
    • Blocked private or invalid addresses consistently, including when DNS returns multiple results.
    • Preserved secure address pinning and SSRF protections for proxy requests.
  • Tests

    • Added coverage for single-address and multi-address DNS responses.
    • Added tests verifying that private addresses are rejected.

…tor asked for

The SSRF guard behind the dev server's `__geolibre_*_proxy` endpoints pins each
connection to a pre-validated address through a custom undici `connect.lookup`.
It queried DNS with `all: true` (correct: every candidate must be checked) but
always replied with the three-argument `(err, address, family)` form.

Node's `net.Socket` enables `autoSelectFamily` by default on Node 20+, so it
asks for `all: true` and then reads `addresses[0].address` off the reply. Given
a string it indexes into that string and throws
`ERR_INVALID_IP_ADDRESS: undefined`, so every proxied fetch failed. In practice
that meant a 502 on each `__geolibre_raster_proxy` range request and STAC / COG
imagery that never rendered in `npm run dev`, with the real cause buried in the
terminal.

Reply in whichever shape the caller asked for, still validating every resolved
address first, so the rebinding window stays closed either way. The lookup is
now a named export with an injectable resolver so the reply-shape and
private-address paths are covered offline.
Copilot AI lite review requested due to automatic review settings August 5, 2026 23:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9ef49cf7-c747-4144-b0bd-f0703c360cb0

📥 Commits

Reviewing files that changed from the base of the PR and between dd7e3d6 and 7bda201.

📒 Files selected for processing (2)
  • apps/geolibre-desktop/vite-proxy-guard.ts
  • tests/edge-proxy-redirect.test.ts

📝 Walkthrough

Walkthrough

The proxy guard now exposes a reusable guardedLookup function. It validates every DNS result, supports both callback response shapes, maps invalid results to ENOTFOUND, and allows injected resolvers. The dispatcher uses this function, with tests covering valid and private-address responses.

Changes

Guarded DNS lookup

Layer / File(s) Summary
Guarded lookup validation
apps/geolibre-desktop/vite-proxy-guard.ts, tests/edge-proxy-redirect.test.ts
Adds lookup types and guardedLookup. The function validates all resolved addresses, supports single-address and array callbacks, returns ENOTFOUND for failures or blocked addresses, and supports injected resolvers. Tests cover these callback forms and rejection behavior.
Dispatcher integration
apps/geolibre-desktop/vite-proxy-guard.ts
Replaces inline DNS validation in the undici dispatcher with guardedLookup while preserving address pinning and SSRF checks.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

Suggested reviewers: copilot

Poem

A rabbit checks each address twice,
Blocks private paths with careful eyes.
One lookup serves both forms with grace,
The proxy keeps its guarded place.
Tests hop through the DNS trail.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: correcting the proxy guard's DNS lookup response format for the connector.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/dev-proxy-dns-lookup-shape

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed: dependency version conflict. Check your lock file or package.json.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🔍 Cloudflare PR preview

Item Value
Site https://fb13652e.geolibre-preview.pages.dev
Demo app https://fb13652e.geolibre-preview.pages.dev/demo/
Commit 7bda201

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Code review

I reviewed the DNS lookup shape fix in apps/geolibre-desktop/vite-proxy-guard.ts and the new tests in tests/edge-proxy-redirect.test.ts.

Bugs: None found. guardedLookup always resolves with all: true (so every candidate address is validated) but now correctly replies in the array form callback(null, list) when options.all === true and the single-address form callback(null, chosen.address, chosen.family) otherwise — matching the shape Node's net.Socket (with autoSelectFamily enabled by default on Node 20+) actually expects. This directly addresses the ERR_INVALID_IP_ADDRESS: undefined crash described in the PR. Traced through both reply paths and confirmed the private-address check still runs over the full resolved list before either reply is sent, so the check-then-connect rebinding window stays closed in both shapes. Error/no-address paths are unchanged from the prior implementation (just extracted into a fail() helper), so no new behavior there. (High confidence.)

Security: No regressions. The SSRF guard's invariant — never hand the connector an address that wasn't validated — is preserved in both the array and single-address reply paths. isPrivateHost is invoked over every entry in the resolved list regardless of wantsAll. (High confidence.)

Performance: No concerns; this is a low-frequency dev-server code path (per-connection DNS resolution), and the change doesn't add extra resolver calls versus the prior implementation.

Quality: Minor, non-blocking observation: the extracted fail() helper and the DNS-error branch both hardcode family: 4 in the callback regardless of whether the caller asked for the array or single-address shape; this is inherited unchanged from the pre-existing code (not new behavior from this diff) and is harmless since callers only inspect the error argument on failure. Not worth a fix. (Low confidence, cosmetic only.)

CLAUDE.md: No applicable guidelines are implicated by this change (it touches dev-server proxy internals, not any of the areas CLAUDE.md calls out for mirrored constants, i18n, or Tauri CSP).

Overall this is a well-scoped, correctly-reasoned fix with tests that reproduce the original failure (array-reply case) and cover the private-address refusal in both reply shapes.

@giswqs
giswqs merged commit 9f87629 into main Aug 5, 2026
13 checks passed
@giswqs
giswqs deleted the fix/dev-proxy-dns-lookup-shape branch August 5, 2026 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants