fix(security): close Web Push SSRF + LC-152 TOCTOU via shared http_client #246

Merged

longjacksonle merged 4 commits from feat/lc-152-shared-http-client-no-bypass into main 2026-05-28 14:18:00 +02:00

Conversation 0 Commits 4 Files changed 13 +986 -157

longjacksonle commented 2026-05-28 14:14:53 +02:00

Owner

Copy link

⚠️ Security fix — Web Push SSRF reachable in shipped versions

Live in shipped versions: a malicious user could register a Web Push subscription whose endpoint points at an internal host or cloud-metadata endpoint, and lets-chat would POST encrypted notification payloads there on every mention / DM that arrives for that user.

The most concrete vector is http://169.254.169.254/latest/meta-data/ — the AWS / GCP / Azure instance-metadata endpoint. On a cloud-hosted deployment, repeated POSTs to that endpoint can be used to (a) confirm the deployment IS cloud-hosted, (b) attempt credential exfiltration via mis-configured metadata service interactions, (c) hit internal admin services exposed on private addresses. The encrypted payload body is opaque to the attacker (encrypted under the subscriber's own keys), but the side effects of the POST and the response timing / status leak are not.

Root cause: push/mod.rs::ReqwestPushClient constructed a raw reqwest::Client::new() with no SSRF guard. Nothing in the codebase prevented this; the audit that motivated this PR found it precisely because nothing prevented it.

🔴 BLOCKING question — release handling

Live user-controllable SSRF-to-cloud-metadata in shipped versions is the severity class where the "next release vs. backported security release" question must be asked and answered deliberately, not defaulted to next-release. Does this warrant a backported security point-release + CVE, or does the next feature release suffice?

Inputs to the decision:

• Severity: high (user-controllable destination, no auth required to register a push sub, cloud-metadata vector documented).
• Exploit complexity: low (any logged-in user can register a push subscription via the standard pushManager.subscribe() JS API and choose any endpoint).
• Observability: low (the request looks like normal Web Push delivery; only an outbound-traffic audit would catch it).
• Affected versions: every shipped version that has Web Push (i.e., since LC-147 landed).

Secondary fix — LC-152 hostname use-time TOCTOU

The originally-scoped fix. ssrf::host_resolves_public was called before .send() on every guarded path (LC-75 delivery, slash, unfurl, avatar fetch), but reqwest then performed its OWN DNS resolution between the check and the connect. A rapid-flip DNS record could resolve public at the check and private at reqwest's resolve. Closed by routing every outbound call through a custom reqwest::dns::Resolve (PublicOnlyResolver) that filters inside reqwest's own resolution path — same resolution, no second resolve.

The PublicOnlyResolver alone is not sufficient: reqwest 0.12's connector takes a literal-IP fast path that skips dns::Resolve entirely. A daemon- or user-supplied URL with a literal private IP (e.g. http://127.0.0.1/, http://10.0.0.1/, or the metadata endpoint) slips past the resolver. The killer test pair surfaced this; the fix is a second layer — URL-input validation in the helper's public API — that runs BEFORE reqwest sees the URL.

What ships

New module server/src/http_client.rs — the only public API for outbound HTTP from this server:

pub async fn outbound_get(url: &str) -> Result<RequestBuilder, OutboundError>
pub async fn outbound_post(url: &str) -> Result<RequestBuilder, OutboundError>
pub async fn outbound_get_following_redirects(url: &str) -> Result<RequestBuilder, OutboundError>

pub enum OutboundError { InvalidUrl(_), UnsupportedScheme(_), HostNotPublic }

#[doc(hidden)] pub fn outbound_unchecked() -> reqwest::Client  // test seam; grep-banned in src/

The underlying reqwest::Clients are private to the module. No server/src/ file can obtain a Client to call .get() / .post() on; the verb-problem in the grep-ban (banning .get() would have massive false positives from HashMap::get etc.) disappears structurally.

Two layers of SSRF guard, each closing what the other can't:

1. URL-input validation in outbound_* async fns. Parses + scheme check + ssrf::host_resolves_public. lookup_host returns the IP itself for literal-IP URLs, so this layer catches the literal-IP fast-path bypass that reqwest's dns::Resolve does not see.
2. PublicOnlyResolver inside reqwest's resolution path. For hostnames, filters every resolution to publicly-routable IPs. Same resolution as reqwest's connect, no second resolve. Closes the hostname use-time TOCTOU.

Removing either layer reopens the corresponding attack class.

Grep-ban test (tests/lc152_no_raw_reqwest_in_src.rs) — structural no-bypass enforcement. Walks server/src/, fails the build on reqwest::Client::new / reqwest::Client::builder / reqwest::ClientBuilder / reqwest::get( / reqwest::blocking:: / outbound_unchecked. Exception list is the FULL PATH src/http_client.rs (NOT a basename match — a future src/foo/http_client.rs would otherwise silently inherit the exemption). Sibling sanity test allow_list_actually_finds_the_allowed_file proves the exception list isn't a defanged no-op.

5 migrations to the new API:

Site	Before	After
outgoing.rs (LC-75)	Client::builder() + per-delivery host_resolves_public pre-check	run_delivery_tick(chat) takes no client; per-delivery outbound_post(&t.url) with mark_failed("blocked: non-public URL") on rejection
routes/unfurl.rs	Per-hop pre-check + manual Client::builder()	Per-hop outbound_get(url); empty_preview() on any OutboundError
routes/slash.rs	Pre-check + per-dispatch Client::builder()	outbound_post(url) with BadRequest("URL is not allowed") on rejection
routes/api.rs::post_bridge_message (LC-78 avatar fetch spawn)	Per-task Client::builder(), passed to fetch_and_cache	bridge_avatar::fetch_and_cache(&chat, &hash, &url) — no client
push/mod.rs::ReqwestPushClient (THE UNGUARDED ONE)	Held reqwest::Client::new() field, no SSRF check anywhere	Holds NO client field. Every send() calls outbound_post(&url). Hardwired to the helper by design; future refactors can't inject around the filter.

Killer test suite — every rejection asserts the SPECIFIC variant

The lesson from the prior round was: bare is_err() is a false-positive trap because connect-failed / network-unreachable also satisfy it. Every rejection test in tests/lc152_resolver_property_pair.rs asserts matches!(_, Err(OutboundError::HostNotPublic)) (or the matching variant), proving the FILTER caught the URL, not the network. Plus a timing bound (< 3s) on rejection_is_fast_no_tcp_attempt proves the URL-input layer rejects BEFORE any TCP connect (a connect timeout would also produce HostNotPublic-shaped error if the resolver path were the only mechanism — the timing assertion guards against that regression). 8 tests, all green.

Test plan

• cargo check clean in standalone + saas.
• just test clean (123 binaries, 0 failed).
• just test-saas — one timing-dependent flake in retention_sweep::message_exactly_at_cutoff_survives_strict_less_than under concurrent-binary load; passes in isolation; unrelated to LC-152.
• Grep-ban (lc152_no_raw_reqwest_in_src) — 2/2 pass. Full-path exception list.
• Killer pair revised (lc152_resolver_property_pair) — 8/8 pass. Destination-arrival contract; HostNotPublic variant on literal loopback, literal RFC 1918, the metadata endpoint, literal-IP POST; UnsupportedScheme on file://; InvalidUrl on malformed; fast-fail < 3 s.
• Per-site error propagation (lc152_per_site_error_propagation) — 4/4 pass. LC-75 marks-failed terminal not retried; bridge avatar fetch marks failed without panic (private IP + non-http scheme); Web Push send returns Err on the 169.254.169.254 vector without crashing the loop.

Deferred — desktop ureq self-updater

Filed as a follow-up. The desktop crate (separate binary) uses ureq to fetch LETS_CHAT_UPDATE_URL for the self-update flow. Same TOCTOU class but narrower exploit surface: the URL is operator-configured at deploy time, not user-supplied. The real concern there is redirect-following (does the updater follow an attacker-controlled redirect to internal hosts), not the initial operator-set URL. Lower urgency than the server-side work; queued behind the LC-152 server fix.

Why this can't be split

• The literal-IP layer alone leaves the hostname-TOCTOU open.
• The PublicOnlyResolver alone leaves the literal-IP fast path open.
• Migrating only the four pre-LC-152 guarded sites leaves Push unguarded.
• The grep-ban without the migrations would catch existing violations; without the grep-ban the migrations rely on future-author discipline (the audit found exactly that posture's failure mode).

The 5 migrations + the 2-layer helper + the exception-free grep-ban + the variant-asserting killer suite are one transaction.

## ⚠️ Security fix — Web Push SSRF reachable in shipped versions > **Live in shipped versions**: a malicious user could register a Web Push subscription whose `endpoint` points at an internal host or cloud-metadata endpoint, and lets-chat would POST encrypted notification payloads there on every mention / DM that arrives for that user. > > The most concrete vector is **`http://169.254.169.254/latest/meta-data/`** — the AWS / GCP / Azure instance-metadata endpoint. On a cloud-hosted deployment, repeated POSTs to that endpoint can be used to (a) confirm the deployment IS cloud-hosted, (b) attempt credential exfiltration via mis-configured metadata service interactions, (c) hit internal admin services exposed on private addresses. The encrypted payload body is opaque to the attacker (encrypted under the subscriber's own keys), but the **side effects** of the POST and the response timing / status leak are not. > > Root cause: `push/mod.rs::ReqwestPushClient` constructed a raw `reqwest::Client::new()` with no SSRF guard. Nothing in the codebase prevented this; the audit that motivated this PR found it precisely because nothing prevented it. ### 🔴 BLOCKING question — release handling Live user-controllable SSRF-to-cloud-metadata in shipped versions is the severity class where the "next release vs. backported security release" question must be **asked and answered deliberately**, not defaulted to next-release. **Does this warrant a backported security point-release + CVE, or does the next feature release suffice?** Inputs to the decision: - Severity: high (user-controllable destination, no auth required to register a push sub, cloud-metadata vector documented). - Exploit complexity: low (any logged-in user can register a push subscription via the standard `pushManager.subscribe()` JS API and choose any endpoint). - Observability: low (the request looks like normal Web Push delivery; only an outbound-traffic audit would catch it). - Affected versions: every shipped version that has Web Push (i.e., since LC-147 landed). ## Secondary fix — LC-152 hostname use-time TOCTOU The originally-scoped fix. `ssrf::host_resolves_public` was called before `.send()` on every guarded path (LC-75 delivery, slash, unfurl, avatar fetch), but reqwest then performed its OWN DNS resolution between the check and the connect. A rapid-flip DNS record could resolve public at the check and private at reqwest's resolve. **Closed by routing every outbound call through a custom `reqwest::dns::Resolve` (`PublicOnlyResolver`) that filters inside reqwest's own resolution path — same resolution, no second resolve.** The PublicOnlyResolver alone is **not sufficient**: reqwest 0.12's connector takes a literal-IP fast path that skips `dns::Resolve` entirely. A daemon- or user-supplied URL with a literal private IP (e.g. `http://127.0.0.1/`, `http://10.0.0.1/`, or the metadata endpoint) slips past the resolver. The killer test pair surfaced this; the fix is **a second layer** — URL-input validation in the helper's public API — that runs BEFORE reqwest sees the URL. ## What ships **New module `server/src/http_client.rs`** — the only public API for outbound HTTP from this server: ```rust pub async fn outbound_get(url: &str) -> Result<RequestBuilder, OutboundError> pub async fn outbound_post(url: &str) -> Result<RequestBuilder, OutboundError> pub async fn outbound_get_following_redirects(url: &str) -> Result<RequestBuilder, OutboundError> pub enum OutboundError { InvalidUrl(_), UnsupportedScheme(_), HostNotPublic } #[doc(hidden)] pub fn outbound_unchecked() -> reqwest::Client // test seam; grep-banned in src/ ``` The underlying `reqwest::Client`s are **private** to the module. No `server/src/` file can obtain a `Client` to call `.get()` / `.post()` on; the verb-problem in the grep-ban (banning `.get()` would have massive false positives from `HashMap::get` etc.) disappears structurally. **Two layers of SSRF guard, each closing what the other can't:** 1. **URL-input validation** in `outbound_*` async fns. Parses + scheme check + `ssrf::host_resolves_public`. `lookup_host` returns the IP itself for literal-IP URLs, so this layer catches the literal-IP fast-path bypass that reqwest's `dns::Resolve` does not see. 2. **`PublicOnlyResolver`** inside reqwest's resolution path. For hostnames, filters every resolution to publicly-routable IPs. Same resolution as reqwest's connect, no second resolve. Closes the hostname use-time TOCTOU. Removing either layer reopens the corresponding attack class. **Grep-ban test (`tests/lc152_no_raw_reqwest_in_src.rs`)** — structural no-bypass enforcement. Walks `server/src/`, fails the build on `reqwest::Client::new` / `reqwest::Client::builder` / `reqwest::ClientBuilder` / `reqwest::get(` / `reqwest::blocking::` / `outbound_unchecked`. Exception list is the **FULL PATH** `src/http_client.rs` (NOT a basename match — a future `src/foo/http_client.rs` would otherwise silently inherit the exemption). Sibling sanity test `allow_list_actually_finds_the_allowed_file` proves the exception list isn't a defanged no-op. **5 migrations to the new API:** | Site | Before | After | |---|---|---| | `outgoing.rs` (LC-75) | `Client::builder()` + per-delivery `host_resolves_public` pre-check | `run_delivery_tick(chat)` takes no client; per-delivery `outbound_post(&t.url)` with `mark_failed("blocked: non-public URL")` on rejection | | `routes/unfurl.rs` | Per-hop pre-check + manual `Client::builder()` | Per-hop `outbound_get(url)`; `empty_preview()` on any `OutboundError` | | `routes/slash.rs` | Pre-check + per-dispatch `Client::builder()` | `outbound_post(url)` with `BadRequest("URL is not allowed")` on rejection | | `routes/api.rs::post_bridge_message` (LC-78 avatar fetch spawn) | Per-task `Client::builder()`, passed to `fetch_and_cache` | `bridge_avatar::fetch_and_cache(&chat, &hash, &url)` — no client | | `push/mod.rs::ReqwestPushClient` (THE UNGUARDED ONE) | Held `reqwest::Client::new()` field, no SSRF check anywhere | Holds NO client field. Every `send()` calls `outbound_post(&url)`. Hardwired to the helper by design; future refactors can't inject around the filter. | ## Killer test suite — every rejection asserts the SPECIFIC variant The lesson from the prior round was: bare `is_err()` is a false-positive trap because connect-failed / network-unreachable also satisfy it. Every rejection test in `tests/lc152_resolver_property_pair.rs` asserts `matches!(_, Err(OutboundError::HostNotPublic))` (or the matching variant), proving the FILTER caught the URL, not the network. Plus a timing bound (`< 3s`) on `rejection_is_fast_no_tcp_attempt` proves the URL-input layer rejects BEFORE any TCP connect (a connect timeout would also produce `HostNotPublic`-shaped error if the resolver path were the only mechanism — the timing assertion guards against that regression). 8 tests, all green. ## Test plan - [x] `cargo check` clean in standalone + saas. - [x] `just test` clean (123 binaries, 0 failed). - [x] `just test-saas` — one timing-dependent flake in `retention_sweep::message_exactly_at_cutoff_survives_strict_less_than` under concurrent-binary load; passes in isolation; unrelated to LC-152. - [x] **Grep-ban** (`lc152_no_raw_reqwest_in_src`) — 2/2 pass. Full-path exception list. - [x] **Killer pair revised** (`lc152_resolver_property_pair`) — 8/8 pass. Destination-arrival contract; HostNotPublic variant on literal loopback, literal RFC 1918, the metadata endpoint, literal-IP POST; UnsupportedScheme on `file://`; InvalidUrl on malformed; fast-fail < 3 s. - [x] **Per-site error propagation** (`lc152_per_site_error_propagation`) — 4/4 pass. LC-75 marks-failed terminal not retried; bridge avatar fetch marks failed without panic (private IP + non-http scheme); Web Push send returns Err on the 169.254.169.254 vector without crashing the loop. ## Deferred — desktop `ureq` self-updater Filed as a follow-up. The desktop crate (separate binary) uses `ureq` to fetch `LETS_CHAT_UPDATE_URL` for the self-update flow. Same TOCTOU class but **narrower exploit surface**: the URL is operator-configured at deploy time, not user-supplied. The real concern there is redirect-following (does the updater follow an attacker-controlled redirect to internal hosts), not the initial operator-set URL. Lower urgency than the server-side work; queued behind the LC-152 server fix. ## Why this can't be split - The literal-IP layer alone leaves the hostname-TOCTOU open. - The PublicOnlyResolver alone leaves the literal-IP fast path open. - Migrating only the four pre-LC-152 guarded sites leaves Push unguarded. - The grep-ban without the migrations would catch existing violations; without the grep-ban the migrations rely on future-author discipline (the audit found exactly that posture's failure mode). The 5 migrations + the 2-layer helper + the exception-free grep-ban + the variant-asserting killer suite are one transaction.

longjacksonle added 4 commits 2026-05-28 14:14:54 +02:00

helper + custom Resolve + OnceLock client 503055b3ae

migrate 5 outbound sites bd6887b4c8

elper + migrated sites for revie aabe4cde94

killer pair + grep-ban + per-site error propagation ... 80fdda1602

Full-path exception in the grep-ban (basename match would have silently
exempted a future server/src/foo/http_client.rs). Killer suite asserts
on the specific OutboundError::HostNotPublic variant (not bare is_err()
which is satisfied by a connect-failed false positive) plus a fast-fail
timing bound proving the URL-input layer rejects BEFORE any TCP connect.

Per-site error-propagation tests cover the high-blast-radius sites:
LC-75 outgoing webhook marks failed terminal not retried, bridge avatar
fetch marks_failed without panic on both private-IP and non-http(s)
schemes, Web Push send() returns Err (not panic) on the 169.254.169.254
metadata vector. Unfurl and slash route paths are mechanically simpler
(existing match arms catch the Err) and are documented inline.

longjacksonle merged commit 4074068cd1 into main 2026-05-28 14:18:00 +02:00

longjacksonle deleted branch feat/lc-152-shared-http-client-no-bypass 2026-05-28 14:18:00 +02:00

longjacksonle referenced this pull request from a commit 2026-05-28 14:18:02 +02:00

Merge pull request 'fix(security): close Web Push SSRF + LC-152 TOCTOU via shared http_client' (#246) from feat/lc-152-shared-http-client-no-bypass into main

nrupard referenced this pull request from a commit 2026-05-28 16:21:01 +02:00

style: rustfmt the LC-78 / LC-152 bridge files (LC-187)

nrupard referenced this pull request 2026-05-28 16:21:14 +02:00

style: rustfmt the LC-78/LC-152 bridge files (LC-187) #250

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

psa-systems/lets-chat!246

No description provided.