Repository navigation
fix(codecs-numbers): reject truncated and overlong shortU16 encodings - #2067
Conversation
The shortU16 decoder loop had no upper bound, so a continuation chain longer than the documented three-byte encoding was accepted, producing values far outside the u16 domain the encoder enforces. Worse, once the chain ran past the end of the buffer, the out-of-bounds byte read as undefined terminated the loop silently: truncated input decoded to garbage and returned an offset beyond bytes.length instead of failing. Bound the loop at three bytes and assert per-byte availability through assertByteArrayHasEnoughBytesForCodec, matching every other number decoder. A chain that still continues after three bytes now throws NUMBER_OUT_OF_RANGE, mirroring the encoder's own range assertion.
BundleMonFiles updated (4)
Unchanged files (146)
Total files change +363B +0.06% Final result: ✅ View report in BundleMon website ➡️ |
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Bounds the shortU16 decoder's read loop at three bytes. Each iteration now asserts the byte is actually present (INVALID_BYTE_LENGTH, same helper the fixed-size number decoders use), and a continuation bit on the third byte throws NUMBER_OUT_OF_RANGE. Two tests cover truncation at 1 and 2 bytes and the 3-continuation-bytes case; patch changeset for @solana/codecs-numbers.
The core change is correct and I traced the loop by hand: valid 1/2/3-byte encodings return at the same offsets as before, [0x80] and [0x80, 0x80] now hit the length assert with the exact contexts the tests claim, and [0xff, 0xff, 0xff] (with or without a trailing byte) reaches the throw. The existing 0..65535 round-trip sweep guards against regressions on the happy path. Nice that the overlong check doesn't depend on a fourth byte existing.
Things to watch
The u16 bound is only half-enforced. The new error says the value must be in 0..65535, but a 3-byte encoding whose third byte has no continuation bit is still returned unchecked: [0x80, 0x80, 0x04] decodes to 65536 and [0xff, 0xff, 0x7f] to 2097151. Agave's reference decoder (short_vec.rs) has a distinct Overflow error for exactly this case alongside ByteThreeContinues. Since assertNumberIsBetweenForCodec is already imported in this file, closing the gap is a one-liner before the return — see inline. I'd fold it into this PR so the decoder's contract matches the error it now throws, rather than leaving a second "shortU16 accepts out-of-range values" fix for later.
Import grouping — a blank line was introduced between the two @solana/* imports in short-u16.ts. Elsewhere in the repo (e.g. codecs-data-structures/src/array.ts) package imports sit in one group with no blank line, which is what simple-import-sort enforces, so test:lint will likely flag it. pnpm style:fix should sort it out.
Notes for other reviewers / possible follow-ups (non-blocking)
- The
getShortU16Codecdocblock (unchanged, ~L130–140) still describes the third byte as using "all 8 bits" and lists a 3-byte range of16,384 to 4,194,303. That was already inaccurate (the implementation masks every byte to 7 bits, and the encoder rejects anything above 65535), but this PR is explicitly about tightening the decoder's contract, so it's a natural moment to correct that comment to16,384 to 65,535and drop the "all 8 bits" line. Adding@throwsentries to the decoder docblock would also fit the repo's docblock guidance. - Agave additionally rejects non-canonical "aliased" encodings (a
0x00byte anywhere after the first, e.g.[0x80, 0x00]for0). Kit still accepts those. Probably fine to leave — it's a stricter canonicality rule rather than a domain violation — but worth knowing if anyone wants byte-for-byte parity with the validator later. - Behavioural note for the changelog: inputs that previously decoded (to garbage) now throw. Patch is the right bump for a fix; just flagging that any consumer relying on lenient decoding of malformed data will start seeing errors.
…t imports Address review: a terminated 3-byte encoding can still exceed the u16 domain ([0x80,0x80,0x04] is 65536), so assert the decoded value with assertNumberIsBetweenForCodec before returning, mirroring the encoder. Also fix simple-import-sort grouping and correct the docblock's third byte description (low 7 bits, max 65535).
🦋 Changeset detectedLatest commit: fa6e525 The changes in this PR will be included in the next version bump. This PR includes changesets to release 48 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@trevor-cortex addressed, thanks for the sharp trace:
Left the aliased-encoding leniency as-is per your non-blocking note. |
Fully enumerate the terminated 1/2/3-byte spaces (accept iff value in 0..65535), the complete first-overflow row, and 5000 seeded arbitrary inputs asserting in-range values, in-bounds offsets, and re-encode round-trips.
|
The current
The production behavior and exhaustive coverage look useful; this appears limited to reshaping the test assertions and applying the repository's import sort before rerunning CI. |
Fixes the two test:lint failures blocking CI on anza-xyz#2067: adds the missing blank line between the external and relative import groups in short-u16.ts per simple-import-sort, and rewrites the two new short-u16 tests to avoid jest/no-conditional-in-test and jest/no-conditional-expect by splitting the 1-byte enumeration loop and collecting fuzz outcomes before asserting. Test coverage is unchanged — the same seeded inputs run through the same assertions. Verified with compile, lint, typecheck, unit tests on Node and browser, and prettier, all green.
Restores the shortU16 test file to 74c71f1, removing the exhaustive 1/2/3-byte enumeration and fuzz tests that duplicated the full-domain round-trip sweep, per review feedback. The targeted truncation, overlong-chain, and u16-overflow boundary tests stay, as does the import-sort fix in short-u16.ts.
|
@lorisleiva You're right — the sweep in the first test already covers those values. Dropped the exhaustive enumeration tests; kept the targeted boundary pins and the import-sort fix. |
|
Verified the updated |
…chains The decoder only reaches the throw when all three bytes have their continuation bit set, so the input demands a fourth byte. That is a byte-length problem rather than a range problem, and the accumulated value in the error context was misleading anyway. Replaces the magic number with a local constant and trims the explanatory comments.
|
Thanks for the review! All three points are addressed in fa6e525: the overlong-chain case now throws One note: the |
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Solana Kit Docs' |
|
Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up. |
Supersedes #1986, which the stale bot closed during review limbo — reopening it is permission-blocked (
ReopenPullRequest: FORBIDDEN), so re-submitting the identical 2 commits (bc820fd,a3e06ae) with the verification below.Problem
The
shortU16decoder loop had no upper bound, violating the codec's own documented contract of a 1 to 3 byte encoding (maxSize: 3). Two concrete failure modes:0..65535domain.bytes[offset]isundefined;undefined & 0b10000000 === 0terminates the loop, so truncated input decoded to garbage with an offset beyondbytes.length(e.g.decode([0x80])→[0, 2]).Change
Bound the read loop at three bytes with per-byte availability asserts (
INVALID_BYTE_LENGTH, like every other number decoder); a continuation bit after three bytes throwsNUMBER_OUT_OF_RANGE.Verification (executed, 10/10)
Ran the actual pre/post decoder code with the real
@solana/codecs-core+@solana/errorspackages:[0x80]→[0,2],[0x80,0x80]silent,[0xff,0xff,0xff,0x00]→2097151