fix(controller): sendFile honours absolute directory outside the web root - #3101
Conversation
…root When sendFile() received an absolute `directory` argument it post-processed the caller-supplied path instead of using it verbatim: the `/wheels` mapping fallback substring-hijacked any path containing "/wheels" on every engine, and the `ExpandPath()` fallback web-root-prefixed the path on Adobe CF (where ExpandPath resolves against the web root rather than returning absolute paths unchanged as Lucee does). Both broke the documented "must be a full path … outside of the web root" contract. Detect an absolute `directory` (leading "/" or a drive letter) and build `fullPath = directory & "/" & file` directly, skipping both rewrites. Relative filePath-based resolution and the `..`-traversal guard are unchanged. Fixes #3077 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR: This PR makes sendFile() honour an absolute directory argument verbatim, fixing both defects from #3077 (Adobe CF web-root-prefixing via ExpandPath(), and the /wheels substring hijack on all engines). The fix is correctly scoped — the absolute-path branch short-circuits before the legacy rewrites while leaving relative filePath resolution and the ..-traversal guard (which runs earlier, at miscellaneous.cfc:241-250) untouched. Two new BDD specs cover both defects with proper try/finally cleanup, the changelog fragment is present, and the commit is conventional with a matching DCO sign-off. Verdict: comment — two minor, non-blocking observations below; nothing blocks merge.
Correctness
Minor (non-blocking): sub-path file values behave differently in the new absolute branch. At vendor/wheels/controller/miscellaneous.cfc:283, the absolute branch sets local.file = arguments.file; verbatim. The old ExpandPath path re-derived local.file = ListLast(local.fullPath, "/"), so a call like sendFile(file="sub/report.pdf", directory="/abs/dir") previously (on Lucee, where this combination happened to work) produced the bare download name report.pdf — it now produces sub/report.pdf at local.name = local.file; (line 334), and the Content-Disposition sanitizer at line 360 strips backslashes but not forward slashes. The extension-guess fallback is similarly affected: at line 320, filter = "#local.file#.*" becomes "sub/report.pdf.*" against local.directory = "/abs/dir", which can't match, so a missing-extension sub-path file now throws Wheels.FileNotFound instead of being extension-guessed. This combination is undocumented (the docblock describes file as a file name and directory as the full path) and was entirely broken on Adobe before this PR, so I don't consider it blocking — but if you want parity, splitting any path component of arguments.file into the directory before line 284 (e.g. local.file = ListLast(local.normalizedFile, "/") with the remainder appended to local.directory) would restore the old display-name behavior.
Tests
The two new specs in vendor/wheels/tests/specs/controller/miscellaneousSpec.cfc:211-262 are well-targeted: the first exercises defect 1 (verbatim absolute directory), the second defect 2 (the /wheels-substring temp dir wheels3077-dl genuinely contains the /wheels substring the old findNoCase("/wheels", …) matched). Cleanup runs in finally with no for loops (Lucee 7 invariant 12 respected) and no local writes in catch (BoxLang invariant 11 not applicable).
Optional follow-up: the long-skipped spec "is specifying a directory" (miscellaneousSpec.cfc:264-266, skipped "to debug path issues in CI") exercises exactly the absolute-directory path this PR fixes — args.directory = ExpandPath(local.dir) now flows through the verbatim branch. It's likely un-skippable now; worth trying in a follow-up rather than this PR, since its prior CI flakiness was cross-engine.
Docs
The PR checklist honestly defers the framework/AI docs and the file-uploads-and-downloads.mdx caution removal to bot-update-docs.yml — fine for the pipeline. One note for that pass: the new behavior also means a literal directory="/wheels/..." (CFML-mapping-style virtual path) is now interpreted as a physical filesystem path, which is the intended resolution of the /wheels ambiguity per #3077 but is worth a sentence in the guide.
Commits
Single commit, header fix(controller): sendFile honours absolute directory outside the web root (74 chars, valid type/scope), body explains the why, Signed-off-by matches the commit author identity. Clean.
Cross-engine / Security (checked, no findings)
- New code avoids all documented engine traps:
Left(local.normalizedDir, Len(...) - 1)is guarded byLen(...) > 1(Lucee 7Left(str, 0)invariant), no closures, no reserved-scope parameter names, no bare tag-in-script statements. - The
..-traversal and null-byte guards atmiscellaneous.cfc:238-250run before the new branch on the raw arguments (URL-decoded, backslash-normalized), so the verbatim path introduces no new traversal surface; the Windows drive-letter regex^[A-Za-z]:correctly classifiesC:\…paths and UNC\\server\sharenormalizes to a leading/.
…on all engines Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Docs updatedAdded a doc commit to this PR:
|
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR: Re-review at head 3ec61e8c2. The only change since the previous review (of c33205a78, verdict: comment) is a one-line docs commit updating the sendFile bullet in web/sites/guides/src/content/docs/v4-0-0/digging-deeper/file-uploads-and-downloads.mdx:198 to say absolute directory paths are used verbatim on all engines. That claim is accurate — the new absolute-path branch at vendor/wheels/controller/miscellaneous.cfc:264-284 is plain engine-independent CFML (no ExpandPath(), no /wheels substring rewrite), and I confirmed no other stale caution about the old Adobe behaviour remains in the page (the remaining Aside type="caution" at line 74 concerns upload temp directories, which is unrelated and still correct). No blocking findings. Verdict: comment — one minor observation carried forward from the prior review, nothing blocks merge.
Correctness
Minor (carried forward, non-blocking): sub-path file values behave differently in the absolute branch. Re-verified at this head: vendor/wheels/controller/miscellaneous.cfc:283 sets local.file = arguments.file; verbatim, so sendFile(file="sub/report.pdf", directory="/abs/dir") yields a Content-Disposition display name of sub/report.pdf (the sanitizer at line 360, ReReplace(local.name, "[\r\n""\\]", "", "all"), strips backslashes but not forward slashes), and the extension-guess fallback at line 320 builds filter = "sub/report.pdf.*" which cannot match. This combination is undocumented (the docblock describes file as a file name) and was entirely broken on Adobe before this PR, so it remains non-blocking — splitting any path component of arguments.file into local.directory before line 284 would restore old display-name parity if desired in a follow-up.
Docs
The new guide wording at file-uploads-and-downloads.mdx:198 ("absolute paths are used verbatim on all engines") matches the implemented behaviour and the changelog fragment changelog.d/3077-sendfile-absolute-directory.fixed.md. One trivial note: the PR body's checklist still marks "Framework Docs — left to bot-update-docs.yml", but this head commit now performs that guide update itself — worth ticking the box so the doc-bot doesn't redo it.
Commits
Both commits conform to commitlint.config.js: fix(controller): sendFile honours absolute directory outside the web root and docs(web/guides): clarify sendFile absolute directory works verbatim on all engines — valid types, free-form scopes, subjects under 100 chars, not ALL-CAPS, and each carries a matching Signed-off-by: trailer.
Tests / Cross-engine / Security (checked, no new findings)
Unchanged since the prior review: the two specs at vendor/wheels/tests/specs/controller/miscellaneousSpec.cfc:211-262 cover both #3077 defects with try/finally cleanup containing no for loops (Lucee 7 finally invariant) and no catch-scoped local writes (BoxLang invariant); the Left() call at miscellaneous.cfc:280 is guarded by Len(...) > 1 (Lucee 7 Left(str, 0) invariant); the ..-traversal and null-byte guards at lines 238-250 run on the raw arguments before the verbatim branch, so no new traversal surface is introduced.
A purely syntactic absolute-path check sent the long-standing webroot-relative idiom (directory="/reports/") down the verbatim branch, so it stopped resolving against the web root via ExpandPath() — on Adobe CF that idiom was previously the only working form of the directory argument. Gate the verbatim branch on the directory actually existing on disk: real absolute paths (the #3077 fix) still bypass the rewrites, and non-existent root-anchored paths fall through to the legacy resolution. Adds a regression spec for the webroot-relative idiom. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
…nnot-serve-files-out Resolves the file-uploads-and-downloads.mdx conflict against ##3096's rewritten wording: the it-is-currently-broken caution is replaced with the fixed-behavior sentence while keeping develop's other improvements. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> # Conflicts: # web/sites/guides/src/content/docs/v4-0-0/digging-deeper/file-uploads-and-downloads.mdx
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR: Re-review at head 81e0ce72d. The delta since the previous review (of 3ec61e8c2, verdict: comment) is commit 4534e02ab — gating the verbatim absolute-directory branch on DirectoryExists() so the long-standing webroot-relative idiom (directory="/reports/") keeps resolving against the web root — plus a third regression spec covering that idiom, matching changelog/guide wording updates, and a clean merge of develop (the PR diff still touches only the same four files). The gate is the right call: a purely syntactic leading-/ check would have broken the only form of directory that historically worked on Adobe CF, and the new spec at vendor/wheels/tests/specs/controller/miscellaneousSpec.cfc:264-291 pins that behaviour. No blocking findings. Verdict: comment — two minor, non-blocking notes below.
Correctness
Minor (non-blocking): the leading-slash idiom's meaning now depends on disk state. The gate at vendor/wheels/controller/miscellaneous.cfc:281-283 (local.isAbsoluteDirectory = Len(...) && (Left(...) == "/" || REFind("^[A-Za-z]:", ...)) && DirectoryExists(local.normalizedDir)) means directory="/reports/" resolves against the web root only while no /reports directory exists at the filesystem root — if an operator later creates one, the same call silently flips to serving from the filesystem path. This is an inherent ambiguity of overloading a leading / for two contracts, the chosen precedence (filesystem path wins when both exist) is the documented one — stated in the code comment at lines 272-275, the changelog fragment, and the guide bullet at file-uploads-and-downloads.mdx:200 — and the collision shape is narrow, so this is an observation, not a request. If you want to harden it later, an explicit boolean argument (e.g. absolute=true) would remove the heuristic entirely.
Minor (carried forward, non-blocking): sub-path file values behave differently in the absolute branch. Re-verified at this head: miscellaneous.cfc:287 sets local.file = arguments.file; verbatim, so sendFile(file="sub/report.pdf", directory="/abs/dir") yields a Content-Disposition display name of sub/report.pdf (the sanitizer at line 364 strips backslashes but not forward slashes), and the extension-guess fallback at line 324 builds filter = "sub/report.pdf.*", which cannot match. Undocumented combination (the docblock describes file as a file name) and entirely broken on Adobe before this PR — fine to leave for a follow-up.
Tests
The new third spec ("still resolves a leading-slash webroot-relative directory against the web root", miscellaneousSpec.cfc:264-291) is well-constructed: it creates the directory under the web root via ExpandPath("/dlprobe3077_rel"), passes directory="/dlprobe3077_rel/", and therefore exercises exactly the fall-through the gate exists to preserve (the DirectoryExists("/dlprobe3077_rel") probe at the filesystem root is false, so resolution proceeds via the legacy ExpandPath() branch at lines 313-318). Cleanup runs in finally with no for loops (Lucee 7 invariant 12) and no catch-scoped local writes (BoxLang invariant 11 n/a); args/_controller come from the suite's beforeEach at lines 94-99, matching the sibling specs. The two earlier #3077 specs are unchanged.
Docs
The updated guide bullet at web/sites/guides/src/content/docs/v4-0-0/digging-deeper/file-uploads-and-downloads.mdx:200 now accurately describes both behaviours ("absolute paths that exist on disk are used verbatim … a leading-slash path that does not exist on disk … resolves relative to the web root"), and the changelog fragment changelog.d/3077-sendfile-absolute-directory.fixed.md was updated in 4534e02ab to state the same gate. Trivial carry-forward: the PR body's "Framework Docs" checklist box is still unticked even though the guide update ships in this PR — worth ticking so bot-update-docs.yml doesn't redo it.
Commits
4534e02ab (fix(controller): gate sendFile verbatim directory on DirectoryExists) conforms to commitlint.config.js — valid type/scope, 64-char subject, body explains the why (the webroot-relative idiom regression the gate prevents), and carries a Signed-off-by: matching the author. The merge commit 81e0ce72d is exempt under commitlint's default merge-commit handling. The two earlier commits were reviewed previously and are unchanged.
Cross-engine / Security (checked, no new findings)
The gating code introduces no engine traps: the Left(local.normalizedDir, Len(local.normalizedDir) - 1) trailing-slash trim at miscellaneous.cfc:278-280 is guarded by Len(...) > 1 (Lucee 7 Left(str, 0) invariant), DirectoryExists/REFind are engine-uniform, and there are no closures, reserved-scope names, or bare tag-in-script statements. Security posture is unchanged from the prior review: the ..-traversal and null-byte guards at lines 238-250 run on the raw arguments before the verbatim branch, and the DirectoryExists gate only narrows the set of inputs that reach the verbatim path relative to the previously-reviewed head.
…tions metadata lock (#3152) * fix(controller): memoize $getStatusCodes, hoist column tokenization, log $includeFile blanked columns Closes the three rendering residuals of the #2961 roll-up (C13/C16/C17): - C17: $getStatusCodes() rebuilt a 63-entry constant struct on every render path. It is now built once per application lifetime and memoized in the application scope together with a deterministic reverse (text-to-code) lookup; $returnStatusCode() reads that lookup instead of running StructFindValue over the rebuilt struct, and duplicated status texts (Unassigned at 427/430/509) deterministically resolve to the lowest code. The numeric branch of $setRequestStatusCode() keeps its validation call but no longer assigns the unused text. - C16: $includeFile() re-ran ListToArray(query.columnList) inside both per-row loops; the column list is constant per query so it is tokenized once above the loops. - C13: the blanket catch that blanked a column $includeFile() could not read now logs a warning (once per column per render) to the wheels log naming the column, partial, first failing row, and underlying error before defaulting to an empty string. Refs #2961 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com> * fix(model): lock the context-independent metadata fill-ins in $expandedAssociations The metadata fill-ins above the JOIN-variant memo (foreign/join keys, table name, column/property lists and structs) wrote the shared application-scoped association struct on every call without a lock — the same unlocked-shared-struct pattern #2910 fixed for the JOIN string itself, flagged in the #2952 coverage-audit comment. They are now filled once under the same double-checked named lock (wheelsJoinMemo), with a fill-once marker written last so lock-skipping readers only ever observe a fully populated metadata set. The values are derived solely from class data, so fill-once is equivalent to the previous per-call rewrite. Refs #2952 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com> * test: fix Adobe-only compile crashes in sendFile and seeder specs Adobe CF validates built-in argument counts and ValueList() operands at COMPILE time, and the core runner compiles every spec in the directory, so each of these crashed the entire Adobe 2023 suite (0 specs run): - miscellaneousSpec.cfc (#3101): DirectoryCreate(path, true) — the createPath boolean is Lucee-only; Adobe's DirectoryCreate takes exactly one parameter. Three call sites, all with existing parents, switched to the single-argument form. - seederSpec.cfc (#3107): ValueList(model(...).findAll(...).id) — Adobe only accepts a plain query.column reference inside ValueList(). The query is assigned to a variable first. Verified: full core suite on Adobe 2023 + SQLite goes from a whole-suite onRequest compile error to 4462 pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com> --------- Signed-off-by: Peter Amiri <peter@alurium.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
sendFile()documents thatdirectory"must be a full path … outside of the web root", but the path-resolution block invendor/wheels/controller/miscellaneous.cfcpost-processed an already-absolute caller-supplieddirectoryinstead of using it verbatim, breaking that contract two ways:#873branch calledExpandPath(). Lucee returns existing absolute paths unchanged so it happened to work; Adobe always resolves against the web root, sosendFile(file="secret.txt", directory="/tmp/dlprobe")threwWheels.FileNotFound. The documented serve-from-outside-the-webroot feature did not work at all on Adobe./wheelssubstring hijack. AfindNoCase("/wheels", fullPath)substring match meant any directory containing/wheels(e.g./var/www/wheels/uploads,/tmp/wheels-dl) was silently rewritten.The fix detects an absolute
directory(leading/or a Windows drive letter) and buildsfullPath = directory & "/" & fileverbatim, skipping both the/wheelsmapping rewrite and theExpandPath()fallback. RelativefilePath-based resolution and the existing..-traversal guard (which runs earlier) are untouched.Related Issue
Fixes #3077
Type of Change
Feature Completeness Checklist
Signed-off-by:vendor/wheels/tests/specs/controller/miscellaneousSpec.cfc: one serves a file from an absolute directory outside the web root (defect 1 / Adobe guard), one asserts an absolute directory containing the/wheelssubstring is not rewritten (defect 2 — failing → passing on Lucee 7)bot-update-docs.yml(the doc caution atfile-uploads-and-downloads.mdxline 198 can be removed there)bot-update-docs.ymlbot-update-docs.ymlchangelog.d/3077-sendfile-absolute-directory.fixed.mdTest Plan
Ran the Lucee 7 + SQLite core suite against the local server (
tools/test-local.shequivalent — the script'ssed -i ''is BSD-only and fails on this Linux runner, so the same/wheels/core/testsendpoint was hit directly):directory=wheels.tests.specs.controller): 0 fail, 0 error after the fix; both new specs failed withWheels.FileNotFoundbeforehand.Screenshots / Output
n/a