fix(cookie): cap chunk count in setChunkedCookie#1469
Conversation
`setChunkedCookie` had no upper bound on the number of chunks, while the reader (`getChunkedCookie`) caps at `MAX_CHUNKED_COOKIE_COUNT` (100) and returns `undefined` above it. An oversized value was written but never re-readable (silent data loss) and emitted more Set-Cookie headers than browsers accept. Now throws when the value would exceed the cap so the set and get sides agree. Also fixes an off-by-one in the stale-chunk cleanup loop that redundantly re-deleted the last overlapping chunk, without breaking cleanup when a value shrinks to a single non-chunked cookie. Co-Authored-By: Claude Fable 5 <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesChunked Cookie Handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
pi0x
left a comment
pi0x
left a comment
There was a problem hiding this comment.
APPROVE
Reviewed the change independently (checked out the branch, ran tests + lint). The fix is correct and well-tested.
What I verified
Cap is placed before any mutation — no partial writes. The chunkCount > MAX_CHUNKED_COOKIE_COUNT throw runs before the getCookie/deleteCookie/setCookie calls, so a rejected value never leaves stray Set-Cookie headers. The test asserting getSetCookie() is empty on throw locks this in.
Set/get cap agreement is exact, no off-by-one. Reader getChunkedCookieCount returns NaN when count > 100, so 100 is valid and 101 is not. Writer throws when chunkCount > 100. Same constant, same > direction — a value needing exactly 100 chunks is both writable and readable, which the boundary round-trip test confirms.
Cleanup rework is correct across all transitions:
- multi -> fewer-multi: deletes
name.(chunkCount+1)..prev, skipping the benign re-delete of the overlapping last chunk (original started atchunkCountand re-deletedname.chunkCount, harmless only because the fresh write overwrites it via setCookie dedup — the author's claim checks out). - multi -> single non-chunked:
newChunkCount = 0, so cleanup starts atname.1and removes every prior chunk. Correct. - plain/single -> multi: previous cookie isn't
__chunked__, so the block is skipped — nothing to clean. - multi -> same/more: loop no-ops; fresh writes overwrite. Correct.
No stale name.N survives and no needed chunk is deleted in any path. The newChunkCount + 1 choice correctly handles the single-value case that the plan's naive chunkCount + 1 would have broken (leaving name.1).
Error type (HTTPError 500): defensible. An oversized sealed session is a server-side condition (session.ts is the only internal caller), and HTTPError integrates with h3's error handling to yield a proper 500 rather than a raw throw; h3 masks 500 bodies by default so the name/counts in the message aren't leaked to clients. A plain TypeError would also be reasonable, but 500 is a fine call — not blocking.
Test quality: the throw test is a genuine regression test — I confirmed all 4 matrix variants fail with the cap check disabled. The boundary and cleanup tests are guards (they also pass on main, since the original already handled multi->single correctly); they don't catch a pre-existing bug but usefully lock in the refactor. Worth a mention, not a concern.
Style/checks: import of HTTPError added cleanly, comments are clear, matches AGENTS.md conventions. pnpm vitest run test/cookies.test.ts -> 108 passed; pnpm lint -> clean.
Nice catch on the cleanup off-by-one while adding the cap. LGTM.
🤖 Generated with Claude Code
2 participants
setChunkedCookiehad no upper bound on chunk count, but the readergetChunkedCookiecaps atMAX_CHUNKED_COOKIE_COUNT(100) and returnsundefinedabove it — so an oversized value (e.g. a large sealed session) was written yet never re-readable, silently losing data and emitting moreSet-Cookieheaders than browsers accept. It now throws when a value would exceed the cap so the set and get sides agree.This also fixes an off-by-one in the stale-chunk cleanup loop (it re-deleted the last overlapping chunk), keeping cleanup correct when a value shrinks to a single non-chunked cookie. Regression tests cover the cap (throw + boundary round-trip) and the cleanup.
🤖 Generated with Claude Code
Summary by CodeRabbit