review remediation: model SQL layer — replace scope-arg denylist with parameterization, define select= validation policy, drop per-WHERE Migration instantiation (held: sql.cfc occupied by PR #2910)

[bpamiri]

Problem

Three injection-surface design defects plus two held perf/quality defects in the model SQL layer. All were held out of wave 2 because vendor/wheels/model/sql.cfc was occupied by PR #2910 (JOIN-memo cache key) — that PR has since merged, so this work is now unblocked. Line numbers verified against origin/develop @ 8971094 except where noted.

1. $sanitizeScopeHandlerArgs silently corrupts legitimate values (M9 / SEC-22)
vendor/wheels/model/properties.cfc:763 (invoked from ScopeChain.cfc:209): a blocklist strips --, /*, ; and whole-word UNION/EXEC/SLEEP/etc. from every simple scope-handler arg, mutating data in place — "Union Pacific" becomes " Pacific", "Estimated delay" loses "delay". The code's own comment admits it is a stopgap.

2. The denylist is also bypassable (SEC-22)
The sanitizer is quote-escaping plus an incomplete SQL keyword denylist. Quote-escaping protects only quoted contexts; an arg interpolated unquoted/numeric (where="age > #args.age#") gets only the denylist, and inputs like 0 OR 1=1 survive it.

3. SELECT clause passes dotted/aliased items through with zero validation (SEC-21)
vendor/wheels/model/sql.cfc:501 appends any select-list item containing . or AS verbatim, while $orderByClause and $groupByClause validate their inputs. So findAll(select=params.fields) — API field-selection — becomes an injection/exfiltration primitive (e.g. id, (SELECT secret FROM users) AS x).

4. SQL layer instantiates wheels.migrator.Migration per WHERE build just to read the dialect name (M3, held)
vendor/wheels/model/sql.cfc:712, :766 and update.cfc:76 — Migration.init() issues an uncached $dbinfo(type="version") (plus SELECT version() on Postgres) on every updateAll/deleteAll and every findAll shell-cache miss, solely to call adapter.adapterName() — which the model already caches and uses at sql.cfc:27.

5. updateAll(include=…) builds WHERE via JOIN.Split("ON")[2] string surgery (M4, held)
sql.cfc:770, :785 (review-time lines): a case-sensitive bare-token split on "ON" corrupts joins on uppercase identifiers (H2 preserves uppercase; tables/columns like TRANSACTIONS/POSITIONID); classes[2] hard-index ignores additional includes; the loops increment an unscoped i.

Impact

• Legitimate user data is silently rewritten before hitting the database (data-integrity bug).
• The sanitizer creates a false sense of safety for unquoted interpolation contexts; select= from request params is a working exfiltration primitive.
• Every updateAll/deleteAll pays a $dbinfo round-trip; uppercase-identifier schemas get corrupted JOIN conditions in updateAll(include=).

Suggested approach

• Route scope-handler values through parameterized binding; reserve rejection (throw) for the truly unparameterizable; never silently rewrite user data. Document that the sanitizer does not make unquoted interpolation safe.
• Define a validation policy for dotted/aliased select= items: apply the same ; / -- / comment / subquery-paren rejection and dot-notation regex used by the ORDER BY/GROUP BY paths, without breaking existing apps that legitimately pass table.column and expr AS alias items. Needs a deliberate policy decision (deprecation window or dev-mode warning) before hard-rejecting.
• Derive the dialect name by stripping "Model" from get("adapterName"), or cache it at $assignAdapter time, instead of instantiating Migration per WHERE build.
• Split the JOIN on the ON keyword with position arithmetic or store the ON-condition on the association struct; loop over all associations instead of hard-indexing classes[2]; scope the loop counter as local.i.

Acceptance criteria

• Scope-handler args reach SQL via parameterized binding (or throw); "Union Pacific" round-trips unchanged through a scope filter.
• A documented policy for select= validation exists and is enforced; findAll(select="id, (SELECT secret FROM users) AS x") is rejected.
• No Migration instantiation (and no $dbinfo call) per WHERE build; updateAll/deleteAll hot path verified.
• updateAll(include=) works with uppercase identifiers and multiple includes; loop counters scoped.
• Cross-engine specs cover the above (bash tools/test-local.sh model + matrix spot-check).

Source

Internal multi-agent framework review 2026-06-09, wave 2 (issues phase). Findings: model-orm M9, security-pass SEC-21/SEC-22; held package model-orm M3/M4 (held: model/sql.cfc occupied by PR #2910, since merged).

[wheels-bot]

Wheels Bot — Triage

Classified as bug.

Suspected layer

model — see .ai/wheels/security/ and the source under vendor/wheels/model/ for canonical patterns (no dedicated .ai/wheels/model/ doc exists).

Fix sketch

Five distinct defects bundled across vendor/wheels/model/properties.cfc ($sanitizeScopeHandlerArgs at :763), vendor/wheels/model/sql.cfc ($selectClause at :501, plus Migration instantiations at :712 and :766), vendor/wheels/model/update.cfc (:76 Migration instantiation, plus the JOIN.Split("ON")[2] string surgery at :770/:785), and the scope-arg call site in vendor/wheels/model/ScopeChain.cfc:209. Likely shape: (1) replace the blocklist sanitizer with parameterized binding (or throw); (2) extend the dotted/aliased select-list validation to apply the same ; / -- / comment / subquery-paren rejection used by ORDER BY/GROUP BY paths (this one explicitly requires a deprecation-window or dev-mode-warning policy decision before hard-rejecting); (3) cache the dialect name at $assignAdapter time and stop instantiating wheels.migrator.Migration per WHERE build; (4) parse JOIN ON conditions by position arithmetic or store the ON-condition on the association struct, loop over all associations, scope local.i. Each item could land as its own PR.

Confidence: low

The report is precise, but it bundles five separate defects and item 3 explicitly notes a deliberate policy decision is required (deprecation window or dev-mode warning) before any hard-rejection of select= items — that is design work, not a mechanical fix; the SEC-flavored items and the cross-engine concern (H2 preserves uppercase identifiers in the JOIN string-surgery path) also push this below the medium auto-fire threshold.

<!-- wheels-bot:triage:2952 -->
<!-- wheels-bot:triage-class:bug -->

[bpamiri]

Coverage-audit addition (2026-06-10): the context-independent metadata fill-ins above the JOIN memo in vendor/wheels/model/sql.cfc ~:1183-1216 still write the shared application-scoped association struct without a lock — the same pattern H4/#2910 fixed for the JOIN string itself. Belongs with this issue's model-SQL-layer batch.

[bpamiri]

PR #3011 lands four of the five bundled defects from this issue:

1. SEC-22 / M9 — $sanitizeScopeHandlerArgs denylist removed; quote-escaping only, with the quoted literal re-extracted and bound via cfqueryparam downstream. "Union Pacific" round-trips unchanged. The doc comment now states explicitly that unquoted interpolation is not made safe by escaping — handlers should validate/cast or return whereParams.
2. SEC-21 — dotted/aliased select= items containing ;, --, /*, or parenthesized subqueries now log a development-mode warning (deprecation window per the campaign policy decision — no hard rejection yet).
3. M3 — per-WHERE-build wheels.migrator.Migration instantiation replaced with $dialectName() (strips Model from the cached adapter name). Also fixes the dialect being probed from the app default datasource for models on custom datasources.
4. M4 — JOIN.Split("ON") string surgery replaced with joinOnConditions (position arithmetic on " ON "); multi-include updateAll now contributes all join conditions and tables; loop counters scoped.

Remaining on this issue after #3011 merges:

• Hard enforcement of the select= validation policy once the deprecation window closes (currently dev-mode warning only).
• Lock the context-independent metadata fill-ins above the JOIN memo in $expandedAssociations (~sql.cfc:1199-1216) — the unlocked shared-struct writes flagged in the coverage-audit comment above.
• (Optional hardening) full whereParams parameterization for scope-handler args instead of escaping + downstream literal re-extraction.

[bpamiri]

Resolved across #3011 (M3/M4 mechanical items, SEC-22 scope-arg parameterization, SEC-21 dev-mode warning) and #3152 (the $expandedAssociations lock). The remaining breaking item — SEC-21 hard enforcement of the select= validation policy — moves to a narrow 5.0-scoped successor issue so this audit umbrella can close.