test: comprehensive coverage for indefinite-length CBOR map support (#23)#24
Merged
Conversation
Real AWS Nitro attestation documents use indefinite-length CBOR maps (0xBF) which were not supported. Added ai==31 handling in CborDecode and 0xFF break-marker detection in NitroValidator's map parsing loop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover the two changes in the fix PR: ai==31 handling in CborDecode.sol and 0xFF break marker detection in NitroValidator.sol. 18 new tests across 2 contracts: - CborDecodeIndefiniteLengthTest: 7 tests for mapAt/arrayAt with indefinite-length, definite-length regression, and reserved AI reverts - NitroValidatorIndefiniteLengthTest: 11 tests covering synthetic and real AWS attestation data, indefinite/definite equivalence, edge cases (empty map, early break), and negative cases (unknown keys) Real attestation data exercises 20 embedded 0xFF bytes in DER-encoded certificate content, verifying the break-marker check does not false-trigger on content bytes.
5 new tests covering gaps identified in test coverage review: 1. test_neg_missingBreakMarker_reverts: indefinite-length map without trailing 0xFF — parser reads into garbage and reverts 2. test_converter_structurallyCorrect: independently verify that _toIndefiniteLength produces correct output (length, break marker, map header, bstr length, preserved content) 3. test_edge_certValueContainingFF_beforeBreak: byte-string value containing 0xFF immediately before the break marker — verifies break check examines header positions, not value content 4. test_edge_innerIndefinitePcrsEmpty_outerBreakTriggered: empty indefinite-length inner PCRs map — documents that the inner 0xFF break marker triggers the outer loop's break check, causing silent early termination of remaining entries 5. test_neg_nestedIndefiniteNonEmptyArray_reverts: non-empty indefinite-length inner array — inner elements are not consumed, causing type mismatch revert
The individual definite/indefinite tests already verify all fields against expected constants, making the explicit field-by-field comparison tests logically redundant. Removes test_synth_indefiniteMatchesDefinite, test_real_indefiniteMatchesDefinite, and the now-unused _assertPtrsMatch helper.
Per RFC 8949, indefinite-length encoding is only defined for major types 4 (array) and 5 (map). Other major types with ai=31 (e.g. 0x5F indefinite byte string, 0x7F indefinite text, 0x1F reserved) are not supported by this decoder. Downstream validation in validateAttestation() would catch these cases regardless, but rejecting here gives an immediate, unambiguous revert rather than a confusing downstream failure.
robriks
approved these changes
Mar 30, 2026
robriks
left a comment
robriks
left a comment
Contributor
There was a problem hiding this comment.
LGTM!
The require(_type == 0xa0 || _type == 0x80, ...) guard in CborDecode.elementAt is a meaningful improvement over the original fix — without it, a crafted attestation containing an indefinite-length byte/text string (0x5F/0x7F) would have silently been treated as an empty string, potentially bypassing certificate and key length checks in validateAttestation.
Test coverage is thorough 👍
jackchuma
approved these changes
Mar 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds 23 tests across 2 test contracts covering the indefinite-length CBOR changes introduced in #23 (
ai==31handling inCborDecode.soland0xFFbreak marker detection inNitroValidator.sol).Tests
CborDecode-level (7 tests):
ai==31returnsvalue=0)ai==31branch)NitroValidator-level (16 tests):
0xFFbytes in DER-encoded certificate content — verifies the break-marker check doesn't false-trigger on content bytes_toIndefiniteLengthhelper output (length, break byte, map header, bstr length, content preservation)0xFFvalue bytes immediately before the break marker0xFFbreak triggers the outer loop's break check (silent early termination) — safe for real attestations since AWS always uses definite-length inner structuresNotes
_toIndefiniteLength()programmatically converts any definite-length TBS to indefinite-length, avoiding hand-maintained duplicate test dataNitroValidator.t.sol