[gerau]

No description provided.

[KyrylR]

Note

🤖 Claude Opus 5.5 responding on behalf of KyrylR
Posted with KyrylR's explicit authorization. This review comes from a model council, run in two rounds. Round 1 was independent reviews, each with a focus area. In round 2 every member checked the merged findings, and all nine voted the review complete.

• Core advisor: Claude Mythos 5.2 (claude-mythos-5-2, high effort)
• Council:
  • Claude Opus 5.5 (claude-opus-5-5)
  • Claude Mythos 5.2 (claude-mythos-5-2)
  • 3× GPT-6 Astra (gpt-6-astra)
  • 3× GPT-6.1 Sol (gpt-6.1-sol)
• Compiled and verified against the code by: Claude Opus 5.5

Verdict: request changes.

The core cryptography matches the spec:

• AAD layout and role binding (§11)
• receiver checks run in order, and the counterpart is pinned only after authentication (§10)
• replay classification
• the all-zero X25519 check, and separate HKDF outputs for the key and the topic
• retransmissions resend the identical frame

The problems are in how traffic is confined to the session key, in resource bounds, in secret hygiene, in reliability behaviour, and in an incomplete spec edit.

Validation:

• cargo build --workspace --all-targets passes.
• cargo test --workspace passes. The client has 28 unit tests, e2e 1/1 and plaintext 5/5. All 6 live_relay tests are #[ignore]d.
• Default clippy is clean for the client.

---

Blocking

B1 · High · The pairing key keeps its authority after the session is established

The client sends and accepts application messages on the pairing topic, under the pairing key, both before and after the session exists.

• The only kind/topic check runs in the reverse direction: it rejects handshake kinds on the session topic (client.rs:446-456).
• observe() ends in _ => {} (client.rs:528). So request, response, disconnect and session_delete on the pairing topic are acked and delivered (client.rs:474-483).
• send() routes non-handshake kinds to active_topic(), which is the pairing topic until approval (client.rs:262-270).
• wait_session_ready() sets SessionState::Active without checking the handshake (client.rs:252).
• The pairing link is never retired.

Because from is self-declared, anyone who holds the pairing key can inject messages as the counterpart and bypass the X25519 session key. That includes a leaked QR code or deep link, and a relay that has the key. A relay without the key cannot exploit this. The problem is that pairing authority is never retired; initial pairing authentication is not bypassed.

The tests rely on this behaviour (tests/plaintext.rs:255, :515-522, :617). The spec says otherwise: §9 (new paragraph) "application traffic uses the session topic", and §3.1 says pairing URIs are short-lived bearer secrets.

Fix:

• On the pairing topic, accept only proposal, approve, reject and ack.
• Refuse application sends until the handshake reaches Established.
• Report Active only after establishment.
• Retire the pairing link once approve is acked or its retry window ends.

B2 · High · Inbound frames are queued without any memory bound

• The inbox is an unbounded channel (client.rs:131). The transport task fills it for every parsed frame (relay.rs:180-183), before any authentication and whether or not the app is polling.
• connect_async uses tungstenite's default limits (relay.rs:146): 64 MiB messages and 16 MiB frames. The relay caps frames at 256 KiB.
• Two attackers can use this:
  • a malicious relay;
  • an untrusted counterpart behind an honest relay. The relay allows 512 KiB per 2 s per connection (relay/src/limits.rs:131-134).
• Some growth needs authenticated, distinct envelopes, so a keyless relay cannot drive it:
  • the replay memory keeps full envelopes with no cap (replay.rs:22-25,94-104);
  • the pending queue used by wait_session_ready (client.rs:247).
• The outbound and event channels are also unbounded.

Fix:

• Use connect_async_with_config with limits close to the relay's.
• Use a bounded inbox so backpressure reaches the socket.
• Cap the replay memory, or store digests instead of full envelopes.

B3 · Medium (blocking for consistency with #7) · Bearer secrets reach logs and error strings

• log::debug!("connecting to relay {url}") prints the full URL, including auth=<JWT>&topic= (relay.rs:145). live_relay.rs:70-72 acknowledges this.
• Raw topics are logged in relay.rs:171,182 and client.rs:195,342. Core already provides Topic::log_tag().
• ClientError::PairingUri(uri.to_owned()) puts the pairing key into the error text (pairing.rs:80,94-95), and pairing.rs:47 includes the encoded key.

Spec §3.1: a logged token "displaces the legitimate peer".

Fix: log only the host and log_tag(), and use fixed error messages.

B4 · Medium · Garbage the relay can inject comes back as an Err from recv() / wait_session_ready()

• An envelope that fails to parse (client.rs:346) or fails AEAD (client.rs:399-403) is returned with ?. The neighbouring §10 step 1–3 rejections drop silently with Ok(None).
• The client stays usable. But the API returns these recoverable errors as the same plain Err it uses for fatal ones like Closed, and the usual recv().await? loop treats every one as fatal. So one injected frame can end the app's session loop.
• client.rs:435 (ClientMessage::from_bytes) is reached only after authentication. A failure there leaves the envelope unrecorded and unacked, so each retransmission from the peer errors again. Combined with B9, every message from a peer that follows the spec literally hits this path.

Fix: log and drop, or return a distinct non-fatal error variant.

B5 · Medium · The reconnect flush breaks §13

(a) No pacing. On reconnect every pending entry gets next = now and is sent on one 50 ms tick (reliability.rs:106-115,189-208). §13 says the sender "must spread them within the relay's inbound budget". Effects:

• Anything above the relay's 32 messages per 2 s is dropped but still charged an attempt.
• A backlog of roughly 65 or more also crosses the relay's limit of 32 consecutive drops and triggers a disconnect (relay connection.rs:47,715-721). The loop is self-limiting: after at most 5 reconnects every entry is reported Failed.

(b) Flush runs before session_ready. The flush is triggered on socket Connected, not on session_ready (client.rs:188,202-207). The relay drops frames while the counterpart is absent, so those attempts are wasted. An entry already at its last attempt is marked Failed on reconnect without being resent.

(c) Duplicate sends. Frames queued in the persistent outbound channel while offline are written on reconnect and also re-flushed.

(d) Stale connection flag. connected updates only inside recv(), so the retry task keeps queueing copies while the socket is down. This is bounded by ack_max_attempts per message.

(e) Ordering. Messages are retransmitted in HashMap order. Low.

Fix: trigger the flush and gate retries on the topic's session_ready, pace the sends, and drain stale queued copies on reconnect.

B6 · Medium · Acks are processed only while the app is inside recv()

• Acks, re-acks and Connected/Disconnected events are handled only inside recv() and wait_session_ready() (client.rs:177-200,246,315,442).
• Retransmission runs on its own timer (reliability.rs:173-212).
• poll_event() is a synchronous try_recv (client.rs:499-506). It, recv() and send() all take &mut self.

Consequences:

• An app that polls for Delivered without driving recv() gets a false Failed about 126 s after the first send. Resends go at 2/6/14/30/62 s.
• A busy receiver, for example a wallet showing a signing prompt, can still accept the message until ts + 300. It is then delivered after the sender was told Failed, and an app that re-sends on Failed creates a duplicate.
• connected starts false (reliability.rs:81), so a send made before the first poll gets neither a retransmission nor a Failed.
• The only workable pattern is a select! or timeout around recv(). That pattern is cancel-safe only by accident, and this is not documented.

Fix: run a background driver for acks and lifecycle events, or document the contract and provide an awaitable event API.

B7 · Medium · The spec edit is incomplete, which blocks a second implementation

• §9 does not specify the session-key or session-topic derivation. The code uses HKDF-SHA256 with salt = None and the info labels wallet-session/v1/session-key and wallet-session/v1/session-topic (handshake.rs:9-13,79-88). A wallet implementation cannot derive the same key or topic from the spec alone.
• §7 (spec.md:135) still says topics are SHA-256 of the pairing key.
• The spec does not say when the pairing connection may close. Meanwhile each session holds two relay connections, so the per-identity cap of 16 limits a wallet to 8 sessions.
• The plaintext encoding must be defined. See B9: the client requires a type tag that §12 does not list.

Fix:

• Specify the HKDF salt, info labels and output order, with a test vector.
• Fix §7.
• Define the pairing-connection lifetime.
• Optional hardening: bind the ephemeral public keys, the pairing topic and the DIDs into the KDF. This is not a spec violation.

---

Should fix

B8 · Low–Medium · Sends before session_ready

propose() can be called right after connect(), and application sends can go out right after approve(), before the topic's session_ready (client.rs:278-303, relay.rs:166-174). This breaks §3 and the ordering in §9 (spec.md:167-168). The relay drops these frames while the counterpart is absent (relay connection.rs:746-752), and they are recovered only by retry. Same root cause as B5(b).

B9 · Medium · The plaintext type discriminator

• Interoperability. The client requires a serde type tag (messages.rs:20) that §12 does not define. A peer that sends §12 shapes such as {"ackId": …} cannot be parsed. This is inherited from core's Payload.
• Unchecked mismatch. The tag is never compared with the authenticated envelope kind. New messages are acked according to the inner type (client.rs:474), but retransmissions according to wire.kind (client.rs:421). Both values are authenticated, so this is inconsistent handling between key holders, not something a relay can forge.

Fix: reject the envelope when message.message_type() != wire.kind, and either make the tag normative in the spec or decode by kind.

B10 · Medium · An unexpected or failing approve destroys handshake state

std::mem::replace(&mut self.handshake, Handshake::Idle) runs before the variant check and before key derivation (client.rs:519-526). Effects:

• A responder in PeerProposed loses the peer key permanently.
• An established proposer drops back to Idle while session_link is still Some, so propose() works again.
• A legitimate approve that carries a low-order public key consumes the proposer's ephemeral keypair.
• When observe errors, the envelope is neither recorded nor acked (client.rs:465-472), so each of the peer's retransmissions surfaces the same error.

Fix: validate the state and derive the key first, then commit the transition.

B11 · Medium · Terminal and readiness state transitions

Inbound messages.

• An inbound reject, disconnect or session_delete does not change state, because observe handles only Proposal and Approve, and only on the pairing topic.
• ProposalReceived is never set.

PeerDisconnected.

• It sets the state to Disconnected but leaves link.ready = true (client.rs:236-241,320-325). The next wait_session_ready() returns immediately and restores Active with no peer present.
• A repeated session_ready after the peer returns does not restore the state (Low).

Local disconnect and closure.

• disconnect() does not close the links or block further sends.
• After Inbox::Closed, the error is returned once. Later recv() calls hang, because Client holds its own inbox_tx (client.rs:106,139,190), which also makes client.rs:197 unreachable.
• Pending entries on a closed link never emit Failed. dispatch inserts into pending before the outbox.send that fails (reliability.rs:130-138), so the entry leaks.

B12 · Medium · Transport liveness

• sink.send(..).await runs inside a select! branch with no deadline (relay.rs:172,189,203). A relay that stops reading blocks the pong deadline and shutdown.
• connect_async has no timeout (relay.rs:146).
• Normal shutdown works when nothing is blocked: Reliability::drop aborts its retry task, and the transport exits when the outbound channel closes (relay.rs:176). But a connect or write that is already pending never notices the client is gone.
• No WebSocket Close frame is sent on shutdown (relay.rs:176), so every drop looks like an abrupt loss and puts the slot into grace (Low).

Spec: §14.

B13 · Low–Medium · Replay memory is per Client instance

A new Client on the same pairing key starts with an empty replay memory and no counterpart (client.rs:95, session.rs:53, replay.rs:42-47). It therefore accepts replays of pairing-topic envelopes from the last 300 s and can pin an old counterpart. tests/plaintext.rs:250-271 presents exactly this as the resume pattern.

• B1 makes this worse: a replayed request is delivered again.
• Fixing B1 reduces the exposure to handshake kinds.
• An honest path exists too: if one instance dies after delivering but before acking, the peer's retransmission is delivered again by the next instance.

Fix: document one Client per pairing, or persist the replay state.

B14 · Medium · Cipher state and the §11 nonce rules

• The public XChaCha20Poly1305Cipher, re-exported at lib.rs:19, derives Clone over its key, prefix and counter (crypto.rs:137-141). Calling seal() once on each clone repeats a nonce under the same key.
• The session cipher is built by self.pairing.cipher.clone() followed by rekey (client.rs:541-542). rekey keeps the prefix and counter (crypto.rs:213-215), which breaks §11's "fresh random sender prefix per key" (spec.md:231) and "counter starts at 0" (spec.md:235) on every session.
• With the built-in cipher there is no reuse today, because the key changes. But Client<C> accepts any C: MessageCipher + Clone (client.rs:115-117), and the core rekey cannot fail. A user cipher whose rekey does nothing, like the test PlaintextCipher (plaintext.rs:89), would reuse nonces between the pairing and session links under one key.

Fix:

• Build the session cipher fresh, with a new random prefix and a counter starting at 0.
• Drop Clone, or give each clone a new prefix.
• Restrict with_prefix to tests.
• Use checked_add instead of wrapping_add (crypto.rs:187).

B15 · Medium · Reconnect loop

• Too few reconnects. A dropped connection gets only 4 reconnect attempts, not reconnect_max_attempts (5): attempt is incremented before the first retry (relay.rs:96,111-115, config.rs:38). The total outage tolerated is about 3.75 s of backoff.
• Endless reconnect. A relay that upgrades and then closes resets attempt every time. The concrete trigger is a 1008 "role already taken", for example a second wallet scanning the same URI or a squatted slot (relay connection.rs:240-247). The client then reconnects every 250 ms with a fresh JWT, never reports Closed, and spends pending attempts through B5(b).
• Token expiry. The close code is ignored (relay.rs:192), so a token-expiry close takes the normal backoff. §14 says to reconnect "promptly … without applying the liveness backoff".
• Hidden errors. Connect errors such as a bad URL, a TLS failure or a 400 appear only in debug logs, and the app sees Closed.

B16 · Low · No ts + 300 give-up

retry_loop never checks the envelope's ts (§13). It also pauses while disconnected, so expired envelopes are resent after a long outage. For reference, Failed fires about 126 s after the first send. That is consistent with §13 but worth documenting for app authors.

B17 · Medium · response wire shape

respond() accepts both or neither of response and error (client.rs:630-640). ResponsePayload has no skip_serializing_if (core/src/payload.rs:55-65), so even a normal success emits "error": null. This breaks §12's alternatives (spec.md:277,282). Validate inbound responses too.

B18 · Low · Secret hygiene

• No zeroization for:
  • the SessionKey, PairingKey, Seed and IdentityKey wrappers;
  • the shared secret and HKDF output buffers (handshake.rs:74,81,85);
  • the copy made in rekey (crypto.rs:214).
• EphemeralSecret itself is zeroized.
• The core payload Debug derives print the full Opaque application data if a caller logs them. No log line in the client does this, so the exposure is plausible rather than confirmed.

B19 · Low (Low–Medium for ws://) · Smaller gaps

• Plaintext WebSocket. ws:// relay URLs are accepted with no opt-in, which would expose the JWT to the network (relay.rs:142-146). TLS is terminated in front of the relay (§3), so require wss:// unless the caller opts in.
• Outer frame version. The outer relay-frame v is ignored (client.rs:334-336). The envelope version is checked.
• Unused config and errors. sessionExpiry is not enforced. idle_timeout, ClientError::Timeout and ClientError::SessionNotActive are unused.
• Role not enforced. The handshake methods do not enforce the §6 roles.
• Zero ping interval. with_ping_interval(Duration::ZERO) panics the transport task inside tokio::time::interval_at (config.rs:100, relay.rs:159). No Closed is emitted, so recv() hangs. Validate the config.

B20 · Low · Packaging, if the crate is meant to be published

The core dependency is path-only (crates/client/Cargo.toml:7), and the license field is missing.

---

Tests

T1 · Adversarial tests use a cipher that ignores keys and AAD. The adversarial integration tests use PlaintextCipher, which ignores both (tests/plaintext.rs:77). Own and foreign identity rejection is covered at the Client level, but only with that cipher. There are no Client-level negative tests with the real cipher for:

• a tampered tag;
• a reflected envelope;
• a kind mismatch;
• a conflict;
• an approve after establishment;
• application kinds on the pairing topic.

T2 · Nothing tests rekey isolation (Medium). If rekey were a no-op, both sides would stay on the pairing key and e2e.rs would still pass. Add:

• an "a session envelope does not open under the pairing key" test;
• a check that the session prefix differs and the counter is 0;
• HKDF key and topic known-answer vectors.

T3 · Timing and other gaps.

• plaintext.rs:252 sleeps a fixed 300 ms inside a 2 s grace window, which may be flaky.
• retries_then_fails only asserts resends >= 2, so the schedule is not pinned.
• There is no flush-pacing test.
• There is no explicit low-order-point test.
• All live_relay tests are ignored, and no CI job for them is evident.

The AAD layout is already checked against independently assembled fields (crypto.rs:245-273).

None of the current tests would catch B1–B4 or B6. B5 and B7 are only partly covered.

live_relay.rs and plaintext.rs are not duplicates; the equal line count is a coincidence.

---

Questions for the author

1. Is pre-handshake application traffic an intended mode? If so, the spec has to say so, and B1 becomes a spec decision.
2. Should the pairing connection close once approve is acked?
3. Is session resumption across app restarts in scope? This relates to B13.
4. Should the plaintext type tag become normative in the spec?
5. Does "maximum of 5 attempts" mean 5 retries or 5 sends in total?
6. Should wss:// be required outside tests? Is TLS 1.2 needed?

[KyrylR]

Note

🤖 Claude Opus 5.5 responding on behalf of KyrylR

B1 · High, blocking. This check only runs one way: it rejects handshake kinds on the session topic, but nothing rejects application kinds on the pairing topic.

• observe() falls through to _ => {} (L528), so request, response, disconnect and session_delete on the pairing topic are acked and delivered.
• This holds before and after establishment.
• send() routes non-handshake kinds to the pairing topic until approval (L262-270).
• wait_session_ready() sets Active with no handshake (L252).

Because from is self-declared, any pairing-key holder can inject messages as the counterpart after the session exists, which bypasses the X25519 session key. On the pairing topic, accept only proposal, approve, reject and ack, and retire the pairing link after approve is acked.

[KyrylR]

Note

🤖 Claude Opus 5.5 responding on behalf of KyrylR

B2 · High, blocking. The inbox is unbounded, and relay.rs:183 pushes every parsed frame into it before any authentication, whether or not the app is polling. A malicious relay, or an untrusted peer behind an honest relay, can grow memory without limit. connect_async also uses tungstenite defaults (64 MiB messages). Bound the channel and pass a WebSocketConfig with limits close to the relay's 256 KiB.

[KyrylR]

Note

🤖 Claude Opus 5.5 responding on behalf of KyrylR

B3 · blocking. This logs the full URL, including auth=<JWT>&topic=. A logged token displaces the legitimate peer (§3.1). Log only the host and Topic::log_tag(), and apply the same to the topic logs at L171/L182 and client.rs:195,342.

[KyrylR]

Note

🤖 Claude Opus 5.5 responding on behalf of KyrylR

B3. ClientError::PairingUri(uri.to_owned()) puts the whole URI into the error text, including a valid pairing key when only relay is missing (same at L80; L47 embeds the encoded key). Use fixed error messages.

[KyrylR]

Note

🤖 Claude Opus 5.5 responding on behalf of KyrylR

B4 · Medium, blocking. An AEAD failure here, and a JSON failure at L346, is returned with ? and so surfaces as an Err from recv() / wait_session_ready(). The other §10 rejections drop with Ok(None). With the usual recv().await? loop, one frame injected by the relay ends the session. Log and drop, or use a non-fatal error variant. Also, L435 runs after authentication: a failure there leaves the envelope unacked, so every retransmission errors again.

[KyrylR]

Note

🤖 Claude Opus 5.5 responding on behalf of KyrylR

B11 · Medium. PeerDisconnected sets Disconnected but leaves link.ready = true (also L320-325). The next wait_session_ready() returns immediately and restores Active with no peer present. Inbound reject / disconnect / session_delete also never change state, and after Inbox::Closed later recv() calls hang, because Client holds its own inbox_tx.

[KyrylR]

Note

🤖 Claude Opus 5.5 responding on behalf of KyrylR

B14 · Medium. Clone copies the key, prefix and counter, so sealing once on each clone repeats a nonce under the same key. The session cipher is built as clone() + rekey (client.rs:541-542), keeping the pairing prefix and counter. That breaks §11 (fresh prefix per key, counter starts at 0). With this cipher there is no reuse today, but Client<C> accepts any C whose rekey cannot fail; a no-op rekey (like the test PlaintextCipher) would reuse nonces across the links. Build the session cipher fresh.

[KyrylR]

Note

🤖 Claude Opus 5.5 responding on behalf of KyrylR

B15 · Medium.

• Resetting attempt on every successful upgrade means a relay that accepts and then closes with 1008 "role already taken" causes a reconnect every 250 ms forever.
• Conversely, a dropped connection gets only 4 retries, not reconnect_max_attempts.
• Close codes are ignored (L192), so token expiry takes the liveness backoff, contrary to §14.

[KyrylR]

Note

🤖 Claude Opus 5.5 responding on behalf of KyrylR

B12 · Medium. sink.send(..).await inside a select! arm with no deadline (also the ping and pong sends): a relay that stops reading blocks the pong deadline and shutdown. connect_async has no timeout either (§14).

[KyrylR]

Note

🤖 Claude Opus 5.5 responding on behalf of KyrylR

B17 · Medium. This accepts both or neither of response / error, and ResponsePayload lacks skip_serializing_if, so even a success emits "error": null. That does not match the §12 alternatives. Validate inbound responses too.