feat: add internal/fetch package with SSRF-protected HTTP client

[ggallen]

Summary

• Adds internal/fetch package implementing an SSRF-hardened HTTP fetcher (ADR-0038 PR 2)
• Enforces HTTPS-only, domain allowlist, DNS pre-resolution with internal IP validation, transport-level IP pinning (DNS rebinding protection), redirect blocking, response size limits, double-encoding rejection, and offline mode
• Includes comprehensive tests covering all security controls and edge cases (22 test cases across 3 test functions)

Design

• FetchPolicy struct controls allowed domains, max size, timeout, and offline mode
• FetchURL performs the full SSRF-safe fetch pipeline: offline check, HTTPS-only, %25 rejection, domain allowlist, DNS resolution, IP validation, pinned dial, no redirects, 200-only, size limit
• isInternalIP normalizes IPv4-mapped IPv6 and checks loopback/private/link-local/unspecified/multicast plus CIDRs for 0.0.0.0/8, 100.64.0.0/10, 198.18.0.0/15
• ComputeSHA256 utility for content integrity verification
• Only stdlib imports

Test plan

• TestFetchURL subtests: HTTPSOnly, DomainAllowlist, WildcardDomain, NoRedirects, SizeLimit, Timeout, OfflineMode, DoubleEncoding, NonOKStatus, Success
• TestIsInternalIP: loopback, RFC1918, link-local, CGNAT, benchmark, unspecified, multicast, IPv4-mapped IPv6, public IPs (22 cases)
• TestComputeSHA256: known-value verification
• go test -race -vet=all ./internal/fetch/... passes
• All pre-commit hooks pass (go vet, gitleaks, trailing whitespace, etc.)

🤖 Generated with Claude Code

[github-actions]

Site preview

Preview: https://0eeb1852-site.fullsend-ai.workers.dev

Commit: 6808d8bb0236edde50d3610c7bf87e90319f048a

[fullsend-ai-review]

Review

Findings

No findings.

Previous run

Review

Findings

Low

• [correctness] internal/fetch/fetch.go:165 — isAllowedDomain compares the lowercased hostname against AllowedDomains entries as-is. If a caller configures AllowedDomains: []string{"GitHub.com"}, the match will silently fail because the hostname is lowered to "github.com" but the pattern is not. Consider lowering each pattern in the comparison (or documenting that entries must be lowercase).
  Remediation: Add pattern = strings.ToLower(pattern) at the top of the loop in isAllowedDomain, or add a doc comment on AllowedDomains requiring lowercase entries.

• [correctness] internal/fetch/fetch_test.go:47 — The comment // Ensure the TLS config can verify the test server's certificate when connecting via IP address. is misleading — the next line sets InsecureSkipVerify = true, which disables TLS verification entirely. The comment should say the opposite: that verification is intentionally skipped because httptest servers use self-signed certs.
  Remediation: Update the comment to: // Skip TLS verification — httptest servers use self-signed certificates.

• [correctness] internal/fetch/fetch.go:127 — Each call to FetchURL creates a new http.Transport that is never explicitly closed. The idle connection pool in the transport will persist until GC. For the intended use case (infrequent resource fetching) this is fine, but if FetchURL is ever called in a loop, it could accumulate idle connections. Consider calling transport.CloseIdleConnections() in a defer, or documenting this as a known limitation.

Previous run (2)

Review

Findings

Medium

• [correctness] internal/fetch/fetch.go:167 — isInternalIP duplicates IP-checking logic from internal/security/ssrf.go:checkIP(). The ADR-0038 implementation plan explicitly calls for reusing or extracting this logic to a shared package (internal/netutil/). The new implementation is more complete (adds IPv4-mapped IPv6 normalization, 0.0.0.0/8, 198.18.0.0/15, pre-parsed CIDRs), but having two divergent implementations creates maintenance risk — IP range additions to one function may be missed in the other.
  Remediation: Extract shared IP-checking to internal/netutil/ (or similar) and have both internal/fetch and internal/security import from it. Backport the improvements (IPv4-mapped IPv6 normalization, pre-parsed CIDRs, benchmark range) to the shared implementation.

Low

• [platform-security] internal/fetch/fetch.go:111 — No port restriction on fetched URLs. https://allowed.com:8443/path passes the domain allowlist check. While the domain allowlist + internal IP validation limits the practical risk, non-standard ports could target internal HTTPS services running on allowed domains. Consider restricting to port 443 or making allowed ports configurable via FetchPolicy.

• [correctness] internal/fetch/fetch.go:148 — Wildcard domain matching *.example.com matches multi-level subdomains (e.g., deep.sub.example.com). This is tested and intentional, but differs from TLS wildcard certificate semantics (which match only one level). If single-level matching is intended, the check should verify no additional dots appear in the matched prefix. If multi-level is intended (current behavior), consider documenting this explicitly in the AllowedDomains field comment.

Previous run (3)

Review

Findings

Info

• [style] internal/fetch/fetch.go / internal/security/ssrf.go — IP validation logic remains duplicated across isInternalIP and checkIP. Both independently check loopback, private, link-local, CGNAT, documentation, and benchmark ranges. Consider extracting shared IP classification to a common internal/netutil package in a follow-up.

No blocking findings. The implementation is a well-structured SSRF-hardened HTTP fetcher with comprehensive security controls (HTTPS-only, domain allowlist, DNS pre-resolution, IP validation, transport-level IP pinning, redirect blocking, size limits, double-encoding rejection) and thorough test coverage (22 IP classification cases, 10 fetch scenarios).

Previous run (4)

Review

Findings

Info

• [style] internal/fetch/fetch.go / internal/security/ssrf.go — IP validation logic remains duplicated across isInternalIP and checkIP. Both independently check loopback, private, link-local, CGNAT, documentation, and benchmark ranges. Consider extracting shared IP classification to a common internal/netutil package in a follow-up.

No blocking findings. All three low-severity findings from the prior review have been addressed in this revision:

1. DialContext now properly forwards the caller's context via net.Dialer{}.DialContext(dialCtx, ...) instead of using net.DialTimeout.
2. The dial loop now iterates all pre-validated IPs for IPv4/IPv6 fallback, matching the plan doc pattern.
3. TEST-NET/documentation CIDRs (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24) have been added to internalCIDRs.

Previous run (5)

Review

Findings

Info

• [style] internal/fetch/fetch.go / internal/security/ssrf.go — IP validation logic remains duplicated across isInternalIP and checkIP. Both independently check loopback, private, link-local, CGNAT, documentation, and benchmark ranges. Consider extracting shared IP classification to a common internal/netutil package in a follow-up.

No blocking findings. All three low-severity findings from the prior review have been addressed in this revision:

1. DialContext now properly forwards the caller's context via net.Dialer{}.DialContext(dialCtx, ...) instead of using net.DialTimeout.
2. The dial loop now iterates all pre-validated IPs for IPv4/IPv6 fallback, matching the plan doc pattern.
3. TEST-NET/documentation CIDRs (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24) have been added to internalCIDRs.

Previous run

Review

Findings

Low

• [correctness] internal/fetch/fetch.go:140 — DialContext closure ignores the context parameter, using net.DialTimeout instead of net.Dialer{}.DialContext(ctx, ...). If the caller cancels the context passed to FetchURL, in-flight dials will not be interrupted until the timeout expires. The plan doc (docs/plans/universal-harness-access.md:714-720) specifies the context-forwarding pattern. The http.Client.Timeout backstops overall request duration, so this is not a blocking issue, but it can cause brief goroutine leaks on cancellation.
  Remediation: Replace net.DialTimeout("tcp", net.JoinHostPort(pinnedIP, port), policy.Timeout) with (&net.Dialer{Timeout: policy.Timeout}).DialContext(ctx, "tcp", net.JoinHostPort(pinnedIP, port)) and accept the ctx parameter instead of discarding it.

• [correctness] internal/fetch/fetch.go:136 — Only the first resolved IP (addrs[0]) is used for connection pinning. The plan doc specifies iterating all validated IPs for IPv4/IPv6 fallback. If the first IP is unreachable but subsequent IPs are reachable, the fetch fails unnecessarily.
  Remediation: Loop through addrs in the DialContext closure, attempting each in sequence, matching the pattern in docs/plans/universal-harness-access.md:716-726.

• [correctness] internal/fetch/fetch.go:188-200 — isInternalIP does not block documentation/TEST-NET ranges (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24) that the existing internal/security/ssrf.go:checkIP (lines 217-223) does block. While unlikely to be exploited in practice, this is an inconsistency between the two IP validation implementations.
  Remediation: Add the three documentation CIDRs to internalCIDRs in init().

Info

• [style] internal/fetch/fetch.go / internal/security/ssrf.go — IP validation logic is duplicated across isInternalIP and checkIP. Both check loopback, private, link-local, CGNAT, and related ranges independently. Consider extracting shared IP classification to a common internal/netutil or similar package in a follow-up.

Previous run (6)

Review

Findings

Info

• [style] internal/fetch/fetch.go / internal/security/ssrf.go — IP validation logic remains duplicated across isInternalIP and checkIP. Both independently check loopback, private, link-local, CGNAT, documentation, and benchmark ranges. Consider extracting shared IP classification to a common internal/netutil package in a follow-up.

No blocking findings. All three low-severity findings from the prior review have been addressed in this revision:

1. DialContext now properly forwards the caller's context via net.Dialer{}.DialContext(dialCtx, ...) instead of using net.DialTimeout.
2. The dial loop now iterates all pre-validated IPs for IPv4/IPv6 fallback, matching the plan doc pattern.
3. TEST-NET/documentation CIDRs (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24) have been added to internalCIDRs.

Previous run (7)

Review

Findings

Low

• [correctness] internal/fetch/fetch.go:140 — DialContext closure ignores the context parameter, using net.DialTimeout instead of net.Dialer{}.DialContext(ctx, ...). If the caller cancels the context passed to FetchURL, in-flight dials will not be interrupted until the timeout expires. The plan doc (docs/plans/universal-harness-access.md:714-720) specifies the context-forwarding pattern. The http.Client.Timeout backstops overall request duration, so this is not a blocking issue, but it can cause brief goroutine leaks on cancellation.
  Remediation: Replace net.DialTimeout("tcp", net.JoinHostPort(pinnedIP, port), policy.Timeout) with (&net.Dialer{Timeout: policy.Timeout}).DialContext(ctx, "tcp", net.JoinHostPort(pinnedIP, port)) and accept the ctx parameter instead of discarding it.

• [correctness] internal/fetch/fetch.go:136 — Only the first resolved IP (addrs[0]) is used for connection pinning. The plan doc specifies iterating all validated IPs for IPv4/IPv6 fallback. If the first IP is unreachable but subsequent IPs are reachable, the fetch fails unnecessarily.
  Remediation: Loop through addrs in the DialContext closure, attempting each in sequence, matching the pattern in docs/plans/universal-harness-access.md:716-726.

• [correctness] internal/fetch/fetch.go:188-200 — isInternalIP does not block documentation/TEST-NET ranges (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24) that the existing internal/security/ssrf.go:checkIP (lines 217-223) does block. While unlikely to be exploited in practice, this is an inconsistency between the two IP validation implementations.
  Remediation: Add the three documentation CIDRs to internalCIDRs in init().

Info

• [style] internal/fetch/fetch.go / internal/security/ssrf.go — IP validation logic is duplicated across isInternalIP and checkIP. Both check loopback, private, link-local, CGNAT, and related ranges independently. Consider extracting shared IP classification to a common internal/netutil or similar package in a follow-up.

[fullsend-ai-review]

Review follow-ups

Created follow-up issues for actionable non-blocking review findings:

• #1193 — IP validation logic remains duplicated across isInternalIP (internal/fetch/fetch.go) and checkIP (internal/security/ssrf.go). Both independently check loopback, private, link-local, CGNAT, documentation, and benchmark ranges. Consider extracting shared IP classification to a common internal/netutil package.

Previous run

Review follow-ups

Created follow-up issues for actionable non-blocking review findings:

• #1127 — IP validation logic remains duplicated across isInternalIP (fetch.go) and checkIP (ssrf.go). Both independently check loopback, private, link-local, CGNAT, documentation, and benchmark ranges. Consider extracting shared IP classification to a common internal/netutil package.

Previous run (2)

Review follow-ups

Warning: follow-up issue creation is capped at 3 per review run; 1 actionable non-blocking finding(s) were not filed.

Created follow-up issues for actionable non-blocking review findings:

• #1100 — DialContext closure ignores context parameter, using net.DialTimeout instead of context-aware net.Dialer.DialContext. Cancelled contexts will not interrupt in-flight dials until timeout expires.
• #1101 — Only first resolved IP used for connection pinning. Plan doc specifies iterating all validated IPs for IPv4/IPv6 fallback.
• #1102 — isInternalIP does not block documentation/TEST-NET IP ranges (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24) that internal/security/ssrf.go checkIP blocks.

[ggallen]

/fs-review