Skip to content

Suppress the gosec G704 SSRF false positive in Client.Do - #49

Merged
korya merged 1 commit into
masterfrom
korya-fix-gosec-g704
Aug 7, 2026
Merged

Suppress the gosec G704 SSRF false positive in Client.Do#49
korya merged 1 commit into
masterfrom
korya-fix-gosec-g704

Conversation

@korya

@korya korya commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Problem

just pre-commit fails on a clean tree: gosec's new G704 rule flags the tool's own outbound request as SSRF. Nothing in the repo changed — gosec 2.28.0 introduced the rule, and both the Justfile and CI install gosec@latest, so the failure appeared overnight and blocks every commit and every CI run until it is addressed.

[/Users/dmitri/dev/http-assert/main.go:296] - G704 (CWE-918): SSRF via taint analysis (Confidence: HIGH, Severity: HIGH)
    295: 	startedAt := time.Now()
  > 296: 	res, err := c.getHttpClient().Do(req)
    297: 	if err != nil {

error: recipe `security` failed on line 81 with exit code 1

The taint analysis is correct about the data flow — args[0] does reach http.Client.Do. It is wrong about the conclusion, for reasons the analyzer cannot see (below).

Solution

Annotate the call site with #nosec G704 and a written justification, rather than excluding the rule globally.

Before / after — the security recipe:

$ just pre-commit

  Issues : 1
error: recipe `security` failed on line 81 with exit code 1
$ echo $?
1
$ just pre-commit

  Nosec  : 2
  Issues : 0
$ echo $?
0

Why this is a false positive, not a vulnerability

SSRF requires a trust boundary: an attacker supplies a URL to a service that holds network reach the attacker lacks. Neither half holds here.

SSRF precondition This tool
URL originates from an untrusted party It is argv[1] of a CLI the operator runs
Requester has privileges the supplier lacks Same user, same shell, same machine
Response can pivot to further hosts Redirects are hard-disabled (CheckRedirect returns ErrUseLastResponse)

Anyone who can pass that argument can already run curl. Fetching an operator-supplied URL is not a side effect of this program — it is the program.

Why annotate instead of gosec -exclude=G704

A global exclusion in the Justfile would silence the rule for code that does not yet exist. If a future change fetches a URL derived from a response body or a config file — where the trust boundary is real — a global exclusion hides it, while the call-site annotation leaves the rule armed everywhere else. The repo already uses this pattern for G402.

Not in scope

This does not pin the gosec version. Justfile and .github/workflows/build.yml both install gosec@latest (and golangci-lint at version: latest), so the next upstream release can break the pipeline again with no change on our side. Pinning both and letting dependabot bump them converts "CI broke overnight" into a reviewable PR — worth doing, but it is a build-config change with a different blast radius than this one-line annotation, so it belongs in its own PR.

No visual change — this repo is a headless CLI with no rendered UI.

Other Changes

Corrects a typo in the adjacent G402 annotation (askefasked), on the sibling line of the same kind.

🤖 Generated with Claude Code

gosec 2.28.0 added rule G704 (SSRF via taint analysis), which traces the URL from argv into
c.getHttpClient().Do(req) and fails `just pre-commit` on an otherwise unchanged tree.

The finding is a false positive by construction. SSRF requires an attacker to supply a URL to a
service holding network reach the attacker lacks. Here the URL is a command-line argument of a CLI
the operator runs themselves, so no trust boundary is crossed -- anyone who can pass that argument
can already run curl. Disabling redirects (CheckRedirect returns ErrUseLastResponse) further keeps
a response from steering the request somewhere unintended.

Previously `just pre-commit` exited 1 on the security recipe. Now it exits 0 via a call-site
annotation rather than a global -exclude, so G704 still applies to any future call site.

Out of scope: pinning the gosec version. Both the Justfile and CI install gosec@latest, so a future
release can break the pipeline again with no code change on our side.

Also corrects a typo in the adjacent G402 annotation ("askef" -> "asked").

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019EMMhgmTkbzAsmeNy97PrP
@korya
korya marked this pull request as ready for review August 7, 2026 18:57
@korya
korya merged commit 1b891c0 into master Aug 7, 2026
1 check passed
@korya
korya deleted the korya-fix-gosec-g704 branch August 7, 2026 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant