fix(embodied-action): use the existing RFC 8785/JCS canonicalizer instead of a non-compliant one

[rajnisht7]

What

Updated canonical_json_bytes() to use the existing RFC 8785/JCS canonical JSON implementation instead of json.dumps().

Added tests for:

• Raw UTF-8 output
• RFC 8785 UTF-16 key ordering
• Float rejection
• Independent JCS reference hash
  Also verified the existing ROS 2 fixture still works.

Why

The previous serializer was not RFC 8785 compliant because it escaped non-ASCII characters and used Unicode code-point ordering instead of UTF-16 ordering. This could produce different hashes and action_ref values between conformant implementations.

Using the existing tested JCS implementation keeps canonicalization consistent and reproducible.

Note: the shared implementation rejects floats and integers outside the safe 2^53-1 range. Current action refs and fixtures are unaffected, but floats in future detached payloads will now be rejected.

Security impact

Improves hash and action_ref consistency by ensuring RFC 8785/JCS-compliant canonicalization. No other security boundary changes.

Test plan

• pytest passes
• ruff check passes
• mypy passes
• Manual test performed (describe steps below if applicable)

DCO sign-off

• I certify that I wrote or have the right to submit this contribution, and I agree to the
  Developer Certificate of Origin (https://developercertificate.org).

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[altrudev]

I think this PR has one scope boundary worth fixing before merge.

approval.canonical_json is not only a JCS byte serializer; it also carries the catalog-approval binding's admissibility profile. In particular it rejects every float via CatalogApprovalError, while RFC 8785 itself canonicalizes finite IEEE-754 numbers. That stricter refusal set is exactly useful for the execution action/intent binding discussed in #588, but docs/spec/embodied-action-evidence.md currently says only that the detached payload is RFC 8785/JCS. Its optional receipt / approval_context content is not restricted to the catalog-approval value domain.

So this changes more than byte representation. For example a detached payload containing an otherwise valid JCS optional value such as:

{"approval_context":{"risk_score":0.25}}

was hashable before this PR and is valid JCS, but now hash_embodied_action_payload() raises CatalogApprovalError.

There is a second consequence in the verifier path: verify_embodied_action_evidence() calls _verify_hash_value() -> hash_embodied_action_payload() without catching canonicalization failure. A detached payload containing such a value can therefore terminate verification with an exception instead of returning an EmbodiedActionEvidenceResult whose failures records the invalid/unsupported evidence. That makes the PR's stated "No other security boundary changes" stronger than the implementation currently supports.

I would separate the two questions:

1. JCS bytes for the embodied detached payload: use/introduce a primitive whose domain is RFC 8785's domain, unless this profile is explicitly being narrowed.
2. Stricter binding admissibility (floats, non-exact integers, etc.): keep that profile-owned. Define canonical bytes for execution action/intent bindings #588's maintainer ruling makes the approval refusal set appropriate for the execution action/intent binding; I don't read that as automatically redefining every field of the embodied detached payload.

If the intended ruling is instead that embodied-action v0 also adopts the stricter refusal set, I think the profile text needs to state that explicitly and the verifier should catch canonicalization refusal and report it as a verification failure. A verifier-level regression using a float in an optional nested field would make the failure path load-bearing.

The action_ref preimage itself is simpler: its four fields are required strings, so this float-domain question does not affect that preimage.

[qubeena07]

One thing outside this diff's scope worth flagging: the same non ASCII byte divergence problem this PR fixes for embodied action evidence is still live in two other canonicalizers, trace_claim.py around line 301 and verify.py around line 179. Both still do json.dumps(sort_keys=True, ensure_ascii=True) instead of routing through the JCS implementation this PR reuses.

That path signs and verifies TRACE Claims, which is a more sensitive boundary than embodied action hashing. A signer or verifier that is actually RFC 8785 compliant would produce a different byte string for any non ASCII field, so a legitimate signature could fail to verify, or two payloads that should hash differently under real JCS could still collide under the old escaping scheme.

Might be worth a follow up issue so the fix lands everywhere the codebase claims RFC 8785 compliance, not just this one call site.

[qubeena07]

This looks like the right direction, using the existing JCS canonicalizer instead of duplicating one, but there is one blocking correctness issue before merge, the same one altrudev flagged above.

canonical_json now raises CatalogApprovalError on floats, integers above 2**53 minus 1, or unpaired surrogates. Neither verify_embodied_action_evidence nor _verify_hash_value catches that. So a detached payload with something like {"approval_context": {"risk_score": 0.25}}, valid JCS and hashable before this change, now crashes verification instead of returning a result with failures populated. That turns a graceful verification failure into an unhandled exception on untrusted external input, which breaks the fail closed pattern used everywhere else in this module.

I would want either the verifier to catch canonicalization failure and record it as a verification failure, or an explicit decision that this profile is intentionally narrower than plain JCS, with the spec doc updated to say so.

Once that is resolved I am fine approving. The note on trace_claim.py and verify.py in my other comment is not blocking, can be a follow up issue.

[imran-siddique]

The JCS switch is right and it is the ruling from #588: one canonicalizer, approval.py's, and the embodied_action.py serializer retired rather than kept as a second definition of "canonical". The refusals come with it deliberately.

@qubeena07's review was correct and caught the class, and your fix at 9612204 handles the path they named: _verify_hash_value is now wrapped, a refusal is recorded as a failure, and the comment says why the detached payload deserves that treatment. That is the right shape.

One call site is still unwrapped, and it is reachable.

canonical_json_bytes has three callers. Two are inside hash_embodied_action_payload, reached through _verify_hash_value, which your try/except now covers. The third is compute_action_ref, called at line 208 on the same attacker-supplied detached_payload, with nothing around it.

The guard above it does narrow the exposure:

if all(isinstance(detached_payload.get(field), str) for field in _ACTION_REF_FIELDS):
    expected_action_ref = compute_action_ref(detached_payload)

So @qubeena07's own example, {"risk_score": 0.25}, cannot reach it: a float fails the isinstance check. Big integers are excluded the same way.

A lone surrogate is a str. I ran the real canonicalizer to be sure rather than reasoning about it:

plain ascii      -> ok (80 bytes)
non-ascii        -> ok (81 bytes)
LONE SURROGATE   -> CatalogApprovalError: canonical JSON cannot encode an unpaired surrogate
float            -> CatalogApprovalError: canonical JSON does not accept floating point numbers
big int          -> CatalogApprovalError: integer is outside the range RFC 8785 serializes exactly

canonical_json builds the text with ensure_ascii=False, which is correct and required for JCS, and then text.encode("utf-8") raises on an unpaired surrogate. So a detached payload carrying "agent_id": "\ud800" passes the isinstance(str) guard and raises out of compute_action_ref, unhandled, on untrusted input. Same failure mode you just fixed, one line lower.

Wrapping line 208 the same way you wrapped 192 closes it. A canonicalization refusal there should record something like "action_ref preimage could not be canonicalized" rather than crash, for the same reason: an attacker choosing the input should not choose between a verification failure and an exception.

Worth a test alongside the ones you added: a payload whose agent_id is a lone surrogate, asserting failures is populated rather than an exception escaping. Your existing float-rejection test covers the other branch.

Nothing else here needs changing. The four tests you added, raw UTF-8 output, UTF-16 key ordering, float rejection and the independent JCS reference hash, are the right set, and checking the ROS 2 fixture still passes was the right instinct given the refusals are new.

Push that and I will merge. @qubeena07, thank you for the review; it was specific enough to act on and you were right that this was the blocking one.

[qubeena07]

One more angle worth checking before merge, same failure class as the two already fixed here: recursion depth.

The new canonicalizer recurses in pure Python, the old json.dumps was C accelerated and tolerated much deeper nesting. I tested both directly against nested dicts:

depth 990   new canonical_json: ok               old json.dumps: ok
depth 1200  new canonical_json: RecursionError    old json.dumps: ok

RecursionError is a RuntimeError, not caught by either except CatalogApprovalError block added in this PR. detached_payload is attacker/issuer controlled per this PR's own comments, and verify_embodied_action_evidence has no internal caller in this repo wrapping it broadly, it is exported public API. A payload nested around 1200 levels deep, a few KB at most, crashes verification instead of returning a failure result, the same failure mode the float and lone surrogate fixes just closed.

Suggest either catching RecursionError alongside CatalogApprovalError at both call sites, or adding an explicit depth check before canonicalizing untrusted payloads. Not blocking from my side, fine as a fast follow if you'd rather not hold the PR on it, flagging it now so it's not missed a third time.

[qubeena07]

Confirmed, both call sites are wrapped now, _verify_hash_value and compute_action_ref, and the lone surrogate case imran caught has a dedicated test alongside the float one. Clearing my request.

Left a non-blocking follow-up comment on recursion depth, separate from this approval.