Skip to content

docs(#376): add secure HTTP client guidance to AGENTS.md #421

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(#376): add secure HTTP client guidance to AGENTS.md #421

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3fce2cd
docs(#376): add secure HTTP client guidance to AGENTS.md
fullsend-ai-coder[bot] Jul 12, 2026
8d10b3b
docs(#376): address review feedback on secure HTTP client guidance
fullsend-ai-coder[bot] Jul 12, 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
19 changes: 19 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,25 @@ 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

**Use the `fetch` package:** `internal/fetch/` provides an SSRF-hardened HTTP client (`FetchURL`). New code that fetches external URLs should use it rather than building a custom `http.Client`.

**If you must build a custom client:** these properties are required:

- **HTTPS-only** — reject `http://` URLs.

@fullsend-ai-review fullsend-ai-review Bot Jul 12, 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] completeness

Proxy handling omission: Issue #376 required Transport.Proxy = nil documentation. The PR omits it. The canonical FetchURL is safe (new http.Transport{} defaults Proxy to nil), but custom clients starting from http.DefaultTransport.Clone() would inherit http.ProxyFromEnvironment, enabling proxy-based SSRF bypass.

Suggested fix: Add a bullet to the custom-client checklist: 'Disable proxy — set Transport.Proxy to nil to prevent requests from being routed through HTTP_PROXY/HTTPS_PROXY environment variables.'

@fullsend-ai-review fullsend-ai-review Bot Jul 12, 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] completeness

Port restriction omission: The canonical FetchURL enforces a port allowlist (defaults to 443 only). The custom-client checklist does not mention port validation, allowing custom clients to accept arbitrary ports.

Suggested fix: Add a bullet: 'Port restriction — restrict to port 443 (or an explicit allowlist) to prevent connections on unexpected ports.'

@fullsend-ai-review fullsend-ai-review Bot Jul 12, 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] completeness

Double-encoding rejection omission: FetchURL rejects URLs containing %25 (double-encoded percent signs) as an anti-bypass measure. The custom-client checklist does not mention this protection.

Suggested fix: Add a bullet: 'Double-encoding rejection — reject URLs containing %25 to prevent percent-encoding bypass attacks.'

- **DNS pre-resolution with IP validation** — resolve the hostname and validate the IP using `netutil.IsInternal()` / `netutil.CheckIP()` from `internal/netutil/` to reject loopback, link-local, private, and other reserved addresses.
- **IP-pinned `DialContext`** — use a custom `DialContext` that dials the pre-validated IP to prevent DNS rebinding between resolution and connection.
- **Redirect blocking** — set `CheckRedirect` to block redirects entirely or validate that redirects stay HTTPS-only and pass IP validation.
- **Explicit timeout** — 30 s default; never leave the zero value.
- **Response body limit** — wrap response bodies with `io.LimitReader` (10 MB default).
- **Domain allowlisting** — when the set of valid hosts is known, restrict to that set.
- **Disable proxy** — set `Transport.Proxy` to `nil` to prevent requests from being routed through `HTTP_PROXY`/`HTTPS_PROXY` environment variables.

**Canonical implementation:** `internal/fetch/fetch.go` (`FetchURL`) and `internal/netutil/ip.go` (`CheckIP`, `IsInternal`).

**Prohibited patterns:** Do not use `http.Get`, `http.DefaultClient`, or `&http.Client{}` with no timeout or SSRF protections for any code that contacts URLs from configuration or user input.

## 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