Skip to content

fix(webfetch): manual redirect loop with SSRF + cross-host guard - #6

Merged
RowitZou merged 1 commit into
mainfrom
feature/webfetch-redirect-guard
May 19, 2026
Merged

fix(webfetch): manual redirect loop with SSRF + cross-host guard#6
RowitZou merged 1 commit into
mainfrom
feature/webfetch-redirect-guard

Conversation

@RowitZou

Copy link
Copy Markdown
Owner

Problem

web-fetch-http.ts previously set axios `maxRedirects:10` and trusted the adapter to follow any 3xx Location header blindly. The permission card gates only the initial hostname — any redirect could land on:

  • Cloud metadata IPs (`169.254.169.254` AWS / `100.100.100.200` Alibaba) and exfil instance credentials
  • RFC1918 / loopback / link-local addresses behind a corp proxy
  • A different public hostname the user never approved

The tool_result's `finalUrl` showed where the chain ended, but by then the daemon had already issued the request and (for binary downloads) persisted bytes to `.lightclaw/downloads/`. SSRF-like.

What this PR does

Replaces axios's built-in redirect-follow with manual loop + classify-each-hop:

1. Manual redirect loop in web-fetch-http.ts

  • `maxRedirects: 0` + `validateStatus` widened to accept 3xx
  • Per-hop check, max 10 hops, retries 5xx via existing `withWebRetry`
  • New `DaemonFetchResult.redirectChain: string[]` field — captures every visited URL

2. SSRF hard-block (mode-independent)

`classifyRedirectTarget(url)` rejects when the destination's hostname resolves to:

  • IPv4: 0/8, 10/8, 100.64/10 (CGNAT, incl. Alibaba metadata `100.100.100.200`), 127/8 (loopback), 169.254/16 (link-local + AWS/Azure/GCP metadata), 172.16/12, 192.0.0/24, 192.168/16, 198.18/15
  • IPv6: `::1`, fc00::/7 (ULA), fe80::/10 (link-local), IPv4-mapped IPv6 in both dotted and Node's canonical hex form (`::ffff:a9fe:a9fe`)
  • Hostname aliases: `localhost` / `*.localhost`
  • Non-HTTP(S) schemes: file:// / gopher:// / ftp://

Throws `SsrfRedirectError`. Mode-independent — blocked even in `acceptEdits`/auto mode.

3. Strict same-host follow

Any hostname change throws `CrossHostRedirectError` with the would-be target captured in the chain. The model's recovery: re-issue `WebFetch()` directly, which goes through the normal per-hostname permission flow (auto-allowed in auto mode via existing `policy.ts:119`; card in default mode).

4. Permission-card surface is unchanged

The fix lives entirely in the daemon-fetch layer; it never calls `requestPermission()` and never introduces a new card path. Per the user's explicit constraint: auto mode stays card-free — the existing WebFetch auto-allow rule still applies to all hostnames (initial + cross-host re-issues).

Out of scope for V1 (gaps documented in code header)

  • DNS resolution of named hosts to catch internal hostnames that resolve to private IPs. The cross-host rule catches the typical named-internal case because the hostname differs from the user-approved initial host. Defer until proxy-fronted env has a dogfood signal.
  • Same-registrable-domain follow (`docs.foo.com → static.foo.com`) and short-link whitelist (`t.co` / `bit.ly`). Strict same-host is the safest default; relax after dogfood shows it's a real annoyance.
  • DNS rebinding defense via per-hop IP pinning. The static-IP-literal SSRF case (the high-value cloud-metadata vector) is fully covered.

Test plan

  • Existing 308 same-host trailing-slash case rewritten — axios no longer auto-follows; test now drives two stubbed responses and asserts manual loop + chain
  • New cases (20 total):
    • same-host single redirect + multi-hop chain captured
    • cross-host redirect throws `CrossHostRedirectError` + target never fetched (the actual security property)
    • SSRF: AWS metadata / RFC1918 / loopback / Alibaba / localhost / non-HTTP scheme all throw `SsrfRedirectError`
    • max-hops loop throws `RedirectLimitError`
    • 3xx without Location → `AxiosError` surfaced
    • Existing 4xx-not-retried / timeout / 503-retry / socket-reset retry cases all preserved unchanged
    • `classifyRedirectTarget` exhaustive: ordinary public IPs pass; all private ranges block; IPv6 ULA / link-local / IPv4-mapped IPv6 (both forms) / non-HTTP schemes / unparseable URLs
  • `pnpm typecheck` clean
  • `pnpm test` 1442/1442 PASS (1422 baseline + 20 new)

🤖 Generated with Claude Code

Pre-fix daemonFetchUrl set axios `maxRedirects:10` and trusted the
adapter to follow any 3xx Location header blindly. The WebFetch tool's
permission card gates only the **initial** hostname; any subsequent
redirect could land on:
- cloud metadata IP literals (`169.254.169.254` AWS / `100.100.100.200`
  Alibaba / etc.) and read instance credentials,
- RFC1918 / loopback / link-local addresses behind a corp proxy,
- a different public hostname the user never approved.

The tool_result's `finalUrl` showed where the chain ended, but by then
the daemon had already issued the request and (for binary downloads)
persisted bytes to `.lightclaw/downloads/`. SSRF-like.

New behavior:
- **Manual redirect loop**, max 10 hops. axios `maxRedirects:0` +
  `validateStatus` widened to accept 3xx so we see Location ourselves.
- **SSRF hard-block**: every hop's destination URL is classified via
  the new `classifyRedirectTarget`. IP literals in
  127/8 (loopback), 10/8 / 172.16/12 / 192.168/16 (RFC1918), 169.254/16
  (link-local + cloud metadata), 100.64/10 (CGNAT incl. Alibaba),
  198.18/15 (RFC2544), 0/8, 192.0.0/24, plus IPv6 `::1` / fc00::/7 ULA
  / fe80::/10 link-local / IPv4-mapped IPv6 in both dotted and Node's
  canonical hex form, plus `localhost`/`*.localhost` and non-HTTP(S)
  schemes (file:// / gopher:// / ftp://) all throw `SsrfRedirectError`.
  Mode-independent — even in `acceptEdits` (auto) mode these are blocked.
- **Strict same-host follow**: any hostname change (case-insensitive)
  throws `CrossHostRedirectError` with the would-be target captured.
  The model's recovery is to re-issue `WebFetch(<finalTarget>)` directly,
  which goes through normal per-hostname permission gating — auto-allowed
  in `acceptEdits` mode (no card; existing `policy.ts:119` rule), card
  in default mode.
- **Redirect chain captured**: `DaemonFetchResult.redirectChain: string[]`
  records every visited URL. Empty on no-redirect; populated for
  same-host normalizations (HTTPS upgrade, trailing-slash). Error
  envelopes include the chain so the model can see where the redirect
  wanted to go.
- **Max-hops exceeded**: throws `RedirectLimitError`.

Permission card surface is unchanged. The fix lives entirely in the
daemon-fetch layer; it never calls `requestPermission()` and never
introduces a new card path. Auto mode stays card-free per the existing
WebFetch auto-allow rule in `policy.ts:119`.

Out of scope for V1 (gaps documented in code header):
- DNS resolution of named hosts to catch internal hostnames that
  resolve to private IPs. The cross-host rule catches the named-internal
  case because the hostname differs from the user-approved initial host.
- Same-registrable-domain follow (e.g. `docs.foo.com → static.foo.com`)
  and short-link whitelist (`t.co`/`bit.ly`). Strict same-host is the
  safest default; relax after dogfood.
- DNS rebinding defense via per-hop IP pinning. The static-IP-literal
  SSRF case (the high-value cloud-metadata vector) is fully covered.

Tests:
- Existing 308 same-host trailing-slash case rewritten — axios no
  longer auto-follows, so the test now drives two stubbed responses
  and asserts the manual loop walked both URLs + captured chain.
- New cases (16 in two suites):
  - same-host single redirect + multi-hop chain captured
  - cross-host redirect throws CrossHostRedirectError + target never fetched
  - SSRF: AWS metadata / RFC1918 / loopback / Alibaba / localhost /
    non-HTTP scheme all throw SsrfRedirectError
  - max-hops loop throws RedirectLimitError
  - 3xx without Location → AxiosError surfaced
  - existing 4xx-not-retried / timeout / 503-retry / socket-reset retry
    cases all preserved unchanged
  - `classifyRedirectTarget` exhaustive coverage: ordinary public IPs
    pass; cloud metadata / RFC1918 / loopback / CGNAT / benchmarking /
    localhost aliases / IPv6 ULA / IPv6 link-local / IPv4-mapped IPv6
    (both dotted and hex form) / non-HTTP schemes / unparseable URLs
- `pnpm typecheck` clean
- `pnpm test` 1442/1442 PASS (1422 baseline + 20 new)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 19, 2026 11:44
@RowitZou
RowitZou merged commit 31e819a into main May 19, 2026
1 check failed
@RowitZou
RowitZou deleted the feature/webfetch-redirect-guard branch May 19, 2026 11:44
@RowitZou
RowitZou removed the request for review from Copilot May 19, 2026 12:08
RowitZou pushed a commit that referenced this pull request May 25, 2026
… bundled skill (Phase 17 PR2)

Reviewer role prompt is now persona-only: identity, delivery target,
authority limits (no direct fix, one delegated fix max per pass,
ship-not-gated-on-nits, no path/text invention, no balance padding),
and user-language posture.

New bundled skill src/skill/bundled/pre-delivery-review-workflow/SKILL.md
holds the 8-step workflow (read request, memory check, verify standards,
survey artifact, form severity opinion, optional one-pass delegate fix,
write severity-tiered report, end with verdict), output conventions
(verdict/blockers/important/nits/out-of-scope/specific citations/length/
stale-memory acknowledgement), and procedure rule (do not trust memory
blindly). Frontmatter roles: [reviewer] per-role-exclusive.

allowed-tools includes FeishuRead + FeishuList because reviewer is
generic pre-delivery review (code change, written report, organized
data, Feishu doc, sheet, any draft) — not code-only.

Skill name pre-delivery-review-workflow chosen to mirror role identity
("pre-delivery review specialist") and avoid the "review" generic
ambiguity (log review / retrospective).

Role-end Do not still carries authority limits; the "Workflow #6"
parenthetical in "single delegated in-line fix per pass" rewritten to
"(defined in the workflow)" because the Workflow section now lives in
the skill body.

Prompt snapshot hashes updated for reviewer role.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant