Skip to content

fix(common): encode action payload as CBOR byte string (#443) - #447

Merged
sanity merged 3 commits into
mainfrom
fix/443-action-payload-bytes
Jul 22, 2026
Merged

fix(common): encode action payload as CBOR byte string (#443)#447
sanity merged 3 commits into
mainfrom
fix/443-action-payload-bytes

Conversation

@sanity

@sanity sanity commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Problem

ActionContentV1::payload was a bare Vec<u8>. serde has no distinct byte-string type in the derive path, so ciborium wrote a CBOR array of integers — every byte >= 0x18 costs 2 bytes, and all printable ASCII is >= 0x20. An edit cost ~2.1 bytes per character while a plain TextContentV1 { text: String } message cost ~1.01 (a CBOR text string).

Against the default max_message_size of 1000 that capped edits at ~467 characters while sends allowed ~991 — so a message could be sent and then never edited. Before #431 added the edit-size gate this failed completely silently: the edit form closed, the text reverted, and the over-limit action was pruned by contract validation on every peer.

Surfaced by Matrix reports (2026-07-21) from @ofansifkapital-xmpp and @Ivvvor, who both attributed it to encrypted rooms. It is not privacy-specific: the doubling happens in ActionContentV1::encode() before encryption, and private rooms differ by exactly the 16-byte AES-GCM tag. See #443 for the measurements.

Approach

Serialize payload as a CBOR byte string via a small serde(with = ...) helper.

Decode accepts both encodings. This is required, not cosmetic: rooms created before this change hold action payloads in the array form, and contract migration re-PUTs that existing state into the new contract. Without the legacy arm, every pre-existing edit and reaction would silently stop rendering (ActionContentV1::decode -> Err -> the action is skipped by rebuild_actions_state_with_decrypted).

Compatibility turned out to be bidirectional: a pre-#443 reader still decodes the new encoding, because ciborium's deserialize_seq accepts a byte string. Verified in new_byte_string_payload_decodes_with_legacy_reader. That materially de-risks the rollout — a stale riverctl or UI does not lose newly-authored edits.

deserialize_any is sound here because ActionContentV1 is only ever serialized with ciborium (encode_cbor/decode_cbor) and CBOR is self-describing. Documented at the helper so it is not copy-pasted onto a bincode-handled type.

Result, and the residual

Edit cost drops from ~2.1 to ~1.05 bytes per character (a 900-char edit: 1860 -> 947 bytes).

A residual constant ~54-byte overhead remains (CBOR framing for action_type, target, and the nested EditPayload), so the longest editable message (~946 chars) is still slightly shorter than the longest sendable one (~991). I deliberately did not silently close that: doing so would mean either shrinking the send budget or another wire-format change. It is pinned by edit_overhead_over_send_is_a_small_constant so it cannot creep back into a proportional cost, and flagged here for a product call.

Migration

Both the room-contract and chat-delegate WASM change, so both registries have a V28 entry recorded from the pre-change binaries before rebuilding:

Registry V28 entry
legacy_delegates.toml delegate key 992155c3..., code hash 82da3a0e...
common/legacy_room_contracts.toml code hash c53ded28...

scripts/check-migration.sh and scripts/check-room-contract-migration.sh both confirm the old hash is registered. migration.rs's registry value-pin is updated to 28 entries / 0b83bd66..., which that test's own doc comment instructs when a genuinely new generation is registered.

Testing

New pins in common/src/room_state/content.rs:

  • legacy_array_payload_still_decodes — the migration-critical direction (existing rooms' stored actions).
  • new_byte_string_payload_decodes_with_legacy_reader — the rollout direction (stale clients).
  • edit_action_does_not_cost_two_bytes_per_character — the encoding regression pin.

And in common/src/room_state/message.rs, at the measure_edit level the UI gate actually uses:

  • edit_cost_is_not_proportional_to_length — incl. private rooms costing exactly ENCRYPTION_TAG_OVERHEAD more.
  • edit_overhead_over_send_is_a_small_constant — pins the residual.

cargo make test green (full workspace). cargo test -p river-core --all-features green. cargo check -p river-ui --target wasm32-unknown-unknown --features no-sync clean. cargo fmt applied.

Release coupling (IN this PR)

Both are required and both are done here — an earlier draft of this description
wrongly called them follow-ups "per the convention of a separate chore PR". That
was wrong on the facts: every recent room-contract-WASM PR bumped in-PR (#416,
#411, #394), and .github/workflows/check-cli-wasm.yml makes it a hard CI gate,
not a convention. It failed on the first push with "Room contract WASM changed
but riverctl version (0.2.0) matches crates.io"
.

  1. Version bumpsriver-core 0.1.16 -> 0.1.17, riverctl 0.2.0 -> 0.2.1
    (both current versions are already on crates.io), plus riverctl's river-core
    requirement and Cargo.lock. The bump is embedded via CARGO_PKG_VERSION, so
    the WASMs were re-synced afterwards.
  2. Publish BOTH surfaces. The room-contract key changes, so the UI and
    riverctl must both be republished or riverctl users derive the old key and
    split from UI users. Post-merge order: cargo make publish-river from main
    -> commit published-contract/contract-version.txt -> tag riverctl-v0.2.1
    (the workflow publishes river-core 0.1.17 first, then riverctl).

Review

Full-tier multi-model review (wire format is a high-risk surface): an external
Codex pass plus four independent Claude lenses (wire-format/compat, adversarial
bug hunt, big-picture/release-safety, testing). Two blocking findings were found
and fixed in 869606cd:

  • Unbounded pre-allocation in the new visit_seq from an attacker-declared
    CBOR array length — a regression this PR introduced, reachable inside the room
    contract on untrusted peer data, trapping on wasm32 and panicking on x86_64.
    Reproduced with a failing test before fixing, now clamped to 4096.
  • Stale legacy_set_fingerprint pin in the UI (the delegate-side mirror of
    the migration.rs pin this PR did update) — CI was red on it.

Plus test-coverage and comment-accuracy fixes: a frozen pre-#443 byte fixture, a
full-stack pin that legacy actions still render, coverage for
delete/reaction/emoji, a magnitude assertion on the pre-existing
measure_edit_matches_edit_body (the test that should have caught #443), removal
of a tautological assertion, and a guard on
RoomMessageBody::{Public.data, Private.ciphertext} explaining why they cannot
get the same treatment (they are inside the signed MessageV1).

Notes

  • Local clippy (1.94) reports pre-existing findings on main that CI's pinned toolchain does not flag: ban.rs:41-45 (doc-lazy-continuation), dm_body.rs:358, and the "a".repeat(1) in the existing samples() helper. I left them alone to keep this wire-format diff surgical. This PR adds no new clippy findings.
  • riverctl has no message/edit size gate at all (cli/src/api.rs builds the edit action with no measure_* check), so the Edits cost ~2 bytes/char (Vec<u8> CBOR array): messages over ~467 chars can be sent but never edited #443 symptom still reproduces there, now at ~946 chars instead of ~467. Pre-existing and out of scope here — filed separately.
  • No CI job rebuilds the WASM and compares it to the committed binary, and these builds are documented as non-reproducible, so the shipped contract key is whatever the author's machine produced. Pre-existing; noted because this PR re-synced locally three times.
  • handle_edit_message still returns silently on its other failure paths (no room secret, apply_delta rejection) with only a console log while the edit form closes regardless — a separate UX gap, not addressed here.

Closes #443

[AI-assisted - Claude]

sanity and others added 3 commits July 22, 2026 15:01
## Problem

`ActionContentV1::payload` was a bare `Vec<u8>`. serde has no distinct
byte-string type in the derive path, so ciborium wrote a CBOR array of
integers: every byte >= 0x18 costs 2 bytes, and all printable ASCII is
>= 0x20. An edit therefore cost ~2.1 bytes per character while a plain
`TextContentV1 { text: String }` message cost ~1.01.

Against the default `max_message_size` of 1000 that capped edits at ~467
characters while sends allowed ~991 — a message could be sent and then
never edited. Before #431 added the edit-size gate this failed silently:
the edit form closed, the text reverted, and the over-limit action was
pruned by contract validation on every peer.

## Approach

Serialize `payload` as a CBOR byte string. Decode accepts BOTH the byte
string and the legacy array-of-integers form, which is required rather
than cosmetic: rooms created before this change hold action payloads in
the array form, and contract migration re-PUTs that state into the new
contract. Without the legacy arm every pre-existing edit and reaction
would silently stop rendering.

Compatibility is bidirectional — a pre-#443 reader still decodes the new
encoding, because ciborium's `deserialize_seq` accepts a byte string. So
a stale riverctl/UI does not lose newly-authored edits.

`deserialize_any` is sound here because `ActionContentV1` is only ever
serialized with ciborium and CBOR is self-describing; this is documented
at the helper so it is not copied to a bincode-handled type.

Edit cost goes from ~2.1 to ~1.05 bytes per character. A residual
CONSTANT overhead of ~54 bytes remains (CBOR framing for `action_type`,
`target` and the nested `EditPayload`), so the longest editable message
(~946 chars) is still slightly shorter than the longest sendable one
(~991). That is pinned and documented rather than silently closed —
closing it fully would mean shrinking the send budget or another
wire-format change.

## Migration

Room-contract AND chat-delegate WASM both change, so both registries get
a V28 entry recorded from the pre-change binaries before rebuilding:

- `legacy_delegates.toml` V28 (delegate key 992155c3..., code hash 82da3a0e...)
- `common/legacy_room_contracts.toml` V28 (code hash c53ded28...)

`scripts/check-migration.sh` and `scripts/check-room-contract-migration.sh`
both confirm the old hash is registered. `migration.rs`'s registry
value-pin is updated to 28 entries / 0b83bd66... as that test instructs
when a genuinely new generation is registered.

## Testing

- `legacy_array_payload_still_decodes` — the migration-critical direction.
- `new_byte_string_payload_decodes_with_legacy_reader` — the rollout direction.
- `edit_action_does_not_cost_two_bytes_per_character` — the encoding pin.
- `edit_cost_is_not_proportional_to_length` — pins parity at the
  `measure_edit` level the UI gate actually uses, incl. private rooms
  costing exactly the AES-GCM tag more.
- `edit_overhead_over_send_is_a_small_constant` — pins the residual so it
  cannot grow back into a proportional cost.

`cargo make test` green; `cargo test -p river-core --all-features` green
(240 lib tests); `cargo check -p river-ui --target wasm32-unknown-unknown`
clean; `cargo fmt` applied. No new clippy findings.

Closes #443

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019i3aem4MidW6bjv31uibEk
…ASM change

The room-contract and chat-delegate WASM change in this PR, so riverctl
must be republished or it derives the old contract key and splits from
UI users. CI's check-wasm-sync enforces exactly this: it fails while
cli/Cargo.toml's version still matches the version on crates.io.

Both current versions are already published (river-core 0.1.16,
riverctl 0.2.0), so both are bumped, along with riverctl's river-core
requirement. WASMs re-synced afterwards because the bump is embedded in
the binaries via CARGO_PKG_VERSION.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019i3aem4MidW6bjv31uibEk
…ings

## Blocking

**Unbounded pre-allocation from an attacker-declared CBOR length.**
`visit_seq` pre-allocated from `seq.size_hint()`, which ciborium returns
verbatim from the array header without checking it against the remaining
input. serde's derived `Vec<u8>` visitor bounds this via
`size_hint::cautious`; the hand-rolled visitor dropped that bound, so
this was a REGRESSION introduced by this PR, not a pre-existing issue.

A ~45-byte signed action message whose payload is just the header
`0x9B FF..FF` reaches `ActionContentV1::decode` through
`MessagesV1::apply_delta` -> `rebuild_actions_state` — inside the room
contract, on untrusted peer data. On x86_64 that is a capacity-overflow
panic; on wasm32 (the shipping target for both the contract and the UI)
`with_capacity(4G)` traps on `memory.grow`. Note `validate_state` never
decodes content, so a poisoned state passes validation and then traps
every peer that applies a delta to it.

Now clamped to `MAX_PREALLOC = 4096`; the Vec still grows as needed.
Pinned by `legacy_payload_with_lying_length_header_errors_not_panics`,
which carries three vectors because they discriminate on DIFFERENT
targets: `0x9B FF..FF` reproduces on x86_64 (where CI runs) but is
rejected by `usize::try_from` on wasm32; `0x9A FF FF FF FF` is the
vector that matters on wasm32; the 1,000,000 case pins clean truncation
handling. Verified the test fails without the clamp before applying it.

**Stale legacy-delegate fingerprint pin.** Adding V28 to
`legacy_delegates.toml` changes `legacy_set_fingerprint()`, which keys
the per-user migration-done flag. The room-contract analogue in
`migration.rs` was updated but this one was missed, and CI's
`cargo test -p river-ui --bins` was red on it. Updated to
`b2824be852437587` (value confirmed by running the test, and
independently derived by two reviewers), plus the stale "24 entries
spanning V1..V27" doc.

## Test coverage gaps

- `frozen_pre_443_bytes_still_decode` — a FROZEN hex fixture of real
  pre-#443 bytes. The mirror-struct tests rebuild "legacy" bytes from
  today's types, so they would move with a future `EditPayload` or
  ciborium change and keep passing against bytes that no longer resemble
  what is stored. This literal cannot drift.
- `legacy_decode_covers_every_action_kind` — edit/delete/reaction/
  remove_reaction incl. emoji and multi-byte text. Only `edit` was
  covered, and delete (empty payload) and emoji (two-byte CBOR ints) are
  distinct paths through `visit_seq`.
- `legacy_payload_with_out_of_range_element_is_rejected`.
- `legacy_encoded_actions_still_render_through_rebuild_actions_state` —
  full-stack pin that pre-existing actions still SURFACE, not merely
  decode. The doc comment claimed this behaviour but nothing tested it.
- `legacy_array_payload_still_decodes` now asserts the fixture really is
  a CBOR array, so it cannot silently degenerate into a duplicate of
  `test_edit_action_roundtrip`.
- `measure_edit_matches_edit_body` now asserts MAGNITUDE, not just
  consistency — it is the test that should have caught #443 and did not,
  and it covers the multi-byte/boundary samples the ASCII-only pins miss.

## Correctness of comments

- The `u16` rationale was wrong: `next_element::<u8>()` range-checks and
  errors, it does not truncate. Corrected so nobody hardens against a
  non-existent bug.
- Noted that `deserialize_any` does not skip CBOR tags (marginally
  stricter than the derived path it replaces).
- Removed a tautological `private - public == ENCRYPTION_TAG_OVERHEAD`
  assertion that holds by construction and inflated apparent coverage.
- Added a guard on `RoomMessageBody::{Public.data, Private.ciphertext}`:
  they are also bare `Vec<u8>` and look like the same easy win, but they
  sit INSIDE the signed `MessageV1`, so changing their encoding would
  invalidate every existing message signature in every room. The #443 fix
  was safe precisely because it stopped at that boundary.

Static error string instead of `format!` to keep `core::fmt` out of the
contract WASM.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019i3aem4MidW6bjv31uibEk
@sanity
sanity marked this pull request as ready for review July 22, 2026 20:32
@sanity
sanity merged commit b6c9cfb into main Jul 22, 2026
7 checks passed
sanity added a commit that referenced this pull request Jul 22, 2026
…ayload publish

Publishes the room-contract WASM generation from #447 (V28) together with
the expanded nickname pools from #448.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019i3aem4MidW6bjv31uibEk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Edits cost ~2 bytes/char (Vec<u8> CBOR array): messages over ~467 chars can be sent but never edited

1 participant