Skip to content

docs(#233): add secure HTTP client guidance to AGENTS.md #277

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fullsend-ai-coder wants to merge 2 commits into from
Closed

docs(#233): add secure HTTP client guidance to AGENTS.md #277

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3add372
docs(#233): add secure HTTP client guidance to AGENTS.md
fullsend-ai-coder[bot] Jul 10, 2026
26a222a
fix(#233): clarify IP validation and SSRF validator roles in secure H…
fullsend-ai-coder[bot] Jul 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,20 @@ The e2e tests require GitHub credentials. There are three ways to provide them:

If only `E2E_GITHUB_USERNAME` and a password source are available, `make e2e-test` will automatically generate a session file before running tests. See `make help` for all available targets.

### Secure HTTP clients

Any new `http.Client` that fetches external or user-supplied URLs **must** include all of the following protections:

1. **SSRF-safe dialer.** Use a custom `DialContext` that pre-resolves DNS, validates every resolved IP with `netutil.IsInternal` or `netutil.CheckIP` (`internal/netutil/ip.go`), and dials the validated IP directly (pinning against DNS rebinding). Reject loopback, private (RFC 1918), link-local, and other reserved addresses. Fail closed — if DNS resolution fails or returns no addresses, do not connect.
2. **HTTPS only.** Reject `http://` URLs at parse time. Block redirects entirely (`CheckRedirect` returning `http.ErrUseLastResponse`) or validate that every redirect target also uses HTTPS.
3. **No proxy.** Set `Transport.Proxy = nil` to prevent the client from honoring `HTTP_PROXY`/`HTTPS_PROXY` environment variables, which an attacker could use to redirect traffic.

@fullsend-ai-review fullsend-ai-review Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] technical documentation accuracy

The documentation instructs "Set Transport.Proxy = nil" and claims internal/fetch/fetch.go implements all five properties. While the guidance is correct (Go docs: "If Proxy is nil or returns a nil *URL, no proxy is used"), fetch.go satisfies this property implicitly via zero-value rather than an explicit Proxy: nil assignment. Adding the explicit field would make the canonical implementation self-documenting.

Suggested fix: Add Proxy: nil to the Transport literal in fetch.go, or reword the guidance to clarify that constructing a new http.Transport (rather than cloning http.DefaultTransport) inherently disables proxy usage.

4. **Explicit timeout.** Set `http.Client.Timeout` (30 s default). Never use `http.DefaultClient` or `http.Get` — both have no timeout.
5. **Response size limit.** Wrap the response body with `io.LimitReader` (10 MB default) to prevent memory exhaustion from malicious or unexpectedly large responses.

See `internal/fetch/fetch.go` for the canonical SSRF-hardened HTTP client (`FetchURL` / `FetchPolicy`) that implements all five properties. See `internal/netutil/ip.go` for the `CheckIP` implementation and `internal/security/ssrf.go` for host/IP/scheme validation (note: HTTPS enforcement is in `fetch.go`, not `ssrf.go`).

@fullsend-ai-review fullsend-ai-review Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] incomplete-reference

The parenthetical "(note: HTTPS enforcement is in fetch.go, not ssrf.go)" is factually correct but could be marginally clearer as "HTTPS-only enforcement" since ssrf.go does contain scheme validation logic (permits both http and https while blocking dangerous schemes).

Suggested fix: Optionally change "HTTPS enforcement" to "HTTPS-only enforcement."


**When reviewing PRs:** Flag any new `http.Get`, `http.DefaultClient`, or `http.Client{}` without a custom transport and timeout as a medium-severity finding when the client fetches external URLs.

## Forge abstraction

All git forge operations (GitHub API calls, PR comments, issue creation, workflow dispatch, etc.) **must** go through the `forge.Client` interface defined in `internal/forge/forge.go`. This is a fundamental architectural rule — the codebase supports multiple forges (GitHub, GitLab, Forgejo) and direct coupling to any single forge breaks the abstraction.
Expand Down
Loading