fix(repo): spec-correct MST and hardened DAG-CBOR / CAR parsing #147

Merged

Grandiras merged 1 commit from issue-111-mst-cbor into main 2026-09-24 18:11:17 +00:00

Conversation 5 Commits 1 Files changed 26 +2832 -1482

Grandiras commented 2026-09-24 18:04:55 +00:00

Owner

Copy link

Closes #111.

Correctness: MST

• Canonical encoding. Nodes always carry l and every entry's t, written as CBOR null when absent. The empty tree hashes to bafyreie5737gdxlw5i64vzichcalba3z2v5n6icifvx5xytvske7mr3hpm.
• Decoding null links. MstNodeData.FromBytes reads null (and, leniently, omitted) l/t. MerkleSearchTree.Deserialize now loads real getRepo trees.
• One simple design. The incremental Insert/Split/Remove/Merge code is gone. The tree keeps a sorted entry set (key → CID + cached key height) and derives the node structure from it on every ComputeRootCid / Serialize / SerializeProof. Empty ranges become null links, and ranges whose keys all sit lower get one entry-less wrapper node per skipped layer. Any edit sequence therefore yields the bulk-build root by construction, and a seeded property test checks it anyway (100 trials × 150 random add/update/delete ops, against a shuffled bulk build and a Serialize→Deserialize→Validate round trip).
• Keys are validated like the reference isValidMstKey: collection/rkey, charset A-Z a-z 0-9 _ ~ - : ., ≤ 1024 characters.
• SerializeProof now ports the reference getCoveringProof (the key's path plus the left and right neighbour paths). Relays need those nodes to invert a #commit. It is a superset of the old path-only proof.
• Validate() now rebuilds and compares. After Deserialize it checks that the canonical rebuild equals the loaded root. For in-memory trees it is trivially true.
• ComputeNodeCid/SerializeNode and Create(entries)/CreateFromEntries are deduplicated. MstNodeData, MstTreeEntry and MstKeyDepth are now internal.

Security: untrusted input

• MST PrefixLen: 0 ≤ p ≤ previousKey.Length is enforced in MstNodeData.FromBytes before any slicing. p is read as Int64, so out-of-range, negative, 2^31, 2^40, long.MinValue and 2^64-1 all fail with FormatException.
• MerkleSearchTree.Deserialize also rejects:
  • invalid keys (non-ASCII bytes are decoded as Latin-1, so they fail the key check rather than being replaced)
  • keys that are out of order across the in-order walk. This also makes a subtree linked twice fail on its first repeated key.
  • empty inner nodes
  • depth > 64 (checked on a 10 000-node chain)
  • missing or malformed blocks
• CarReader header: replaced the recursive hand-written CBOR parser with CborReader + SkipValue. A 100 000-deep nested header value is now skipped without a crash, and a truncated one throws FormatException. The header must carry version, its roots must be tag-42 links with the 0x00 prefix, and no trailing bytes are allowed.
• CarReader framing: every length (header, block, CID digest) is checked against the remaining input before it is narrowed or sliced. Varints must be ≤ 9 bytes and minimal.
• CIDv0: CarReader.FromBytes rejects CIDv0 blocks, and VerifyBlockCid reports them as UnknownCodec.
• Depth limit: 64 JSON levels (the System.Text.Json default, counting the $link/$bytes wrappers) in the shared transcoder and in DagCborDecoder.TryValidate. Chains of unknown tags are iterated, not recursed. DagCborDecoder.Decode normalizes all malformed input to FormatException. FirehoseEventParser.Parse returns null as before.

Consolidation

• DagCborLink (internal) is the one null-aware tag-42 read/write helper. It is used by CarWriter, CarReader, MstNodeData, RepoCommit, DagCborEncoder, SpaceRepoCar and the transcoder, and throws FormatException for malformed links. Writes go through a stack buffer.
• DagCborJson (internal) is the one-pass DAG-CBOR→Utf8JsonWriter transcoder, in Wrapped ($link/$bytes) and Flattened (firehose models) forms, with the depth limit. DagCborDecoder.Decode parses its output with JsonElement.ParseValue, with no JsonNode tree.
• DagCborEncoder writes with CborConformanceMode.Lax over its own sort and detects duplicate keys itself, so the writer no longer sorts maps a second time. CompareCanonical now compares equal-length keys by UTF-8 bytes. Before, it used UTF-16 order, which was wrong where U+10000+ meets U+E000–U+FFFF; the Canonical writer's re-sort had been hiding that bug. Output is asserted byte-identical to a Canonical-writer reference on 500 random documents with adversarial keys.
• SpaceRepoCar.Serialize writes the index map directly with CborWriter, byte-identical to the old JsonObject→encoder path (asserted).
• Removed: DagCborDecoder.DecodeToNode, DagCborEncoder.ComputeCid, CarBlock.DataLength, and the unused locals in CarReader. CidComputation.TryDecodeCidString and Verify are kept, as are CarWriter.WriteToAsync and Tid.FromInt64/ToInt64.

Breaking changes (migration)

Change	Migration
MerkleSearchTree.Add/Create reject invalid MST keys; Create rejects duplicate keys	Key the tree by repo paths
MerkleSearchTree.CreateFromEntries removed	MerkleSearchTree.Create(entries)
MstNodeData, MstTreeEntry, MstKeyDepth internal	Use MerkleSearchTree
DagCborDecoder.Decode throws FormatException (was InvalidOperationException / CborContentException); Deserialize depth → FormatException	Catch FormatException
DagCborDecoder.DecodeToNode, DagCborEncoder.ComputeCid, CarBlock.DataLength removed	JsonSerializer.SerializeToNode(Decode(b)), CidComputation.ComputeForDagCbor, Data.Length
CarReader rejects CIDv0 blocks, headers without version, untagged roots, non-minimal varints	None for AT Protocol data

Verification

• Unit tests: 1910 passed, 0 failed, 5 skipped (the pre-existing env-gated skips). The full solution build has 0 warnings and 0 errors, and the integration tests compile.
• MST reference vectors, all passing:
  • mst.test.ts known maps and edge cases: 11 roots, including add/delete sequences
  • the allowable-keys lists: 23 rejected, 8 allowed, plus the 1024-character boundary
  • interop common_prefix.json (13), key_heights.json (9) and example_keys.txt (156 heights)
  • interop firehose/commit-proof-fixtures.json: 6 fixtures × (root before, root after, blocksInProof ⊆ SerializeProof)
• Trees built with the published @atproto/repo 0.10.14 from a deterministic LCG key generator that is reproduced in C#:
  • 9 trees of 1–5000 keys, pinned by root CID, block count and a SHA-256 digest of the sorted block CIDs
  • a 500-key tree with every third key deleted
  • 8 covering proofs (present, absent-before, absent-after and absent-between keys) that match getCoveringProof exactly
• The shipped v0.6.0 code matched 0 of 5 probe vectors, per the audit.
• DAG-CBOR: the 3 interop data-model-fixtures.json vectors (bytes and CID) encode exactly and decode→re-encode to identical bytes.
• Crafted input: tests cover deep nesting, huge, negative and beyond-UInt64 p, CIDv0, truncated and overlong varints, oversized lengths, malformed headers and nodes, and malformed CID links. All throw FormatException, or return null from the firehose parser.
• Write path: RepoWritePathTests writes 120 records → MST → signed RepoCommit → CarWriter. It reads the result back with CarReader(verifyBlockCids: true), verifies the signature via FirehoseVerifier.ExtractSignedView, then runs Deserialize and Validate() and checks every record. A second test carries a commit diff with its covering proof.
• Mutation check: reverting CompareCanonical to UTF-16 order fails both encoder-ordering tests.

Benchmarks

Release build, best of 5, base 7f672c0 vs this branch:

Operation	Before	After
DagCborDecoder.Decode(post)	3.4–3.7 µs, 8.5 KB	3.0–3.1 µs, 5.3 KB
DagCborEncoder.Encode(post)	3.7 µs, 8.9 KB	3.3 µs, 8.1 KB
FirehoseEventParser.Parse(#commit)	3.1 µs, 12.3 KB	3.0 µs, 12.2 KB
MST ComputeRootCid, 10k entries	4.0 ms, 10.5 MB	3.7 ms, 7.1 MB
MST Serialize, 10k entries	5.8 ms, 11.2 MB	5.5 ms, 7.7 MB
MST SerializeProof(1 key), 10k entries	6.3 ms, 11.4 MB	3.3–3.9 ms, 7.1 MB
CarReader.FromBytes(verify), 10k-entry MST	1.2–1.4 ms	1.2–1.4 ms (noise)

The "before" MST numbers are for the old, non-conformant tree.

Deliberately deferred / notes

• Every ComputeRootCid / Serialize / SerializeProof rebuilds the node structure (≈3.7 ms per call at 10k entries). This was chosen for obvious correctness. Caching subtree CIDs between edits is a possible later optimization, and the property test and vectors would guard it.
• Deserialize does not re-hash blocks. It documents passing CID-verified blocks, and Validate() catches any mismatch against the root.
• DagCborDecoder still reads in Lax mode, so duplicate map keys and invalid UTF-8 are accepted as before. Rejecting them (DRISL strictness) would be a separate behaviour change.
• FirehoseVerifier.cs:11 still claims MST-proof verification. That belongs to the streaming work package (WP12 T8) and was not touched here.
• Edits outside the listed area: none. SpaceRepoCar.cs is the only Spaces file touched, and docs/crypto.md and docs/api-reference.md were updated for the removed or now-internal APIs.

🤖 Generated with Claude Code

Closes #111. ## Correctness: MST - **Canonical encoding.** Nodes always carry `l` and every entry's `t`, written as CBOR `null` when absent. The empty tree hashes to `bafyreie5737gdxlw5i64vzichcalba3z2v5n6icifvx5xytvske7mr3hpm`. - **Decoding `null` links.** `MstNodeData.FromBytes` reads `null` (and, leniently, omitted) `l`/`t`. `MerkleSearchTree.Deserialize` now loads real `getRepo` trees. - **One simple design.** The incremental Insert/Split/Remove/Merge code is gone. The tree keeps a sorted entry set (key → CID + cached key height) and derives the node structure from it on every `ComputeRootCid` / `Serialize` / `SerializeProof`. Empty ranges become null links, and ranges whose keys all sit lower get one entry-less wrapper node per skipped layer. Any edit sequence therefore yields the bulk-build root by construction, and a seeded property test checks it anyway (100 trials × 150 random add/update/delete ops, against a shuffled bulk build and a Serialize→Deserialize→Validate round trip). - **Keys** are validated like the reference `isValidMstKey`: `collection/rkey`, charset `A-Z a-z 0-9 _ ~ - : .`, ≤ 1024 characters. - **`SerializeProof`** now ports the reference `getCoveringProof` (the key's path plus the left and right neighbour paths). Relays need those nodes to invert a `#commit`. It is a superset of the old path-only proof. - **`Validate()`** now rebuilds and compares. After `Deserialize` it checks that the canonical rebuild equals the loaded root. For in-memory trees it is trivially true. - `ComputeNodeCid`/`SerializeNode` and `Create(entries)`/`CreateFromEntries` are deduplicated. `MstNodeData`, `MstTreeEntry` and `MstKeyDepth` are now `internal`. ## Security: untrusted input - **MST `PrefixLen`:** `0 ≤ p ≤ previousKey.Length` is enforced in `MstNodeData.FromBytes` before any slicing. `p` is read as Int64, so out-of-range, negative, 2^31, 2^40, `long.MinValue` and `2^64-1` all fail with `FormatException`. - **`MerkleSearchTree.Deserialize`** also rejects: - invalid keys (non-ASCII bytes are decoded as Latin-1, so they fail the key check rather than being replaced) - keys that are out of order across the in-order walk. This also makes a subtree linked twice fail on its first repeated key. - empty inner nodes - depth > 64 (checked on a 10 000-node chain) - missing or malformed blocks - **`CarReader` header:** replaced the recursive hand-written CBOR parser with `CborReader` + `SkipValue`. A 100 000-deep nested header value is now skipped without a crash, and a truncated one throws `FormatException`. The header must carry `version`, its roots must be tag-42 links with the `0x00` prefix, and no trailing bytes are allowed. - **`CarReader` framing:** every length (header, block, CID digest) is checked against the remaining input before it is narrowed or sliced. Varints must be ≤ 9 bytes and minimal. - **CIDv0:** `CarReader.FromBytes` rejects CIDv0 blocks, and `VerifyBlockCid` reports them as `UnknownCodec`. - **Depth limit:** 64 JSON levels (the `System.Text.Json` default, counting the `$link`/`$bytes` wrappers) in the shared transcoder and in `DagCborDecoder.TryValidate`. Chains of unknown tags are iterated, not recursed. `DagCborDecoder.Decode` normalizes all malformed input to `FormatException`. `FirehoseEventParser.Parse` returns `null` as before. ## Consolidation - **`DagCborLink`** (internal) is the one null-aware tag-42 read/write helper. It is used by `CarWriter`, `CarReader`, `MstNodeData`, `RepoCommit`, `DagCborEncoder`, `SpaceRepoCar` and the transcoder, and throws `FormatException` for malformed links. Writes go through a stack buffer. - **`DagCborJson`** (internal) is the one-pass DAG-CBOR→`Utf8JsonWriter` transcoder, in `Wrapped` (`$link`/`$bytes`) and `Flattened` (firehose models) forms, with the depth limit. `DagCborDecoder.Decode` parses its output with `JsonElement.ParseValue`, with no `JsonNode` tree. - **`DagCborEncoder`** writes with `CborConformanceMode.Lax` over its own sort and detects duplicate keys itself, so the writer no longer sorts maps a second time. `CompareCanonical` now compares equal-length keys by UTF-8 bytes. Before, it used UTF-16 order, which was wrong where U+10000+ meets U+E000–U+FFFF; the Canonical writer's re-sort had been hiding that bug. Output is asserted byte-identical to a Canonical-writer reference on 500 random documents with adversarial keys. - **`SpaceRepoCar.Serialize`** writes the index map directly with `CborWriter`, byte-identical to the old `JsonObject`→encoder path (asserted). - **Removed:** `DagCborDecoder.DecodeToNode`, `DagCborEncoder.ComputeCid`, `CarBlock.DataLength`, and the unused locals in `CarReader`. `CidComputation.TryDecodeCidString` and `Verify` are kept, as are `CarWriter.WriteToAsync` and `Tid.FromInt64`/`ToInt64`. ## Breaking changes (migration) | Change | Migration | |---|---| | `MerkleSearchTree.Add`/`Create` reject invalid MST keys; `Create` rejects duplicate keys | Key the tree by repo paths | | `MerkleSearchTree.CreateFromEntries` removed | `MerkleSearchTree.Create(entries)` | | `MstNodeData`, `MstTreeEntry`, `MstKeyDepth` internal | Use `MerkleSearchTree` | | `DagCborDecoder.Decode` throws `FormatException` (was `InvalidOperationException` / `CborContentException`); `Deserialize` depth → `FormatException` | Catch `FormatException` | | `DagCborDecoder.DecodeToNode`, `DagCborEncoder.ComputeCid`, `CarBlock.DataLength` removed | `JsonSerializer.SerializeToNode(Decode(b))`, `CidComputation.ComputeForDagCbor`, `Data.Length` | | `CarReader` rejects CIDv0 blocks, headers without `version`, untagged roots, non-minimal varints | None for AT Protocol data | ## Verification - **Unit tests:** 1910 passed, 0 failed, 5 skipped (the pre-existing env-gated skips). The full solution build has 0 warnings and 0 errors, and the integration tests compile. - **MST reference vectors, all passing:** - `mst.test.ts` known maps and edge cases: 11 roots, including add/delete sequences - the allowable-keys lists: 23 rejected, 8 allowed, plus the 1024-character boundary - interop `common_prefix.json` (13), `key_heights.json` (9) and `example_keys.txt` (156 heights) - interop `firehose/commit-proof-fixtures.json`: 6 fixtures × (root before, root after, `blocksInProof` ⊆ `SerializeProof`) - **Trees built with the published `@atproto/repo` 0.10.14** from a deterministic LCG key generator that is reproduced in C#: - 9 trees of 1–5000 keys, pinned by root CID, block count and a SHA-256 digest of the sorted block CIDs - a 500-key tree with every third key deleted - 8 covering proofs (present, absent-before, absent-after and absent-between keys) that match `getCoveringProof` exactly - **The shipped v0.6.0 code** matched 0 of 5 probe vectors, per the audit. - **DAG-CBOR:** the 3 interop `data-model-fixtures.json` vectors (bytes and CID) encode exactly and decode→re-encode to identical bytes. - **Crafted input:** tests cover deep nesting, huge, negative and beyond-UInt64 `p`, CIDv0, truncated and overlong varints, oversized lengths, malformed headers and nodes, and malformed CID links. All throw `FormatException`, or return `null` from the firehose parser. - **Write path:** `RepoWritePathTests` writes 120 records → MST → signed `RepoCommit` → `CarWriter`. It reads the result back with `CarReader(verifyBlockCids: true)`, verifies the signature via `FirehoseVerifier.ExtractSignedView`, then runs `Deserialize` and `Validate()` and checks every record. A second test carries a commit diff with its covering proof. - **Mutation check:** reverting `CompareCanonical` to UTF-16 order fails both encoder-ordering tests. ### Benchmarks Release build, best of 5, base `7f672c0` vs this branch: | Operation | Before | After | |---|---|---| | `DagCborDecoder.Decode(post)` | 3.4–3.7 µs, 8.5 KB | 3.0–3.1 µs, 5.3 KB | | `DagCborEncoder.Encode(post)` | 3.7 µs, 8.9 KB | 3.3 µs, 8.1 KB | | `FirehoseEventParser.Parse(#commit)` | 3.1 µs, 12.3 KB | 3.0 µs, 12.2 KB | | MST `ComputeRootCid`, 10k entries | 4.0 ms, 10.5 MB | 3.7 ms, 7.1 MB | | MST `Serialize`, 10k entries | 5.8 ms, 11.2 MB | 5.5 ms, 7.7 MB | | MST `SerializeProof(1 key)`, 10k entries | 6.3 ms, 11.4 MB | 3.3–3.9 ms, 7.1 MB | | `CarReader.FromBytes(verify)`, 10k-entry MST | 1.2–1.4 ms | 1.2–1.4 ms (noise) | The "before" MST numbers are for the old, non-conformant tree. ## Deliberately deferred / notes - Every `ComputeRootCid` / `Serialize` / `SerializeProof` rebuilds the node structure (≈3.7 ms per call at 10k entries). This was chosen for obvious correctness. Caching subtree CIDs between edits is a possible later optimization, and the property test and vectors would guard it. - `Deserialize` does not re-hash blocks. It documents passing CID-verified blocks, and `Validate()` catches any mismatch against the root. - `DagCborDecoder` still reads in `Lax` mode, so duplicate map keys and invalid UTF-8 are accepted as before. Rejecting them (DRISL strictness) would be a separate behaviour change. - `FirehoseVerifier.cs:11` still claims MST-proof verification. That belongs to the streaming work package (WP12 T8) and was not touched here. - Edits outside the listed area: none. `SpaceRepoCar.cs` is the only Spaces file touched, and `docs/crypto.md` and `docs/api-reference.md` were updated for the removed or now-internal APIs. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit 2026-09-24 18:04:55 +00:00

fix(repo): spec-correct MST and hardened DAG-CBOR / CAR parsing (closes #111) ... 941ed782c5

The MST did not follow the atproto spec: absent l/t links were omitted
instead of written as null, empty subtrees became empty nodes, and
incremental Add/Delete disagreed with the bulk build. Every root CID
differed from the network's, and Deserialize threw on every real node.
The tree now keeps a sorted entry set and derives the canonical node
structure from it, so any edit sequence yields the bulk-build root. It
is pinned to the upstream mst.test.ts vectors, the interop commit-proof
fixtures, and trees built with @atproto/repo. SerializeProof now returns
the reference covering proof and Validate rebuilds and compares.

The parsers run on relay data and could be crashed by crafted input:
CarReader's recursive CBOR skipper overflowed the stack on a nested
header, the DAG-CBOR to JSON walkers had no depth limit, MST prefix
lengths were unchecked (indigo SEC-4), and CIDv0 blocks bypassed the
codec rule. All now fail with FormatException.

CID links are read and written through one DagCborLink helper, and
DagCborDecoder and the firehose parser share one depth-limited one-pass
transcoder. MstNodeData, MstTreeEntry and MstKeyDepth become internal;
CreateFromEntries, DecodeToNode, DagCborEncoder.ComputeCid and
CarBlock.DataLength are removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Grandiras commented 2026-09-24 18:05:02 +00:00

Author

Owner

Copy link

@claude /review

@claude /review

claude-bot commented 2026-09-24 18:05:07 +00:00

Collaborator

Copy link

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/582); the reply will link the full session transcript. <!-- claude-run:pr_review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-147/run-582.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes 2026-09-24 18:08:59 +00:00

claude-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

This is an extensive, carefully engineered rewrite of the MST and DAG-CBOR/CAR parsing paths, and the diff is internally consistent with the PR description's claims. I traced through the trickiest pieces by hand:

• MST canonical build (Build/BuildRange in MerkleSearchTree.cs): the layer-by-layer construction correctly produces null links for empty ranges and entry-less wrapper nodes for gaps whose keys sit at lower layers, matching the described spec-conformant shape. Recursion terminates safely (base case on start == end regardless of layer, so no negative-layer issues).
• MstNodeData.FromBytes prefix-length validation: p is bounds-checked against the previous key's reconstructed byte length before any slicing, closing the SEC-4-class bug the PR describes.
• CarReader framing: header/block/digest lengths are all checked against the actually-remaining bytes (via a truncated span for CID digest bounds) before narrowing/slicing, and ReadUvarint correctly enforces the 9-byte/63-bit cap with minimality checking.
• DagCborEncoder.CompareCanonical's UTF-16→UTF-8-byte-order fix: I worked through the surrogate remapping (CodePointOrder) by hand against the UTF-8 lead-byte-ordering property and it is correct — supplementary code points are correctly ranked above all BMP code points, and the char-by-char walk stays index-aligned with the true codepoint sequence as long as it's scanning for the first difference (which it is).
• SerializeProof's covering-proof mirror of getCoveringProof: complex, but the Children()/FindGreaterOrEqualEntry indexing is consistent, and this is exactly the kind of logic the PR's 8 pinned interop covering-proof vectors are built to catch.

I did not execute the test suite myself, so the vector/benchmark numbers are taken on faith, but nothing in the diff contradicts them and the code changes are coherent with the stated design.

One minor, non-blocking observation below about a depth-limit semantic mismatch between DagCborDecoder.TryValidate and Decode.

---

⏱ 3m 32s · 1 turns · tokens: 74.4k in / 21.2k out (+23.9k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is an extensive, carefully engineered rewrite of the MST and DAG-CBOR/CAR parsing paths, and the diff is internally consistent with the PR description's claims. I traced through the trickiest pieces by hand: - **MST canonical build** (`Build`/`BuildRange` in `MerkleSearchTree.cs`): the layer-by-layer construction correctly produces null links for empty ranges and entry-less wrapper nodes for gaps whose keys sit at lower layers, matching the described spec-conformant shape. Recursion terminates safely (base case on `start == end` regardless of `layer`, so no negative-layer issues). - **`MstNodeData.FromBytes` prefix-length validation**: `p` is bounds-checked against the previous key's reconstructed byte length before any slicing, closing the SEC-4-class bug the PR describes. - **`CarReader` framing**: header/block/digest lengths are all checked against the actually-remaining bytes (via a truncated span for CID digest bounds) before narrowing/slicing, and `ReadUvarint` correctly enforces the 9-byte/63-bit cap with minimality checking. - **`DagCborEncoder.CompareCanonical`'s UTF-16→UTF-8-byte-order fix**: I worked through the surrogate remapping (`CodePointOrder`) by hand against the UTF-8 lead-byte-ordering property and it is correct — supplementary code points are correctly ranked above all BMP code points, and the char-by-char walk stays index-aligned with the true codepoint sequence as long as it's scanning for the first difference (which it is). - **`SerializeProof`'s covering-proof mirror of `getCoveringProof`**: complex, but the `Children()`/`FindGreaterOrEqualEntry` indexing is consistent, and this is exactly the kind of logic the PR's 8 pinned interop covering-proof vectors are built to catch. I did not execute the test suite myself, so the vector/benchmark numbers are taken on faith, but nothing in the diff contradicts them and the code changes are coherent with the stated design. One minor, non-blocking observation below about a depth-limit semantic mismatch between `DagCborDecoder.TryValidate` and `Decode`. --- ⏱ 3m 32s · 1 turns · tokens: 74.4k in / 21.2k out (+23.9k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-147/run-582.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:941ed782c574ee8f9d1549224f37e2d567880b86 -->

src/ATProtoNet/Repo/DagCborDecoder.cs

claude-reviewer commented 2026-09-24 18:08:59 +00:00

Collaborator

Copy link

TryValidate's depth counter only increments on raw CBOR map/array nesting, while Decode's JSON-emitting depth counter (DagCborJson) also charges +1 for the $link/$bytes wrapper object a CID or byte string produces. So input that sits at exactly the container depth limit with a CID/bytes value at the deepest level can pass TryValidate(..., out error) == true yet still throw FormatException from Decode. Not a security issue (Decode is the stricter one), but a caller relying on TryValidate as a pre-check before Decode could be surprised. Worth a doc note or aligning the two depth counts if that's not intentional.

`TryValidate`'s depth counter only increments on raw CBOR map/array nesting, while `Decode`'s JSON-emitting depth counter (`DagCborJson`) also charges +1 for the `$link`/`$bytes` wrapper object a CID or byte string produces. So input that sits at exactly the container depth limit with a CID/bytes value at the deepest level can pass `TryValidate(..., out error) == true` yet still throw `FormatException` from `Decode`. Not a security issue (Decode is the stricter one), but a caller relying on `TryValidate` as a pre-check before `Decode` could be surprised. Worth a doc note or aligning the two depth counts if that's not intentional.

claude-bot commented 2026-09-24 18:08:59 +00:00

Collaborator

Copy link

🔧 Let Claude fix this — tick the box and claude-bot picks up the review above, on this PR branch. Nothing runs until you do.

• @claude-bot Address the findings of the latest review on this pull request.

🔧 **Let Claude fix this** — tick the box and claude-bot picks up the review above, on this PR branch. Nothing runs until you do. - [ ] @claude-bot Address the findings of the latest review on this pull request. <!-- claude-fix-button:147 -->

Grandiras force-pushed issue-111-mst-cbor from 941ed782c5 to dd0dd566fd 2026-09-24 18:09:58 +00:00 Compare

Grandiras commented 2026-09-24 18:11:16 +00:00

Author

Owner

Copy link

Rebased onto main to resolve a CHANGELOG-only conflict (#148 merged). verify-rebase: the PR's patch outside CHANGELOG.md is byte-identical between the approved head 941ed782 and the new head dd0dd566 (4344 changed lines, hunk offsets ignored). CI is green on the new head, so the approval carries over (maintainer-approved policy for CHANGELOG-only rebases).

Rebased onto main to resolve a CHANGELOG-only conflict (#148 merged). `verify-rebase`: the PR's patch outside `CHANGELOG.md` is byte-identical between the approved head `941ed782` and the new head `dd0dd566` (4344 changed lines, hunk offsets ignored). CI is green on the new head, so the approval carries over (maintainer-approved policy for CHANGELOG-only rebases).

Grandiras merged commit c55611f2fb into main 2026-09-24 18:11:17 +00:00

Grandiras deleted branch issue-111-mst-cbor 2026-09-24 18:11:17 +00:00

Grandiras referenced this pull request from a commit 2026-09-24 18:11:18 +00:00

fix(repo): spec-correct MST and hardened DAG-CBOR / CAR parsing (#147)

Grandiras referenced this pull request 2026-09-24 18:14:11 +00:00

fix(identity)!: spec-conformant identifier types and a monotonic TID generator #146

Sign in to join this conversation.

Reviewers

No reviewers

claude-reviewer

Labels

Clear labels   

breaking-change

Introduces a breaking API change

  

bug

Something is not working correctly

  

documentation

Documentation improvements

  

duplicate

This issue or PR already exists

  

enhancement

New feature or improvement

  

good first issue

Good for newcomers to the project

  

help wanted

Extra attention is needed

  

performance

Performance-related issue or improvement

  

question

Further information is requested

  

wontfix

This will not be worked on

No labels

breaking-change

bug

documentation

duplicate

enhancement

good first issue

help wanted

performance

question

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

3 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Grandiras/ATProto.NET!147

No description provided.