Skip to content

[ARCHIVED] Benchmark issue 491 - #491

Closed
guyoron1 wants to merge 2 commits into
mainfrom
lsp-test/490-secure-http-guidance
Closed

[ARCHIVED] Benchmark issue 491#491
guyoron1 wants to merge 2 commits into
mainfrom
lsp-test/490-secure-http-guidance

Conversation

@guyoron1

Copy link
Copy Markdown
Owner

Closes #490

LSP experiment v2: clean first-review with gopls-lsp enabled.
Code identical to PR #421 (issue #376).
Branch created from commit 8d10b3b.

Add a "Secure HTTP clients" subsection under "Go code" documenting
required security properties for outbound HTTP clients. Points
agents to the SSRF-hardened fetch package (internal/fetch/) and
lists the mandatory protections when a custom client is needed:
HTTPS-only, DNS pre-resolution with IP validation via netutil,
IP-pinned DialContext, redirect blocking, explicit timeout, and
response size limiting. Prohibits http.Get and http.DefaultClient
for external URLs from configuration or user input.

Closes #376
- Add Transport.Proxy bullet to custom-client checklist (proxy omission)
- Fix bold formatting to use label-colon pattern for consistency

Addresses review feedback on #421
@guyoron1

Copy link
Copy Markdown
Owner Author

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 13, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:22 AM UTC · Completed 10:29 AM UTC
Commit: 758c511 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review — approve

Reviewed: AGENTS.md (+19/-0) — new "Secure HTTP clients" subsection under "Go code"

Summary

Clean, well-scoped documentation PR that adds SSRF-hardening guidance for Go HTTP clients to AGENTS.md. All eight security properties listed (HTTPS-only, DNS pre-resolution, IP-pinned DialContext, redirect blocking, timeout, body size limit, domain allowlisting, proxy disabling) are technically accurate and verified against the canonical implementations in internal/fetch/fetch.go and internal/netutil/ip.go.

Verification details

Claim Verified against Result
FetchURL exists in internal/fetch/ fetch.go:93
CheckIP / IsInternal exist in internal/netutil/ ip.go:39,70
HTTPS-only fetch.go:104
DNS pre-resolution with IsInternal fetch.go:120-135
IP-pinned DialContext fetch.go:157-169
Redirect blocking fetch.go:178-180
30s default timeout DefaultPolicy at fetch.go:76
10 MB response body limit DefaultPolicy at fetch.go:74
Domain allowlisting fetch.go:114-117
Transport.Proxy = nil (no proxy) fetch.go:156 — zero-value nil, Go docs confirm no proxy when nil

Scope

Matches issue #490 authorization. Placement under ## Go code as a ### subsection is consistent with existing AGENTS.md structure (parallel to ### Running e2e tests). No scope creep.

Findings

No findings at or above the severity threshold.


Reviewed with fullsend-ai 🤖


Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

  • AGENTS.md
Previous run

Review — Approve

PR: #491 — Add AGENTS.md guidance for secure HTTP client construction in Go
Scope: 1 file changed (AGENTS.md), +19 lines, documentation only

Summary

This PR adds a well-scoped "Secure HTTP clients" subsection under the Go code section of AGENTS.md, documenting required security properties for HTTP clients that fetch external URLs. The change traces directly to issue #490 (a retro-action from PR fullsend-ai#3002 where review agents caught missing SSRF protections).

Findings

No blocking findings.

Verified claims against codebase:

  • internal/fetch/fetch.go exports FetchURL — confirmed
  • internal/netutil/ip.go exports CheckIP and IsInternal — confirmed
  • FetchURL implements all listed properties: HTTPS-only, DNS pre-resolution with IP validation via netutil.IsInternal(), IP-pinned DialContext, redirect blocking, 30s timeout, 10 MB body limit via io.LimitReader, domain allowlisting
  • Transport.Proxy guidance is correct — Go's net/http docs confirm: "If Proxy is nil or returns a nil *URL, no proxy is used." A new &http.Transport{} has nil Proxy (no proxy), unlike http.DefaultTransport which sets Proxy: http.ProxyFromEnvironment
  • ✅ Section placement under "Go code" is appropriate, between e2e test guidance and forge abstraction
  • ✅ Formatting matches existing AGENTS.md conventions (H3 heading, bold labels, em-dash bullets, backtick code references)
  • ✅ Scope matches issue [ARCHIVED] Benchmark issue 490 #490 with no creep

Dimensions reviewed

Dimension Result
Correctness ✅ All technical claims verified against source
Security ✅ No injection patterns, no secrets, no prompt injection
Intent & coherence ✅ Traces to #490, scope matches, fits architecture
Style & conventions ✅ Matches existing AGENTS.md formatting
Documentation currency ✅ All referenced identifiers exist
Cross-repo contracts ⏭ Skipped — no exported interfaces modified

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

  • AGENTS.md

Labels: PR adds developer guidance documentation for secure HTTP client patterns

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment documentation Improvements or additions to documentation labels Jul 13, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 13, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:30 AM UTC · Completed 10:36 AM UTC
Commit: fa6bdf7 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jul 13, 2026
@guyoron1 guyoron1 closed this Jul 14, 2026
@guyoron1 guyoron1 changed the title [LSP] [RTK+Ponytail] Add AGENTS.md guidance for secure HTTP client construction in Go [ARCHIVED] Benchmark issue 491 Jul 14, 2026
@github-actions
github-actions Bot deleted the lsp-test/490-secure-http-guidance branch August 16, 2026 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ARCHIVED] Benchmark issue 490

1 participant