fix(model): block SQL injection in legacy finders via adapter-side value validation#2418
Merged
Conversation
…lue validation (#2417) PR #2416 closed the chainable QueryBuilder's SQL injection by validating typed values before $quoteValue. The same root-cause pattern still existed in the legacy ORM paths that pre-date the chainable builder: - findByKey / updateByKey / deleteByKey via $keyWhereString (sql.cfc:1325) - findByX / findOneByX / findAllByX via dynamic finders (onmissingmethod.cfc:165) - $buildWhereClausePart used by validatesUniquenessOf (validations.cfc:774) All three paths reach the adapter's $quoteValue with user-supplied values bound to integer/float/boolean columns, where the adapter passes them through unquoted. The downstream RESQLWhere regex re-extracts bare numerics into cfqueryparam placeholders but leaves any injected OR / UNION / comment structurally intact — so /users/1%20OR%201%3D1 produced WHERE id=1 OR 1=1, a tautology returning the first row regardless of the intended key. Push the validation into Base.$quoteValue itself so every caller (including any future one) inherits the protection. The same regex/allowlist as #2416 — ^-?[0-9]+$ for integer, ^-?[0-9]+(\.[0-9]+)?$ for float, 0/1/true/false/yes/no for boolean. Mismatches throw Wheels.InvalidValue. String columns are untouched; the adapter still wraps + escapes them. Adapter-side rather than caller-side per the audit notes in #2417: read.cfc:170 round-trips DB-stored PK values (always valid integers); validations.cfc:774 feeds model property values (a non-numeric on an integer column was already a DB error today, now it's a clearer Wheels.InvalidValue at the framework boundary); Base.cfc:66 fires only on parameterize=false query plans where malformed numerics were also already broken. Adds 16 regression tests in vendor/wheels/tests/specs/security/ LegacyFinderInjectionSpec.cfc covering tautology, UNION SELECT, comment- truncation, and stacked-statement payloads against findByKey / updateByKey / deleteByKey / findAllByX / findOneByX on integer + float columns, plus accept-cases for legitimate integer/float/string values. Closes #2417. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #2417. PR #2416 closed the SQL injection vector in the chainable QueryBuilder by validating typed values before they reach
$quoteValue. The same root-cause pattern still existed in the legacy ORM paths that pre-date the chainable builder:findByKey/updateByKey/deleteByKeyvia$keyWhereString(sql.cfc:1325)findByX/findOneByX/findAllByXvia dynamic finders (onmissingmethod.cfc:165)$buildWhereClausePartused byvalidatesUniquenessOf(validations.cfc:774)All three reach
Base.$quoteValue(databaseAdapters/Base.cfc:476) with user-supplied values bound to integer/float/boolean columns. The adapter returns them unquoted (a known contract for the downstreamRESQLWhereregex that re-extracts bare numerics intocfqueryparam). The post-hoc regex parameterizes what it sees, but doesn't strip injectedOR/UNION/ comment structure — so/users/1%20OR%201%3D1producedWHERE id=1 OR 1=1, a tautology returning the first row.Severity: HIGH (framework-level, affects every downstream Wheels app following the canonical CRUD pattern documented in our guides).
What changed
Push the validation into
Base.$quoteValueitself (approach B from #2417 — the maintainer's recommended shape). One fix point, defense for any future caller.vendor/wheels/databaseAdapters/Base.cfc: new$validateValueShape(str, type)runs before the adapter passes integer/float/boolean values through unquoted. Same regex/allowlist as fix(model): block SQL injection in chainable QueryBuilder via value-shape validation #2416 —^-?[0-9]+$for integer,^-?[0-9]+(\.[0-9]+)?$for float,0,1,true,false,yes,nofor boolean. Mismatches throwWheels.InvalidValue. String columns are untouched — the adapter still wraps and escapes them.vendor/wheels/tests/specs/security/LegacyFinderInjectionSpec.cfc: 16 regression tests covering tautology, UNION SELECT, comment-truncation, and stacked-statement payloads againstfindByKey/updateByKey/deleteByKey/findAllByX/findOneByXon integer + float columns, plus accept-cases for legitimate integer (positive, negative, integer-shaped string), float, and string values.Why adapter-side rather than caller-side
The issue flagged the audit risk for internal callers. Verified each:
read.cfc:170(pagination)validations.cfc:774($buildWhereClausePart)Wheels.InvalidValueat the framework boundaryBase.cfc:66(parameterize=false)No regressions in the suite (see Test plan).
Test plan
tools/test-local.sh(with port-collision workaround for the user's daily-driver server): 3413 passed, 0 failed, 0 errored — the 9 non-passes are all in browser fixture-server specs unrelated to$quoteValue.LegacyFinderInjectionSpec.McpCommandInjectionSpectemporarily set aside): 148 passed, 0 failed, 0 errored — including 16/16 on the new spec.pr.yml(Lucee 5/6/7 + Adobe 2018/2021/2023/2025 + boxlang × SQLite/MySQL/Postgres/SQL Server)Side finding (separate fix, spawned)
vendor/wheels/tests/specs/security/McpCommandInjectionSpec.cfccallsmid(s, 7)on lines 105, 117, 133, 150 — Lucee accepts the 2-arg form, Adobe CF requires 3. Pre-existing since PR #2083 (commitdb722c713); crashes the Adobe 2025 security suite at compile time. Spawned as a separatefix(test)task — out of scope for this PR.Cross-references
develop: #2416 — chainable QueryBuilder fix🤖 Generated with Claude Code