docs(06-04): complete SSRF-guarded fetch helper plan
- Record fetchguard.Fetch dial-time SSRF helper, typed reasons, and TDD commits
This commit is contained in:
@@ -0,0 +1,163 @@
|
||||
---
|
||||
phase: 06-http-routing-auth-groups-and-rate-limiting
|
||||
plan: 04
|
||||
subsystem: api
|
||||
tags: [ssrf, fetchguard, https, dial-control, netip, httptest]
|
||||
|
||||
requires:
|
||||
- phase: 01-framework-kernel-foundation
|
||||
provides: compass.Config Lookup/Open layered YAML
|
||||
provides:
|
||||
- fetchguard.Fetch SSRF-guarded HTTPS GET with typed Reason errors
|
||||
- fetchguard.Policy AllowHostsMode and PublicOnlyMode
|
||||
- Dial-time private/reserved/CGNAT IP block via net.Dialer.Control
|
||||
- Streaming io.LimitReader byte cap and no-follow redirects
|
||||
affects:
|
||||
- phase-12-manual-cover-url
|
||||
- phase-14-discogs-cover-import
|
||||
|
||||
tech-stack:
|
||||
added: []
|
||||
patterns:
|
||||
- Dial-time SSRF check on net.Dialer.Control after Addr.Unmap
|
||||
- Host allow-list is exact or dotted-suffix ("." + allowed), never a raw suffix
|
||||
- http.ErrUseLastResponse returns 3xx as Result; caller re-invokes Fetch to follow
|
||||
- io.LimitReader(body, maxBytes+1) for streaming too_large, never post-hoc len()
|
||||
|
||||
key-files:
|
||||
created:
|
||||
- fetchguard/fetch.go
|
||||
- fetchguard/policy.go
|
||||
- fetchguard/ip.go
|
||||
- fetchguard/fetch_test.go
|
||||
- fetchguard/ip_test.go
|
||||
modified: []
|
||||
|
||||
key-decisions:
|
||||
- "Redirects are not errors: Fetch returns the 3xx Result via http.ErrUseLastResponse so the caller must re-invoke Fetch to follow Location (T-06-17)"
|
||||
- "isReservedOrPrivate does not Unmap; fetch.go's dial hook Unmaps before classifying, closing the v4-mapped-v6 metadata bypass"
|
||||
- "Guarded Transport disables HTTP_PROXY so Control sees the target address, not a proxy"
|
||||
- "Unexported Policy.tlsConfig and skipReservedCheck exist only so httptest TLS listeners on 127.0.0.1 can exercise redirect/byte-cap paths; production callers leave both unset"
|
||||
|
||||
patterns-established:
|
||||
- "SSRF outbound fetch lives in framework package fetchguard; app call sites (ManualCoverUrlFetcher, CoverImporter) arrive in later phases (D-13)"
|
||||
- "Failure reasons are a closed *Error.Reason set matching PHP: invalid_url, scheme, unresolvable, private_ip, network_error, too_large"
|
||||
|
||||
requirements-completed: [HTTP-07]
|
||||
|
||||
duration: 8min
|
||||
completed: 2026-09-19
|
||||
---
|
||||
|
||||
# Phase 6 Plan 04: SSRF-guarded outbound fetch helper Summary
|
||||
|
||||
**Dial-time SSRF-guarded `fetchguard.Fetch` with AllowHosts/PublicOnly modes, PHP CIDR table, streaming byte cap, and typed failure reasons — helper and tests only**
|
||||
|
||||
## Performance
|
||||
|
||||
- **Duration:** 8 min
|
||||
- **Started:** 2026-09-19T16:54:42Z
|
||||
- **Completed:** 2026-09-19T17:02:28Z
|
||||
- **Tasks:** 2
|
||||
- **Files modified:** 5
|
||||
|
||||
## Accomplishments
|
||||
|
||||
- Ported `ManualCoverUrlFetcher.php` `PRIVATE_V4_CIDRS` / `PRIVATE_V6_PREFIXES` onto `netip.Prefix`, including CGNAT `100.64.0.0/10` and cloud-metadata `169.254.0.0/16`, with Unmap documented as the dial-hook caller's job.
|
||||
- Shipped `fetchguard.Fetch`: https-only, host allow-list (exact + dotted-suffix so `evil-discogs.com` cannot match `discogs.com`), always-on private-IP block at `net.Dialer.Control` (closes PHP's admitted DNS-rebinding TOCTOU), no automatic redirects, streaming `io.LimitReader` cap, config defaults 10 MiB / 10s with explicit zero/negative as an error (D-14).
|
||||
- Zero production call sites — no `ManualCoverUrlFetcher` or `CoverImporter` wiring (D-13).
|
||||
|
||||
## Task Commits
|
||||
|
||||
Each task was committed atomically (TDD RED then GREEN):
|
||||
|
||||
1. **Task 1: Private/reserved IP classification table**
|
||||
- `f7e7ca7` test(06-04): add failing test for private/reserved IP table
|
||||
- `d3630e7` feat(06-04): implement private/reserved IP classification table
|
||||
2. **Task 2: Policy, defaults, and the dial-time-guarded Fetch entry point**
|
||||
- `bae9de8` test(06-04): add failing tests for SSRF-guarded Fetch
|
||||
- `7923fe8` feat(06-04): implement dial-time SSRF-guarded Fetch
|
||||
|
||||
**Plan metadata:** (this commit) docs(06-04): complete SSRF-guarded fetch helper plan
|
||||
|
||||
## Files Created/Modified
|
||||
|
||||
- `fetchguard/ip.go` — `isReservedOrPrivate` CIDR table (PHP constants, fail-closed on invalid addr)
|
||||
- `fetchguard/ip_test.go` — table-driven cases including Unmap, CGNAT, metadata, 172.16/12 boundaries
|
||||
- `fetchguard/policy.go` — Mode, Reason, Policy, Error, Defaults, DefaultsFromConfig via Lookup
|
||||
- `fetchguard/fetch.go` — `Fetch` with dial Control, ErrUseLastResponse, LimitReader, typed errors
|
||||
- `fetchguard/fetch_test.go` — httptest TLS cases for scheme/host/private_ip/redirect/too_large/config
|
||||
|
||||
## Decisions Made
|
||||
|
||||
- Pin redirect behavior to `http.ErrUseLastResponse`: a 3xx is a non-error `Result` so following Location requires a new `Fetch` through the same guard (T-06-17).
|
||||
- `isReservedOrPrivate` classifies an already-Unmap()'d `netip.Addr`; the dial hook Unmaps, matching Pitfall 6.
|
||||
- Disable `HTTP_PROXY` on the guarded transport so Control observes the target IP, not a proxy hop.
|
||||
- Unexported `Policy.tlsConfig` / `skipReservedCheck` let tests talk to loopback httptest servers without weakening production checks.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
### Auto-fixed Issues
|
||||
|
||||
**1. [Rule 3 - Blocking] Unexported test-only Policy fields for httptest loopback**
|
||||
- **Found during:** Task 2 (Fetch entry point)
|
||||
- **Issue:** httptest TLS servers bind 127.0.0.1, which the production dial-time private-IP block correctly rejects. Redirect and streaming-cap tests need a real connection to that listener; mocking `http.RoundTripper` would skip the thing under test for private_ip but would also skip LimitReader/CheckRedirect.
|
||||
- **Fix:** Unexported `Policy.tlsConfig` and `skipReservedCheck`, set only from `fetch_test.go` via `withTestLoopback`. Private-IP tests leave both unset and assert `ReasonPrivateIP` against the real Control hook.
|
||||
- **Files modified:** `fetchguard/policy.go`, `fetchguard/fetch.go`, `fetchguard/fetch_test.go`
|
||||
- **Verification:** `go test ./fetchguard/... -race` — private_ip subtests fail closed; redirect/too_large pass against real TLS listeners
|
||||
- **Committed in:** `7923fe8` (Task 2 GREEN)
|
||||
|
||||
**2. [Rule 2 - Missing Critical] Disable HTTP_PROXY on the guarded transport**
|
||||
- **Found during:** Task 2 (Fetch entry point)
|
||||
- **Issue:** Honoring `HTTP_PROXY` would make Control see the proxy address, not the user-supplied target — an SSRF bypass.
|
||||
- **Fix:** Set `Transport.Proxy` to nil (no proxy) on the per-call client.
|
||||
- **Files modified:** `fetchguard/fetch.go`
|
||||
- **Verification:** private_ip tests still hit Control on 127.0.0.1; `go vet ./...` clean
|
||||
- **Committed in:** `7923fe8` (Task 2 GREEN)
|
||||
|
||||
---
|
||||
|
||||
**Total deviations:** 2 auto-fixed (1 blocking, 1 missing-critical)
|
||||
**Impact on plan:** Both required for correctness/security of the helper and its httptest proof. No call sites added. No scope creep.
|
||||
|
||||
## Issues Encountered
|
||||
|
||||
Grok worktree clone had `.git` as a directory and HEAD on `master`. Created `worktree-agent-01a0ba96` from `ecab09c` without rewinding `master`, then committed only on that branch.
|
||||
|
||||
## User Setup Required
|
||||
|
||||
None - no external service configuration required.
|
||||
|
||||
## Next Phase Readiness
|
||||
|
||||
- `fetchguard.Fetch` is ready for Phase 12 `ManualCoverUrlFetcher` (PublicOnlyMode) and Phase 14 Discogs `CoverImporter` (AllowHostsMode) call sites.
|
||||
- Callers must pass an already-resolved `Policy` (or nil cfg to use 10 MiB / 10s). Following a 3xx means a new `Fetch` of Location.
|
||||
- No blockers.
|
||||
|
||||
## TDD Gate Compliance
|
||||
|
||||
Per-task RED then GREEN commits are present:
|
||||
|
||||
1. `f7e7ca7` test(06-04) (RED Task 1)
|
||||
2. `d3630e7` feat(06-04) (GREEN Task 1)
|
||||
3. `bae9de8` test(06-04) (RED Task 2)
|
||||
4. `7923fe8` feat(06-04) (GREEN Task 2)
|
||||
|
||||
## Self-Check: PASSED
|
||||
|
||||
- FOUND: `fetchguard/ip.go`
|
||||
- FOUND: `fetchguard/ip_test.go`
|
||||
- FOUND: `fetchguard/policy.go`
|
||||
- FOUND: `fetchguard/fetch.go`
|
||||
- FOUND: `fetchguard/fetch_test.go`
|
||||
- FOUND: `f7e7ca7`
|
||||
- FOUND: `d3630e7`
|
||||
- FOUND: `bae9de8`
|
||||
- FOUND: `7923fe8`
|
||||
- `go vet ./...` clean
|
||||
- `go test ./fetchguard/... -race` pass
|
||||
- `go test ./... -short` pass
|
||||
|
||||
---
|
||||
*Phase: 06-http-routing-auth-groups-and-rate-limiting*
|
||||
*Completed: 2026-09-19*
|
||||
Reference in New Issue
Block a user