Skip to content

fix(rql): emit groupBy columns as quoted identifiers#1691

Merged
rohilsurana merged 1 commit into
mainfrom
fix/rql-groupby-quote-identifiers
Jun 9, 2026
Merged

fix(rql): emit groupBy columns as quoted identifiers#1691
rohilsurana merged 1 commit into
mainfrom
fix/rql-groupby-quote-identifiers

Conversation

@AmanGIT07

Copy link
Copy Markdown
Contributor

Summary

Switch the RQL groupBy SQL helpers (pkg/utils/rql.go) to emit column names as quoted identifiers via goqu.C / goqu.I instead of splicing them as raw SQL with goqu.L(fmt.Sprintf(...)) and goqu.L(col).

Changes

  • buildGroupByColumns: goqu.L(col)goqu.C(col).
  • buildSelectColumns: fmt.Sprintf("%s AS values", col)goqu.C(col).As("values"); two-column CONCAT uses goqu.L("CONCAT(?, ',', ?)", goqu.I(c0), goqu.I(c1)).As("values").
  • AddGroupInQuery signature, whitelist behavior, and return shape unchanged.
  • Adds pkg/utils/rql_test.go with table-driven coverage for: whitelist accept/reject, SQL output for every production whitelist column, and helper unit tests.

Technical Details

  • Whitelisted columns (event, actor_type, resource_type, target_type, org_id, org_name, activity, status, source, verified) are all lowercase identifiers, created unquoted in the table migrations — event and "event" resolve to the same column in Postgres, so the SQL change is behaviorally equivalent for the two RPCs that use this helper (ListAuditRecords, ListProspects).
  • Result-column aliases stay values / count; sqlx scanning via the existing db:"values" / db:"count" struct tags is unaffected (Postgres strips identifier quotes when reporting column names).
  • ExportAuditRecords uses AddRQLSortInQuery only and is not on this path.

Test Plan

  • go test ./pkg/utils/... — all unit tests pass, including 8-case SQL-output regression for production whitelist columns.
  • go build ./... passes.
  • golangci-lint run ./pkg/utils/... — 0 issues.
  • internal/store/postgres/audit_record_repository_test.go::TestList_Grouping (requires Docker — run before merging).
  • internal/store/postgres/prospect_repository_test.go group-by cases (requires Docker).

SQL Safety (if your PR touches *_repository.go or goqu.*)

  • Values flow through ? placeholders, goqu.Ex{}, or goqu.Record{} — never fmt.Sprintf or + building a query that gets executed.
  • ToSQL() callers capture and forward params (query, params, err := stmt.ToSQL(); db.…Context(ctx, …, query, params...)). Never query, _, err := ….
  • No ? placeholders inside single-quoted SQL literals in goqu.L (use make_interval(hours => ?)-style functions instead).
  • Any //nolint:forbidigo or // #nosec G20x annotation has a one-line justification on the same line that a reviewer can verify.

🤖 Generated with Claude Code

Replace goqu.L(fmt.Sprintf(...)) and goqu.L(col) in the groupBy helpers
with goqu.C / goqu.I so column names render as quoted identifiers
instead of raw SQL fragments. The existing whitelist in AddGroupInQuery
continues to gate the input; the SQL builder no longer relies on it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Jun 9, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
frontier Ready Ready Preview, Comment Jun 9, 2026 9:00am

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7c107eed-8d8e-427e-bd30-bb2388153ca3

📥 Commits

Reviewing files that changed from the base of the PR and between d1acb6b and 4fdd8e9.

📒 Files selected for processing (2)
  • pkg/utils/rql.go
  • pkg/utils/rql_test.go

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved SQL query generation with safer identifier handling to prevent SQL injection vulnerabilities.
  • Tests

    • Added comprehensive test coverage for query building and filtering functionality.

Walkthrough

This PR refactors RQL query builders to use parameterized expressions instead of raw SQL string concatenation for GROUP BY and select-value clauses. buildGroupByColumns switches to column identifiers, and buildSelectColumns replaces fmt.Sprintf construction with expression-based building. A new test suite validates column whitelisting, SQL injection detection, identifier quoting, and exact SQL output.

Changes

RQL Query Builder SQL Safety Refactor

Layer / File(s) Summary
SQL Expression Refactor
pkg/utils/rql.go
buildGroupByColumns switches from raw literal (goqu.L(col)) to column identifier (goqu.C(col)). buildSelectColumns replaces string-based SQL construction with goqu.Expression-based building using goqu.C(...).As("values") for single columns and parameterized CONCAT(?, ',', ?) for multi-column aggregation.
Test Suite and Validation
pkg/utils/rql_test.go
Comprehensive tests validate column whitelisting, reject injection-shaped inputs (semicolons, keywords, subqueries), verify identifier quoting for unsafe strings, assert exact generated SQL for single/multi-column GROUP BY with CONCAT expressions, and test helper function outputs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • rohilsurana
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 27195324845

Coverage increased (+0.1%) to 43.371%

Details

  • Coverage increased (+0.1%) from the base build.
  • Patch coverage: 6 of 6 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 37013
Covered Lines: 16053
Line Coverage: 43.37%
Coverage Strength: 12.34 hits per line

💛 - Coveralls

@rohilsurana rohilsurana merged commit 54e8269 into main Jun 9, 2026
8 checks passed
@rohilsurana rohilsurana deleted the fix/rql-groupby-quote-identifiers branch June 9, 2026 10:08
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.

3 participants