Repository navigation
test(cachekit): drive decode-bounds vectors through both ByteStorage.unpack bindings (LAB-6334) - #245
Conversation
…unpack bindings cachekit-core 0.7.0 pre-scans the envelope bytes in ByteStorage::retrieve before its typed decode. Run every decode-bounds.json vector through the NAPI unpack in the Node lane and the wasm unpack inside workerd, so both envelope entry points prove they apply it. Reject vectors must fail with the pre-scan message matched from its start; vectors that break one rule alone also pin that rule's reason, because the pre-scan's end-of-input rejection carries the same prefix. Accept vectors must get past the pre-scan. The vector loader and these assertions move to a shared fixture module that workerd can load.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Summary by CodeRabbit
WalkthroughThe change adds shared helpers for decode-bounds vectors and uses them to test envelope pre-scan behaviour through NAPI and Workers ChangesDecode-bounds test coverage
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly summarises the main change: testing decode-bounds vectors through both ByteStorage.unpack bindings. Description check The description directly explains the shared test vectors, NAPI and Workers coverage, assertions, mutation evidence, and validation results. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Accept vectors now require core's typed-decode failure rather than any error without the pre-scan tag, so a trap or other non-decode error cannot pass for "past the pre-scan". build() hands every site an offset view with reserved-marker guard bytes on both sides, restoring the byteOffset coverage the pooled Buffer gave the value sites. The fixture is checked by its typed JSON import, as wire-vectors.ts does, and build() reuses construct().
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
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}
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
# Conflicts: # packages/cachekit/test/protocol/decode-bounds.protocol.test.ts
3a57806
|
@kody review --force |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
@coderabbitai review |
|
What
cachekit-core 0.7.0 (on
mainsince #240) pre-scans the envelope bytes inByteStorage::retrievebefore its typed decode. Nothing in this repo proved that eitherunpackbinding applies it. This PR runs everydecode-bounds.jsonvector (1.2.0, 19 reject / 3 accept) through both envelope entry points:decode-bounds.protocol.test.tsgains aByteStorage.unpack (NAPI)site,new ByteStorage()from@cachekit-io/cachekit-core-ts.test/workers/decode-bounds.workers.test.tsruns the same assertions through the wasmByteStorage.unpackinside workerd.build()and the two envelope assertions move totest/fixtures/decode-bounds-vectors.ts. It has nonode:*imports, so workerd loads it. This follows thewire-vectors.tspattern. The Node lane keeps its sha256 pin over the raw file bytes.No production code changes.
The assertions
Each reject vector must throw a message that starts with
deserialization failed: decode pre-scan:. The match runs from the start of the message, because a typed-decode error can echo attacker bytes in the middle of its message. The prefix alone is not enough: the pre-scan's own end-of-input rejection carries it too. So, as cachekit-core'stests/decode_bounds_vectors.rsdoes, depth-only and overclaim-only vectors also pin their rule's reason, while incomplete-only and multi-reason vectors pin the prefix:depthmust givenests deeper than 100 levels, core's envelope bound;overclaimmust give a reason startingdeclares more.Each accept vector must get past the pre-scan. These are not envelopes, so core's typed decode refuses them, and the test requires that refusal:
deserialization failed:not followed bydecode pre-scan:. Any other error, such as a wasm trap, fails the test.build()hands every site its input as a view at a non-zero offset, with the reserved marker0xc1on both sides. A pooled NodeBufferor a network read arrives this way. A decoder that ignoresbyteOffset, or reads pastbyteLength, sees bytes that are not the input. The oldBuffer.concatbuild gave the value sites this coverage by accident, through Node's buffer pool. Now it is deliberate, and it reaches bothunpackbindings too. The fixture is typed by its JSON import and checked bypnpm type-check:tests, aswire-vectors.tsis.Evidence
Both bindings were built from source against a mutated copy of cachekit-core 0.7.0, and both lanes were run against each build:
The slot-check row is the case a prefix-only assertion misses: with that check gone, every one of those vectors still fails with the
decode pre-scan:prefix, at end of input. DroppingbyteOffsetfrom the value decoders'DataView(serializer.ts:127) turns 3 value-site tests red.pnpm lint,pnpm type-check,pnpm type-check:tests,pnpm test(2013 passed, 1 skipped) andpnpm --filter @cachekit-io/cachekit test:workers(193 passed) pass locally.Docs
No user-facing surface changes. Published
@cachekit-io/cachekit0.2.1 pinscachekit-core-tsandcachekit-core-wasmat exactly 0.2.0, and both of those embed core 0.6.0. So the protocol's SDK feature matrix keeps the TypeScript cells unreleased until a published@cachekit-io/cachekitdepends on bindings that embed core 0.7.0. That matrix change is cachekit-io/protocol#203.