fix: enable goqu prepared statements globally#1579
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces SQL injection prevention across the codebase by enforcing parameterized query execution. It enables linter rules for detecting unsafe SQL patterns, configures goqu to use prepared statements globally, updates repository methods to capture and pass parameters from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
.github/pull_request_template.md (1)
20-23: Clarify exception policy to avoid contradictory checklist signals.Line 20 reads as an absolute prohibition, but Line 23 allows justified suppressions (which can include safe dynamic SQL cases). Consider explicitly noting “except narrowly scoped, reviewer-verifiable cases with inline justification” to keep author guidance unambiguous.
.golangci.yml (1)
36-37: Broaden discarded-params detection inforbidigopattern.The current regex only catches cases where the third variable is literally named
err. This can miss equivalent discards with different variable names.♻️ Proposed regex tweak
- - pattern: '\w+,\s*_,\s*err\s*[:=]+[^=]*\.ToSQL\(\)' + - pattern: '\w+,\s*_,\s*\w+\s*[:=]+[^=]*\.ToSQL\(\)'internal/store/postgres/relation_repository.go (1)
237-237: UseListByFieldsas the timeout operation label.Line 237 still uses
"GetByFields"for the list path, which makes DB traces/metrics harder to distinguish.Suggested tweak
- if err = r.dbc.WithTimeout(ctx, TABLE_RELATIONS, "GetByFields", func(ctx context.Context) error { + if err = r.dbc.WithTimeout(ctx, TABLE_RELATIONS, "ListByFields", func(ctx context.Context) error {internal/store/postgres/billing_invoice_repository_test.go (1)
213-214: Strengthen placeholder assertions beyond$1presence.Current checks can still pass if later placeholders regress. Consider also asserting placeholder count/index progression matches
len(params)in each case.Also applies to: 240-241, 253-254, 267-268, 281-282
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 469f5416-d647-4e79-b8f9-00858ab3f086
📒 Files selected for processing (13)
.github/pull_request_template.md.golangci.ymlcmd/serve.gointernal/store/postgres/audit_record_repository.gointernal/store/postgres/billing_invoice_repository.gointernal/store/postgres/billing_invoice_repository_test.gointernal/store/postgres/org_users_repository_test.gointernal/store/postgres/organization_repository.gointernal/store/postgres/policy_repository.gointernal/store/postgres/project_repository.gointernal/store/postgres/relation_repository.gointernal/store/postgres/role_repository.gointernal/store/postgres/session_repository.go
Coverage Report for CI Build 25049997558Coverage decreased (-0.01%) to 42.342%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Summary
Forces every goqu dataset to use prepared statements ($N placeholders + separate args) instead of inlining values into the SQL string, by setting
goqu.SetDefaultPrepared(true)globally incmd/serve.go::setupDB. SQL injection becomes structurally impossible at the goqu layer rather than relying on per-sitediscipline.
Flipping the flag surfaced several latent bugs that worked silently under value-interpolation but broke under prepared mode. They're fixed in the same PR so the flip is safe
to ship.
Changes
goqu.SetDefaultPrepared(true)incmd/serve.go::setupDB.query, _, err := stmt.ToSQL()followed by an exec without forwardingparams...):relation_repository.GetByFields/ListByFieldsorganization_repository.ListtotalCountproject_repository.ListtotalCountbilling_invoice_repository.ListtotalCountsession_repository.UpdateValidity—INTERVAL '? hours'(goqu substitutes?regardless of quote context, breaking under prepared mode) →make_interval(secs => ?).role_repository.Upsert/Update— drop the manualgoqu.L("$N")placeholder workaround; pass real values throughgoqu.Record{}. Wire size grows by ~5 params onUpsert; database state is byte-for-byte identical.policy_repository.GroupMemberCount/ProjectMemberCount/OrgMemberCount—goqu.From("policies")→dialect.From("policies"). The package-levelgoqu.Fromusesgoqu's default dialect (
?placeholders), which PostgreSQL rejects in prepared mode.audit_record_repository.go— added// #nosec G201annotations with justifications on the twofmt.Sprintfcalls buildingDECLARE … CURSOR FOR …andFETCH … FROM …(Postgres has no parameter binding for cursor identifiers;
cursorNameis server-generated viacrypto/rand)..golangci.yml:gosecwith G201/G202 only.forbidigowith three narrow patterns:?inside a single-quoted goqu.L, params-discard fromToSQL(), andgoqu.From((usedialect.Frominstead)..github/pull_request_template.md— added a 4-item SQL Safety checklist.Technical Details
Three bug shapes lint catches:
?inside single-quoted goqu.L — goqu's literal templater walks char-by-char, substituting?regardless of quote context. Prepared mode emitsINTERVAL '$1 hours'literal + a bound arg, so Postgres rejects with
bind message supplies 1 parameters, but prepared statement requires 0.paramsfromToSQL()— works in interpolated mode (params is[]); under prepared mode the SQL has$Nplaceholders without values bound.goqu.From(...)— uses goqu's default dialect (?placeholders). PostgreSQL parses?as a syntax error.role_repository's oldgoqu.L("$1")…goqu.L("$8")was a workaround for goqu not reusing parameter positions acrossINSERT VALUESandON CONFLICT DO UPDATE. Withprepared mode on, goqu emits fresh placeholders ($9..$13 for the UPDATE clause) and the values get sent twice on the wire — strings/json/UUIDs, negligible.
Test Plan
make compose-up-dev)make build,golangci-lint run ./...— 0 issues)make testpassesgo test -v ./test/e2e/...regression suite passes (TestProjectAPI/7,8,TestCheckoutAPI/2,TestCheckFeatureEntitlementAPI/2were failing pre-fix — all routedthrough
policy_repository.*MemberCount)SQL Safety
?placeholders,goqu.Ex{}, orgoqu.Record{}— neverfmt.Sprintfor+building a query that gets executed.ToSQL()callers capture and forward params (query, params, err := stmt.ToSQL(); db.…Context(ctx, …, query, params...)). Neverquery, _, err := ….?placeholders inside single-quoted SQL literals ingoqu.L(usemake_interval(hours => ?)-style functions instead).//nolint:forbidigoor// #nosec G20xannotation has a one-line justification on the same line that a reviewer can verify.