fix(jcs): count open containers for the nesting bound, so an empty container leaf is refused - #1294
Conversation
🧪 Code Coverage (vs
|
|
@a2aproject/google-a2a-eng, could you review this for merge? It fixes the depth-boundary defect and incorporates Probity's attributed regression corpus. All 16 checks pass at bfa47f3. The fix is independent of the pending canonicalization-scope decision in A2A #2122. Please merge it if the approach is acceptable, or identify the remaining blocker here. Please retain the Probity source reference and digest checks when landing it, so future changes continue to run against the same published cases. |
An empty container was accepted one level past MAX_DEPTH because the counter charges a level per value on the path (0-based) instead of per open container (outermost at 1), so the effective bound depended on whether the innermost value was a container or a scalar. `_clean_empty` runs before canonicalization and carried the same counter. Adds the jcs_depth_v1 corpus as tests: preimage digests first, published bytes and SHA-256 for the accepts, a catchable refusal for the rejects, and the empty-container boundary pair through both the canonicalizer and the pre-pass. Review follow-up: only containers recurse, so only containers carry a depth worth checking. The per-child `_child_depth` helper and the two `isinstance` ternaries in `_clean_empty` are gone; the recursion passes `depth + 1` outright. The deep vector drops from ten million levels to 500, shallow enough that the JSON decoder accepts it, so the reject cases now prove the bound refuses the input instead of the decoder running out of stack first.
bfa47f3 to
e75cc75
Compare
|
All five review points are in e75cc75, on top of current
Evidence on this head: 4 files changed, +342/−6; Thanks for the careful read: the |
3 participants
What
_jcs.canonicalizebounds nesting atMAX_DEPTH = 128, but the counter tracks values on the path (0-based) rather than open containers (outermost at 1). An empty container is therefore accepted one level past the bound, while the same shape wrapped around a scalar is refused — the effective limit depends on what the innermost value is.This counts open containers instead, and applies the same rule to
signing._clean_empty, which runs before canonicalization and would otherwise be the exhaustion path.Why
jcs_depth_v1(a2aproject/A2A#2246) pins eight inputs and their verdicts; it is added here astests/utils/jcs_depth_vectors.json, retained as published with attribution. On the current code the empty-container case is mis-accepted:jcs-depth-001…-004jcs-depth-101,-102CanonicalizationErrorCanonicalizationErrorjcs-depth-103(empty-container leaf)CanonicalizationErrorjcs-depth-104RecursionError, at the decodeRecursionError, at the decodejcs-depth-104never reaches the walk: the JSON decoder refuses that input first, in both trees. The test accepts either refusal, as the vector asks, rather than claiming the canonicalizer produced it.The intended rule is already written down in this repo — the docstring of
test_nesting_at_the_limit_is_acceptedsays "the deepest container is at depth n + 1", i.e. containers with the outermost at 1 — so this makes the implementation match its own stated intent, and that boundary test keeps passing unchanged.Tests
tests/utils/test_jcs.pygains the corpus-driven tests: every preimage digest checked before any expectation is used; accepts compared byte-for-byte against the published bytes and SHA-256; rejects asserted to raise; plus a direct empty-container boundary pair and the same pair through_clean_empty.test_jcs.pyandtest_signing.pyboth green.ruff checkandruff format --checkclean on all four files.Scope
Independent of a2aproject/A2A#2122 and of #1287, which are about canonicalization scope, not the nesting bound. Nothing at or below the bound changes: the 24 cards in a2aproject/a2a-tck#246 canonicalize byte-identically before and after.
Reported at a2aproject/A2A#2255. Corpus authored by Sankalp Gilda (Apache-2.0); the JSON keeps the upstream pointer and states that every expected byte string came from the reference implementation it names.