Fix codec and parser bugs found by LLM fuzzing - #69
Conversation
A fleet of property-based fuzzers (round-trip + no-throw invariants over the codec, fragment, base-N, git-patch, and JSON-tree surfaces) surfaced these, each now pinned by a regression test: - arx2/arx3 silently corrupted any content containing U+007F (DEL): 0x7F is the one overlay substitution code JSON.stringify leaves unescaped, so a literal DEL was expanded back into a dictionary pattern on decode -> invalid JSON. Escape it to � at the tuple-JSON boundary (a no-op for DEL-free payloads, so the wire stays compatible). - A ~27-char fragment could peg a core for ~55s / attempt a ~3.8 GB allocation: the base76/base1k/baseBMP decoders trusted an attacker-controlled length prefix before the decoded-size budget ran. Bound the wire byte length up front. - base76 silently corrupted payloads > 5928 bytes (its fixed 2-char length prefix overflowed to the string "undefined"). Add an "="-marked extended prefix; the legacy 2-char shape is preserved byte-for-byte for every length it can represent. - git-patch turned `rename to a/` into an empty displayPath and a degenerate "-0" id. stripDiffPrefix now returns null for an empty result so the file-N fallback applies. - The JSON tree renderer overflowed the React render stack on deeply nested input (a few KB). Cap render depth and add an error boundary that degrades to the raw view, matching DiffRenderer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Deploying agent-render with
|
| Latest commit: |
21ea9ff
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0c01683c.agent-render.pages.dev |
| Branch Preview URL: | https://codex-fuzz-found-fixes.agent-render.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 005d62abf7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // tuple JSON. Escaping it to a \u007F JSON escape (which JSON.parse restores | ||
| // on decode) keeps DEL out of the substitution alphabet. This is a no-op for | ||
| // DEL-free payloads, so the wire form is byte-identical for existing content. | ||
| const tupleJson = JSON.stringify(envelopeToArx2Tuple(envelope)).replace(/\x7f/g, "\\u007f"); |
There was a problem hiding this comment.
When an artifact contains many literal DEL bytes, this expands each one from 1 character to the six-character \u007f escape in the internal tuple JSON. The decoder still enforces MAX_DECODED_PAYLOAD_LENGTH on that escaped tuple before JSON.parse, so a valid payload well under the 200k decoded envelope budget (for example, ~40k DEL characters in markdown: standard JSON is ~40k chars, escaped tuple is ~240k) can be encoded into a tiny arx2/arx3 fragment but then fail to decode as decoded-too-large.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and 👍 — correct and a real edge, though a narrow one. The escape trades exactly this for wire-compatibility: each literal DEL becomes the 6-char � in the internal tuple JSON, and assertDecodedTextBudget runs on that escaped string, so an artifact with ≳33k DEL bytes (escaped tuple > 200k) is rejected as decoded-too-large even though the real envelope is well under budget.
Two things make it lower-stakes than it reads: (1) it only bites content that is mostly DEL bytes (terminal/control dumps), and (2) it is strictly better than main, where the same payload silently corrupted — now it fails loudly and never returns wrong data.
The clean close that keeps wire-compat: run the decoded-size budget against the parsed envelope rather than the escaped tuple string. The tuple is already bounded by MAX_BROTLI_OUTPUT_BYTES (= 4× the 200k budget = 800k), so parsing first is safe, and the � inflation no longer counts against the budget. Happy to fold that into this PR — say the word.
There was a problem hiding this comment.
Fixed in 21ea9ff. The decoded-size budget now runs on the parsed tuple (re-serialized, where each `` escape collapses back to one character) instead of the escaped tuple string, so a DEL-heavy payload is budgeted by its real size. The intermediate dict/overlay strings are still bounded (by the brotli output budget) to keep the expansion-bomb guard. Wire format is unchanged. Pinned by a regression test that round-trips a 40k-DEL artifact through arx2 and arx3.
|
| Filename | Overview |
|---|---|
| src/lib/payload/arx-codec.ts | Five targeted fixes: DEL escape in compressTupleEnvelope, assertWireByteLen guard for all base-N decoders before allocation, extended "=" length prefix for base76 payloads > 5928 bytes, and budget logic fixed to measure the parsed tuple rather than the \u007f-inflated intermediate string. The truncated-extended-prefix NaN guard flagged in the previous review thread is present and correct. |
| src/components/renderers/json-renderer.tsx | Added MAX_JSON_TREE_DEPTH cap (200) in JsonNode and a JsonTreeBoundary error boundary with componentDidCatch logging, matching DiffRendererBoundary parity as noted in the previous thread. The boundary is correctly keyed by artifact.id so switching artifacts resets error state. |
| src/lib/diff/git-patch.ts | stripDiffPrefix now returns null for an empty-string result so the displayPath/id fallback chain applies correctly; the one-liner fix is minimal and correct. |
| tests/arx-codec-robustness.test.ts | New regression file covering DEL round-trip (arx2/arx3/fragment paths), false-rejection of DEL-heavy payloads, and DoS-rejection timing for decodeBaseBMP and decodeFragmentAsync. |
| tests/arx-codec.test.ts | Extended with tests for the legacy ceiling (5928 bytes), one-past-ceiling (5929 bytes), large payload (60 000 bytes, raised timeout), leading-zero preservation, truncated-extended-prefix NaN guard, and full 0x00–0x9F control-byte round-trips. |
| tests/components/json-renderer.test.tsx | One new test exercises 3000-deep nesting and asserts no throw, the renderer mounts, and at least one "max depth reached" label is visible. |
| tests/git-patch.test.ts | New test case for a bare "rename to a/" path that strips to empty, confirming the fallback label "file-1" and id "file-1-0" are produced instead of an empty displayPath. |
Reviews (3): Last reviewed commit: "Budget the decoded arx2/arx3 payload by ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…follow-up) Addresses automated review on the fuzz-fix PR: - decodeBase76: a truncated extended length prefix (e.g. "=B" claims a length digit but carries none) read charCodeAt past the end -> NaN byteLen, and `NaN > MAX_BROTLI_OUTPUT_BYTES` is false, so the guard missed it and the decode silently returned an empty array. Guard the digit count against the string length, and make assertWireByteLen reject non-integer lengths so a NaN can never slip past any base-N decoder. - JsonTreeBoundary: add componentDidCatch to log the swallowed throw (parity with DiffRendererBoundary) instead of falling back to raw with no trace. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ped tuple Follow-up to the DEL-escape fix: literal DEL bytes are escaped to the 6-char � JSON escape in the internal tuple JSON, so the decoded-size budget run on that escaped string over-counts DEL-heavy content ~6x — an artifact with ≳33k DEL bytes (real content well under the 200k limit) was wrongly rejected as decoded-too-large. Run the decoded-payload budget on the parsed tuple (re-serialized, where each escape collapses back to one character), and bound the intermediate dict/overlay-decoded strings by the brotli output budget so the expansion-bomb guard is preserved. Wire format and existing payloads are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Reviewed by gpt-5.4-mini-2026-03-17 · 260,932 tokens |
1 participant
Summary
Bugs found by running a fleet of bespoke property-based fuzzers ("LLM fuzzing") against the codec and parser surfaces — hammering the round-trip (
decode(encode(x)) == x) and no-throw invariants with seeded, adversarial generators — then adversarially confirming each hit with a minimized repro + root cause. Stacked on #67 (basecodex/optimize-codebase).8 violations confirmed across 5 distinct root causes; each fix is pinned by a regression test.
Bugs fixed
U+007F(DEL) anywhere in artifact content silently corrupted the fragment —0x7Fis the one overlay substitution codeJSON.stringifyleaves unescaped, so a raw DEL was expanded back into a dictionary pattern on decode →invalid-json. Any shared URL with a DEL char that auto-selected arx2/arx3 was undecodable for the recipient.decodeBaseBMPto computebyteLen ≈ 3.8e9and attempt a multi-GB allocation + billions of iterations, pegging a core for ~55s — the decoded-size budget only ran after decompression.ALPHABET[77]isundefined, so the wire got an"undefined"prefix and silently round-tripped to a wrong-sized array.RangeError— the recursive<JsonNode>walk had no depth bound and no error boundary (unlikeDiffRenderer).rename to a/stripped to an emptydisplayPathand a degenerate-0id, defeating thefile-Nfallback.Fixes
0x7F→�at the tuple-JSON boundary incompressTupleEnvelope. A no-op for DEL-free payloads (wire stays byte-identical), andJSON.parserestores the DEL on decode — works for both old and new readers, no migration.assertWireByteLen()rejects an implausible declared length (>MAX_BROTLI_OUTPUT_BYTES) before allocating, indecodeBaseBMP/decodeBase1k/decodeBase76. Maps to the existingdecoded-too-largeresult. Rejects in ~68 ms instead of ~55 s.=-marked variable-width extended length prefix for payloads beyond the 2-char ceiling. The legacy 2-char shape is preserved byte-for-byte for every length ≤ 5928, so existing links decode unchanged.MAX_JSON_TREE_DEPTHcap (collapses deep nodes to a leaf) plus aJsonTreeBoundarythat degrades to the raw view, matching theDiffRendererconvention.stripDiffPrefixreturnsnullfor an empty result so thedisplayPath/idfallback chain applies.Verification
npm run check(lint + 34 test files + bench + typecheck + build + budgets) is green. The DEL/base76 fixes are wire-compatible; the DoS guard and depth cap reject only provably-implausible inputs.🤖 Generated with Claude Code