fix(files): bound YAML expansion and buffered reads on the file-serve path - #7319
Merged
Conversation
… path Two unbounded resource paths reachable from the anonymous public share routes. YAML alias expansion: the page compiler parsed sim: fence payloads with no ceiling on what the parsed value expands to. Aliases are shared references, so `columns: &c [...]` plus a row list of aliases to it renders N^2 cells from ~13N source bytes — 25 KB of source cost 3.6s of CPU and 38 MB of HTML per request. The expansion guard already in the file parser is now a shared primitive taking caller-supplied limits, and the compiler charges every fence and the frontmatter to one per-compile budget so splitting across blocks buys no extra rendering. Buffered reads: the Files-module serve path already capped reads at MAX_BUFFERED_TRANSFER_BYTES via fetchWorkspaceFileBuffer, but five sibling paths serving the same objects did not, including both unauthenticated share routes. A workspace object is admitted at 5 GB, so a share link was the one way to make an anonymous request hold gigabytes resident in the shared process.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 13 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Re-trigger cubic
Contributor
Greptile SummaryThe PR bounds YAML expansion during page compilation and buffered file reads across authenticated and public serving paths, including the compiled-artifact lifecycle.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/copilot/tools/server/files/doc-compiled-store.ts | Caps compiled-artifact downloads at the widest consumer ceiling and preserves size-limit errors so serving routes return 413. |
| apps/sim/app/api/files/public/[token]/content/route.ts | Bounds the anonymous source read and checks the final bytes after document, page, and image transformations converge. |
| apps/sim/app/api/files/serve/[...path]/route.ts | Applies bounded reads across local and cloud paths and validates transformed output before responding. |
| apps/sim/lib/file-parsers/yaml-limits.ts | Introduces bounded iterative measurement of expanded YAML nodes, depth, keys, and serialized size. |
| apps/sim/lib/workspace-files/page-compile.ts | Shares one YAML expansion budget across frontmatter and every supported fence in a page compilation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
R[File request] --> S[Bounded source read]
S --> T{Transform required?}
T -- No --> F[Final size assertion]
T -- Page --> P[Bounded YAML compilation]
T -- Document --> A[Bounded artifact read]
T -- Image --> I[Derivative generation]
P --> F
A --> F
I --> F
F --> O[Serve response]
F -- Over limit --> E[HTTP 413]
Reviews (5): Last reviewed commit: "fix(files): bound the shared artifact re..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… guard's own walk Review round 1. Bounding the source read did not bound the response: every branch that replaces the source — a compiled document artifact fetched separately, a page with its images inlined, a transcoded image derivative — could turn a source under the ceiling into a response over it, on the anonymous share route included. The check now sits where those branches converge rather than on the page branch alone. The local read enforced its ceiling on a size measured before the read, so it described the file only as of the stat. It now reads through the same bounded stream reader the S3/Blob/GCS downloads use. The expansion guard held one stack frame per pending node, so proving a wide document too large allocated in proportion to the width it was rejecting. It now holds one frame per level of nesting and consumes each container through a lazy generator, bounding its own working set by depth. A double serializes to up to 24 characters, so numbers are charged their serialized length rather than the flat 16-byte allowance they could outgrow.
Collaborator
Author
The `raw=1` branch returned before the check, so the ceiling held for the branches that transform bytes but not for the one that returns them unchanged. Those bytes are already bounded by the read that produced them, so this changes no behavior — it makes the guarantee hold for everything the resolver returns rather than for every branch someone remembered to cover.
Collaborator
Author
Collaborator
Author
|
@cubic review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 14 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Fix all with cubic | Re-trigger cubic
…e response Review round 2. The response-level ceiling rejected an oversized compiled artifact only after the whole thing was resident, which is the allocation it exists to prevent. The artifact read is bounded at its own funnel instead, and a size breach is rethrown rather than folded into the "not built yet" null — that answer tells callers to retry, which an oversized artifact would never stop doing. The cached image derivative is bounded the same way, but keeps swallowing the breach, because a miss there re-transcodes from an already-bounded source. The local storage branch enforced its ceiling with stat-then-read, so it described the file only as of the stat. It now reads through the same bounded-stream reader the cloud branches use — this is the anonymous share path on a self-hosted deployment, and the route helper was already fixed while the storage service it sits next to was not. The default js-yaml schema turns a timestamp into a Date, which serializes to a 26-character quoted string; charging it the 16-byte flat allowance let a document of them exceed the byte cap.
Collaborator
Author
Collaborator
Author
|
@cubic review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 19 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Fix all with cubic | Re-trigger cubic
…iling Review round 3. The previous round bounded the compiled-artifact and cached-derivative reads at the rendered-document ceiling, which is half what the serving routes will return. That made the bound a policy rather than a backstop: an artifact between the two figures was refused even though the route accepts a response that size, and a derivative in that band read as a cache miss on every preview and re-transcoded the original each time. Both funnels now bound at the widest ceiling any of their consumers allows. A consumer that permits less still enforces its own limit on what it got back — the workspace download path continues to hold artifacts to the rendered-document ceiling.
Collaborator
Author
Collaborator
Author
|
@cubic review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 19 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Re-trigger cubic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two unbounded resource paths, both reachable from the unauthenticated public share routes.
YAML alias expansion in the page compiler.
loadYamlonsim:fence payloads had no ceiling on what the parsed value expands to. Aliases are shared references, soloadis cheap and the DAG is small — the cost lands in whatever walks that DAG as a tree.columns: &c [x ×N]plus a row list of aliases to it renders N² cells throughmarked.parseInlinefor ~13N source bytes: 25 KB of source measured at 3.6s CPU, 38 MB of HTML and ~123 MB RSS, growing quadratically from there.assertYamlWithinLimitsis now a thin wrapper over it; its limits and messages are unchanged.Buffered reads on the serve paths. The Files-module serve path already capped reads at
MAX_BUFFERED_TRANSFER_BYTESviafetchWorkspaceFileBuffer; five sibling paths serving the same objects did not. Workspace objects are admitted at 5 GB, so a share link was the one way to make an anonymous request hold gigabytes resident in the shared process.readFilehad no size check at all).downloadCopilotFilenow takes a requiredmaxBytes, matchingfetchWorkspaceFileBuffer, so the next caller has to name a ceiling.renderSimPageDocumentWithAssetsinlines up to 32 MB of images, so a source under the ceiling can resolve to a document over it.PayloadSizeLimitErrornow answers 413 instead of 500 and logs atinfo— it is a client-side answer, not an on-call page. This also fixes the 500 the already-capped workspace path returned.100 MB is the ceiling this codebase already converged on for buffered work and the one the Files-module serve path already enforced; this brings the rest of the serve paths onto the same line rather than introducing a new policy.
Type of Change
Testing
page-compile-limits.test.ts(8 tests) covering the alias bomb, the byte-vs-node distinction, the shared budget, the frontmatter path, and no-regression on ordinary pages. Verified non-vacuous: with the limits raised toMAX_SAFE_INTEGER, 5 of 8 fail.bun run lint,bunx turbo run type-check,bun run check:audits(40/40),check:block-registry,docs-manifest:check,check:migrationsall pass. 1,265 tests pass acrossapp/api/files,lib/workspace-files,lib/file-parsers,lib/uploads.Follow-up not in this PR
The cap is correct for paths that must transform (page compile, doc artifact resolution, HEIC derivative) — those need the whole buffer. Plain passthrough downloads do not and should stream;
downloadFileStreamalready exists for S3/Blob/GCS/local. That removes the ceiling for passthrough entirely, and needs a streaming response helper plus restructuringresolveServableBytesto take a lazy reader so the transform decision stays in one place. Worth its own PR.Checklist