fix(basket): stop buffering an unbounded /errors body to measure it - #718
Conversation
The no-content-length branch read the whole request into a string before checking its size, so the one path the guard exists to cover was also the one that let a client stream as much as it liked into memory. Read the cloned stream chunk by chunk instead and stop at the first chunk that crosses the cap, so at most one chunk beyond the limit is ever held. The cancel is deliberately not awaited: clone() tees the body, and awaiting the cancel of one branch while the other is never read deadlocks the request. The declared-content-length branch stays as it was. A caller cannot use it to smuggle a larger body, because the server stops reading at the length it was given.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
The latest updates on your projects. Learn more about Unkey Deploy
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR replaces unbounded buffering of chunked
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking async-style violation in the new stream-reading loop. The incremental reader addresses the unbounded buffering path and the added test exercises early rejection; the only accepted concern is compliance with repository rules governing loop awaits and Promise handling. Files Needing Attention: apps/basket/src/routes/basket.ts Important Files Changed
Reviews (1): Last reviewed commit: "fix(basket): stop buffering an unbounded..." | Re-trigger Greptile |
There was a problem hiding this comment.
No issues found across 2 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Fixes unbounded memory read on /errors when content-length is absent by streaming and stopping at the cap; adds a test proving the early bail. Bounded, well-tested bug fix.
Re-trigger cubic
Greptile flagged the floating promise from reader.cancel(). Attach a catch so a failed cancel cannot surface as an unhandled rejection, and say in the code why it is not awaited, since awaiting it deadlocks.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Fixes unbounded memory read on /errors when content-length is absent by streaming and stopping at the cap; adds a test proving the early bail. Bounded, well-tested bug fix.
Re-trigger cubic
Closes the last live cubic finding on the release PR (#714),
apps/basket/src/routes/basket.ts:174.The bug
markOversizedErrorsBodyguards/errorswith a 128 KB cap. Whencontent-lengthis present it trusts the declared value, which is fine — the server stops reading at the length it was given, so a caller cannot understate it to smuggle more.When the header is absent it did this:
So the one path the guard exists to cover was also the one that let a client stream an arbitrary amount into memory before the size was ever checked.
The fix
Read the cloned stream chunk by chunk and stop at the first chunk that crosses the cap, so at most one chunk beyond the limit is ever held.
The
cancel()is deliberately not awaited.clone()tees the body, and awaiting the cancel of one branch while the other is never read deadlocks the request. I hit this: the first version of this fix usedawait reader.cancel()in afinally, and it hung both the new test and the pre-existingoversized body without content-length is still rejectedtest at 5s. Worth knowing before anyone "tidies" that line.Verification
New test streams a body whose producer is willing to emit 1 MB and asserts the producer is stopped within 2× the cap, which is what proves the early bail rather than just the 413:
bunx vitest run src/routes/integration.test.ts→ 57/57, including the pre-existing oversized-body tests. Basketcheck-typesandtestboth re-run with--forceto bypass the turbo cache. Repo lint clean, 14/14 policy tests.Not addressed
Chunk granularity means the effective ceiling is the cap plus one chunk rather than exactly 128 KB. Bounding it precisely would mean truncating mid-chunk for a request that is being rejected anyway.
Summary by cubic
Fixes an unbounded memory read in the
/errorsroute when a request lacks acontent-lengthheader. Previously the whole body was buffered into a string before the 128 KB cap was checked; now the cloned stream is read chunk by chunk and stops as soon as the cap is crossed.Bug Fixes
reader.cancel()unawaited because awaiting it deadlocks the teed request body; a.catch()handles a failed cancel so it doesn't surface as an unhandled rejection.Written for commit f1b737c. Summary will update on new commits.