fix: [#1002] quote column names in ORM where and order methods - #1565
darakanoit wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1565 +/- ##
==========================================
+ Coverage 72.84% 73.13% +0.28%
==========================================
Files 412 412
Lines 26846 26868 +22
==========================================
+ Hits 19557 19649 +92
+ Misses 7287 7217 -70
Partials 2 2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🤖 Automated reviewThis is an AI-generated code review. Please double-check each finding before acting. You don't need to address every issue if they are inaccurate, but please point them out if any exist. SummaryThis PR makes the ORM's column-based helpers ( Verdict
FindingsMust Fix
Should Fix
Nits
Note: No prior automated review rounds were found on this PR, so there was nothing to reconcile or tick. |
…that runs the query
|
Addressed in e2bb64a.
|
📑 Description
Closes goravel/goravel#1002
WhereInand the other column based helpers of the ORM put the column into the SQL string as is:A reserved or case sensitive column name breaks the query on every driver
(
WHERE group IN (...)is a syntax error).What changed
The column is now passed to GORM as a
clause.Column, so GORM quotes it when the query is built, with the dialect ofthe connection that runs it (
"group"on Postgres, SQLite and SQL Server,`group`on MySQL). Quoting at call timewould use the wrong dialect for a model whose
Connection()differs from the query's connection. This applies to:WhereIn,OrWhereIn,WhereNotIn,OrWhereNotIn,WhereBetween,WhereNotBetween,OrWhereBetween,OrWhereNotBetween,WhereNull,OrWhereNull,WhereNotNull,WhereAll,WhereAny,WhereNone,OrderBy,OrderByDesc.As agreed in the issue, only plain identifiers are quoted:
groupandusers.group(→"users"."group").Anything else is passed through unchanged, so existing code keeps working:
LOWER(name);data->name, whichbuildWherestill compiles through the grammar;"group".OrderBy/OrderByDescquote the column forascanddesc(GORM omitsASC:ORDER BY "order"). Any otherdirection, e.g.
DESC NULLS LAST, keeps the raw form, since GORM can't express it for a column.OrderByRawand thedeprecated
Orderare raw by definition and are not touched.Behavior change
On Postgres a quoted identifier is case sensitive, so
WhereIn("Name", ...)against a column namednameused to matchthrough case folding and now fails. This is consistent with
Where("Name", ...), which already quotes, and with Laravel,which always wraps column names.
Limitations
Only plain identifiers are quoted, anything else still goes into the SQL as is. This fixes reserved and case sensitive
names, but it does not make these methods safe for a column name taken from user input:
WhereIn("id) OR 1=1 --", ...)is not a plain identifier and is passed through. Such input still has to be checked against an allow list. Closing this
fully would need Laravel's rule (always wrap, expressions go through a raw expression), which we decided against in the
issue to keep existing code working.
Plain identifiers are ASCII only, so a non ASCII column name is not quoted and behaves as before.
Verification
database/gormTestQuoteColumns(methods above, built without a GORM instance)database/gormTestQuoteColumns(expressions, JSON selector, quoted name, custom direction stay raw)TestCustomConnectionQuotesColumns(MySQL default connection, model on Postgres)TestToSqlTestSuite/TestQuoteColumns(19 cases for the methods above)TestToSqlTestSuite/TestQuoteColumns(expression and quoted name stay unchanged)TestIsPlainIdentifiergo test ./...cd tests && go test ./...(MySQL, Postgres, SQLite, SQL Server)Scope
database/gorm/query.go: the methods above pass plain columns asclause.Columndatabase/gorm/utils.go:isPlainIdentifier,columnConditiondatabase/gorm/query_test.go,database/gorm/utils_test.go: unit teststests/query_test.go,tests/models.go:TestCustomConnectionQuotesColumnstests/to_sql_test.go: the generated SQL on PostgresNo public API changed.
database/dbhas the same problem and will be fixed in a follow up.✅ Checks