fix(typia): hold every Protobuf reader fault to one prefix and one varint limit#2139
Conversation
Self-Review round 1Complete round over the base-to-head diff: 1. 2. 3. An over-long varint nested in a group unwinds two atomic frames. Not covered. That is a distinct state-restoration path from the top-level skip, and groups are where atomicity is subtlest. Added 4. Wire type 4 reaches the same Checked and found clean: no remaining dependent on the old Docs: no change. The "Wire errors" section states the general contract — a malformed payload raises rather than reports — which this diff makes more true, not less. It quotes no message text. The Verification after these changes: |
Self-Review round 2Fresh complete round over the current state. Two findings, both in the regressions rather than the fix, applied in d228b9a. 1. 2. A local helper was named Also formatted the two new files with the repository's prettier config, scoped to those files alone. This is not Self-Review round 3Fresh complete round over the whole surface again: the source diff, both regressions, and the consequence surface. Nothing to improve, so the round closes clean and Self-Review ends here. Re-verified at
The three merged contracts in this file were re-run and hold: Both halves of the fix were proven load-bearing by independent mutation, each with the other left intact — reverting only the prefix fails the wire-type cases while the varint cases pass, and reverting only the limit fails the varint cases while the wire-type cases pass. Not merging, rebasing, or bumping the version: all lead-owned. No version bump is due regardless, since the final diff touches no native Go. |
Self-Review round 4Round 3 closed clean, but re-reading the PR body against the tree turned up one claim that was not yet true, so the round reopened. The body claimed Nothing else surfaced. Verified at
Self-Review ends here: round 5 over the full surface produced nothing further. Four rounds, three of which applied an improvement. Leaving the merge, any rebase, and the version to the lead. No bump is due — the final diff is TypeScript runtime and tests only, touching no native Go, matching the #2066 precedent. |
Reserves #2135: the skipType default error lacks the standard typia decode prefix, and skipVarint enforces no 10-byte varint limit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fg7AePEdpjaxHrmVffuSjy
…rint limit `skipType`'s default threw `Invalid wire type 6 at offset 0.`, the one reader error carrying no `Error on typia.protobuf.decode(): ` prefix, so a caller could not attribute it to typia. It is reachable: a generated decoder skips unknown fields with `skipType(tag & 0x07)`, and wire types 4, 6, and 7 survive the mask. `skipVarint` enforced no length cap, so a varint with excess continuation bytes was skipped where `varint32` and `varint64`, both structurally bounded at ten bytes, would not read past their limit. Both are one contract implemented in more than one place with a copy left behind, so both are fixed by removing the copy. A module-local `error()` now builds every reader fault, leaving the prefix literal in exactly one place, and `VARINT_MAX_BYTES` names the limit once, feeding `skipVarint`'s bound and its message. `varint32` and `varint64` keep their unrolled structural limit, since routing them through the constant would change what `uint32` does with a malformed varint. `atomic`'s catch binding is renamed off `error` so it no longer shadows the builder. The `buffer overflow.` and `invalid UTF-8 string.` texts are unchanged, a varint truncated before the limit still reports overflow, and a zero-length unknown `LEN` still advances exactly its two bytes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fg7AePEdpjaxHrmVffuSjy
Self-Review follow-ups. Pin an over-long varint nested in a group, which unwinds two atomic frames rather than one, and cover a bare END_GROUP beside the reserved wire types 6 and 7, since `tag & 0x07` reaches it through the same switch default. Correct the decode regression's premise: a reader fault propagates out of every decoder variant rather than being reported as `null` or an unsuccessful `IValidation`, because a framing fault is not a validation failure. That is pre-existing behavior documented under "Wire errors", so the matrix now asserts all eight variants surface both faults alike. Mirror the peer regression by making `assertSkip`'s closure required rather than a defaulted positional, and rename the local `expect` helper off the name a test framework would own. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fg7AePEdpjaxHrmVffuSjy
The literal ten survived in `varint32`'s comment, the one place documenting the other copy of the varint limit. Naming `VARINT_MAX_BYTES` there leaves the file with a single occurrence of the limit, so a reader who changes it finds both the looping consumer that enforces it and the unrolled reads that assume it. Comment only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fg7AePEdpjaxHrmVffuSjy
e328350 to
78d51c9
Compare
|
Lead certification before merge. Lead verification on
|
| Gate | Result |
|---|---|
pnpm --filter test-typia-schema start |
Success |
go -C packages/typia/test test ./... |
0 failures |
| diff surface | _ProtobufReader.ts plus two regression files — zero native Go, so no version bump, per the #2066 precedent |
Rebased onto 1d0fab689a (#2130); four commits replayed with no conflict.
The single-sourcing judgment is the best thing here
I briefed this batch to prefer single-sourcing the limit and the prefix over adding a third copy. It did that for the prefix — one error(message) builder, the literal down from three inline copies to one — and then correctly refused to do it for the limit.
varint32 and varint64 are unrolled, and routing them through the constant would change what uint32 does with a malformed varint — which this very issue holds fixed. So it named the constant once for skipVarint and pointed varint32's comment (which still claimed "we read 10 bytes") at it, linking the copies rather than merging them. Recognising where "single-source everything" would break a contract the issue explicitly preserves is exactly the judgment I wanted and did not specify.
Evidence
Both regressions written first and observed failing on master, then mutated independently with the other intact:
| Mutation | Result |
|---|---|
| revert the prefix only | wire-type cases fail; varint cases pass |
| revert the varint limit only | eleven-byte varint was accepted; wire-type cases pass |
Verification at the final head: test-typia-schema Success (154), test-typia-automated Success (3,947), the full public Go tree ok, test-feature-identity Success.
Three findings beyond the brief
atomic's catch binding was namederrorand shadowed the new builder — a live trap for the next fault anyone adds to this file. Renamed tothrown. That is a defect the batch created and removed before it could bite someone else.- Wire type 4 reaches the same
default:. My issue named 6 and 7; a bareEND_GROUPfalls through the same switch andtag & 0x07reaches it. Covering the real class rather than the two cases I listed is right — a published issue's case list is a hypothesis, not an inventory. That is the second time this session an agent has found consumers or cases my issue did not name. - A test premise was wrong, not the code.
isDecode/validateDecodedo not swallow reader faults — they propagate, because a framing fault is not a validation failure. Pre-existing, already documented under "Wire errors", and now pinned across all eight variants.
Two judgment calls, both correct
- An over-long varint throws rather than caps. The issue's matrix says "rejected", and capping would park the reader mid-varint on a continuation byte — a worse state than the error.
- A varint whose tenth byte still carries the continuation bit reports
varint exceeds 10 bytes, while truncation before byte 10 still reportsbuffer overflow. The two faults stay distinguishable, which is the property fix(typia): reject invalid Protobuf UTF-8 strings #2090 established and fix(typia): reject truncated Protobuf payloads #2084 depends on.
The three merged contracts hold
#2090's structure is preserved by construction — string() still resolves bytes() before the try, and only the throw inside the catch changed. #2084's boundary errors and #2119's zero-length skip are covered by the six pre-existing protobuf tests, all passing.
Exact-SHA cancellation gate
78d51c985b settled after three polls, all six runs cancelled, no race.
Merging under the campaign's local-verification and solo Self-Review gates.
Closes #2135.
Two contract gaps in the Protocol Buffer reader,
packages/typia/src/internal/_ProtobufReader.ts. Neither is a soundness defect: no valid payload decodes differently, and no invalid payload becomes accepted as data.skipType'sdefault:threw without the standard prefix. Every other reader error isError on typia.protobuf.decode(): …; this one wasInvalid wire type 6 at offset 0., so a user could not attribute the failure to typia. It is user-reachable: the generated decoder skips unknown fields withskipType(tag & 0x07), and wire types 4, 6, and 7 all survive that mask and land on thedefault:.skipVarintenforced no length cap.varint32andvarint64are both structurally bounded at ten bytes, the maximum a 64-bit value can occupy.skipVarintconsumed to the first terminator or the buffer end, so a malformed varint with excess continuation bytes was skipped rather than rejected. It was bounded by the buffer end, so nothing ran away, and the skipped bytes were discarded anyway — a leniency where its two siblings hold the line.Approach
Both gaps are one contract implemented in more than one place with one copy left behind, so both are fixed by removing the copy rather than adding another.
error(message)builds every reader error, so the literalError on typia.protobuf.decode():appears exactly once in the file instead of the three inline copies it had. The four throw sites route through it; thebuffer overflow.andinvalid UTF-8 string.texts are byte-for-byte unchanged.VARINT_MAX_BYTES = 10is the only occurrence of the literal in the file, and it feeds bothskipVarint's bound and its message.varint32andvarint64encode the same limit structurally in their unrolled reads; they are left alone deliberately, because routing them through the constant would change whatuint32does with a malformed varint, which Protobuf reader: skipType default error lacks the standard prefix, and skipVarint has no 10-byte cap #2135 explicitly holds fixed.New messages:
Error on typia.protobuf.decode(): invalid wire type <n> at offset <m>.andError on typia.protobuf.decode(): varint exceeds 10 bytes.An over-long varint is rejected rather than capped. Capping would leave the reader parked mid-varint on a continuation byte, which has no valid interpretation; a skip that silently stops there is worse than an error, and #2083 already set this file's posture of rejecting rather than clamping.
Case matrix
varint exceeds 10 bytes.buffer overflow.— #2083 unchangedLENstring()buffer overflow., not relabelled — #2090 unchangeduint32on an 11-byte varintA varint whose tenth byte still carries the continuation bit is reported as over-long even when the buffer ends there, because no eleventh byte could make those ten bytes legal. Truncation before byte ten stays
buffer overflow, since the length fault is not yet decided.Tests
Added first and observed failing against
master, mirroring the reader-plus-decoder pair that #2084, #2090, and #2119 each established:tests/test-typia-schema/src/features/protobuf.decode/test_protobuf_reader_unknown_field_faults.tstests/test-typia-schema/src/features/protobuf.decode/test_protobuf_decode_unknown_field_faults.tsThey cover the 10-vs-11-byte boundary, the reserved and bare-END_GROUP wire types directly and nested in a group, the reported offset from a non-zero position, atomic restoration, the legal wire types as negative twins, and the three merged contracts above.
Verification
TypeScript runtime only. Per the #2066 precedent, no
packages/typia/package.jsonversion bump: the final diff touches no native Go.pnpm --filter ./tests/test-typia-schema startpnpm --filter ./tests/test-typia-automated startgo -C packages/typia/test test ./...Campaign CI is suspended; each pushed SHA passes the exact-SHA cancellation gate and verification is local.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Fg7AePEdpjaxHrmVffuSjy