fix(storage/sql): reject filter keys that aren't plain column names

[Lutherwaves]

Problem

buildQuery writes filter keys into the SQL unchanged. Only the values are bound. A key can close GORM's parentheses around the filter and widen it:

filter := map[string]any{"1=1) OR (1=1": nil, "name": "orig"}
adapter.Update(&Item{Id: "a", Name: "changed"}, filter) // updated every row
adapter.Delete(&Item{}, map[string]any{"1=1 OR name": "x"}) // deleted every row

On Update this also bypasses the primary-key condition, because the key rewrites the whole filter group. Get, List and Count use the same path.

This only matters when an application builds filter keys from untrusted input. Values have always been bound.

Fix

buildQuery checks every key against validColumnName (^[a-zA-Z_][a-zA-Z0-9_]*$), the pattern validateSortKey already uses, and returns an error. Get, List, Count, Update and Delete return that error before any SQL runs.

Behavior change

Keys that aren't plain column names, such as table.column or quoted identifiers, now return an error. No test or doc in the repo uses them.

Tests

TestSQLAdapterRejectsUnsafeFilterKeys calls all five methods with the key above and checks that each one returns an error and that no row changes. Without the fix, Update succeeds and changes every row.

Related

• Covers the "Keys aren't validated" item of issue: storage/sql: harden buildQuery filter handling #256. The other items there stay open.
• Raised in the fix(storage/sql)!: scope Update to the filtered row #258 review. Merge this before fix(storage/sql)!: scope Update to the filtered row #258: fix(storage/sql)!: scope Update to the filtered row #258 changes the same UpdateContext lines, so it will need a small rebase.

Summary by CodeRabbit

• Bug Fixes

  • Added validation for filter field names to prevent unsafe query input.
  • Operations using invalid filter names now return an error instead of executing.
  • Prevented unintended data changes caused by malformed or injection-style filters.

• Tests

  • Added coverage confirming unsafe filters are rejected across retrieval, listing, counting, updating, and deleting operations.
  • Verified that rejected filters leave existing data unchanged.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 1e8bc35b-de06-40b9-91b1-8faeb7b9ab97

📥 Commits

Reviewing files that changed from the base of the PR and between 8152e37 and 389aac8.

📒 Files selected for processing (2)

• storage/sql.go
• storage/sql_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The SQL adapter now validates filter keys before embedding them in SQL clauses. Query operations propagate validation errors. List builds the validated filter query before applying pagination.

Changes

SQL filter validation

Layer / File(s)	Summary
Filter key validation storage/sql.go	buildQuery validates filter keys against validColumnName and returns an error for invalid keys.
Operation error propagation storage/sql.go	GetContext, UpdateContext, DeleteContext, ListContext, and CountContext propagate filter validation errors. ListContext builds the filter query before creating the pagination scope.

Priority: ⬆️ High

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 389aa

The change rejects unsafe SQL filter keys across all five operations while preserving valid query pagination, with regression coverage for rejection and mutation protection.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: rejecting SQL filter keys that are not plain column names.
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.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks each query key,
And stops the unsafe ones with glee.
Get and list now heed the sign,
Update, delete, and count align.
Safe filters hop through SQL free.}

_{Comment @coderabbitai help to get the list of available commands.}