docs(security): document the bounded decompression path (LAB-2504) - #117
docs(security): document the bounded decompression path (LAB-2504)#11727Bslash6 wants to merge 2 commits into
Conversation
SECURITY.md covered reporting and scope but said nothing about the read pipeline, so a reader had no way to tell whether this SDK decompresses anything itself. It does not: both bindings (NAPI and the Workers wasm build) are thin wrappers over cachekit-core's extract(), which bounds output at min(512 MiB, 1000 x compressed_len) before decompressing, does not trust the envelope's declared original_size, and does not rely on the unkeyed xxHash3-64 checksum for anything but corruption detection. Calls out that maxDecodedSize is checked on already-decompressed bytes, so it is downstream of that bound rather than a substitute for it. Flags the Workers case specifically: 512 MiB is a server-class ceiling and an isolate has roughly 128 MiB, so on that build a payload well inside core's limits can still exhaust it. Sizing belongs to LAB-2505; this records the constraint so nobody reads the ceiling as tuned for Workers.
Expert-panel findings on the previous commit. 'maxDecodedSize is checked on the already-decompressed bytes' was true but buried the consequence: the default is 10 MiB and core will materialize up to 512 MiB before it is ever consulted, so the two ceilings differ by ~51x and turning maxDecodedSize down to harden a Workers deployment does nothing to what unpack may allocate. cache-core.ts:574 currently claims the blast radius IS bounded by maxDecodedSize, which is wrong by that same factor; filed as LAB-2732 and cross-referenced here. The Workers callout also said 'bound payload size at the caller' without naming a lever, while the only knob the section mentions is the one that does not work -- so a reader would reasonably reach for maxDecodedSize and stay exposed. Now names the two levers that exist: check the fetched byte length before handing it to the cache, or cap value size at the backend.
|
Warning Review limit reachedNext included review available in 43 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 96 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (1)
Comment |
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
1 participant
Docs-only. Part of the LAB-2504 cross-SDK decompression-bomb audit. Depends on cachekit-core#75 — the inline link targets an anchor that PR adds, so merge core first.
Why
SECURITY.mdcovered reporting and scope but said nothing about the read pipeline, so a reader had no way to tell whether this SDK decompresses anything itself.Audit outcome for this repo: no bypass, but a misleading ceiling
It does not decompress anything itself. Both bindings —
cachekit-core-ts(NAPI,src/lib.rs:118) andcachekit-core-wasm(Workers,src/lib.rs:98) — are one-line pass-throughs to cachekit-core'sretrieve→extract(), which bounds output atmin(512 MiB, 1000 × compressed_len)before decompressing. No SDK-side LZ4, no pre-extractallocation.The audit did surface a real mismatch, which this PR documents and LAB-2732 tracks for a code fix:
serializer.maxDecodedSizedefaults to 10 MiB but is only applied insideserializer.decode, i.e. afterunpackreturns. Core may have materialized up to 512 MiB by then, so the two ceilings differ by ~51×, andcache-core.ts:574currently claims the blast radius is bounded bymaxDecodedSize. Turning that knob down to harden a Workers deployment changes nothing about whatunpackmay allocate.What this PR corrects about its own first draft
The panel found the Workers callout said "bound payload size at the caller" without naming a lever, while the only knob the section mentions is precisely the one that does not work — so a reader would reasonably reach for
maxDecodedSizeand stay exposed. It now names the two levers that exist: check the fetched value's byte length before handing it to the cache, or cap value size at the backend.Scope
No code. The
maxDecodedSizemismatch is LAB-2732; core's constants are LAB-2505.Closes LAB-2504 for this repo.