fix(buzz-audit): close object_id/detail field-boundary collision in compute_hash (#4173)

[iroiro147]

Summary

Fixes #4173 — takes the issue author's suggested fix #2 (move a fixed-width field between the two variable-length fields).

compute_hash in crates/buzz-audit/src/hash.rs hashed object_id (Option<String>, free text) immediately followed by canonical_json(detail) (arbitrary serde_json::Value, possibly a bare scalar), unframed. Any byte shift across their boundary left the preimage identical:

	object_id	detail	concatenated tail
A	"x"	12	x ‖ 12 = x12
B	"x1"	2	x1 ‖ 2 = x12

Both entries hashed to the same digest. A detail edit survived verify_chain — the property entry.rs documents ("Arbitrary JSON context. Included in the hash (serialized with sorted keys for determinism) so tampering with it is detectable.") was violated.

Fix

Move the fixed-width prev_hash into the preimage between object_id and detail:

…community_id, seq, created_at, action,
actor_pubkey, object_id, prev_hash, canonical_json(detail)

The 32-byte prev_hash can't slide, so no byte shift across the (now separated) object_id/detail boundary can preserve the full preimage.

This is a chain-invalidating field-order change, by design — the same migration treatment any change to compute_hash's fixed field order requires. Existing chains must be re-hashed, or the deployment must wipe and rebuild its audit chain. Maintenance of existing chains is the relay operator's responsibility, as documented on the issue.

Picked fix #2 over length-prefixing because it is the smallest structural change with no framing format to spec, parse, or bug on, and it composes with the existing Some(vec![tag]) / None presence tags unchanged. Length-prefixing every variable-length field is a strictly stronger option if a future format revision is planned.

Why this is safe

• Only crates/buzz-audit/src/hash.rs changes — 44 insertions, 1 deletion.
• All other compute_hash callers (service, lib, entry, error) pass entries, not preimages — they don't depend on field order.
• prev_hash was already part of the hash; only its position changed.

Testing

New regression test hash::tests::object_id_detail_boundary_is_unambiguous pins both the exact repro pair from the issue (object_id="x"/detail=12 versus object_id="x1"/detail=2) and a second string-skewed pair ("ab"+"c" versus "a"+"bc") that exercises the same boundary at different byte lengths.

$ cargo test -p buzz-audit
running 20 tests
test hash::tests::object_id_detail_boundary_is_unambiguous ... ok
test hash::tests::sensitive_to_each_field ... ok
test hash::tests::canonical_json_key_order_is_stable ... ok
... (14/14 pass, 6 ignore — require Postgres)
test result: ok. 14 passed; 0 failed; 6 ignored; 0 measured; 0 filtered out

🤖 Generated with Claude Code

[alanshurafa]

Cross-linking #4216, which fixes the same collision by versioning the encoding instead (rows verify under their stored version; new rows are length-framed; no data migration). The two approaches could genuinely compose — this reorder could simply be the next encoding version — so it may be worth a maintainer call on the shape.

One cost worth surfacing either way: after a field-order change, every pre-existing row fails verify_chain with HashMismatch on untampered data, which is the condition #2638 dug this crate out of ("a genuinely forged row is indistinguishable from the permanent baseline failure, so HashMismatch carries no signal"). Without a shipped re-hash path, the upgrade converts existing audit history from verifiable to permanently failing.

Also: the reorder closes the reported boundary, but the preimage keeps other unframed variable-length neighbours — created_at (after to_storage_precision, to_rfc3339 emits 0/3/6 sub-second digits, so it's variable-width) next to action, and actor_pubkey next to object_id separated only by a constant presence-tag byte, which is not length framing (pk is Vec<u8>; 32 bytes by convention only). Same shape #4173 warned about — worth documenting at minimum.

[iroiro147]

@alanshurafa is right on both counts, and the cost he surfaces is the deciding factor.

Conceding the two points explicitly so the maintainer call is on the real question:

1. verify_chain regression. My field-reorder alone converts every pre-existing row's verify_chain to a permanent HashMismatch on untampered data — precisely the failure mode fix(audit): hash created_at at the precision Postgres stores #2638 dug this crate out of ("a genuinely forged row is indistinguishable from the permanent baseline failure, so HashMismatch carries no signal"). A reorder without a re-hash/version path silently degrades existing audit history from verifiable to permanently failing. That alone disqualifies the bare reorder as a standalone fix.

2. Unframed variable-width neighbours. The reorder closes the reported object_id/detail boundary, but the preimage still keeps other unframed variable-length neighbours (created_at → to_storage_precision → to_rfc3339 emits 0/3/6 sub-second digits; actor_pubkey/object_id separated only by a constant presence-tag byte, pk being Vec<u8>, 32 by convention). Same shape as buzz-audit: field-boundary collision in compute_hash lets a detail edit survive verify_chain #4173.

So the honest read: this reorder should not land as-is. #4216's versioned-length framing is the strictly better shape — rows verify under their stored version, new rows are length-framed, no migration, and it fixes the whole preimage family rather than one boundary.

The compose option: if the maintainers want the reorder too, it becomes the v2 layout inside #4216's framing (v1 = legacy rows verified as-is, v2 = length-framed + reordered). I'm happy to either (a) rebase my branch onto #4216 to make the reorder be its v2 encoding, or (b) close this PR in favour of #4216 and fold anything useful there. Which shape do the maintainers want? I don't want to keep both open competing if there's a preference.