Skip to content

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

Closed
ggallen wants to merge 1 commit into
mainfrom
feat/ssrf-hardened-fetcher-v3
Closed

feat: add internal/fetch package with SSRF-protected HTTP client#1097
ggallen wants to merge 1 commit into
mainfrom
feat/ssrf-hardened-fetcher-v3

Conversation

@ggallen

@ggallen ggallen commented May 18, 2026

Copy link
Copy Markdown
Member

Summary

  • New internal/fetch/ package with SSRF-hardened HTTP client
  • HTTPS-only, domain allowlist, DNS rebinding protection, internal IP rejection, no redirects, size limits
  • Foundation for ADR-0038 universal harness access — no callers yet

Test plan

  • Unit tests for SSRF protections, domain allowlist, IP blocking
  • go test ./internal/fetch/... passes
  • make lint passes

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Site preview

Preview: https://55599278-site.fullsend-ai.workers.dev

Commit: 02956ae06c34390d936b61a2921b8cad9e270260

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Medium

  • [correctness] internal/fetch/fetch_test.go:117,149,167 — Three test functions (TestFetchURL_NoRedirects, TestFetchURL_SizeLimit, TestFetchURL_NonOKStatus) never exercise their claimed behavior. Each hits DNS resolution or internal-IP rejection before reaching the code path under test. Additionally, newTLSTestServer (line 19) is dead code — it creates a TLS test server that FetchURL cannot reach because the server binds to 127.0.0.1, which is rejected as an internal IP.

    The core security primitives are well-tested (TestIsInternalIP, TestFetchURL_WildcardDomain, TestFetchURL_HTTPSOnly, TestFetchURL_DomainAllowlist, TestFetchURL_OfflineMode, TestFetchURL_DoubleEncoding), so the SSRF protections are validated. But the redirect-rejection, size-limit, and non-OK-status code paths have zero effective coverage.

    Remediation: Make the HTTP transport injectable for testing — e.g., accept an optional *http.Transport override in FetchPolicy, or extract the post-DNS-validation fetch logic into a function testable with httptest.NewTLSServer. This lets tests exercise redirect handling, size limits, and status code rejection against a real HTTP server.

Low

  • [correctness] internal/fetch/fetch.go:131 — Each FetchURL call creates a new http.Transport that is never closed. Idle connections won't be reused across calls and may leak file descriptors under sustained use. Add defer transport.CloseIdleConnections() after the response is read. Not urgent at current call frequencies but good hygiene before callers are added.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label May 18, 2026
@ggallen

ggallen commented May 18, 2026

Copy link
Copy Markdown
Member Author

Duplicate of #1096.

@ggallen ggallen closed this May 18, 2026
@github-actions
github-actions Bot deleted the feat/ssrf-hardened-fetcher-v3 branch June 21, 2026 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant