Skip to content

fix(G202): don't flag strings.Builder built from constants - #1707

Merged
ccojocar merged 2 commits into
securego:masterfrom
ravisastryk:fix/g202-strings-builder-false-positive
Jul 20, 2026
Merged

fix(G202): don't flag strings.Builder built from constants#1707
ccojocar merged 2 commits into
securego:masterfrom
ravisastryk:fix/g202-strings-builder-false-positive

Conversation

@ravisastryk

Copy link
Copy Markdown
Contributor

fix(G202): don't flag strings.Builder built from constants

Problem

G202 (SQL string concatenation) reports a false positive when a query is built by concatenating the result of a strings.Builder (or bytes.Buffer) whose content is only constant literals — the common, efficient idiom for building
SQL IN (...) placeholder lists.

The inconsistency: the equivalent += construction is not flagged, but the strings.Builder replacement (recommended for efficiency) is.

Fixes: #1701

var ph strings.Builder
for i := range ids {
    if i > 0 {
        ph.WriteString(",")
    }
    ph.WriteString("?")
}
placeholders := ph.String()
q := "SELECT col1 FROM table1 WHERE id IN (" + placeholders + ")" // G202 (false positive)

Root cause

TryResolve treats any function-call result as unresolved (tainted), so builder.String() was always considered non-constant, regardless of what was written into the builder.

Fix

resolveCallExpr now recognizes strings.Builder.String() / bytes.Buffer.String() and resolves it to a constant only when every write into that builder is a constant. It stays conservative - a non-constant write, an opaque initializer, an escaping address, or a package-level builder all keep the query flagged. Genuinely tainted builders are still detected.

How to test (quick)

Unit tests (covers the new positive + negative samples):

go test -run TestRules ./rules/

End-to-end — drop this into a file and scan it:

// repro.go
package main

import (
	"database/sql"
	"os"
	"strings"
)

func main() {
	db, _ := sql.Open("sqlite3", ":memory:")

	// SAFE — builder fed only constants: should NOT be flagged
	var ok strings.Builder
	for i := 0; i < 3; i++ {
		if i > 0 {
			ok.WriteString(",")
		}
		ok.WriteString("?")
	}
	rows, _ := db.Query("SELECT * FROM t WHERE id IN (" + ok.String() + ")")
	defer rows.Close()

	// TAINTED — builder fed os.Args: should STILL be flagged
	var bad strings.Builder
	for _, a := range os.Args {
		bad.WriteString(a)
	}
	rows2, _ := db.Query("SELECT * FROM t WHERE id IN (" + bad.String() + ")")
	defer rows2.Close()
}
go run ./cmd/gosec/ -include=G202 ./path/to/repro/...

Expected: exactly 1 issue — the tainted builder is reported, the constant-only builder is not. Before this change, both were reported.

…1701)

Signed-off-by: Ravi Sastry Kadali <ravisastryk@gmail.com>
@ravisastryk
ravisastryk marked this pull request as ready for review July 18, 2026 18:40
@ravisastryk

Copy link
Copy Markdown
Contributor Author

Heads-up: the barry-ai-security-review job is failing on the new actions/checkout@v7 "pull_request_target" guard, not on this PR's changes, the checkout step needs allow-unsafe-pr-checkout: true (or an equivalent adjustment). Happy to open a separate workflow PR if that's helpful @ccojocar so you can make that change and then this should pass.

@codecov

codecov Bot commented Jul 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.33735% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.56%. Comparing base (1769a22) to head (ff94ce1).

Files with missing lines Patch % Lines
resolve.go 84.33% 7 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1707      +/-   ##
==========================================
+ Coverage   80.53%   80.56%   +0.02%     
==========================================
  Files         110      110              
  Lines       10255    10337      +82     
==========================================
+ Hits         8259     8328      +69     
- Misses       1512     1519       +7     
- Partials      484      490       +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Ravi Sastry Kadali <ravisastryk@gmail.com>
@ccojocar
ccojocar merged commit 45b083a into securego:master Jul 20, 2026
13 of 14 checks passed
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.

G202 with strings.Builder

2 participants