fix(security): re-validate every redirect hop in unfurl against SSRF guard (LC-150) #183

Merged

nrupard merged 2 commits from feat/lc-150-unfurl-redirect-ssrf into main 2026-05-22 19:51:28 +02:00

Conversation 0 Commits 2 Files changed 1 +143 -28

nrupard commented 2026-05-22 19:46:46 +02:00

Owner

Copy link

What

Closes the unfurl redirect SSRF from the LC-148 audit (finding S4).

Problem

get_unfurl resolved the initial URL's host and rejected non-globally-routable IPs, then fetched with reqwest::redirect::Policy::limited(3). reqwest follows redirects itself and connects to each target without re-running the host check, so an attacker-controlled page could 302 the fetcher to http://169.254.169.254/ (cloud metadata) or http://127.0.0.1/ and defeat the pre-flight entirely. AuthUser-gated, so any logged-in user could trigger it.

Fix

• Switch to redirect::Policy::none() and follow redirects manually in a loop.
• On every hop (initial URL included): re-check the scheme is http/https (a redirect can jump to file:// etc.) and that the host resolves to only globally-routable addresses, before connecting.
• Relative redirects resolved against the current URL; chain capped at MAX_REDIRECTS = 3 (matches the prior limit).
• Extracted the per-URL SSRF check into host_resolves_public, shared by the initial fetch and each hop.

Residual DNS-rebinding across the resolve-then-connect gap is unchanged (mitigated by the 5s timeout + 1 MiB cap), and now applies per hop rather than only to the first.

Test

Added a unit-test module (file had none): is_globally_routable accept/reject matrix for v4/v6 (private, loopback, link-local, CGNAT, benchmark, reserved, ULA, IPv4-mapped-v6) and host_resolves_public rejecting IP-literal URLs in private ranges incl. 169.254.169.254. IP literals resolve without DNS, so tests are offline-safe. just check / just test / just test-saas green.

Note: a full redirect-to-loopback integration test isn't feasible here because a localhost test server is itself rejected by the guard (first hop); the loop's per-hop call to host_resolves_public is the security invariant and is covered by the predicate tests.

Part of the LC-159 post-audit story.

🤖 Generated with Claude Code

## What Closes the unfurl redirect SSRF from the LC-148 audit (finding S4). ## Problem `get_unfurl` resolved the initial URL's host and rejected non-globally-routable IPs, then fetched with `reqwest::redirect::Policy::limited(3)`. reqwest follows redirects itself and connects to each target **without** re-running the host check, so an attacker-controlled page could `302` the fetcher to `http://169.254.169.254/` (cloud metadata) or `http://127.0.0.1/` and defeat the pre-flight entirely. AuthUser-gated, so any logged-in user could trigger it. ## Fix - Switch to `redirect::Policy::none()` and follow redirects **manually** in a loop. - On **every** hop (initial URL included): re-check the scheme is `http`/`https` (a redirect can jump to `file://` etc.) and that the host resolves to only globally-routable addresses, before connecting. - Relative redirects resolved against the current URL; chain capped at `MAX_REDIRECTS = 3` (matches the prior limit). - Extracted the per-URL SSRF check into `host_resolves_public`, shared by the initial fetch and each hop. Residual DNS-rebinding across the resolve-then-connect gap is unchanged (mitigated by the 5s timeout + 1 MiB cap), and now applies per hop rather than only to the first. ## Test Added a unit-test module (file had none): `is_globally_routable` accept/reject matrix for v4/v6 (private, loopback, link-local, CGNAT, benchmark, reserved, ULA, IPv4-mapped-v6) and `host_resolves_public` rejecting IP-literal URLs in private ranges incl. `169.254.169.254`. IP literals resolve without DNS, so tests are offline-safe. `just check` / `just test` / `just test-saas` green. Note: a full redirect-to-loopback integration test isn't feasible here because a localhost test server is itself rejected by the guard (first hop); the loop's per-hop call to `host_resolves_public` is the security invariant and is covered by the predicate tests. Part of the LC-159 post-audit story. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

nrupard added 1 commit 2026-05-22 19:46:46 +02:00

fix(security): re-validate every redirect hop in unfurl against SSRF guard (LC-150) ... 02f73c1060

The unfurl handler resolved the initial URL's host and rejected non-globally-routable IPs, then fetched with reqwest's redirect::Policy::limited(3). reqwest follows redirects itself, connecting to each target WITHOUT re-running the host check, so an attacker-controlled page could 302 the fetcher to http://169.254.169.254/ (cloud metadata) or http://127.0.0.1/ and defeat the pre-flight entirely (audit S4). The handler is AuthUser-gated, so any logged-in user could trigger it.

Switch to redirect::Policy::none() and follow redirects manually in a loop that, on every hop (initial URL included), re-checks the scheme is http/https and that the host resolves only to globally-routable addresses before connecting. Relative redirects are resolved against the current URL; the chain is capped at MAX_REDIRECTS (3), matching the prior limit. The per-URL SSRF check is extracted into host_resolves_public so the initial fetch and every hop share one implementation.

Adds a unit-test module (the file previously had none): is_globally_routable accept/reject matrix for v4/v6 including private/loopback/link-local/CGNAT/benchmark/reserved/ULA and IPv4-mapped-v6, plus host_resolves_public rejecting IP-literal URLs in private ranges (incl. 169.254.169.254). IP literals resolve without DNS so the tests are offline-safe.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

nrupard added 1 commit 2026-05-22 19:50:07 +02:00

fix(unfurl): bail on empty redirect Location instead of re-fetching self (LC-150) ... ed8e9fdcda

Review follow-up: a 3xx with a missing-or-empty Location header previously fell through to current.join("") (same URL) and re-fetched it until the redirect cap, burning 3 pointless fetches. Trim the Location and treat empty as "nothing to follow" -> empty_preview. Bounded before, just wasteful; this removes the wasted hops. No security change.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

nrupard merged commit 4aa811922b into main 2026-05-22 19:51:28 +02:00

nrupard deleted branch feat/lc-150-unfurl-redirect-ssrf 2026-05-22 19:51:28 +02:00

nrupard referenced this pull request from a commit 2026-05-22 19:51:30 +02:00

Merge pull request 'fix(security): re-validate every redirect hop in unfurl against SSRF guard (LC-150)' (#183) from feat/lc-150-unfurl-redirect-ssrf into main

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!183

No description provided.