Skip to content

review remediation: wave-3 deferred subsystem cleanups — rendering coercion/Chr(7), MSSQL/Oracle adapters, EventMethods memo, route-tester XSS (held: PRs #2900/2901/2908/2909 now merged) #2961

Closed

Description

@bpamiri

Problem

A set of deferred subsystem cleanups that were held out of wave 2 because their files were occupied by then-open PRs. Those PRs (#2900, #2901, #2908, #2909) have since merged, so all of this is now unblocked. Grouped here to bound the issue count; each section is independently actionable. Line numbers verified against origin/develop @ 8971094.


A. controller/rendering.cfc (was held behind PR #2901)

  • C2 [Medium, robustness] renderWith() JSON coercion — vendor/wheels/controller/rendering.cfc:275-306: the coercion gate is StructCount(arguments) > 8 against an 8-name $defined list, but renderWith now declares 9 params, so status leaks into coercion args (stale arity check). Replace(content, Chr(7), "", "all") (:287, :293-295) deletes legitimate BEL bytes anywhere in the serialized payload (data corruption). The force-to-integer regex rewrites serialized JSON by key name across nested structures, hitting nested same-named keys, with a full-payload regex/replace pass per key (perf). Fix: build the param-exclusion list from function metadata; coerce the data structure before serialization instead of regex-editing serialized JSON. Note the Chr(7) marker is a deliberate cross-engine SerializeJSON workaround — validate any pre-serialization cast across engines.
  • C13 [Low, robustness] $includeFile silently blanks columns — :653-657: catch(any e){ arguments[property]="" } masks unreadable/binary-column read errors as blank values with no logging. Fix: narrow the catch or log before defaulting.
  • C16 [Low, perf] $includeFile re-tokenizes the column list per row — :621, :649, :653-657: ListToArray(query.columnList) runs inside both per-row loops. Fix: hoist above the loops.
  • C17 [Low, perf] $getStatusCodes() rebuilds a 63-entry constant struct per render — :809-876: reached from every render path; local.statusText computed then unused in the numeric branch; $returnStatusCode does StructFindValue over the freshly rebuilt struct. Fix: memoize in application scope, add a reverse map for value lookup.

B. DB adapters (was held behind PR #2908)

  • DA1 [Low, quality] GROUP BY dropped from paginated SQL Server queries — vendor/wheels/databaseAdapters/MicrosoftSQLServer/MicrosoftSQLServerModel.cfc:191-192: local.afterWhere = "GROUP BY ..." is overwritten by a plain = assignment of "ORDER BY ..." (should be &=). Currently latent (the only limit>0 caller never forwards group). Fix: = → &=; add a findAll(group=..., page=...) SQL Server spec.
  • DA5 [Medium, quality] Oracle $randomOrder returns RANDOM(), invalid on Oracle — vendor/wheels/databaseAdapters/Oracle/OracleModel.cfc:226-227: ORDER BY RANDOM() raises ORA-00904; the legacy adapter returned dbms_random.value(). findAll(order="random") throws on Oracle; soft-fail CI hides it. Fix: return DBMS_RANDOM.VALUE; add an Oracle random-order test.
  • DA14 [Low, perf] SQL Server pagination re-strips the whole SELECT list per ORDER BY column (O(n²)) — MicrosoftSQLServerModel.cfc:116-199: $stripIdentifierQuotes(thirdSelect) recomputed inside the per-column loop; ListGetAt/ListFind over growing lists. Fix: hoist the strip; convert lists to arrays once.

C. events/EventMethods.cfc (was held behind PR #2900)

  • DC6 [Low, quality/perf] Misspelled memo guard forces a second GetHTTPRequestData() body materialization per request — vendor/wheels/events/EventMethods.cfc:173-174: the guard checks the singular key $wheelsHeader (written nowhere) while the next line writes the plural $wheelsHeaders, so the memo never hits; $initializeRequestScope already stored the full result in request.wheels.httpRequestData. Fix: fix the guard key spelling and reuse the already-stored headers.

D. public/helpers.cfm route tester (was held behind PR #2909)

  • SEC-8 / P9 [Medium, security] Reflected XSS in the verb-mismatch message — vendor/wheels/public/helpers.cfm:226: raw arguments.path is interpolated into the verb-mismatch message while requestMethod is encoded; sinks at routetester.cfm:19 and routetesterprocess.cfm:43. Fix: wrap arguments.path in EncodeForHTML (line 237 already does so for the RouteNotFound message — line 228 does not).
  • P14 [Low, perf] Route tester always runs a second full regex scan — helpers.cfm:198-228: the alternatives loop (consumed only when matches is empty) runs unconditionally on every invocation, including lazy .regex writes onto application-scope route structs from a request thread. Fix: hoist the alternatives loop into the no-match branch, matching Dispatch.cfc's structure.

Impact

C2's Chr(7) strip silently corrupts JSON payloads; DA5 makes order="random" throw on Oracle (hidden by soft-fail CI); SEC-8 is a reflected-XSS in the dev route tester; the rest are latent correctness/perf cleanups.

Suggested approach

Address per section above. SEC-8 and C2 (security + data-integrity) should lead; the perf/quality items can follow. Run bash tools/test-local.sh plus a matrix spot-check (Adobe 2023 + Lucee 7); add the Oracle and SQL Server specs noted.

Acceptance criteria

  • renderWith excludes all framework params via metadata; JSON payloads are not regex-edited post-serialization; BEL bytes in data survive (cross-engine verified).
  • MSSQL paginated GROUP BY is retained (spec added); Oracle order="random" works (spec added); MSSQL pagination strip hoisted out of the loop.
  • EventMethods memo guard fixed; no second GetHTTPRequestData() per request.
  • Route-tester verb-mismatch message HTML-encodes arguments.path; the alternatives scan runs only on no-match.

Source

Internal multi-agent framework review 2026-06-09, wave 2 (issues phase). Held packages (occupying PRs #2900/#2901/#2908/#2909 now merged): controller C2/C13/C16/C17, db-adapters DA1/DA5/DA14, dispatch-core DC6, public-ui P9/P14, security-pass SEC-8.

Activity

  1. wheels-bot commented on Jun 10, 2026

    @wheels-bot
    Contributor

    Wheels Bot — Triage

    Classified as bug.

    Suspected layer

    controller — see .ai/wheels/controllers/ for canonical patterns. Note: this roll-up actually spans four subsystems (controller rendering, DB adapters, dispatch events, public route-tester helpers); controller is the primary because C2 (renderWith JSON coercion) is the lead data-integrity item. Downstream stages should also consult .ai/wheels/cross-engine-compatibility.md for the Chr(7) SerializeJSON workaround and treat the four sections as independently addressable.

    Fix sketch

    Nine concrete fixes across four files. A. vendor/wheels/controller/rendering.cfc — rebuild the param-exclusion list from function metadata so renderWith's 9th status param stops leaking into coercion (C2:275-306); coerce the data structure before SerializeJSON instead of Replace(..., Chr(7), "") and the by-key regex pass on the serialized payload (cross-engine validate); narrow/log $includeFile's blanket catch (C13:653-657); hoist ListToArray(query.columnList) above the per-row loops (C16:621,649); memoize $getStatusCodes() in application scope with a reverse-value map (C17:809-876). B. DB adapters — MicrosoftSQLServer/MicrosoftSQLServerModel.cfc:191-192 swap = to &= so paginated GROUP BY survives (DA1); Oracle/OracleModel.cfc:226-227 return DBMS_RANDOM.VALUE instead of RANDOM() (DA5); hoist $stripIdentifierQuotes(thirdSelect) out of the per-ORDER-BY-column loop and pre-convert lists to arrays in MicrosoftSQLServerModel.cfc:116-199 (DA14). C. vendor/wheels/events/EventMethods.cfc:173-174 — fix the singular-vs-plural memo key ($wheelsHeader → $wheelsHeaders) and reuse request.wheels.httpRequestData (DC6). D. vendor/wheels/public/helpers.cfm — wrap arguments.path in EncodeForHTML at line 226 to match line 237 (SEC-8); hoist the alternatives regex loop into the no-match branch to match Dispatch.cfc's structure (P14:198-228).

    Confidence: medium

    Each finding is mechanically clear with a named file:line and concrete fix shape, but the cross-engine auto-downgrade fires (C2's Chr(7) strip is a deliberate SerializeJSON workaround that must be re-validated across Lucee/Adobe/BoxLang); the multi-subsystem scope and a reflected-XSS item (SEC-8) also warrant the propose-fix safety net.

    <!-- wheels-bot:triage:2961 -->
    <!-- wheels-bot:triage-class:bug -->
    <!-- wheels-bot:triage-confidence:medium -->

  2. wheels-bot commented on Jun 10, 2026

    @wheels-bot
    Contributor

    Wheels Bot — Fix on hold for human review

    The proposed fix touches sensitive areas (vendor/wheels/controller/rendering.cfc JSON-coercion path, vendor/wheels/public/helpers.cfm reflected-XSS) and the bot's safety net requires a human in the loop before any code is written.

    Specifically:

    • C2 (cross-engine): the Chr(7) wrap/strip at rendering.cfc:287,293-295 is a deliberate SerializeJSON workaround. The triage already flagged this — re-validation across Lucee 5/6/7, Adobe CF 2018/2021/2023/2025, and BoxLang is required before changing the coercion strategy. The "coerce data before serialization" approach proposed in the fix sketch needs a human to verify each engine's SerializeJSON actually preserves string vs. integer typing without the BEL-marker round-trip.
    • SEC-8 (security): wrapping arguments.path in EncodeForHTML at helpers.cfm:226 is mechanically simple, but the surrounding route-tester rendering path (routetester.cfm:19, routetesterprocess.cfm:43) should be audited for sibling untrusted-input sinks before a one-line patch lands in isolation.
    • Multi-subsystem scope: nine findings across four files (rendering, two DB adapters, EventMethods, public helpers) is too broad for a single bot-authored PR. A human should split this into per-subsystem PRs so each is independently reviewable and revertible.

    Suggested split for whoever picks this up:

    PR Items Risk
    controller/rendering C2, C13, C16, C17 C2 is cross-engine; needs design
    db-adapters DA1, DA5, DA14 low; add MSSQL group-by + Oracle random specs
    events/EventMethods DC6 trivial; spelling + reuse memoized headers
    public/helpers (route tester) SEC-8, P14 SEC-8 is XSS; verify no sibling sinks

    The DC6 (EventMethods memo guard typo) and DA1/DA5 (MSSQL = → &=, Oracle RANDOM() → DBMS_RANDOM.VALUE) items are the smallest and safest starting points if a human wants to land something incrementally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions