feat: add Signal protocol feature API and consolidate internal helpers - #474
Conversation
Expose low-level Signal protocol operations (encrypt, decrypt, session management, participant node creation) via `client.signal().*` following the established feature pattern. Add `Event::RawNode` for raw stanza observation gated by an atomic flag. Consolidate duplicated patterns into shared helpers: - `Client::signal_adapter()` / `signal_adapter_from()` — replaces 12 inline `SignalProtocolStoreAdapter::new()` calls - `Client::session_lock_for()` — replaces 6 inline session lock patterns - `Client::get_noise_socket()` — replaces 3 inline noise socket patterns - `SignalProtocolStoreAdapter::as_signal_stores()` — replaces 6 inline `SignalStores` struct constructions - `Client::send_raw_bytes()` — `send_node` now delegates to it
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Signal feature and Client-level Signal handle, centralizes Signal store and per-session lock access via new Client helpers, introduces raw-node forwarding and a public raw-bytes send API, exposes encryption result types in wacore, and refactors send/decrypt/retry flows to use the new helpers. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(150,150,255,0.5)
participant C as Client
participant S as Signal
participant P as Persistence
participant W as wacore::send
participant N as NoiseSocket
end
C->>S: signal().encrypt_message(jid, plaintext)
S->>P: load devices & acquire per-session locks
S->>W: as_signal_stores -> encrypt_for_devices
W-->>S: participant nodes + ciphertexts
S->>P: flush signal cache
S-->>C: participant nodes (+ prekey flag)
C->>C: marshal Node -> plaintext bytes
C->>N: send_raw_bytes(plaintext) -> get_noise_socket() -> encrypt + transmit
N-->>C: ack / update last_data_sent_ms
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f84b8c8bdb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Delete Signal sessions for the given JIDs (cache + persistent store). | ||
| pub async fn delete_sessions(&self, jids: &[Jid]) -> Result<()> { | ||
| let device_store = self.client.persistence_manager.get_device_arc().await; | ||
| let device_guard = device_store.read().await; |
There was a problem hiding this comment.
delete_sessions holds the device store read lock before awaiting the per-address session mutex, but other Signal paths take the session mutex first and can then require a device write lock (e.g. identity updates during decrypt). That opposite lock order can deadlock when these paths overlap, stalling message processing and session cleanup. Acquire the session lock first (or narrow device-lock scope per JID) so lock ordering matches the rest of the Signal flows.
Useful? React with 👍 / 👎.
| let result = wacore::send::encrypt_for_devices( | ||
| &mut stores, | ||
| self.client, | ||
| &device_jids, | ||
| &plaintext, |
There was a problem hiding this comment.
create_participant_nodes calls encrypt_for_devices without taking session_locks, unlike the normal send path that locks all involved session keys before encrypting. Because this routine mutates Signal sessions, concurrent decrypt/send operations on the same devices can interleave ratchet state updates and produce duplicate/failed decrypt behavior. Wrap this encryption block with the same per-session locking strategy used in send_message_impl.
Useful? React with 👍 / 👎.
Bencher Report
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 59s —— View job Code Review: Signal Protocol Feature API
Overall this is a clean DRY consolidation. The helper extraction ( Here are the issues I found: 1.
|
| Priority | Issue | Location |
|---|---|---|
| High | Lock ordering inversion in delete_sessions (deadlock risk) |
signal.rs:183-203 |
| High | Missing session locks in create_participant_nodes (ratchet corruption risk) |
signal.rs:211-236 |
| Low | Missing concurrency docs on encrypt_group_message |
signal.rs:109-143 |
| Low | Consider doc comment on newly-public encrypt_for_devices |
wacore/src/send.rs:328 |
The Codex review flagged issues #1 and #2 correctly — I've verified both are real concerns after tracing the lock ordering through the codebase.
The benchmark alert (+8.79% on bench_decrypt_with_previous_session) appears unrelated — this PR doesn't change any Signal protocol internals, and the benchmark measures raw libsignal operations. Likely CI noise.
- delete_sessions: move device read lock inside loop body, after session lock acquisition, to match encrypt/decrypt lock ordering and prevent AB/BA deadlock - create_participant_nodes: acquire per-device session locks before calling encrypt_for_devices (matches DM send path) - encrypt_group_message: document concurrent usage restriction - encrypt_for_devices: document session lock requirement for callers
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/sessions.rs (1)
201-223:⚠️ Potential issue | 🟠 MajorCanonicalize the LID JID before locking and storing the session.
This block normalizes only the prekey-bundle lookup key.
process_prekey_bundle()andsession_lock_for()still usejid.to_protocol_address()from the raw JID, so a non-canonical LID input can create/lock a different session address than the rest of the send path, which already forces LID agent0before session creation. That letsensure_e2e_sessions()report success while later encrypt/decrypt still misses the session.🔧 Proposed fix
- for jid in jids { - if let Some(bundle) = prekey_bundles.get(&jid.normalize_for_prekey_bundle()) { - let signal_addr = jid.to_protocol_address(); + for jid in jids { + let canonical_jid = jid.normalize_for_prekey_bundle(); + if let Some(bundle) = prekey_bundles.get(&canonical_jid) { + let signal_addr = canonical_jid.to_protocol_address();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/sessions.rs` around lines 201 - 223, The prekey bundle lookup uses jid.normalize_for_prekey_bundle() but session_lock_for(), to_protocol_address(), and process_prekey_bundle() still use the original jid, allowing non-canonical LIDs to create different session addresses; fix by canonicalizing the JID once (e.g., let canonical = jid.normalize_for_prekey_bundle()) and then use canonical.to_protocol_address() for signal_addr, use canonical for the prekey_bundles lookup, pass the canonical signal_addr into session_lock_for() and process_prekey_bundle(), and ensure any session storage/locking consistently uses that canonical JID rather than the raw jid.wacore/src/send.rs (1)
320-335: 🛠️ Refactor suggestion | 🟠 MajorExpose a locked wrapper, not an unlocked session-mutating primitive.
encrypt_for_devicesnow becomes public, but its correctness still depends on callers serializing per-sender Signal operations. That makes the new low-level API easy to misuse from parallel tasks. Please either expose only locked wrappers fromclient.signal()or add an explicit doc contract that this function must run under the caller’s per-sender session lock.Based on learnings: Use
session_locksto serialize per-sender Signal encrypt/decrypt operations andmessage_enqueue_locksto serialize per-chat incoming message processing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/send.rs` around lines 320 - 335, encrypt_for_devices is a public low-level function that mutates Signal sessions and must be run under the per-sender session lock; to fix, do one of two: (A) hide this primitive (make encrypt_for_devices non-public) and add a locked wrapper method on the Signal client (e.g., client.signal().encrypt_for_devices_locked(...)) that acquires the appropriate session_locks for the sender (and message_enqueue_locks for chat-level serialization if applicable) before calling encrypt_for_devices, or (B) keep it public but add a mandatory documentation contract and runtime assert that the caller holds the per-sender session lock (using session_locks) and serialize caller usage via message_enqueue_locks where needed; update visibility and add the wrapper on the type that exposes SignalStores (client.signal()) and reference the symbols encrypt_for_devices, SignalStores, session_locks, message_enqueue_locks, and client.signal() so callers use the safe locked API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client.rs`:
- Around line 1588-1594: The dispatch of Event::RawNode is currently after
early-return paths so some decoded stanzas (e.g., IQ responses and xmlstreamend)
never get forwarded; move the block that checks raw_node_forwarding and calls
self.core.event_bus.dispatch(&Event::RawNode(Arc::clone(&node))) so it executes
immediately after decoding the stanza and before any early returns/handling code
paths (keep the atomic load raw_node_forwarding.load(Ordering::Relaxed) and the
Arc::clone(&node) usage intact) to ensure every decoded node is emitted to raw
observers prior to router dispatch or short-circuit handlers.
In `@src/features/signal.rs`:
- Around line 45-51: The public Signal-mutating paths (e.g., the message_encrypt
call in signal.rs as well as the sender-key and encrypt_for_devices code paths)
advance session/sender-key state but do not persist it; after each successful
mutating operation you must call the crate-private flush function that persists
the deferred-write signal cache (flush_signal_cache or the equivalent used in
client.rs) using the same adapter/signal_cache instance so ratchet updates are
durable; add the flush call immediately after the awaited success returns (only
on success) for message_encrypt, the sender-key encryption path, and
encrypt_for_devices to ensure session_store/identity_store advances are written
before returning.
- Around line 97-98: The low-level decryptors in signal.rs currently hard-code
WhatsApp v2 unpadding by calling MessageUtils::unpad_message_ref(&padded, 2)
(also at the similar site around lines 166-167), which breaks raw round-trips
and ignores the stanza `v`; change the decryptor APIs to stop assuming a padding
scheme: remove the fixed "2" and either (a) accept an explicit unpad parameter
(e.g., pass-through a v/pad value from the caller) or (b) return the raw padded
bytes from the low-level decrypt functions (leave unpadding to higher-level
code). If you still want WhatsApp-specific convenience, add a separate helper
(e.g., unpad_whatsapp_v2 or decrypt_with_whatsapp_v) that reads the stanza `v`
and calls MessageUtils::unpad_message_ref with the correct value. Ensure
references to MessageUtils::unpad_message_ref are updated accordingly and that
encrypt_message round-trips are preserved.
- Around line 114-121: The code obtains own_jid by directly reading
persistence_manager.get_device_snapshot().pn which always yields a phone-number
JID; for groups using LID-addressing this is wrong—use the client's helper that
respects the group's addressing mode instead. Replace the direct snapshot access
when computing own_jid with a call to Client::get_own_jid_for_group (i.e.,
self.client.get_own_jid_for_group(...)) passing the group identifier/context
used here so the correct JID (PN or LID) is returned; update any error handling
to propagate the same anyhow!("not logged in") behavior if that helper returns
None/Err. Ensure you reference own_jid and get_own_jid_for_group in the change
so the sender-key/ciphertext is generated under the proper sender identity.
In `@src/send.rs`:
- Around line 1326-1328: The dedup lock uses sender.to_non_ad() which differs
between PN and LID aliases and thus can allow duplicate tasks to race on the
same tc-token row; replace the computation of bare and the lock acquisition to
use the exact canonical key used for tc-token storage (i.e., call the same
helper you use when reading/writing tc-token rows) instead of
sender.to_non_ad().to_string(), then pass that canonical key into
self.session_lock_for(...) so session_lock_for and tc-token operations use the
identical identifier.
In `@wacore/src/types/events.rs`:
- Around line 453-458: You added a new Event::RawNode(Arc<Node>) variant which
is a breaking change for downstream exhaustive matches; either revert adding the
variant and instead expose raw-node delivery via a separate handler/callback API
(e.g., add Client::set_raw_node_handler or a RawNodeHandler trait and route raw
nodes there, leaving Event unchanged) and wire that to
Client::set_raw_node_forwarding(true), or if you truly intend a breaking
release, document and perform a semver-major bump and release note for the Event
enum change; do not add the RawNode variant to the public Event enum without one
of these two approaches.
---
Outside diff comments:
In `@src/client/sessions.rs`:
- Around line 201-223: The prekey bundle lookup uses
jid.normalize_for_prekey_bundle() but session_lock_for(), to_protocol_address(),
and process_prekey_bundle() still use the original jid, allowing non-canonical
LIDs to create different session addresses; fix by canonicalizing the JID once
(e.g., let canonical = jid.normalize_for_prekey_bundle()) and then use
canonical.to_protocol_address() for signal_addr, use canonical for the
prekey_bundles lookup, pass the canonical signal_addr into session_lock_for()
and process_prekey_bundle(), and ensure any session storage/locking consistently
uses that canonical JID rather than the raw jid.
In `@wacore/src/send.rs`:
- Around line 320-335: encrypt_for_devices is a public low-level function that
mutates Signal sessions and must be run under the per-sender session lock; to
fix, do one of two: (A) hide this primitive (make encrypt_for_devices
non-public) and add a locked wrapper method on the Signal client (e.g.,
client.signal().encrypt_for_devices_locked(...)) that acquires the appropriate
session_locks for the sender (and message_enqueue_locks for chat-level
serialization if applicable) before calling encrypt_for_devices, or (B) keep it
public but add a mandatory documentation contract and runtime assert that the
caller holds the per-sender session lock (using session_locks) and serialize
caller usage via message_enqueue_locks where needed; update visibility and add
the wrapper on the type that exposes SignalStores (client.signal()) and
reference the symbols encrypt_for_devices, SignalStores, session_locks,
message_enqueue_locks, and client.signal() so callers use the safe locked API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d926d68c-8afe-4b01-9d50-1eefc3302e45
📒 Files selected for processing (11)
src/client.rssrc/client/sessions.rssrc/features/mod.rssrc/features/signal.rssrc/lib.rssrc/message.rssrc/retry.rssrc/send.rssrc/store/signal_adapter.rswacore/src/send.rswacore/src/types/events.rs
| // Dedup via session_locks — bare JID won't collide with protocol addresses ("user:device") | ||
| let bare = sender.to_non_ad().to_string(); | ||
| let mutex = self | ||
| .session_locks | ||
| .get_with_by_ref(bare.as_str(), async { | ||
| std::sync::Arc::new(async_lock::Mutex::new(())) | ||
| }) | ||
| .await; | ||
| let mutex = self.session_lock_for(&bare).await; |
There was a problem hiding this comment.
Deduplicate reissue using the same canonical key you use for tc-token storage.
sender.to_non_ad() is different for PN and LID aliases of the same contact, but both paths later collapse to the same tc-token row. That lets duplicate fire-and-forget tasks issue the same IQ and race on the same backend key.
♻️ Proposed fix
- // Dedup via session_locks — bare JID won't collide with protocol addresses ("user:device")
- let bare = sender.to_non_ad().to_string();
- let mutex = self.session_lock_for(&bare).await;
+ let token_jid = self.resolve_to_lid_jid(sender).await.user;
+ let mutex = self
+ .session_lock_for(&format!("tctoken:{token_jid}"))
+ .await;
let Some(_guard) = mutex.try_lock() else {
return;
};
-
- let token_jid = if sender.is_lid() {
- sender.user.clone()
- } else {
- match self.lid_pn_cache.get_current_lid(&sender.user).await {
- Some(lid) => lid,
- None => sender.user.clone(),
- }
- };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/send.rs` around lines 1326 - 1328, The dedup lock uses sender.to_non_ad()
which differs between PN and LID aliases and thus can allow duplicate tasks to
race on the same tc-token row; replace the computation of bare and the lock
acquisition to use the exact canonical key used for tc-token storage (i.e., call
the same helper you use when reading/writing tc-token rows) instead of
sender.to_non_ad().to_string(), then pass that canonical key into
self.session_lock_for(...) so session_lock_for and tc-token operations use the
identical identifier.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/features/signal.rs (3)
45-58:⚠️ Potential issue | 🟠 MajorSession state mutations are not flushed to persistent storage.
After
message_encryptadvances the Signal ratchet, changes remain only in the in-memory cache. Without callingflush_signal_cache(), a reconnect or process restart will roll back session state, causing message decryption failures on the peer side.This applies to all mutating paths in this file:
encrypt_message,decrypt_message,encrypt_group_message,decrypt_group_message, andcreate_participant_nodes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/signal.rs` around lines 45 - 58, The Signal ratchet state changes performed by message_encrypt (and other mutating functions encrypt_message, decrypt_message, encrypt_group_message, decrypt_group_message, create_participant_nodes) are not persisted; after calling these functions you must call adapter.flush_signal_cache().await and propagate any errors so state is written to persistent storage; locate the match block handling CiphertextMessage in encrypt_message (and the equivalent return paths in decrypt_message, encrypt_group_message, decrypt_group_message, create_participant_nodes) and insert a call to flush_signal_cache().await before returning Ok(...), handling/returning failures from flush_signal_cache() so the function fails rather than silently losing state.
97-98:⚠️ Potential issue | 🟠 MajorHard-coded v2 unpadding breaks raw byte round-trips.
encrypt_messageaccepts arbitrary raw bytes, butdecrypt_messagealways callsunpad_message_ref(&padded, 2). This makes raw round-trips impossible and ignores the actual stanzavvalue. Consider either returning raw bytes from this low-level API or accepting an explicit padding version parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/signal.rs` around lines 97 - 98, decrypt_message currently hard-codes unpadding with MessageUtils::unpad_message_ref(&padded, 2), which breaks round-trips for arbitrary raw bytes and ignores the stanza version; change decrypt_message (or the low-level API) to derive the padding version from the stanza (use the stanza.v field) or accept an explicit padding_version parameter and pass that into MessageUtils::unpad_message_ref, or alternatively document/implement that the API returns raw padded bytes; update references in encrypt_message and any callers to use the matching padding behavior so round-trips succeed.
117-124:⚠️ Potential issue | 🟠 MajorHardcoded phone-number JID ignores LID-addressing groups.
This always uses
.pnfrom the device snapshot, but sender-key groups can use LID addressing. When the group is LID-addressed, the SKDM and ciphertext will be created under the wrong sender identity, causing peers to look up a different sender-key record.Use
Client::get_own_jid_for_group(group_jid)which respects the group's addressing mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/signal.rs` around lines 117 - 124, The code currently obtains the sender JID from the device snapshot via self.client.persistence_manager.get_device_snapshot().pn which forces PN addressing and breaks LID-addressed groups; replace that logic in the section that computes own_jid for group messages by calling the client helper that respects group addressing: Client::get_own_jid_for_group(group_jid) (e.g., self.client.get_own_jid_for_group(group_jid)). Ensure you pass the group JID being handled and propagate any resulting error (same anyhow!("not logged in") style or appropriate error) instead of using .pn so sender-key records and ciphertext are created under the correct identity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/signal.rs`:
- Around line 148-171: Add the same concurrency caveat to
decrypt_group_message's doc comment (matching encrypt_group_message) or
implement locking around sender-key ops: protect calls that access
adapter.sender_key_store (in decrypt_group_message and any sender-key paths)
with the same mutex used by encrypt_group_message (or introduce a SenderKey lock
in the client) to prevent concurrent encrypt/decrypt races; also replace the
hard-coded unpad version argument in decrypt_group_message (currently
MessageUtils::unpad_message_ref(&padded, 2)) with the shared/central version
constant or API (e.g., use MessageUtils::DEFAULT_VERSION or a
MessageUtils::unpad_message_ref(&padded,
MessageUtils::detected_version(&padded))) so the unpad version is not
hard-coded.
- Around line 186-206: delete_sessions currently acquires per-JID session locks
in caller order which can deadlock against other callers that sort lock keys
(e.g. create_participant_nodes/build_session_lock_keys); to fix, compute the
canonical lock ordering before acquiring any locks (reuse the same
build_session_lock_keys logic or sort by the same key used by
build_session_lock_keys), iterate over the sorted keys to acquire each session
lock via session_lock_for, and then perform the signal_cache.delete_session and
backend.delete_session calls for each corresponding Jid (map sorted keys back to
their Jid) so locks are always taken in the same global order; update
delete_sessions to use this sorted acquisition strategy (functions to touch:
delete_sessions, session_lock_for, build_session_lock_keys,
signal_cache.delete_session, backend.delete_session).
---
Duplicate comments:
In `@src/features/signal.rs`:
- Around line 45-58: The Signal ratchet state changes performed by
message_encrypt (and other mutating functions encrypt_message, decrypt_message,
encrypt_group_message, decrypt_group_message, create_participant_nodes) are not
persisted; after calling these functions you must call
adapter.flush_signal_cache().await and propagate any errors so state is written
to persistent storage; locate the match block handling CiphertextMessage in
encrypt_message (and the equivalent return paths in decrypt_message,
encrypt_group_message, decrypt_group_message, create_participant_nodes) and
insert a call to flush_signal_cache().await before returning Ok(...),
handling/returning failures from flush_signal_cache() so the function fails
rather than silently losing state.
- Around line 97-98: decrypt_message currently hard-codes unpadding with
MessageUtils::unpad_message_ref(&padded, 2), which breaks round-trips for
arbitrary raw bytes and ignores the stanza version; change decrypt_message (or
the low-level API) to derive the padding version from the stanza (use the
stanza.v field) or accept an explicit padding_version parameter and pass that
into MessageUtils::unpad_message_ref, or alternatively document/implement that
the API returns raw padded bytes; update references in encrypt_message and any
callers to use the matching padding behavior so round-trips succeed.
- Around line 117-124: The code currently obtains the sender JID from the device
snapshot via self.client.persistence_manager.get_device_snapshot().pn which
forces PN addressing and breaks LID-addressed groups; replace that logic in the
section that computes own_jid for group messages by calling the client helper
that respects group addressing: Client::get_own_jid_for_group(group_jid) (e.g.,
self.client.get_own_jid_for_group(group_jid)). Ensure you pass the group JID
being handled and propagate any resulting error (same anyhow!("not logged in")
style or appropriate error) instead of using .pn so sender-key records and
ciphertext are created under the correct identity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bcb24335-4f9b-4f6f-99a7-6e14f6a90f5a
📒 Files selected for processing (2)
src/features/signal.rswacore/src/send.rs
| /// Decrypt a group (sender-key) message. | ||
| pub async fn decrypt_group_message( | ||
| &self, | ||
| group_jid: &Jid, | ||
| sender_jid: &Jid, | ||
| ciphertext: &[u8], | ||
| ) -> Result<Vec<u8>> { | ||
| let sender_key_name = SenderKeyName::new( | ||
| group_jid.to_string(), | ||
| sender_jid.to_protocol_address().to_string(), | ||
| ); | ||
|
|
||
| let mut adapter = self.client.signal_adapter().await; | ||
|
|
||
| let padded = wacore::libsignal::protocol::group_decrypt( | ||
| ciphertext, | ||
| &mut adapter.sender_key_store, | ||
| &sender_key_name, | ||
| ) | ||
| .await?; | ||
|
|
||
| let unpadded = MessageUtils::unpad_message_ref(&padded, 2)?; | ||
| Ok(unpadded.to_vec()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Sender-key decryption also lacks concurrency protection.
The doc comment on encrypt_group_message warns it's not safe to call concurrently with decrypt_group_message, but decrypt_group_message has no corresponding warning. Consider adding the same caveat here for API symmetry, or implementing proper locking for sender-key operations.
Also, line 169 has the same hard-coded v2 unpadding issue as decrypt_message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/signal.rs` around lines 148 - 171, Add the same concurrency
caveat to decrypt_group_message's doc comment (matching encrypt_group_message)
or implement locking around sender-key ops: protect calls that access
adapter.sender_key_store (in decrypt_group_message and any sender-key paths)
with the same mutex used by encrypt_group_message (or introduce a SenderKey lock
in the client) to prevent concurrent encrypt/decrypt races; also replace the
hard-coded unpad version argument in decrypt_group_message (currently
MessageUtils::unpad_message_ref(&padded, 2)) with the shared/central version
constant or API (e.g., use MessageUtils::DEFAULT_VERSION or a
MessageUtils::unpad_message_ref(&padded,
MessageUtils::detected_version(&padded))) so the unpad version is not
hard-coded.
…order - RawNode dispatch moved before xmlstreamend/IQ-response early returns so all decoded stanzas reach external observers - Add flush_signal_cache() after all Signal-mutating operations in the feature API (encrypt_message, decrypt_message, encrypt_group_message, decrypt_group_message, create_participant_nodes) — the signal cache uses deferred writes and needs explicit flushing - decrypt_message/decrypt_group_message now return raw padded bytes instead of hardcoding v2 unpadding — callers use MessageUtils::unpad_message_ref with the stanza's v attribute - encrypt_group_message uses get_own_jid_for_group() instead of hardcoded .pn, respecting LID-addressing groups - Add #[non_exhaustive] to Event enum for forward compatibility
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19a5b6145b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let signal_addr = jid.to_protocol_address(); | ||
| let signal_addr_str = jid.to_protocol_address_string(); |
There was a problem hiding this comment.
When a contact has migrated to LID sessions (a common path in this codebase’s send flow), encrypt_message/decrypt_message use jid.to_protocol_address() directly instead of first normalizing with resolve_encryption_jid. In that scenario, callers passing a PN JID will look up/mutate the wrong session key, causing unexpected SessionNotFound/decrypt failures even though a valid LID session exists; this diverges from the normal send path that does LID-first resolution.
Useful? React with 👍 / 👎.
… API - encrypt_message/decrypt_message: resolve PN→LID via resolve_encryption_jid before Signal session lookup, matching the internal send/receive paths that do LID-first resolution - encrypt_message/decrypt_message: derive lock key from ProtocolAddress display cache instead of computing to_signal_address_string() twice - decrypt_group_message: use to_protocol_address_string() directly for SenderKeyName, avoiding intermediate ProtocolAddress allocation - decrypt_group_message: add concurrency caveat matching encrypt side - delete_sessions: sort lock keys to prevent AB/BA deadlocks with create_participant_nodes; hoist get_device_arc() outside loop
|
@claude review |
|
Claude finished @jlucaso1's task in 2m 5s —— View job Code Review: Signal Protocol Feature API (Round 2)
The previous review rounds caught real issues — lock ordering in Previously flagged issues — now fixed
Remaining observations1.
This is a minor API consistency issue — callers who use the encrypt/decrypt APIs would naturally pass the same JIDs here. But it could be a footgun for external consumers who pass PN JIDs from their contact list. Suggestion (non-blocking): Either resolve PN→LID in these methods too, or document that callers should pass the same JID form used for encrypt/decrypt. 2. This is a powerful primitive that bypasses node marshaling and the Suggestion (non-blocking): Add a brief doc comment noting that the bytes must be a valid marshaled WABinary stanza. 3.
SummaryThe PR is in good shape after 3 rounds of fixes. The DRY consolidation is clean, the Signal feature API follows the established pattern, and the critical concurrency/persistence issues from earlier reviews are resolved. The two non-blocking suggestions above are minor consistency items. Verdict: Looks good to merge once the PN→LID resolution consistency is considered (can be a follow-up). |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/features/signal.rs (1)
117-176:⚠️ Potential issue | 🟠 MajorSender-key APIs still race each other.
encrypt_group_message()anddecrypt_group_message()both mutateadapter.sender_key_store, but neither takes any internal mutex for theSenderKeyName. The doc warning only moves the burden to callers; concurrent use on the same group/sender can still corrupt sender-key state or produce ciphertext/plaintext from different ratchet steps. Please add an internal cached lock keyed by the sender-key identity and hold it throughflush_signal_cache().As per coding guidelines, "Use `session_locks` to serialize per-sender Signal encrypt/decrypt operations and `message_enqueue_locks` to serialize per-chat incoming message processing".🔒 Suggested direction
pub async fn encrypt_group_message( &self, group_jid: &Jid, plaintext: &[u8], ) -> Result<(Vec<u8>, Vec<u8>)> { let own_jid = self.client.get_own_jid_for_group(group_jid).await?; + let lock = self.client.sender_key_lock_for(group_jid, &own_jid).await; + let _guard = lock.lock().await; let mut adapter = self.client.signal_adapter().await; @@ pub async fn decrypt_group_message( &self, group_jid: &Jid, sender_jid: &Jid, ciphertext: &[u8], ) -> Result<Vec<u8>> { + let lock = self.client.sender_key_lock_for(group_jid, sender_jid).await; + let _guard = lock.lock().await; let sender_key_name = SenderKeyName::new( group_jid.to_string(), sender_jid.to_protocol_address().to_string(), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/signal.rs` around lines 117 - 176, encrypt_group_message and decrypt_group_message both mutate adapter.sender_key_store without per-sender locking; add a cached per-sender lock (using the existing session_locks mechanism) keyed by the SenderKeyName (or group_jid+sender_jid identity) and acquire it at the start of encrypt_group_message and decrypt_group_message, hold the lock across calls to wacore functions and through the call to self.client.flush_signal_cache(), then release the lock; ensure the lock key generation matches SenderKeyName construction so concurrent ops for the same sender/group serialize while different keys remain parallel.wacore/src/types/events.rs (1)
382-460:⚠️ Potential issue | 🟠 MajorThis is still a breaking public
Eventchange.
#[non_exhaustive]makes future additions easier, but adding it to an existing public enum still forces downstream exhaustivematches to add a wildcard arm. If this release is meant to stay non-breaking, this needs a different surface; otherwise it needs the corresponding breaking-version bump and release note.In Rust, is marking an existing public enum as `#[non_exhaustive]` a breaking API change for downstream crates that match it exhaustively?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/types/events.rs` around lines 382 - 460, The PR added #[non_exhaustive] to the public enum Event which is a breaking change for downstream crates that previously exhaustively matched Event; either remove the #[non_exhaustive] attribute from Event to keep the API backwards-compatible, or if you intend a breaking release, keep the attribute but update the crate version with a breaking semver bump and add a release note calling out the changed Event enum so downstream consumers can adjust their match arms; locate the enum by the Event type in wacore/src/types/events.rs (and any usages of RawNode/variants) to apply the chosen fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/src/types/events.rs`:
- Around line 454-459: The Event enum currently derives Serialize but has the
RawNode(Arc<Node>) variant marked with #[serde(skip)], which breaks derived
serialization when raw-node forwarding is enabled; implement a custom Serialize
for Event (replace the derive on Event) that serializes every variant as before
but special-cases RawNode to either omit it (skip serialization) or emit a safe
placeholder (e.g., unit or null) so serializing Event never errors; reference
the Event enum and the RawNode(Arc<Node>) variant when making the change and
keep manual/bridge-level serialization for the actual Arc<Node> in the bridge
code.
---
Duplicate comments:
In `@src/features/signal.rs`:
- Around line 117-176: encrypt_group_message and decrypt_group_message both
mutate adapter.sender_key_store without per-sender locking; add a cached
per-sender lock (using the existing session_locks mechanism) keyed by the
SenderKeyName (or group_jid+sender_jid identity) and acquire it at the start of
encrypt_group_message and decrypt_group_message, hold the lock across calls to
wacore functions and through the call to self.client.flush_signal_cache(), then
release the lock; ensure the lock key generation matches SenderKeyName
construction so concurrent ops for the same sender/group serialize while
different keys remain parallel.
In `@wacore/src/types/events.rs`:
- Around line 382-460: The PR added #[non_exhaustive] to the public enum Event
which is a breaking change for downstream crates that previously exhaustively
matched Event; either remove the #[non_exhaustive] attribute from Event to keep
the API backwards-compatible, or if you intend a breaking release, keep the
attribute but update the crate version with a breaking semver bump and add a
release note calling out the changed Event enum so downstream consumers can
adjust their match arms; locate the enum by the Event type in
wacore/src/types/events.rs (and any usages of RawNode/variants) to apply the
chosen fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ade7452c-4d6e-42ad-a273-71f7d1c9dce9
📒 Files selected for processing (3)
src/client.rssrc/features/signal.rswacore/src/types/events.rs
| /// Raw decoded stanza, emitted before router dispatch. | ||
| /// Allows external consumers to observe protocol-level nodes (e.g. for voice calls). | ||
| /// Gated by `Client::set_raw_node_forwarding(true)` to avoid overhead when unused. | ||
| /// Serialized manually in the bridge (not via serde). | ||
| #[serde(skip)] | ||
| RawNode(Arc<Node>), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Serde in Rust, what happens when serializing an enum variant marked #[serde(skip)]?
💡 Result:
When serializing an enum variant marked with #[serde(skip)] in Rust using Serde's derive macros, serialization fails with an error. The generated Serialize impl explicitly returns an error like "the enum variant MyEnum::SkippedVariant cannot be serialized" if code attempts to serialize that variant. This prevents serialization of skipped variants entirely, unlike fields where #[serde(skip_serializing)] unconditionally skips them. This behavior is documented in the official Serde variant attributes page, which states #[serde(skip)] means "Never serialize or deserialize this variant." GitHub issues confirm attempting to serialize such a variant fails, as the derive code generates an error branch for skipped variants. There are known inconsistencies with binary formats like bincode due to variant indices, but for self-describing formats like JSON, the primary effect is the serialization error when hitting the skipped variant. To conditionally skip serialization of a variant's data (e.g., serialize only the tag), use #[serde(skip_serializing)] on its fields instead, or implement a custom Serialize impl.
Citations:
- 1: https://serde.rs/variant-attrs.html
- 2: Allow skipping an enum variant serde-rs/serde#2062
- 3: Binary serialize/deserialize inconsistency of enums with skipped fields serde-rs/serde#2614
- 4: https://stackoverflow.com/questions/62175415/does-serde-skip-attribute-actually-skip-an-enum-variant
- 5: https://doc.servo.org/src/serde_derive/ser.rs.html
🏁 Script executed:
# Read the Event enum definition around the RawNode variant
sed -n '440,470p' wacore/src/types/events.rsRepository: jlucaso1/whatsapp-rust
Length of output: 1264
🏁 Script executed:
# Search for where Event is serialized in the codebase
rg -t rs 'serialize.*Event|Event.*serialize' --context 2Repository: jlucaso1/whatsapp-rust
Length of output: 92
🏁 Script executed:
# Check if there's a custom Serialize impl for Event
ast-grep --pattern 'impl Serialize for Event { $$$ }'Repository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Search for bridge or logger code that might serialize Event
rg -t rs -i 'bridge|logger' --context 1 | grep -A 5 -B 5 'Event'Repository: jlucaso1/whatsapp-rust
Length of output: 92
🏁 Script executed:
# Check for Serialize derive on Event enum
rg 'pub enum Event' wacore/src/types/events.rs -A 5Repository: jlucaso1/whatsapp-rust
Length of output: 222
🏁 Script executed:
# Search for serialization of Event in the codebase
rg 'serde_json::to|serde_json::value|\.serialize\(' --max-count 20Repository: jlucaso1/whatsapp-rust
Length of output: 10896
🏁 Script executed:
# Look for bridge-related files that might serialize Event
fd -t f -i bridgeRepository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Check what the Event enum derives
head -100 wacore/src/types/events.rs | grep -A 10 'pub enum Event'Repository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Look for Event enum definition with its derive macros
rg 'derive.*Serialize' wacore/src/types/events.rs -B 5 | head -30Repository: jlucaso1/whatsapp-rust
Length of output: 729
🏁 Script executed:
# Check for custom Serialize impl for Event
rg 'impl.*Serialize.*Event' wacore/src/types/events.rs -A 20Repository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Search for where Event is actually serialized (JSON, etc)
rg 'serde_json.*Event|Event.*json' --max-count 20Repository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Look for any references to RawNode in the codebase
rg 'RawNode' --max-count 20Repository: jlucaso1/whatsapp-rust
Length of output: 393
🏁 Script executed:
# Search for bridge-related code that might serialize Event
rg 'bridge|Bridge' wacore/src --type rust -lRepository: jlucaso1/whatsapp-rust
Length of output: 155
🏁 Script executed:
# Look in src/ directory for bridge code
rg 'bridge|Bridge' src --type rust -lRepository: jlucaso1/whatsapp-rust
Length of output: 89
🏁 Script executed:
# Search more broadly for any code that serializes Event to JSON or other formats
rg 'Event' wacore/src/types/events.rs | grep -i serializeRepository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Check if there's a custom Serialize impl that handles Event specially
rg 'impl.*Serialize' wacore/src/types/events.rs -A 30Repository: jlucaso1/whatsapp-rust
Length of output: 2367
🏁 Script executed:
# Check the bridge implementations found
cat wacore/src/store/in_memory.rs | head -100Repository: jlucaso1/whatsapp-rust
Length of output: 3244
🏁 Script executed:
# Look for serialization patterns in in_memory bridge
rg 'serialize.*Event|Event.*serialize|serde_json' wacore/src/store/in_memory.rsRepository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Check message_processing for bridge-related serialization
rg 'serialize.*Event|serde_json.*Event' wacore/src/message_processing.rsRepository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Check if there's an actual bridge module or FFI layer
fd -t f 'bridge|ffi' --hiddenRepository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Look for any logger that might serialize Event
rg -i 'logger|log.*Event|Event.*log' --max-count 20Repository: jlucaso1/whatsapp-rust
Length of output: 15053
RawNode cannot round-trip through Event's derived Serialize.
Event derives Serialize, but the RawNode variant is marked with #[serde(skip)], which causes serialization to error rather than silently omit the value. Any generic bridge, logger, or telemetry that serializes Event directly will fail as soon as raw-node forwarding is enabled.
The code comment notes manual serialization in the bridge, but there's no custom Serialize impl to enforce this. Implement a custom serializer or use a bridge-specific DTO to prevent accidental failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/types/events.rs` around lines 454 - 459, The Event enum currently
derives Serialize but has the RawNode(Arc<Node>) variant marked with
#[serde(skip)], which breaks derived serialization when raw-node forwarding is
enabled; implement a custom Serialize for Event (replace the derive on Event)
that serializes every variant as before but special-cases RawNode to either omit
it (skip serialization) or emit a safe placeholder (e.g., unit or null) so
serializing Event never errors; reference the Event enum and the
RawNode(Arc<Node>) variant when making the change and keep manual/bridge-level
serialization for the actual Arc<Node> in the bridge code.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b403569b88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // and the DM send path, preventing AB/BA deadlocks. | ||
| let mut keyed: Vec<(String, &Jid)> = jids | ||
| .iter() | ||
| .map(|jid| (jid.to_protocol_address_string(), jid)) |
There was a problem hiding this comment.
When callers pass a PN JID for a contact whose active session was migrated to LID, this method computes lock/delete keys from the PN address and never touches the LID session entry. The call can return Ok(()) while the real session remains usable, and the lock key also won’t serialize against the session actually used by send/decrypt paths (which normalize via LID-first logic). Normalize each input JID with resolve_encryption_jid before building lock keys and deleting.
Useful? React with 👍 / 👎.
|
|
||
| /// Check whether a Signal session exists for `jid`. | ||
| pub async fn validate_session(&self, jid: &Jid) -> Result<bool> { | ||
| let signal_addr = jid.to_protocol_address(); |
There was a problem hiding this comment.
validate_session checks only jid.to_protocol_address(), so a PN JID will report false if the established session is stored under the contact’s LID address. That yields false negatives for migrated users and can drive incorrect caller behavior (e.g., unnecessary session re-establishment). This should use the same PN→LID resolution used by encrypt/decrypt paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/signal.rs`:
- Around line 210-236: delete_sessions currently derives lock keys from
jid.to_protocol_address_string(), which is inconsistent with
encrypt_message/decrypt_message and build_session_lock_keys (they use
resolve_encryption_jid), causing ineffective locking and wrong session
deletions; fix by resolving each JID with resolve_encryption_jid before building
keys and before deleting sessions so locking and deletion use the same canonical
address: update the keyed construction in delete_sessions to call
resolve_encryption_jid(jid) and
to_protocol_address_string()/to_protocol_address() on the resolved JID, perform
deduping on those resolved keys, acquire session locks from
session_lock_for(resolved_key), and call signal_cache.delete_session and
device_store.backend.delete_session with the resolved addr (use the same
addr.as_str()) so behavior matches create_participant_nodes,
build_session_lock_keys, encrypt_message, and decrypt_message.
- Around line 192-202: validate_session currently builds the signal address with
jid.to_protocol_address(), which can mismatch encrypt_message/decrypt_message
that first call resolve_encryption_jid; change validate_session to resolve the
jid via resolve_encryption_jid(jid) and then call to_protocol_address() on the
resolved Jid before checking the session so
signal_cache.has_session(&signal_addr, &*device_guard.backend) uses the same
PN→LID-resolved address as encrypt_message/decrypt_message; keep the same
device_store/read and error mapping logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d7e6694-21a4-45fb-872f-ef5eb7f6abd1
📒 Files selected for processing (1)
src/features/signal.rs
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 58s —— View job Code Review: Signal Protocol Feature API (Round 3)
All critical issues from previous rounds have been addressed. Here's the current state: Previously flagged issues — all resolved
Remaining observations (non-blocking)1. The current code acquires one lock per loop iteration, which means it acquires-and-releases sequentially rather than holding all locks simultaneously. This is fine for correctness (no AB/BA deadlock since only one lock is held at a time), but it means a concurrent For a 2.
Suggestion: Either implement a custom 3. The doc comment now explicitly states "Callers must hold per-device session locks before calling this function" — this addresses the concern about the newly-public API being misusable. 4. The doc now warns about malformed data and explains it bypasses node logging. Well done. DRY consolidation verificationAll refactored sites are strict 1:1 replacements:
The VerdictThe PR is in good shape. All critical concurrency, persistence, and correctness issues from earlier rounds are fixed. The two non-blocking observations above are minor and could be addressed in follow-ups. LGTM. |
1 participant
Summary
New
Signalfeature API (client.signal().*) exposing low-level Signal protocol operations, plus DRY consolidation of internal helpers across the codebase.Signal Feature API (
src/features/signal.rs)encrypt_message(jid, plaintext)(EncType, ciphertext)decrypt_message(jid, enc_type, ciphertext)encrypt_group_message(group_jid, plaintext)(Option<skdm>, ciphertext)decrypt_group_message(group_jid, sender_jid, ciphertext)create_participant_nodes(jids, message)validate_session(jid)delete_sessions(jids)deleteRemoteSession)assert_sessions(jids)get_user_devices(jids)All methods that take JIDs resolve PN→LID via
resolve_encryption_jid, matching the internal send/receive paths.WA Web compliance
encrypt_group_messageonly creates SKDM when no sender key exists (matches WA Web — not on every call)delete_sessionsremoves both session and identity (matches WA Web'sdeleteRemoteSession)checkPnToLidMapping)flush_signal_cache()after success (matches WA Web'sflushBufferToDiskIfNotMemOnlyMode)EncTypeenum (PreKeyMessage/Message/SenderKey) instead of stringly-typed"msg"/"pkmsg"vattribute (matches how internal paths handle versioned padding)Other changes
Event::RawNode: New event variant for raw stanza observation before router dispatch, gated byClient::set_raw_node_forwarding()(zero-cost atomic check when disabled). Library extension — no WA Web equivalent.#[non_exhaustive]added toEventenum for forward compatibility.Client::send_raw_bytes: Send pre-marshaled bytes through noise socket.send_nodenow delegates to it.tokio::spawn→self.runtime.spawn().detach()inmessage.rstctoken reissue (aligns with runtime abstraction).DRY consolidation
Client::signal_adapter()/signal_adapter_from()SignalProtocolStoreAdapter::new(...)Client::session_lock_for()session_locks.get_with_by_ref(...)Client::get_noise_socket()SignalProtocolStoreAdapter::as_signal_stores()SignalStores { ... }struct initTest plan
cargo fmt --all— cleancargo clippy --all --tests— cleancargo test --all --lib— all 1,076 tests pass, 0 failures