Fix CBOR tag handlers not recognizing tags 0-5 and 21-23 - #5331
Fix CBOR tag handlers not recognizing tags 0-5 and 21-23#5331sahilkamate03 wants to merge 2 commits into
Conversation
96e9fb4 to
4d41812
Compare
| SECTION("0xC0..0xD7") | ||
| { | ||
| for (const auto b : std::vector<std::uint8_t> | ||
| { | ||
| 0xC6, 0xC7, 0xC8, 0xC9, 0xCA, 0xCB, 0xCC, 0xCD, 0xCE, 0xCF, 0xD0, 0xD1, 0xD2, 0xD3, 0xD4 | ||
| 0xC0, 0xC1, 0xC2, 0xC3, 0xC4, 0xC5, | ||
| 0xC6, 0xC7, 0xC8, 0xC9, 0xCA, 0xCB, 0xCC, 0xCD, 0xCE, 0xCF, 0xD0, 0xD1, 0xD2, 0xD3, 0xD4, | ||
| 0xD5, 0xD6, 0xD7 |
There was a problem hiding this comment.
The tests only wrap a string value for each new tag byte. Might be worth a case wrapping a binary payload under cbor_tag_handler_t::store to confirm 0xC0-0xC5/0xD5-0xD7 take the same "unwrap, don't treat as subtype" path as 0xC6-0xD4 already do.
There was a problem hiding this comment.
The "Incomplete mapping" warning still lists date/time (0xC0..0xC1), bignum (0xC2..0xC3), decimal fraction (0xC4), bigfloat (0xC5), and expected conversions (0xD5..0xD7) as types that "are not supported and will yield parse errors." That's now only true for the default error handler — under ignore/store these parse fine, same as 0xC6..0xD4/0xD8..0xDB, which were already correctly omitted from this list. Since the PR already updates the "Tagged items" warning just below to say the full 0xC0..0xDB range is tag-handler-dependent, these five bullets should be deleted from the "Incomplete mapping" box to avoid the two warnings contradicting each other.
|
Apologies for the AI-assisted review here — I don't have time for a deeper pass on this one right now, so this is a quick comparison rather than a full re-review. We got a second PR for the same issue, #5367, which has the identical code fix (same The other ask from the review above (a Given this one already has a clean CI run and is otherwise the more complete PR, it's the better one to land — just needs the docs fix and the extra test case. — posted by Claude Code on behalf of @nlohmann |
The "Incomplete mapping" warning still listed tags 0-5 (date/time, bignum, decimal fraction, bigfloat) and 21-23 (expected conversions) as unsupported, even though they now parse correctly under cbor_tag_handler_t::ignore/store, same as 0xC6..0xD4/0xD8..0xDB. Remove those five bullets and cross-reference the "Tagged items" warning below, matching the equivalent docs fix landed independently in PR nlohmann#5367. Also add a cbor_tag_handler_t::store test that wraps a binary payload (not just a string) for every byte in 0xC0..0xD7, confirming these tags are unwrapped the same way as 0xC6..0xD4 rather than mistaken for the 0xD8..0xDB binary-subtype marker syntax, per review feedback on nlohmann#5331. Signed-off-by: sahilkamate03 <45514385+sahilkamate03@users.noreply.github.com>
0114cb1 to
ad29f64
Compare
The "Incomplete mapping" warning still listed tags 0-5 (date/time, bignum, decimal fraction, bigfloat) and 21-23 (expected conversions) as unsupported, even though they now parse correctly under cbor_tag_handler_t::ignore/store, same as 0xC6..0xD4/0xD8..0xDB. Remove those five bullets and cross-reference the "Tagged items" warning below, matching the equivalent docs fix landed independently in PR nlohmann#5367. Also add a cbor_tag_handler_t::store test that wraps a binary payload (not just a string) for every byte in 0xC0..0xD7, confirming these tags are unwrapped the same way as 0xC6..0xD4 rather than mistaken for the 0xD8..0xDB binary-subtype marker syntax, per review feedback on nlohmann#5331. Signed-off-by: sahilkamate03 <45514385+sahilkamate03@users.noreply.github.com>
The tagged-item switch in binary_reader::parse_cbor_internal() only handled head bytes 0xC6-0xD4 and 0xD8-0xDB. Bytes 0xC0-0xC5 (tags 0-5: date/time, epoch, bignum, decimal, bigfloat) and 0xD5-0xD7 (tags 21-23: base64url, base64, base16 conversion hints) fell through to the default case and were reported as invalid bytes, even under cbor_tag_handler_t::ignore and ::store, despite being valid CBOR major-type-6 tags per RFC 8949. Add the missing case labels so the full 0xC0-0xDB range is handled uniformly. Extend the "Tagged values" test in unit-cbor.cpp to cover 0xC0-0xD7, and update the CBOR docs to state the corrected tag range. Fixes nlohmann#5315 Signed-off-by: sahilkamate03 <45514385+sahilkamate03@users.noreply.github.com>
The "Incomplete mapping" warning still listed tags 0-5 (date/time, bignum, decimal fraction, bigfloat) and 21-23 (expected conversions) as unsupported, even though they now parse correctly under cbor_tag_handler_t::ignore/store, same as 0xC6..0xD4/0xD8..0xDB. Remove those five bullets and cross-reference the "Tagged items" warning below, matching the equivalent docs fix landed independently in PR nlohmann#5367. Also add a cbor_tag_handler_t::store test that wraps a binary payload (not just a string) for every byte in 0xC0..0xD7, confirming these tags are unwrapped the same way as 0xC6..0xD4 rather than mistaken for the 0xD8..0xDB binary-subtype marker syntax, per review feedback on nlohmann#5331. Signed-off-by: sahilkamate03 <45514385+sahilkamate03@users.noreply.github.com>
ad29f64 to
2858840
Compare
The "Incomplete mapping" warning still listed tags 0-5 (date/time, bignum, decimal fraction, bigfloat) and 21-23 (expected conversions) as unsupported, even though they now parse correctly under cbor_tag_handler_t::ignore/store, same as 0xC6..0xD4/0xD8..0xDB. Remove those five bullets and cross-reference the "Tagged items" warning below, matching the equivalent docs fix landed independently in PR nlohmann#5367. Also add a cbor_tag_handler_t::store test that wraps a binary payload (not just a string) for every byte in 0xC0..0xD7, confirming these tags are unwrapped the same way as 0xC6..0xD4 rather than mistaken for the 0xD8..0xDB binary-subtype marker syntax, per review feedback on nlohmann#5331. Signed-off-by: sahilkamate03 <45514385+sahilkamate03@users.noreply.github.com>
2858840 to
6b54d47
Compare
Fixes #5315.
What changed
case 0xC0-0xC5andcase 0xD5-0xD7labels to the tagged-item switch in binary_reader::parse_cbor_internal()(include/nlohmann/detail/input/binary_reader.hpp), so the full0xC0-0xDB` tag-header range is handled the same way."Tagged values"test intests/src/unit-cbor.cppto cover0xC0-0xD7(previously only0xC6-0xD4was tested) undererror,ignore, andstorehandlers.docs/mkdocs/docs/features/binary_formats/cbor.mdto state the tagged-item range as0xC0..0xDBinstead of leaving it unspecified.single_include/nlohmann/json.hppviamake amalgamate.Why
0xC0-0xC5: RFC 3339 date/time, epoch time, bignum, decimal, bigfloat) and tags 21-23 (0xD5-0xD7: base64url, base64, base16 conversion hints) are valid major-type-6 tags per RFC 8949, but the switch statement only recognized0xC6-0xD4and0xD8-0xDB.defaultcase and throwparse_error.112("invalid byte"), even undercbor_tag_handler_t::ignoreand::store— handlers that are documented to accept any tag. This broke interop with compliant ncoders that use preferred (shortest) serialization, since e.g. tag 23 is encoded as the single byte0xD7.Checklist
make amalgamate(verified: diff matches theinclude/nlohmannsource change exactly).Test plan
cmake -S. -B build && cmake --build build -j10 && ctest --test-dir build -j10→ 100% tests passed, 0 failed out of 109test-cbor_cpp11covers the new0xC0-0xD7tag range undererror,ignore, andstorehandlers