fix(controller): sendEmail multipart handling and sendFile path containment

[bpamiri]

Summary

Fixes nine review findings in the controller mailer (sendEmail) and file-streaming (sendFile) mixins in vendor/wheels/controller/miscellaneous.cfc. The sendEmail fixes remove three crash paths (3+ templates, layout="", detectMultipart=false without type), make detectMultipart=false actually honor the documented text-first template order, emit a real CRLF-CRLF blank line in writeToFile output, stop email view arguments from leaking into (or permanently overwriting parts of) the controller's variables scope, and pass Adobe's S/MIME signing/encryption attributes through to cfmail instead of silently swallowing them. The sendFile fixes add a path-traversal guard on file/directory (null-byte strip + .. rejection after URL-decode and backslash normalization, same shape as $generateIncludeTemplatePath) and sanitize the download display name so it cannot break out of the quoted Content-Disposition filename parameter.

Findings addressed

• ML1 [High] Out-of-bounds layout indexing crashes on 3+ templates or an empty layout string @ vendor/wheels/controller/miscellaneous.cfc:42-45 — zero-length layout is coerced to false before the list split, and the 3+-template sub-cases now route into the ML2 throw before the render loop.
• ML2 [High] Hardcoded two-part typing makes any 3+-part email impossible @ vendor/wheels/controller/miscellaneous.cfc:47-55 (docblock at :10) — ListLen(arguments.template) > 2 now throws a friendly Wheels.IncorrectArguments documenting the 2-template limit, before any side effects.
• ML3 [High] detectMultipart=false + single template + no type → undefined-variable crash @ vendor/wheels/controller/miscellaneous.cfc:135-138 — new else if defaults arguments.type = "text" (matching cfmail's own default) when no explicit type is passed.
• ML4 [Medium] detectMultipart=false does not actually disable multipart detection @ vendor/wheels/controller/miscellaneous.cfc:91-103 — the <-count reordering is now gated on arguments.detectMultipart; when false, the given order is preserved and labeled [1]=text / [2]=html per the documented contract.
• ML5 [Low] writeToFile joins text and html bodies with a single bare CR, not a blank line @ vendor/wheels/controller/miscellaneous.cfc:176-184 — plain concatenation with a real CRLF-CRLF separator (CFML list functions treat each delimiter character separately); separator skipped when either side is empty.
• ML6 [Medium] Custom email-view arguments are injected into the controller's variables scope and never cleaned up @ vendor/wheels/controller/miscellaneous.cfc:62-72 (snapshot) and :109-118 (restore) — shadowed values are restored and added keys deleted after the email templates render.
• ML7 [Medium] Unknown mail attributes are silently swallowed by the hardcoded allowlist @ vendor/wheels/controller/miscellaneous.cfc:40 — allowlist gains sign, keystore, keystorepassword, keyalias, keypassword, encrypt, recipientcert, encryptionalgorithm; docblock at :5 now documents the allowlist semantics.
• C10 [Medium] sendFile() has no path-traversal guard on file/directory @ vendor/wheels/controller/miscellaneous.cfc:225-238 — null bytes stripped, then .. rejected on the URL-decoded, backslash-normalized file AND directory before any path math; throws Wheels.InvalidPath. Blocks literal, percent-encoded, and backslash variants.
• SEC-23 [Low] Content-Disposition filename built from an unsanitized display name in sendFile() @ vendor/wheels/controller/miscellaneous.cfc:322-323 — [\r\n"\\] stripped from the display name at the finalization point, covering both the deliver=true header and the deliver=false test struct.

Findings verified already-fixed

None — all nine findings reproduced against origin/develop at implementation time (zero stale references). The reviewer independently spot-checked ML3 (develop's if without else at the type-assignment site), ML5 (develop's 4-character ListAppend delimiter), and C10 (no guard on develop) and confirmed all were live bugs.

Source

Internal multi-agent framework review 2026-06-09, wave 2, package controller-mailer-sendfile.

Tests

11 new BDD specs in vendor/wheels/tests/specs/controller/miscellaneousSpec.cfc (plus a new view asset vendor/wheels/tests/_assets/views/test/bracketsemailtemplate.cfm), each written to fail against the pre-fix code: path-traversal rejection (literal, encoded, backslash, and directory variants), display-name sanitization, writeToFile blank-line bytes, layout="", >2 templates, detectMultipart=false without type, order preservation under detectMultipart=false, variables-scope restore (shadow + leak), and S/MIME attribute pass-through.

Local verification: single-bundle run of wheels.tests.specs.controller.miscellaneousSpec on Lucee 7 + SQLite via the prebuilt docker test image — HTTP 200, 39 pass / 0 fail / 0 error (1 pre-existing intentional skip). Full cross-engine coverage deferred to the CI compat-matrix, which is the real gate.

Cross-engine notes

No new closures, no arguments-as-attributeCollection, no Left(str, 0), no local assignment inside catch; all touched functions remain public. The ReReplace("[\r\n""\\]", ...) sanitizer has direct in-tree precedent in vendor/wheels/controller/sse.cfc (runs on all engines), and the URLDecode + backslash-normalize traversal guard mirrors $generateIncludeTemplatePath (vendor/wheels/global/rendering.cfc). The new test asset's bare < sequences are not tag-parseable (Lucee tag-scanner safe). Reviewer flagged two non-blocking follow-up suggestions (try/finally around the ML6 restore; padding layoutArray to template count for the degenerate layout="," input) — deferred to keep this diff minimal.

Changelog

Entry deliberately omitted; consolidated at campaign end.

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This PR fixes nine real bugs across sendEmail (multipart crash paths, variables-scope leak, S/MIME attributes) and sendFile (path traversal, Content-Disposition injection) with solid BDD coverage. The cross-engine notes check out, the $mail wrapper already copies arguments to a plain struct so Adobe 2023/2025 is fine. Requesting changes for one correctness gap in the ML6 restore and flagging a test-description discrepancy and a minor edge-case note.

---

Correctness

ML6 restore: no try/finally leaves variables scope dirty on render exceptions

vendor/wheels/controller/miscellaneous.cfc, lines 76–118

// render loop — can throw if template file is missing or view code errors
for (local.i = 1; local.i <= local.iEnd; local.i++) {
    local.content = $renderView($template = local.item, $layout = local.layoutArray[local.i]);
    ...
}

// restore loop — never reached if $renderView threw above
local.iEnd = ArrayLen(local.customViewVariables);
for (local.i = 1; local.i <= local.iEnd; local.i++) {
    local.key = local.customViewVariables[local.i];
    if (StructKeyExists(local.shadowedVariables, local.key)) {
        variables[local.key] = local.shadowedVariables[local.key];
    } else {
        StructDelete(variables, local.key);
    }
}

If $renderView throws (template not found, view code error, etc.), the restore loop is skipped and any custom arguments the developer passed remain injected into the controller's variables scope. This affects subsequent rendering in the same controller instance — including onError handlers that call renderPage. The PR body explicitly calls this a "non-blocking follow-up suggestion," but since the restore code is already written and the fix is a one-liner to hoist into a try/finally, deferring it leaves ML6 partially fixed:

try {
    for (local.i = 1; local.i <= local.iEnd; local.i++) {
        local.item = local.templateArray[local.i];
        local.content = $renderView($template = local.item, $layout = local.layoutArray[local.i]);
        ...
    }
} finally {
    // Restore inside finally so it runs even if $renderView throws.
    local.iEnd = ArrayLen(local.customViewVariables);
    for (local.i = 1; local.i <= local.iEnd; local.i++) {
        ...
    }
}

try/finally is supported on Lucee 5/6/7, Adobe CF 2018–2025, and BoxLang. Please address before merge.

---

Minor: Find("..") fires on .. as a substring, not only as a path component

vendor/wheels/controller/miscellaneous.cfc, line 229

Find("..", Replace(URLDecode(arguments.file), "\", "/", "all"))

Find("..", "summary..v2.pdf") returns non-zero, so a call like sendFile(file="report..final.pdf") throws Wheels.InvalidPath even though the path is legitimate. This is the same limitation carried by $generateIncludeTemplatePath (line 530 of rendering.cfc), so the behaviour is consistent within the framework. Not new to this PR, but worth documenting: the guard intentionally rejects filenames with .. anywhere in the component, and the error message should mention that filenames must not contain the two-dot sequence (not just directory-traversal attempts).

---

Tests

Backslash traversal test missing despite being listed in the PR description

The PR body says tests cover "literal, encoded, backslash, and directory variants." The actual new specs in miscellaneousSpec.cfc have tests for literal (../../../../config/settings.cfm), URL-encoded (%2e%2e/...), and directory, but there is no backslash variant for the file argument:

it("rejects backslash path traversal in the file argument", () => {
    args.file = "..\..\..\config\settings.cfm"
    expect(function() {
        _controller.sendFile(argumentCollection = args)
    }).toThrow("Wheels.InvalidPath")
})

The PathTraversalSpec for $generateIncludeTemplatePath includes an explicit backslash test (it("rejects partial names with backslashes", ...)). Adding the equivalent for sendFile keeps the test inventory consistent with the description and the sibling spec. Low effort, and it verifies the Replace(..."\", "/", "all") normalization path specifically.

---

Docs

No CHANGELOG.md [Unreleased] entry

The PR body states this is intentionally deferred to a campaign-end consolidation. That is noted; flagging only for completeness so reviewers don't accidentally merge without changelog coverage.

---

Cross-engine

No new concerns. The $mail() wrapper in Global.cfc (lines 84–87) already copies arguments to a plain struct before passing to cfmail, so the S/MIME attributes added to mailTagArgs will reach Adobe 2023/2025 without hitting the attributeCollection = arguments restriction. The ReReplace regex for display-name sanitization is confirmed in-tree precedent from sse.cfc. No new closures, no Left(str, 0), no local.X inside catch.

---

Commits

fix(controller): sendEmail multipart handling and sendFile path containment — valid type, valid scope, 67-char header, sentence case. Passes commitlint.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is accurate and well-evidenced. The try/finally finding is a real correctness gap, the backslash-test gap is a genuine inventory mismatch, and the Find("..") note is a fair observation. No sycophancy, no false positives. One minor test-coverage weakness A didn't flag is noted below. Aligning on CHANGES_REQUESTED and emitting converged-changes.

Sycophancy

None detected. A's approval language ("solid BDD coverage", "cross-engine notes check out") is backed by specific diff citations, and the verdict is CHANGES_REQUESTED — not an unearned approve.

False positives

None detected.

• try/finally finding: verified against the diff. The render loop iterates over local.templateArray calling $renderView. The restore loop appears sequentially afterwards with no exception guard. A's description of the failure mode (render exception → restore skipped → custom args stay in variables) is accurate. The try/finally fix pattern A supplied is standard CFML supported on all target engines.
• Cross-engine claim ("$mail wrapper copies arguments to plain struct"): the PR description confirms this explicitly, and the diff adds attributes only to local.mailTagArgs (the allowlist fed to the wrapper). A's read is correct.
• No new closures / no Left(str, 0) / no local.X in catch: confirmed by inspection of the diff. The new code is loop-based with no closures and no catch blocks.

Missed issues

Minor: variables-scope leak test does not distinguish fixed from unfixed behavior.

miscellaneousSpec.cfc (the "does not leak custom view arguments" spec):

_controller.sendEmail(argumentCollection = leakArgs) // leaks customArgument if unfixed
result = _controller.sendEmail(argumentCollection = args)
expect(result.text).toBe(textBody)

plainEmailTemplate does not reference customArgument, so result.text equals textBody whether or not the variable leaked. The test passes against the unfixed code too. A more discriminating form would use a template that outputs customArgument when present, or assert absence of the key on the controller's variables scope directly. This does not change the verdict — the try/finally fix is independently correct — but the test doesn't actually exercise the cleanup path it claims to cover. Not a blocker, but worth noting for the follow-up.

Verdict alignment

A's CHANGES_REQUESTED is consistent with the findings: one explicit blocker (try/finally), one test gap (backslash variant), and two informational notes. The verdict matches the severity.

Convergence

Aligned. A's two actionable items are:

1. Wrap the render loop in try/finally so the variables restore runs even when $renderView throws.
2. Add a backslash path-traversal test for the file argument (e.g. args.file = "..\\..\\config\\settings.cfm") to exercise the Replace(..."\", "/", "all") normalization branch.

Both are mechanical, low-risk, and have clear prior-art in the codebase ($generateIncludeTemplatePath guard, PathTraversalSpec backslash test). Emitting converged-changes.

[bpamiri]

Addressed the Reviewer A review in 97a10c2:

ML6 try/finally (blocking) — Agreed and fixed, with one engine-driven twist: putting the restore loop directly inside the finally block crashes Lucee 7 at runtime with variable [local] doesn't exist — Lucee 7 (verified on lucee@7.0.1+100) miscompiles for loops that use local.- or var-scoped variables inside finally blocks (minimal probe: a bare for (local.i = 1; ...) in any finally reproduces it; plain local.x = 1 assignments and function calls are fine). So the render loop is now wrapped in try, and the finally calls a new $restoreEmailViewVariables() helper (public + $-prefix per the mixin-integration rule) that contains the loop. A new spec (does not leak custom view arguments into the controller when rendering throws) covers the render-exception path.

Find("..") substring note — The Wheels.InvalidPath message/extendedInfo now state explicitly that two consecutive dots are rejected anywhere in the value (calling out report..final.pdf-style names), and the @file/@directory docblocks document the guard.

Missing backslash traversal test — Added rejects backslash path traversal in the file argument using Chr(92) construction, matching PathTraversalSpec's precedent. The PR description's test inventory is now accurate.

Changelog — still intentionally deferred to the campaign-end consolidation, as noted.

Verified wheels.tests.specs.controller.miscellaneousSpec on Lucee 7 + SQLite and Adobe 2023 + SQLite (mixin surface touched): both 41 pass / 0 fail / 0 error (1 pre-existing intentional skip).

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This follow-up commit addresses both blocking findings from the previous round: the sendEmail variables-scope restore is now wrapped in a try/finally block (with the restore loop extracted to $restoreEmailViewVariables to work around a Lucee 7 for-loop-in-finally miscompile), and the backslash traversal spec for sendFile is present. No new correctness, cross-engine, or security findings. Commenting only to flag a documentation gap — no changes required for merge.

---

Cross-engine

Lucee 7 for-loop-in-finally miscompile is real but undocumented

vendor/wheels/controller/miscellaneous.cfc, new $restoreEmailViewVariables function docblock and the finally comment.

The commit message records: "Lucee 7 miscompiles for loops that use local (or var) variables inside finally blocks ('variable [local] doesn't exist' at runtime) — verified with minimal probes against lucee@7.0.1+100."

The workaround is correct: extracting the restore for loop into a separate public $-prefixed helper means no for (local.i = ...) appears literally inside a finally block. The call site in finally only reads local.customViewVariables and local.shadowedVariables as call arguments — it does not declare or iterate them there. The verification run on Lucee 7 + SQLite and Adobe 2023 + SQLite confirms this is safe.

The gap: this behaviour is not recorded in .ai/wheels/cross-engine-compatibility.md or CLAUDE.md "Cross-Engine Invariants." Any future developer who reaches for a restore loop in a finally block on Lucee 7 will hit the same miscompile without any documented pointer. A one-paragraph entry modelled after the existing "createDynamicProxy Requires a CFC on Lucee 7" section would be sufficient:

for (local.i = ...) inside finally miscompiles on Lucee 7. Lucee 7.0.1+100 throws "variable [local] doesn't exist" at runtime when a for loop declares its iteration variable in the local scope inside a finally block. Workaround: hoist the loop into a helper function so the finally block contains only a function call (reference: $restoreEmailViewVariables in miscellaneous.cfc).

Not blocking — the in-code comment is adequate for the immediate reviewer. But worth capturing before the next contributor runs into it.

---

Docs

CHANGELOG entry still deferred. The PR body notes this is intentional (campaign-end consolidation). Flagging only so it is not forgotten at merge time.

---

Commits

Both commits are valid conventional-commit headers:

• fix(controller): sendEmail multipart handling and sendFile path containment — 67 chars
• fix(controller): run sendEmail variables restore in finally and add backslash traversal spec — 91 chars

Commit body for 97a10c25 includes DCO Signed-off-by trailer and co-author attribution.

---

Tests

The render-throws path ("does not leak custom view arguments into the controller when rendering throws") is present and directly tests the finally guarantee. The backslash traversal spec uses Chr(92) construction to avoid escape-sequence ambiguity, consistent with PathTraversalSpec.cfc. Both additions are correct.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

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

Sycophancy

None detected. A used COMMENTED (not APPROVE) state with an explicit "no changes required for merge" verdict, cited the specific round-1 fixes verified, and did not approve blind.

False positives

None that affect conclusions. A's commit header character counts are slightly off (commit 1 is 75 chars, not 67; commit 2 is 92 chars, not 91) but both are well under the 100-char limit and the verdict is correct.

Missed issues

None detected. I initially suspected the "does not leak" test pair was a false-pass because the template might not reference customArgument — but plainemailtemplate.cfm does: it conditionally appends the custom argument value to the body, so expect(result.text).toBe(textBody) is a conclusive assertion. A correctly called these tests as directly proving the finally guarantee.

A's documentation-gap flag (Lucee 7 for-in-finally not yet in .ai/wheels/cross-engine-compatibility.md) is accurate and the non-blocking classification is appropriate.

Verdict alignment

A's COMMENTED state with "no changes required for merge" is consistent with the findings — the two round-1 blockers (try/finally scope restore and backslash traversal spec) are both present and correct, and no new issues appear.

Convergence

Aligned. The implementation is correct, the round-1 blockers are fully addressed, and no new correctness, cross-engine, or security issues appear in the diff. Joint recommendation: merge when ready.

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: The third commit is documentation only — it adds the Lucee 7 for-loop-in-finally miscompile to both CLAUDE.md (invariant #12) and .ai/wheels/cross-engine-compatibility.md. This directly addresses the documentation gap called out in round 2. No code changes, no new correctness or cross-engine issues. Approving with two minor nits (no blockers).

---

Docs

Nit: example loop style in .ai/ is inconsistent with CLAUDE.md title

.ai/wheels/cross-engine-compatibility.md, new section WRONG example:

for (local.key in local.savedState) {

CLAUDE.md invariant #12 titles the rule with for (local.i = ...) (C-style numeric loop), but the extended doc example shows a for...in (foreach-over-struct) loop. Both forms are presumably affected by the Lucee 7 miscompile, but a reader who only hits the .ai/ page and uses a numeric loop might assume only the for-in variant is dangerous. A one-line parenthetical such as "Both for (init; cond; step) and for (item in collection) forms are affected" would close the gap. Not blocking.

CHANGELOG entry still deferred. Noted from prior rounds; flagging again so it is not forgotten at merge time.

---

Commits

• fix(controller): sendEmail multipart handling and sendFile path containment - 67 chars, valid
• fix(controller): run sendEmail variables restore in finally and add backslash traversal spec - 91 chars, valid
• docs: document the Lucee 7 finally-block loop miscompile invariant - 65 chars, valid

All three commits conform to commitlint.config.js. DCO Signed-off-by trailer present on the second commit (bot-authored); the third commit is similarly attributed.

---

Cross-engine

The documentation itself introduces no new CFML code. The RIGHT example correctly shows that calling a helper from finally (with local variables as arguments) is safe - the miscompile is specific to loop constructs, not to local-scope references in general. The note that the helper must be public (mixin invariant #7) is accurate and the reference to $restoreEmailViewVariables() in miscellaneous.cfc gives a concrete, in-tree anchor.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is clean. The docs-only commit correctly addresses the documentation gap A flagged in the previous round, and the APPROVED verdict is well-supported by the diff. No sycophancy, no false positives, no missed issues.

Sycophancy

None detected. A's approval cites specific evidence (docs-only commit, no new CFML code, accurate reference anchor) rather than a bare LGTM. The prior implementation was already converged-approve on SHA 97a10c25; this commit specifically addresses the gap A flagged there.

False positives

None detected.

• For-in vs C-style loop inconsistency: A correctly identifies that CLAUDE.md invariant imageTag edit: if height and/or width values are set to false, the attrib #12 titles with for (local.i = ...) (C-style numeric) while the .ai/ WRONG example uses for (local.key in local.savedState) (foreach-over-struct). Both forms are covered by the generic description in both locations, but the title/example mismatch is real. Non-blocking classification is appropriate.
• public mixin requirement note: A says "the helper must be public (mixin invariant New master - readme #7)" -- verified in the diff: $restoreEmailViewVariables is declared public void function, consistent with invariant New master - readme #7 scope rules. Accurate.

Missed issues

None detected. I re-scanned the diff against the CLAUDE.md cross-engine invariants and the CFML patterns listed there. The new documentation section in .ai/ correctly describes the failure mode, provides a minimal WRONG/RIGHT pattern, and anchors to the in-tree reference implementation. No code changes means no new compatibility surface to check.

Verdict alignment

A's APPROVED state is consistent with the findings: two non-blocking nits (loop-style inconsistency, deferred CHANGELOG), no actionable blockers on a docs-only commit. The verdict matches the diff scope.

Convergence

Aligned. The implementation was approved in the prior round; this third commit adds the missing documentation A required. Both nits are editorial and do not affect correctness. Joint recommendation: merge when ready.