docs(#3024): document secure HTTP client construction for Go

[shairevivo]

What

Adds a Secure HTTP clients section to docs/contributing/go-code.md documenting the SSRF protections required for any Go code that fetches config- or user-supplied URLs, and surfaces the topic in the AGENTS.md "Go Code" row so it's discoverable.

Why

Per #3024: on PR #3002 the code agent co-authored an HTTP fetcher with no timeout, no size limit, http:// accepted, and no SSRF protection against redirects to private IPs. Review agents caught these across two rounds. These are deterministic, well-known patterns that can be codified so they're applied on the first commit.

Deviation from the issue's literal proposal (please note)

The issue proposed documenting how to hand-roll safeDialContext + custom transport for each new client, citing internal/repos/manifest.go as the pattern to copy. While investigating I found internal/fetch/fetch.go — a reusable, SSRF-hardened FetchURL(ctx, url, FetchPolicy) that already implements more than manifest.go's hand-rolled version (HTTPS-only, domain allowlist, DNS pre-resolution + IP validation, DNS-rebinding-safe IP pinning, port restriction, redirect blocking, timeout, size limit).

So the guidance leads with "use internal/fetch.FetchURL" and treats the property checklist as the fallback for when a custom client is genuinely unavoidable. Copy-pasting safeDialContext into every new call site would be the wrong lesson. Both files are cited as references.

Change

• docs/contributing/go-code.md: new ## Secure HTTP clients section (prefer the shared helper; required properties for custom clients: HTTPS incl. redirects, internal-IP rejection via netutil, Proxy = nil, timeout, io.LimitReader).
• AGENTS.md: add "secure HTTP client construction" to the Go Code row's "when to read" list.

Validation

• All referenced files/symbols verified to exist (fetch.FetchURL, fetch.DefaultPolicy, netutil.CheckIP/IsInternal, manifest.go helpers).
• make lint passes.

Closes #3024

🤖 Generated with Claude Code

[github-actions]

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can start a comment with 'qodo' or '@qodo' to chat about any finding

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-code-review]

PR Summary by Qodo

Document SSRF-safe HTTP fetching for Go contributors

📝 Documentation 🕐 Less than 10 minutes

AI Description

• Direct Go contributors to the shared SSRF-hardened URL fetcher.
• Define mandatory protections when a custom HTTP client is unavoidable.
• Surface secure HTTP guidance in the contributor topic index.

Diagram

graph TD
  A(["Go contributor"]) --> B["AGENTS.md"] --> C["Go guide"] --> D["FetchURL helper"]
  C --> E["Custom client checklist"]

Loading

High-Level Assessment

The chosen approach is optimal: direct contributors to the existing hardened abstraction first, then document security properties for exceptional custom clients. A custom-client-only recipe was considered but would duplicate sensitive SSRF defenses and increase inconsistency across call sites.

Files changed (2) +15 / -1

Documentation (2) +15 / -1

AGENTS.mdExpose secure HTTP guidance in the Go topic index +1/-1

Expose secure HTTP guidance in the Go topic index

• Adds secure HTTP client construction to the Go Code row so contributors and agents know when the guide is relevant.

AGENTS.md

go-code.mdDefine SSRF-safe Go HTTP client requirements +14/-0

Define SSRF-safe Go HTTP client requirements

• Adds guidance to prefer 'internal/fetch.FetchURL' for externally supplied URLs. Documents mandatory HTTPS, internal-IP rejection, proxy disabling, timeout, response-size, redirect, and allowlist protections for unavoidable custom clients.

docs/contributing/go-code.md

[github-actions]

Site preview

Preview: https://01b154e4-site.fullsend-ai.workers.dev

Commit: 301d07bb70bc4cd2754859a70bd20c717dbcf5c4

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[waynesun09]

Review-only pass on the new Secure HTTP clients section. Four MEDIUM findings, all inline:

1. The fetch.FetchURL recommendation is missing the constraints that decide when it is actually usable — and the doc's own second worked example falls outside them.
2. The pre-resolve-and-pin dial advice conflicts with the redirect allowance three bullets later.
3. The unscoped "must" checklist contradicts accepted in-repo code (internal/cli/fetchskill.go).
4. The IP-pinning bullet omits the SNI / certificate-verification caveat that makes it safe to follow.

No blocking verdict — the section is a good addition, and every finding is a wording or scoping fix within it.

[ralphbean]

/fs-fix address findings from @waynesun09

This is a good change. Just needs a few fixups.

[rh-hemartin]

/fs-fix address feedback, rebase and resolve conflicts.

[shairevivo]

Thanks @waynesun09 — all four are fair and verified against head. Fixed in 6d7e362:

1. FetchURL envelope + why the second example can't use it. Added an explicit envelope: non-empty AllowedDomains is mandatory (empty rejects everything via isAllowedDomain), GET/200-only, no custom headers, buffered body, port 443. Expanded the fallback list with host set not knowable up front, authenticated requests, non-GET methods, and non-200 handling — and stated that manifest.go's LoadManifest falls in that first bucket, which is why fetchManifestURL deliberately doesn't use FetchURL. The domain allowlist now reads as required for FetchURL.

2. Pre-resolve-and-pin vs. redirects. Rewrote the IP bullet to require per-connection validation inside DialContext (split the addr, resolve, check each IP, dial the validated IP), the way safeDialContext does. Added that FetchURL's single up-front resolution is safe only because it blocks redirects (http.ErrUseLastResponse) and must not be copied into a redirect-following client, and that an allowed hop must re-check both scheme and IP.

3. Unscoped must vs. fetchskill.go. Added a scope paragraph: the rules apply to clients whose target URL comes from untrusted config/input/remote content; a fixed first-party endpoint from sandbox/runner bootstrap (the loopback FULLSEND_FETCH_URL service in internal/cli/fetchskill.go) is out of scope for the SSRF hardening, though it should still set a timeout and bound the body.

4. SNI/cert with IP-pinned dial. Added that only the dial address changes — the request URL (and thus SNI + cert verification) keeps the original hostname; build the request from the original URL as FetchURL does, and never rewrite the URL to the IP or use InsecureSkipVerify.

Also extended the size bullet to cover json.NewDecoder, not just io.ReadAll.

[shairevivo]

@rh-hemartin can you merge?

[waynesun09]

Second review-only pass, scoped to the rewritten Secure HTTP clients section at head (36bee79). The round-1 findings were addressed; these four are defects in the new text, verified against the code at this SHA.

1. The new rationale for per-connection validation inverts the DNS-rebinding defense (IP pinning is the defense, not the risk) — and contradicts its own exemplar, which pins too.
2. The scope carve-out's "loopback fetch service" is not what the code does: the fetch server binds 0.0.0.0 per ADR 0046.
3. The carve-out asserts its exemplar bounds the response body; fetchskill.go decodes an unbounded body, which line 342 explicitly forbids.
4. The named "worked example to copy" does not enforce the section's first mandatory control (HTTPS-only on the initial request) — that gate lives in its caller.

No blocking verdict; all four are wording/scoping fixes inside the new section. Since this doc is meant to be applied mechanically by review agents, the self-contradictions in (2)-(4) and the inverted rationale in (1) are worth correcting before it lands.

[shairevivo]

Addressed all four round-two findings in 165db4d:

1. Reframed per-connection validation so dialing the exact validated IP is correctly identified as the DNS-rebinding defense; the limitation of one-time pinning is now scoped to redirects to different hosts.
2. Corrected the fetch service description: it is runner-injected through reserved keys, bearer-token authenticated, bound to 0.0.0.0 per ADR 0046, and reached over a private address.
3. Explicitly noted that fetchskill.go currently decodes an unbounded response body and that this gap is not an example to copy.
4. Expanded the worked pattern to include LoadManifest’s initial HTTPS-only gate alongside fetchManifestURL/safeDialContext.

make lint passes.

[shairevivo]

@waynesun09 i think it is ready for merge