fix(desktop): SSRF-guard the self-updater with per-hop redirect validation (LC-210) #276

Merged

longjacksonle merged 1 commit from fix/lc-210-desktop-updater-ssrf-guard into main

2026-05-30 04:52:01 +02:00

longjacksonle commented

2026-05-30 04:50:33 +02:00

Owner

The desktop self-updater fetched LETS_CHAT_UPDATE_URL (manifest + binary download) via ureq with no SSRF guard. The operator sets the initial URL deliberately, so the real risk is redirects: a compromised/MITM'd endpoint, or a DNS/BGP redirect, could send the updater to an internal or attacker host. The cure is per-hop validation.

Investigation (item 0, the gate)


Redirects today	ureq's default (cap ~5), unvalidated, on both update.rs fetches.
ureq version	2.12.1 - exposes a custom `Resolver` → option (a) is viable, does not collapse.
reqwest in desktop	No → option (b) = heavy transitive tree for one feature, re-implementation anyway.
`is_globally_routable`	Pure `std::net`, ~70 lines → trivially portable.
Other ureq sites	`welcome.rs` server probe (out of scope - see below).

ureq follows redirects internally with one agent, so unguarded-initial would mean unguarded-hops - it can't express "operator chose the initial URL, but redirect targets must stay public." Setting .redirects(0) (confirmed from ureq source: a 3xx returns as Ok under redirects=0) and following hops in code resolves all four refinements at once: partial-bypass per-hop policy, explicit per-hop validation, literal-IP independence (every Location is URL-validated before fetching), and the LC-152 two-layer pattern (URL-input validation + a PublicOnlyResolver on the guarded agent for the resolve-then-connect TOCTOU). is_globally_routable is ported verbatim from server/src/ssrf.rs.

Partial-bypass opt-out

LETS_CHAT_UPDATE_URL_ALLOW_PRIVATE=1 exempts only the initial URL from the filter (private internal mirror, or the documented http://127.0.0.1 test fixture). Redirect targets are always validated - a compromised mirror that 302s to 169.254.169.254 is still refused. The operator's deliberate choice is the initial URL, not "follow arbitrary redirects from my mirror to anywhere."

welcome.rs: the deliberate allow-list

The server-reachability probe stays unguarded (single grep-ban allow-list entry, load-bearing inline rationale): it probes LETS_CHAT_SERVER_URL, legitimately a LAN/private IP for self-hosting, and the webview loads that URL regardless, so the probe adds no SSRF surface; guarding it would break LAN deployments. The grep-ban forbids raw ureq construction outside net_guard.rs; a meta-test asserts each allow-listed file still contains a banned token (a refactor moving the raw ureq elsewhere fails the meta-test, not silently expands the guard).

Tests (6, specific-variant assertions - LC-152 property-not-proxy)

classify + literal-IP spike (filter rejects literal private/metadata/CGNAT; literal-IP URL reaches the check);
end-to-end 302 -> 10.0.0.1 refused with NonPublicTarget{Redirect} (URL layer fast-fails before connect = the filter, not network-unreachable);
per-hop firing (2-hop loopback chain records the redirect target 127.0.0.2 was validated);
partial-bypass property (opt-out ON allows private initial but still refuses a redirect to the metadata IP);
5+6. grep-ban + load-bearing meta-test.

Established the desktop test pattern (bin-only crate → #[cfg(test)] modules) and a just test-desktop recipe (desktop tests didn't run before; just test is server-only). CLAUDE.md now records that desktop PRs must run just test-desktop.

Fixed in passing: the update.rs file:// offline-mirror claim (ureq can't do file://; already non-functional) - corrected, no feature work.

What this PR does NOT close

Update-binary integrity. apply() downloads and self_replaces with no signature/checksum verification (the manifest carries no per-artifact hash). A redirect to a public attacker host still serves an unverified binary - a supply-chain/signing concern, separate from SSRF. Recommend a follow-up ticket: manifest signing (Ed25519/GPG over version+URL+hash, also prevents rollback/substitution) over a bare per-artifact sha256. Named so "guarded the updater" isn't misread as "secured the update process."

Validation

just check clean (both server modes + desktop compile + clippy -D warnings + fmt); just test-desktop 6/6. Server code untouched.

🤖 Generated with Claude Code

The desktop self-updater fetched `LETS_CHAT_UPDATE_URL` (manifest + binary download) via `ureq` with no SSRF guard. The operator sets the initial URL deliberately, so the real risk is **redirects**: a compromised/MITM'd endpoint, or a DNS/BGP redirect, could send the updater to an internal or attacker host. The cure is **per-hop validation**. ## Investigation (item 0, the gate) | | | |---|---| | Redirects today | ureq's default (cap ~5), **unvalidated**, on both update.rs fetches. | | ureq version | **2.12.1** - exposes a custom `Resolver` → option (a) is viable, does **not** collapse. | | reqwest in desktop | **No** → option (b) = heavy transitive tree for one feature, re-implementation anyway. | | `is_globally_routable` | Pure `std::net`, ~70 lines → trivially portable. | | Other ureq sites | `welcome.rs` server probe (out of scope - see below). | ## Architecture: manual redirect loop (one mechanism, four refinements) ureq follows redirects internally with one agent, so unguarded-initial would mean unguarded-hops - it can't express "operator chose the initial URL, but redirect targets must stay public." Setting `.redirects(0)` (confirmed from ureq source: a 3xx returns as `Ok` under `redirects=0`) and following hops in code resolves all four refinements at once: partial-bypass per-hop policy, explicit per-hop validation, literal-IP independence (every `Location` is URL-validated before fetching), and the LC-152 two-layer pattern (URL-input validation + a `PublicOnlyResolver` on the guarded agent for the resolve-then-connect TOCTOU). `is_globally_routable` is ported verbatim from `server/src/ssrf.rs`. ## Partial-bypass opt-out `LETS_CHAT_UPDATE_URL_ALLOW_PRIVATE=1` exempts **only the initial URL** from the filter (private internal mirror, or the documented `http://127.0.0.1` test fixture). **Redirect targets are always validated** - a compromised mirror that 302s to `169.254.169.254` is still refused. The operator's deliberate choice is the initial URL, not "follow arbitrary redirects from my mirror to anywhere." ## welcome.rs: the deliberate allow-list The server-reachability probe stays **unguarded** (single grep-ban allow-list entry, load-bearing inline rationale): it probes `LETS_CHAT_SERVER_URL`, legitimately a LAN/private IP for self-hosting, and the webview loads that URL regardless, so the probe adds no SSRF surface; guarding it would break LAN deployments. The grep-ban forbids raw ureq construction outside `net_guard.rs`; a meta-test asserts each allow-listed file still contains a banned token (a refactor moving the raw ureq elsewhere fails the meta-test, not silently expands the guard). ## Tests (6, specific-variant assertions - LC-152 property-not-proxy) 1. classify + literal-IP spike (filter rejects literal private/metadata/CGNAT; literal-IP URL reaches the check); 2. end-to-end `302 -> 10.0.0.1` refused with `NonPublicTarget{Redirect}` (URL layer fast-fails before connect = the filter, not network-unreachable); 3. per-hop firing (2-hop loopback chain records the redirect target `127.0.0.2` was validated); 4. partial-bypass property (opt-out ON allows private initial but still refuses a redirect to the metadata IP); 5+6. grep-ban + load-bearing meta-test. Established the desktop test pattern (bin-only crate → `#[cfg(test)]` modules) and a `just test-desktop` recipe (desktop tests didn't run before; `just test` is server-only). CLAUDE.md now records that desktop PRs must run `just test-desktop`. Fixed in passing: the `update.rs` `file://` offline-mirror claim (ureq can't do `file://`; already non-functional) - corrected, no feature work. ## What this PR does NOT close **Update-binary integrity.** `apply()` downloads and `self_replace`s with **no signature/checksum verification** (the manifest carries no per-artifact hash). A redirect to a *public* attacker host still serves an unverified binary - a supply-chain/signing concern, separate from SSRF. Recommend a follow-up ticket: manifest signing (Ed25519/GPG over version+URL+hash, also prevents rollback/substitution) over a bare per-artifact sha256. Named so "guarded the updater" isn't misread as "secured the update process." ## Validation `just check` clean (both server modes + desktop compile + clippy `-D warnings` + fmt); `just test-desktop` 6/6. Server code untouched. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

longjacksonle added 1 commit

2026-05-30 04:50:33 +02:00

fix(desktop): SSRF-guard the self-updater with per-hop redirect validation (LC-210)

check-secrets / Kingfisher (push) Successful in 4s

Details

check-secrets / Nosey parker (push) Successful in 4s

Details

check-secrets / TruffleHog (push) Successful in 4s

Details

check-secrets / Nosey parker (pull_request) Successful in 6s

Details

check-secrets / TruffleHog (pull_request) Successful in 9s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

check-secrets / Kingfisher (pull_request) Successful in 9s

Details

Check / clippy + fmt + tests (pull_request) Successful in 6m25s

Details

52266b952a

The desktop self-updater fetched LETS_CHAT_UPDATE_URL (manifest + binary download) via ureq with no SSRF guard. Per the ticket, the operator sets the initial URL deliberately, so the meaningful risk is REDIRECTS: a compromised / MITM'd update endpoint, or a DNS/BGP redirect, could send the updater to an internal or attacker-controlled host. The cure is per-hop validation, not initial-URL-only.

Investigation (item 0) decided the cure shape. ureq is 2.12.1 and exposes a custom Resolver (so a ureq-native guard is viable - option (a) does not collapse); reqwest is absent (option (b) would pull hyper/tower into a binary-size-sensitive Tauri app for one feature, and is a re-implementation anyway since there is no cross-crate pub use). The cure is a desktop-local guarded fetch, not a reqwest port.

Architecture: a manual redirect loop. ureq follows redirects internally with one agent, so an unguarded initial fetch would mean unguarded hops - it cannot express "the operator chose the initial URL, but redirect targets must still be public." Setting .redirects(0) (confirmed from ureq source: a 3xx is returned as Ok under redirects=0) and following hops in code resolves all four refinements with one mechanism: partial-bypass gets per-hop policy variation; per-hop validation is explicit (not dependent on ureq's internal redirect resolution); the literal-IP question dissolves (every Location is URL-validated before fetching, independent of whether ureq's resolver sees literal IPs); and the LC-152 two-layer pattern ports cleanly (URL-input validation + a PublicOnlyResolver on the guarded agent for the resolve-then-connect TOCTOU). is_globally_routable is ported verbatim from server/src/ssrf.rs (pure std::net, ~70 lines; duplication of a stable pure function is correct, workspace extraction is out of scope).

Partial-bypass opt-out (LETS_CHAT_UPDATE_URL_ALLOW_PRIVATE=1): exempts ONLY the initial URL from the public-IP filter, for an operator running a private internal update mirror (or a loopback test fixture - the documented http://127.0.0.1 testing workflow now needs it). Redirect targets are ALWAYS validated regardless: a compromised mirror that 302s to 169.254.169.254 is still refused. This is materially better than a full bypass: the operator's deliberate choice is the initial URL, not "follow arbitrary redirects from my mirror to anywhere."

welcome.rs's server-reachability probe stays deliberately UNGUARDED and is the single grep-ban allow-list entry, with load-bearing inline rationale: it probes the operator-configured LETS_CHAT_SERVER_URL, which is legitimately a LAN/private IP for self-hosted deployments, and the webview loads that exact URL regardless, so the probe adds no SSRF surface; guarding it would break every LAN deployment. The grep-ban forbids raw ureq construction in desktop/src/ outside net_guard.rs, and a meta-test asserts each allow-listed file still contains a banned token (so a refactor moving the raw ureq elsewhere fails the meta-test rather than silently expanding the guard over the LAN probe).

Tests (6, all asserting specific variants per the LC-152 property-not-proxy lesson): (1) classify + literal-IP spike - the filter rejects literal private/metadata/CGNAT IPs and a literal-IP URL reaches the check; (2) end-to-end 302->10.0.0.1 refused with NonPublicTarget{Redirect} (the URL layer fast-fails before any connect, so it is the filter, not network-unreachable); (3) per-hop firing - a 2-hop loopback chain records that the redirect target (127.0.0.2) was validated, not just the initial host; (4) partial-bypass property - opt-out ON allows a private initial URL but STILL refuses a redirect to the metadata IP; plus the grep-ban + load-bearing meta-test.

Established the desktop test pattern: the crate is bin-only, so tests are #[cfg(test)] modules in desktop/src/, run via a new `just test-desktop` recipe (desktop tests did not run before - `just test` is server-only). CLAUDE.md now records that desktop PRs must run `just test-desktop`, so desktop changes do not slip past it the way fmt slipped past `just check` on LC-59.

Fixed in passing: the update.rs doc comment claimed file:// offline mirrors are supported; ureq cannot do file://, so the claim was already non-functional - corrected, no offline-mirror feature work.

What this PR does NOT close: update-BINARY-INTEGRITY. apply() downloads and self_replaces with no signature or checksum verification (confirmed - the manifest carries no per-artifact hash). A redirect to a PUBLIC attacker host still serves an unverified binary. That is a supply-chain/signing concern, separate from SSRF, and warrants its own follow-up ticket (recommend manifest signing - Ed25519/GPG over version+URL+hash, which also prevents rollback/substitution - over a bare sha256-per-artifact). Naming it so "guarded the updater" is not misread as "secured the update process."

Validation: `just check` clean (both server modes + desktop compile + clippy -D warnings + fmt); `just test-desktop` 6/6. Server code untouched.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>