[bpamiri]

Summary

$getRequestFormat() (vendor/wheels/events/EventMethods.cfc:327) returned url.format verbatim, and $runOnError interpolates that value into the on-disk error-template include path — "#application.wheels.eventPath#/onerror.#local.format#.cfm" (vendor/wheels/events/EventMethods.cfc:129) — guarded only by FileExists. An attacker-supplied value such as ?format=../../somefile therefore enabled path traversal / local file inclusion of any .cfm on disk during error rendering. Both call sites are affected via the same helper: the development showErrorInformation branch (EventMethods.cfc:47) and the production template-include branch (EventMethods.cfc:128-134).

Source

Internal multi-agent framework review, 2026-06-09, finding T4 (format-lfi).

Fix

Gate the url.format branch in $getRequestFormat() behind ReFind("^[A-Za-z0-9]+$", url.format), falling through to the existing "html" default for anything else. Applied at the source so both consumers are covered with one change. A plain-alnum token covers every configured format key (html, xml, json, csv, pdf, xls) and any addFormat() extension, while rejecting traversal sequences, dots, slashes, backslashes, null bytes, encoded variants, and duplicate-param comma-joins.

Deliberately out of scope:

• The http_accept else-branch only emits $get("formats") keys (developer-controlled config), so it was left alone.
• $requestContentType (vendor/wheels/controller/rendering.cfc:738) passes params.format through verbatim, but both consumers validate against $acceptableFormats before building any template path — not vulnerable, no change.
• vendor/wheels/public/docs/core.cfm:77 does an unvalidated include "layouts/#request.wheels.params.format#.cfm", but that surface is production-blocked by Public.cfc $blockInProduction() (security(dispatch): verify /wheels/public/* is prod-gated or disable in production #2233) — dev-mode-only exposure, recommended as a separate low-severity follow-up finding rather than folding into this diff.

Tests

Added five regression cases to vendor/wheels/tests/specs/events/onerrorSpec.cfc ($getRequestFormat rejects unsafe format tokens (T4 LFI)): ../ traversal, slash/dot tokens, two valid alphanumeric formats preserved, and empty-string fallback. A private $requestFormatFor helper (mirroring the existing $expectedStatusFor pattern in the same spec) saves/restores url.format with try/finally so the harness's own ?format=json output detection is undisturbed.

Local verification on Lucee 7 + SQLite: red pre-fix (3 of the 5 cases fail — the pre-fix code returns url.format verbatim, so the traversal/empty cases come back unchanged), green post-fix (13 pass / 0 fail for the bundle). Per-PR CI runs the full engine × DB matrix.

Cross-engine notes

• No closures, no catch-body local assignments (the finally only mutates the shared url struct), no attributeCollection, no Left(str, 0), no reserved-scope parameter names (formatValue).
• $getRequestFormat keeps public access with the $ prefix (mixin invariant — $integrateComponents only copies public methods) and its EventHandlerInterface signature is unchanged.
• Regex nano-quirk noted during review: both ORO and java.util.regex allow $ to match before a single trailing newline, so "json\n" passes the gate — but the accepted value is then alnum + one \n (no slash/dot/backslash possible), FileExists misses, and execution lands on the safe onerror.cfm fallback. Harmless, not a bypass; tightening to \A…\z is an optional cosmetic follow-up.

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer A

This PR closes a path traversal / local file inclusion vulnerability in `$getRequestFormat()`, where `url.format` was interpolated verbatim into the error-template include path guarded only by `FileExists`. The fix is minimal, correctly placed at the source, and covers both consumers (the `showErrorInformation` dev branch at line 47 and the production template branch at line 128). Test coverage is solid and cross-engine safe. Two minor nits: a missing CHANGELOG entry and over-long inline comments. Verdict: comment.

Correctness

The fix is sound. `ReFind("^[A-Za-z0-9]+$", url.format)` returns 0 (CFML-falsy) on no match, so the `if` correctly falls through to `"html"` for traversal tokens, dots, slashes, null bytes, and empty strings. The `+` quantifier prevents empty strings from matching. The regex covers every documented format key (`html`, `xml`, `json`, `csv`, `pdf`, `xls`) and any `addFormat()` extension. Pre/post-fix test counts described in the PR body (3 red pre-fix: traversal, slash/dot, and empty-string cases; all 5 green post-fix) are internally consistent with the original code path.

The PR body correctly notes the trailing-newline edge case: Java's `$` matches before a trailing `\n`, so `"json\n"` passes the gate. The resulting include path (`onerror.json\n.cfm`) fails `FileExists` on any sane filesystem, so execution falls to the safe default. This is harmless, not a bypass — tightening to `Trim()` before the check, or validating against `$get("formats")` keys, would be a cleaner follow-up.

The deferred `vendor/wheels/public/docs/core.cfm:76` (`include "layouts/#request.wheels.params.format#.cfm"`) is correctly out of scope: `Public.cfc::$blockInProduction()` guards the entire `/wheels/public/docs` surface, limiting exposure to development mode.

Docs

`CHANGELOG.md` has no `[Unreleased]` section and no entry for this fix. Security patches should always appear in the changelog so operators upgrading know what was fixed. Please add:

```markdown

[Unreleased]

Security

• `$getRequestFormat` now rejects non-alphanumeric `url.format` values, preventing
  path traversal / local file inclusion via the error-template include path in
  `$runOnError` (#2900)
  ```

Conventions

`vendor/wheels/events/EventMethods.cfc:332–339` — The 8-line block comment explaining the security rationale violates CLAUDE.md's "one short line max" rule for inline comments. The WHY is genuinely non-obvious, but one line covers it:

```cfm
// Security: reject non-alphanumeric url.format to prevent LFI via the error-template path.
if (ReFind("^[A-Za-z0-9]+$", url.format)) {
```

`vendor/wheels/tests/specs/events/onerrorSpec.cfc:66–75` and `100–105` — The 9-line block comment preceding the new `describe` and the 5-line docstring on `$requestFormatFor` both violate the same convention. A single comment line on the `describe` is enough; the helper needs no separate docstring. Note that the pre-PR test file already has the same multi-line style (lines 21–30), so this is consistent — the convention just asks us not to add new ones.

These are nits and don't affect correctness or security.

Commits

`fix(events): validate url.format to prevent error-template path traversal` — 72-char header, valid `fix` type, `events` scope, lowercase subject. Clean.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is accurate and well-grounded. All findings check out against the actual diff; the security mechanic analysis is correct; no false positives; no missed correctness or compat issues. The only mild concern is verdict calibration: A used comment (non-blocking) rather than request-changes for a missing CHANGELOG entry on a security patch. That said, the convergence rules allow converged-changes for a comment verdict with concrete actionable findings, which applies here.

Sycophancy

Minor verdict-calibration concern: A issued comment (non-blocking) rather than request-changes for a missing CHANGELOG entry on a security patch. Operators need to know what was fixed when upgrading; that warrants a harder block than a comment. This doesn't undermine the findings, just undersells the CHANGELOG one slightly.

False positives

None detected. I verified all four claims against the actual diff:

• EventMethods.cfc comment block: exactly 8 lines of // comments added (diff lines 10-17). A's description is accurate.
• onerrorSpec.cfc pre-describe comment block: exactly 9 lines of // comments (diff lines 33-41). Accurate.
• $requestFormatFor docstring: 6-line /** */ block (diff lines 67-72). A counted it as 5-line, but the finding is correct regardless of how you count delimiter lines.
• No CHANGELOG.md changes: the diff contains no CHANGELOG.md hunk, confirming A's claim.

All findings stand.

Missed issues

None substantive. I re-checked:

• DCO sign-off: present (Signed-off-by: Peter Amiri <peter@alurium.com>). A didn't call it out explicitly but it passes; no issue.
• Cross-engine: ReFind() is available on all supported engines (Lucee 5/6/7, Adobe CF, BoxLang). No compat concern.
• Test helper CreateObject without full framework init: the url.format branch of $getRequestFormat calls no framework helpers ($get(), etc.), so a bare EventMethods instance is sufficient. Correct reasoning in the PR body.
• Test scope isolation: the try/finally in $requestFormatFor correctly restores url.format in all exit paths. No issue.
• URL-encoded traversal (%2e%2e%2f): CFML decodes query parameters before populating the url scope, so url.format would contain the decoded ../. The regex rejects it. The existing traversal test case covers this.

Verdict alignment

comment is consistent with finding two nits plus a documentation gap, but request-changes would be more appropriate given the CHANGELOG finding is non-trivial for a security patch. The code fix itself is sound; no correctness or security issues remain in the diff.

Convergence

Aligned on the finding set. Both the CHANGELOG entry and the comment-block trimming are concrete and actionable:

1. Add an [Unreleased] section to CHANGELOG.md with a security entry for this fix (per A's suggested text).
2. Reduce the 8-line // block in vendor/wheels/events/EventMethods.cfc (before the ReFind guard) to a single line.
3. Reduce the 9-line // block in vendor/wheels/tests/specs/events/onerrorSpec.cfc (before the new describe) to a single line.
4. Remove the 6-line /** */ docstring on $requestFormatFor (or replace with one line if explanation is still warranted).

Address-review can apply all four changes. Converging on changes needed.

[wheels-bot]

Wheels Bot — Address Review (round 1)

Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1):

• CHANGELOG.md — added an [Unreleased] section with a ### Security entry citing fix(events): validate url.format to prevent error-template path traversal #2900, matching the text Reviewer A suggested. Operators upgrading now see the LFI fix in the release notes.
• vendor/wheels/events/EventMethods.cfc — collapsed the 8-line // rationale block before the ReFind guard into a single line (// Security: reject non-alphanumeric url.format to prevent LFI via the error-template include path in $runOnError.), per CLAUDE.md's inline-comment convention.
• vendor/wheels/tests/specs/events/onerrorSpec.cfc — collapsed the 9-line block before the new describe("$getRequestFormat rejects unsafe format tokens (T4 LFI)", …) into one line, and removed the 6-line /** */ docstring on the $requestFormatFor helper (the helper name + 13-line body are self-describing).

No code or test behavior changed — these are pure documentation / comment-density adjustments.

Test re-run: the local tools/test-local.sh events harness is not available in this sandboxed environment (sed -i '' is BSD-only syntax and trips on Linux), so the framework's per-PR CI matrix is the authoritative gate here. Because none of the three changes alter runtime semantics — comment removals and a CHANGELOG entry only — the existing five $getRequestFormat regression cases continue to exercise the same code path the PR body validated as 13 pass / 0 fail on Lucee 7 + SQLite pre-push.

The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on approve or the outer-loop cap (5 rounds) is reached.

[wheels-bot]

Wheels Bot — Reviewer A

This follow-up commit (31dd6d4) addresses all three findings raised in my round-1 review: the missing [Unreleased] CHANGELOG entry is now present, the 8-line rationale comment in EventMethods.cfc has been collapsed to a single line, and both the 9-line describe block-comment and the 6-line /** docstring on $requestFormatFor in onerrorSpec.cfc have been removed. No functional code or test behavior was altered. The PR is clean; approve.

Correctness

The security fix itself is unchanged and remains sound. The single-line comment in EventMethods.cfc:330 preserves the non-obvious WHY at a glance without bloating the function body. The $requestFormatFor test helper's structure is correct: result is assigned in the try body (not a catch body), so the BoxLang invariant (catch-body local assignments not persisting) does not apply. The url scope access is legitimate CFML scope usage — no reserved-scope naming collision.

Tests

The five regression cases are intact and the test file structure is sound: the two describe blocks are siblings inside run(), the $requestFormatFor helper is a private component-level function (mirroring the pre-existing $expectedStatusFor pattern), and the try/finally save-restore correctly limits url-scope mutation to the duration of each test call.

Commits

fix(events): address Reviewer A/B consensus findings (round 1) — 61-char header, valid fix type, events scope, lowercase subject. DCO Signed-off-by trailer present and matches the bot author identity. Clean.

Docs

CHANGELOG.md [Unreleased] Security section matches the exact wording suggested in round 1. No user-facing guide changes are needed for a security regression fix of this scope.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review checks out. No sycophancy, no false positives, no missed issues on the diff I re-scanned. Verdict alignment is correct.

Sycophancy

None detected. A's approval cites exactly what changed (CHANGELOG entry, two comment reductions) and re-verifies the security mechanic, test structure, and commit metadata.

False positives

None detected. All three claims verified against the diff:

• ## [Unreleased] section is present in CHANGELOG.md
• EventMethods.cfc comment collapsed to one line
• onerrorSpec.cfc block comment and $requestFormatFor docstring removed
• result is assigned in the try body (not catch), so A's BoxLang-invariant analysis is accurate
• arguments.formatValue is the parameter name; url is accessed as a scope, not a parameter — no reserved-scope collision

Missed issues

None detected. The security fix is sound, tests cover the five regression cases, and no cross-engine concerns apply to the changed lines.

Verdict alignment

A's approve is consistent with the findings — all round-1 nits addressed, no functional changes introduced.

Convergence

Aligned. Both reviewers agree the PR is clean and ready to merge. No further changes needed.

[bpamiri]

CHANGELOG entry deferred: with many parallel remediation PRs the per-branch [Unreleased] entries conflict on every sibling merge, so entries are being consolidated into a single changelog PR at the end of the campaign. The entry text is preserved verbatim by the merge captain.

[wheels-bot]

Wheels Bot — Reviewer A

The sole new commit on this round (85eca5b8) removes the [Unreleased] CHANGELOG entry that was added in round 2, deferring it to a consolidated campaign PR. The security fix (ReFind gate in $getRequestFormat) and all five regression tests are intact and unchanged. The code is clean. Verdict: comment — one documentation-risk nit on the deferral approach.

Correctness

No regression. vendor/wheels/events/EventMethods.cfc:330 retains the single-line // Security: comment and the ReFind("^[A-Za-z0-9]+$", url.format) guard. All five test cases in the $getRequestFormat rejects unsafe format tokens (T4 LFI) describe block are present and unchanged. The $requestFormatFor helper's try/finally save-restore pattern is correct; the result variable is assigned in the try body, not a catch body, so the BoxLang catch-body-local invariant does not apply.

Docs

CHANGELOG.md — The [Unreleased] Security entry has been removed. The commit message explains the rationale clearly: a dozen parallel remediation PRs all touch the same header line, so squash-merge order determines which one wins and the rest get conflicts. Deferring to one consolidated PR is a reasonable operational choice.

The residual risk is that the consolidated PR might stall or be forgotten, leaving no changelog trace of this CVE-class fix for operators who read the log before upgrading. Consider one of:

• Reference the campaign tracking issue in the PR description (e.g. "CHANGELOG entry deferred to #XXXX"), so there is a durable pointer from this PR to the follow-up.
• Or add a single one-liner comment in CHANGELOG.md at the [4.0.3] cut point as a marker: <!-- security entry for #2900 deferred to campaign consolidation PR -->, which costs nothing and survives merge.

Neither is a blocker; the fix is the important part.

Commits

chore: defer changelog entry to campaign consolidation — 51-char header, valid chore type, lowercase sentence subject. DCO Signed-off-by: trailer present and matching. Co-Authored-By: trailer present. Clean.

[wheels-bot]

Wheels Bot -- Reviewer B (round 1)

A's review is sound. One minor factual slip on character count; no false positives; no missed issues; verdict consistent with findings. Converging on approve.

Sycophancy

None detected. A verified the ReFind guard at EventMethods.cfc:330 and all five regression cases are present. A explicitly flagged a documentation-risk nit and chose comment rather than approve, which is the more conservative posture for a security fix.

False positives

• Commit header char count: A says "51-char header" for chore: defer changelog entry to campaign consolidation. Actual count is 54 chars. The error has no practical effect (54 is well within the 100-char limit) but is a factual slip worth noting. Not consequential.

No other false positives. A's assertion that the BoxLang catch-body-local invariant does not apply is correct: result is assigned in the try body, not a catch body, so it persists normally. A's DCO claim is accurate.

Missed issues

None detected. Re-scan of the diff:

• The ReFind gate correctly blocks all five test inputs (traversal, slash/dot, empty string) and preserves valid alphanumeric formats. The trailing-newline regex edge case noted in the PR description is non-exploitable -- not a missed issue.
• The $requestFormatFor helper's try/finally save-restore is structurally correct and cross-engine-safe.
• No cross-engine compat concerns: no closures over outer local, no attributeCollection = arguments, no Left(str, 0), no reserved-scope parameter names.
• The http_accept branch is deliberately untouched (only yields developer-configured format keys) -- correct call.
• All three commit headers are valid conventional-commit format well within the 100-char limit.

Verdict alignment

A used comment (non-blocking nit on CHANGELOG deferral). A explicitly stated "Neither is a blocker; the fix is the important part," making the comment verdict consistent with the findings and functionally equivalent to approve.

Convergence

Aligned. The security fix is correct, the regression tests are comprehensive, and the only open item is a non-blocking documentation suggestion. No changes are required to merge. Joint recommendation: approve.