fix(catalog): bind asset reads to checked project files - #3735
Conversation
Fallow audit reportFound 11 findings. Duplication (2)
Health (9)
Generated by fallow. |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
The individual source reads are substantially improved. scripts/catalog-payload-assets.ts:142-183 rejects lexical escapes and resolved external links, opens read-only/nonblocking, checks and reads the descriptor, and closes on every path. The four callers now hash/inline/copy the bytes returned by that one owner; empty files, internal links, and symlinked project roots remain intact. The 15 new focused tests pass locally, as do changed-file lint/format. Exact-head Build and Typecheck are green; required Test/Windows and JavaScript CodeQL remain pending.
Blocker — the directory byte cap is still a separate pathname check from the bytes published. directoryBytes() at scripts/catalog-payload-assets.ts:344-355 stats every candidate, then hostItemDirectory() authorizes the whole copy at :368-369; only afterward do walk()/walkInto() reopen the paths through readProjectFile() at :372-406. A file replaced after its budget stat but before the descriptor open is therefore charged at the old size and published at the new size.
I reproduced this at the exact head with an instrumented test: a one-byte internal asset.png was replaced with a 2,000,001-byte file immediately after directoryBytes() obtained its stat. hostItemDirectory() returned /item/ and wrote the full 2,000,001-byte asset, bypassing MAX_HOSTED_DIRECTORY_BYTES. I removed the probe afterward. The current replacement witnesses swap only after readProjectFile() has opened its fd, so they cannot see the earlier budget→open window.
Please make the budget and copy consume the same checked bytes—e.g. collect { relativePath, bytes } once, sum bytes.length before writing anything, then publish those exact buffers (including the _downloads mirror mapping). Add a replacement witness between budget discovery and source open, and preserve the current all-or-nothing behavior for over-budget directories.
Non-blocking CI note: optional Fallow audit is red at this head with 16 findings, including critical complexity on readProjectFile and a 61-line clone with the existing descriptor reader. It is not in the required set, but the PR should either simplify/share the helper or explicitly account for that result before calling CI clean.
Verdict: REQUEST CHANGES
Reasoning: Per-file identity is pinned correctly, but directory publication still authorizes bytes using stale pathname sizes, so the same replacement race bypasses the advertised 2 MB budget.
— Magi
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
The original budget/identity blocker is closed: hostItemDirectory() now collects each descriptor-checked buffer once, sums those exact lengths before any output write, and reuses the same map for original and _downloads mirror paths (scripts/catalog-payload-assets.ts:351-378). Internal aliases resolve to their real in-project prefix, external aliases avoid traversal, and the second publication loop preserves mirror-wins collision order. The new all-or-nothing, exact-limit, pre-open replacement, post-collection mutation, alias, and collision tests are well targeted. Focused suite is 20/20 and changed-file lint/format pass locally.
Blocker — an over-budget file is now fully buffered before the cap can reject it. readProjectFile() performs an unbounded readFileSync(fd) at scripts/catalog-payload-assets.ts:155-180; only after that allocation returns does hostItemDirectory() add bytes.length and compare against MAX_HOSTED_DIRECTORY_BYTES at :355-360. The old directoryBytes() stat path rejected a 2 MB+ file without reading it. Since HOSTED_EXTENSIONS includes .mp4 and .webm, a large catalog asset—or the same replacement attacker swapping in a huge sparse file before descriptor open—can OOM the script before the 2 MB guard executes. The new cap+1 test itself necessarily reads all 2,000,001 bytes; scaling that fixture exposes the regression.
Please make the directory collector's read bounded by the remaining budget: read at most remaining + 1 bytes from the checked descriptor (a fstat fast-reject is useful, but the bounded read must remain authoritative if the inode grows), signal over-budget without allocating the whole source, and keep reusing the accepted buffer for both output spellings. Add a test that proves an oversized/sparse source is never read beyond cap+1 while output remains all-or-nothing.
The optional Fallow tradeoff is adequately disclosed: the revision removes both duplicate walkers, reduces reader cyclomatic complexity 15→9, adds no suppression, and reserves unchanged cache-write clones for #851/#852. Its remaining red is not the blocker here.
Exact-head required checks are still running; the only completed failure is optional Fallow, already accounted for in the body.
Verdict: REQUEST CHANGES
Reasoning: The revised snapshot model fixes stale authorization and mirror drift, but enforcing the 2 MB output cap only after an unbounded read turns large or swapped-in media into a new memory-exhaustion path.
— Magi
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-review at the bounded-read head: both directory-publication blockers are closed.
readWithinBudget() at scripts/catalog-payload-assets.ts:155-168 reads the checked descriptor in ≤64 KiB chunks and stops after maxBytes + 1; readProjectFile() keeps that inside its existing descriptor finally at :171-199. hostItemDirectory() passes the remaining budget at :372-384, so accepted buffers total at most 2 MB, a sparse or growing source can consume at most cap+1 bytes, and the same accepted map remains authoritative for original and mirror writes. EOF, short reads, a zero-byte file at zero remaining budget, and inode growth all have the right result.
The expanded tests are strong: the 32 MB sparse file proves total actual reads stop at cap+1; the post-fstat growth case proves fstat is not the authority; the short-read case prevents truncation; the failure case pins descriptor cleanup; and the earlier all-or-nothing/mirror/alias/collision cases remain intact. I ran the focused suite (24/24) and changed-file lint/format; all pass.
The optional Fallow failure is adequately accounted for: no suppression, two duplicate walkers removed, and only one unchanged cache-write clone group remains alongside the small covered filesystem routines. It is not in the required set. Required Build/Typecheck/Windows are still running; exact-head required checks have no failures and remain landing gates with JavaScript CodeQL.
Verdict: APPROVE
Reasoning: Directory publication now enforces identity, output budget, and memory budget from the same bounded descriptor reads, then publishes only those captured buffers with preserved mirror semantics.
— Magi
2 participants
Catalog asset processing could check a source pathname and then read a different file after replacement. A shared reader now resolves references within the prepared project, opens read-only/nonblocking, checks and reads the same descriptor, and closes it on every path. It covers hosted/inlined references, mounted composition HTML, directory copies and download mirrors. This addresses CodeQL #850.
Directory publication reads at most the remaining 2 MB source budget plus one byte from each checked descriptor, in chunks of at most 64 KB. It charges the resulting buffer lengths before any output write, so oversized or concurrently growing media cannot cause an unbounded allocation. Both directory layouts and download mirrors reuse those buffers, so replacement between discovery and open cannot bypass the cap, and later changes cannot alter mirrored bytes. The existing source-counting policy, exact-limit acceptance, mirror collision precedence and internal downloads directory aliases are preserved.
External file links and sibling-prefix mounted-composition escapes remain unresolved instead of publishing files outside the prepared project. Internal links and symlinked project roots still work. Asset hashing, MIME handling, empty-file behavior, reference reporting and output write semantics are preserved. The prepared project tree remains trusted against hostile concurrent parent-directory renames. Cache-write alerts #851/#852 are reserved for the next narrow batch.
Validation: all 254 script/catalog tests (188 Node + 66 Vitest), script typecheck, dependency builds, lint/format and signed hooks pass. The 24 focused cases cover pathname replacement, external paths, directory budget replacement, all-or-nothing rejection, exact-limit mirrors, internal aliases, FIFO, short descriptor reads, sparse oversized inputs, growth after fstat and descriptor cleanup. Three bounded-read cases fail on the previous PR head and pass on this revision; the earlier four budget/mirror regression witnesses remain green. The suite is registered in the existing Windows lane; fresh CI and Windows results are pending.
Optional Fallow audit remains red locally: complexity and duplication findings in the small filesystem routines and unchanged cache-write blocks. This revision removes two duplicate directory walkers and simplifies repeated missing-file handling. Remaining new routines are small, covered filesystem checks/traversal; the descriptor cleanup pattern resembles the engine reader, and the two unchanged cache-write blocks are covered by the next batch. No audit suppressions were added. Required checks, CodeQL and approval of the current commit remain merge gates.