fix(send): admin revoke not applied on recipient devices - #621
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughInfer EditAttribute from messages and thread it into DM and group retry stanza builders so retries preserve edit/revoke/pin metadata; centralize decrypt-fail hide logic (exempting AdminRevoke) and apply it to SKDM per-device encryption; add tests. ChangesEdit Attributes in Retry Resends
Sequence DiagramsequenceDiagram
participant Client
participant Unwrap as unwrap_message
participant Infer as EditAttribute::infer_from_message
participant Prepare as prepare_{dm,group}_retry_stanza
participant SKDM as SKDMDistributor
participant Network
Client->>Unwrap: take cached original_msg
Unwrap->>Infer: extract edit/pin/revoke signals
Infer-->>Client: Option<EditAttribute>
Client->>Prepare: call with edit option
Prepare->>SKDM: compute per-device hide_decrypt_fail using predicate(edit, message)
SKDM->>Network: send per-device encrypted payloads
Prepare->>Network: send main retry stanza (with optional edit attribute)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Alright. This needs to work correctly end-to-end; verify tests and CI. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/src/send.rs (1)
989-1000:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd the missing
editparameter to all test call sites—this code doesn't compile right now.The signature changes added an
editparameter, but the tests never got updated. We've got E0061 errors at lines 2865, 2916, 2972, 3007, and 3046 because these calls are using the old arity. PassNoneforeditin each test unless you're explicitly testing edit behavior.Example fixes
let n = prepare_group_retry_stanza( &mut ss, &mut is, group.clone(), p.clone(), p.clone(), &wa::Message::default(), "3EB0ABC".into(), 1, None, AddressingMode::Pn, + None, ) let n = prepare_dm_retry_stanza( &mut ss, &mut is, to.clone(), requester.clone(), encryption, &wa::Message::default(), "dm-retry-1".into(), 1, None, + None, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/src/send.rs` around lines 989 - 1000, The tests calling prepare_dm_retry_stanza now fail because its signature gained the edit: Option<crate::types::message::EditAttribute> parameter; update every test call site that invokes prepare_dm_retry_stanza to include the new edit argument (use None where edit behavior isn’t being tested, or Some(...) with a constructed EditAttribute when testing edits) so the arity matches the function signature; look for invocations of prepare_dm_retry_stanza in your tests and add the extra parameter accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/src/send.rs`:
- Around line 987-988: There are duplicate #[allow(clippy::too_many_arguments)]
attributes applied above functions in send.rs (one duplicate near the block
around startLine ~987 and another around ~1060); remove the repeated attribute
so each function has a single #[allow(clippy::too_many_arguments)] line (ensure
you keep one occurrence directly above the corresponding function signature and
remove the extra identical line), and scan nearby functions to confirm no other
exact-duplicate allow attributes remain.
In `@wacore/src/types/message.rs`:
- Around line 103-121: infer_from_message currently returns early when
msg.protocol_message is None, so it misses top-level edits; change the logic in
infer_from_message to check the top-level edited_message on the incoming msg
(msg.edited_message.is_some()) before bailing out on protocol_message, and
return Some(Self::MessageEdit) when present; keep the existing protocol_message
handling (pm.r#type, pm.key.from_me) intact for revoke logic so both top-level
edits and protocol_message revokes are detected.
---
Outside diff comments:
In `@wacore/src/send.rs`:
- Around line 989-1000: The tests calling prepare_dm_retry_stanza now fail
because its signature gained the edit:
Option<crate::types::message::EditAttribute> parameter; update every test call
site that invokes prepare_dm_retry_stanza to include the new edit argument (use
None where edit behavior isn’t being tested, or Some(...) with a constructed
EditAttribute when testing edits) so the arity matches the function signature;
look for invocations of prepare_dm_retry_stanza in your tests and add the extra
parameter accordingly.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 05d20880-23c4-4bfc-a9bc-1c755a3cf29a
📒 Files selected for processing (3)
src/retry.rswacore/src/send.rswacore/src/types/message.rs
22192ae to
7d9924d
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/src/send.rs`:
- Around line 862-865: Consolidate the repeated predicate that computes
hide_decrypt_fail into a single helper (e.g., fn hide_decrypt_fail(edit:
Option<&EditAttribute>, message: &Message) -> bool) instead of copying the
inline predicate; replace the inline expression used to set the local
hide_decrypt_fail variable (the edit.as_ref().is_some_and(...) ||
should_hide_decrypt_fail(message) occurrence) and the duplicates at the other
spots (the copies around the SKDM and SKMSG paths currently at the other noted
locations) to call this helper, and have the helper call
should_hide_decrypt_fail(message) and centralize the edit checks against
EditAttribute::Empty and EditAttribute::AdminRevoke so future edits use one
canonical rule.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 56dad9a0-2211-4abc-83ea-09abc0e94bcf
📒 Files selected for processing (3)
src/retry.rswacore/src/send.rswacore/src/types/message.rs
7d9924d to
ef61bd1
Compare
|
Thanks @coderabbitai — both suggestions applied in ef61bd1:
Local: |
|
Tip For best results, initiate chat on the files or code changes.
Both implementations look correct from a structural standpoint:
Execution was clean. These changes needed to be right, and they are. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d18f2049-cf2b-40a8-aabe-99e158fb43b8
📒 Files selected for processing (3)
src/retry.rswacore/src/send.rswacore/src/types/message.rs
ef61bd1 to
5806e55
Compare
|
Round 2 applied in 5806e55:
Local: 689 tests passed, no failures, no clippy warnings introduced. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/src/types/message.rs`:
- Around line 113-145: The infer_from_message function currently checks only
top-level fields and misses cases where the semantic payload is wrapped in
neutral envelopes; fix infer_from_message by first unwrapping neutral wrappers
(the same set handled by send.rs::unwrap_message: ephemeral_message,
view_once_message, device_sent_message, etc.) or by calling/replicating that
unwrap helper to obtain the inner Message, then perform the existing checks
(pin_in_chat_message, edited_message, protocol_message and
protocol_message.key.from_me) on the unwrapped message so edits/revokes/pins are
correctly inferred for wrapped payloads.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 10de3d73-95f0-4c16-8bb9-e4531e51197b
📒 Files selected for processing (3)
src/retry.rswacore/src/send.rswacore/src/types/message.rs
Admin revoke (`edit="8"`) had three independent bugs that combined to
leave the message visible on a subset of recipients — typically the
group creator or any device that triggered a retry receipt.
Root causes
-----------
1. `prepare_dm_stanza` (used for peer fanout inside a group) sent every
`<enc>` with `decrypt-fail="hide"` whenever `edit != Empty`. The
test in `wacore/src/types/message.rs` already documents that the
server rejects this for `AdminRevoke`. The SKMSG path on l. 1384
already had the correct check; the peer-fanout path on l. 862 did
not.
2. The SKDM distribution (`prepare_group_stanza` l. 1326) hardcoded
`hide_decrypt_fail = true`. For an admin revoke this meant the
sender-key distribution itself was attribute-tagged in a way the
server drops for revokes, so devices that didn't have the group
sender key already never received it — and therefore never
decrypted the main `<enc type="skmsg">` carrying the REVOKE.
3. Retry path lost the `edit` attribute. When a recipient device
replies with a `<receipt type="retry">`, the bot resends via
`prepare_{group,dm}_retry_stanza`. Those builders did not accept
or emit an `edit` attribute, so the resend went out as a plain
`<message>` — the client received it but had no signal that this
was a revoke and never applied it. Observed in the wild: the
group creator's device (which always retried because its session
needed a prekey bundle) kept showing the revoked image.
Fix
---
- `prepare_dm_stanza`: exclude `AdminRevoke` from `hide_decrypt_fail`
(matches the existing logic in `prepare_group_stanza`'s skmsg path).
- `prepare_group_stanza` SKDM path: compute `skdm_hide_decrypt_fail`
with the same rule used for the main payload instead of hardcoding
`true`. Other infrastructure messages (reactions, edits, pins)
continue to hide decrypt failures as before — only `AdminRevoke`
flips to `false`.
- `prepare_{group,dm}_retry_stanza`: accept an `edit:
Option<EditAttribute>` parameter and emit `attr("edit", ...)` on
the rebuilt `<message>` stanza when present.
- `EditAttribute::infer_from_message`: helper that recovers the wire
`edit` value from a fully-constructed `wa::Message` (Revoke +
`key.from_me` → Sender/AdminRevoke; pin / edited_message → matching
variant). Used by the retry callers since the original stanza's
`edit` attribute is not part of the protobuf payload that
`add_recent_message` caches.
- `retry.rs`: infer `edit_attr` from `original_msg` and forward it
to both retry builders.
Verified end-to-end against a live group: admin revoke now applies on
all recipient devices, including the ones that go through the retry
path because their Signal session needed a prekey bundle.
5806e55 to
78b5e6c
Compare
|
Round 3 applied in 78b5e6c: Neutral wrappers now unwrapped before inference.
Added regression test
Local: 690 tests passed, fmt clean. |
Cleanup of the admin-revoke fix. Trim verbose doc/inline comments per project style (no em dashes, no bug-history narration, no "what" tables). Narrow unwrap_message visibility to pub(crate) since only intra-crate callers use it. Drop the AdminEdit branch from infer_from_message: WAWebSendMsgCommonApi's editAttribute always returns MESSAGE_EDIT for protocol_message.MessageEdit regardless of from_me, and the divergent branch was untriggered in practice.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wacore/src/types/message.rs (1)
125-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore
AdminEditinference for protocol-message edits.Line 125 collapses every
ProtocolType::MessageEditintoMessageEdit, so cached admin edits retry withoutedit="3"and come back as plain resends. Usepm.key.from_mehere the same way revoke already does, and lock it with the missingfrom_me = falseregression test.Suggested fix
- if pm.r#type == Some(ProtocolType::MessageEdit as i32) || pm.edited_message.is_some() { - return Some(Self::MessageEdit); + if pm.r#type == Some(ProtocolType::MessageEdit as i32) { + let from_me = pm.key.as_ref().and_then(|k| k.from_me).unwrap_or(false); + return Some(if from_me { + Self::MessageEdit + } else { + Self::AdminEdit + }); + } + if pm.edited_message.is_some() { + return Some(Self::MessageEdit); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/src/types/message.rs` around lines 125 - 126, The branch that treats ProtocolType::MessageEdit and edited_message as plain MessageEdit must distinguish sender like revoke does: when pm.r#type == Some(ProtocolType::MessageEdit as i32) || pm.edited_message.is_some(), check pm.key.from_me and return Self::MessageEdit if from_me == true, otherwise return Self::AdminEdit; update the logic in the function where pm.r#type, pm.edited_message and Self::MessageEdit are handled and add/adjust a regression test asserting that a protocol edit with pm.key.from_me == false is inferred as AdminEdit (i.e. include from_me = false case).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@wacore/src/types/message.rs`:
- Around line 125-126: The branch that treats ProtocolType::MessageEdit and
edited_message as plain MessageEdit must distinguish sender like revoke does:
when pm.r#type == Some(ProtocolType::MessageEdit as i32) ||
pm.edited_message.is_some(), check pm.key.from_me and return Self::MessageEdit
if from_me == true, otherwise return Self::AdminEdit; update the logic in the
function where pm.r#type, pm.edited_message and Self::MessageEdit are handled
and add/adjust a regression test asserting that a protocol edit with
pm.key.from_me == false is inferred as AdminEdit (i.e. include from_me = false
case).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e76bb077-13b8-44ce-ab26-c8b319eba1cc
📒 Files selected for processing (3)
src/retry.rswacore/src/send.rswacore/src/types/message.rs
2 participants
Summary
Admin revoke (
edit=\"8\") had three independent bugs that combined to leave the revoked message visible on a subset of recipients — most commonly the group admins/creator and any device that triggered a retry receipt.Issue surfaced while writing an anti-content bot: revoke admin call returned
Ok(_)and some clients saw the message disappear, but the bot operator's own device (which routinely retries on first contact) kept the message visible. The existing test inwacore/src/types/message.rsalready documents the expected behavior forAdminRevoke.Root causes & fix
1.
prepare_dm_stanza(peer fanout) sentdecrypt-fail=\"hide\"for admin revokesThe SKMSG path in
prepare_group_stanza(l. 1384) already excludedAdminRevokefromhide_decrypt_fail. The peer-fanout path inprepare_dm_stanza(l. 862) didn't — every<enc>went out with the attribute, and the server dropped the revoke for those recipients.Fix: mirror the same exclusion:
*e != Empty && *e != AdminRevoke.2. SKDM distribution hardcoded
hide_decrypt_fail = trueprepare_group_stanzal. 1326 sent the sender-key distribution withhide_decrypt_fail: truealways. For admin revoke this dropped the SKDM, so any device that didn't already have the group sender key never decrypted the main<enc type=\"skmsg\">carrying the REVOKE protocol message — even though the main payload itself was correctly attribute-clean.Fix: compute
skdm_hide_decrypt_failwith the same rule used for the main payload. Reactions, edits, pins keep thehidebehavior; onlyAdminRevokeflips tofalse.3.
prepare_{group,dm}_retry_stanzalost theeditattributeWhen a recipient device replies with
<receipt type=\"retry\">, the bot rebuilds the stanza via the retry builders and resends. Those builders never accepted or emitted aneditattribute, so the retry resend went out as a plain<message>. Client received it but had no signal that this was a revoke. Symptom: the group creator's device (which always retries because its session needs a prekey bundle on first contact) kept showing the revoked image.Fix:
edit: Option<EditAttribute>and emitattr(\"edit\", ...)when present.EditAttribute::infer_from_message(&wa::Message)helper recovers the wireeditvalue from the cached protobuf (add_recent_messageonly persists the protobuf body, not the original stanza's wire attributes):protocol_message.r#type == Revoke+key.from_me→SenderRevoke/AdminRevokepin_in_chat_message.is_some()→PinInChatprotocol_message.edited_message.is_some()→MessageEditretry.rsinfersedit_attrfromoriginal_msgand forwards it to both retry paths.Test plan
cargo build --release --target x86_64-unknown-linux-musl(the consumer profile) builds clean.test_decrypt_fail_hide_logic_for_editsinwacore/src/types/message.rsdocuments the expected rule and matches the new implementation.Happy to split into three smaller commits if reviewers prefer one fix per commit — kept consolidated here because the three bugs only become observably correct together for the admin-revoke flow.
🤖 Generated with Claude Code