fix(history-sync): match our own pushname entry by JID user - #1203
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughPushname matching now normalizes bare IDs and phone JIDs, removes device suffixes, and rejects non-phone namespaces. Tests cover owner matches, another user’s ID, and rejected namespaces. ChangesPushname matching
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
| Filename | Overview |
|---|---|
| wacore/src/history_sync.rs | Correctly normalizes phone-namespace push-name JIDs for comparison and adds focused regression coverage. |
Reviews (3): Last reviewed commit: "fix(history-sync): match our own pushnam..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/history_sync.rs`:
- Around line 764-768: Update pushname_id_is to validate non-bare IDs through
the repository JID helper and require the supported phone namespace
s.whatsapp.net before comparing the user portion; preserve the existing bare-ID
compatibility path and device-suffix handling. Add tests covering rejection of
`@lid` and an invalid namespace while retaining acceptance of valid bare and
phone-namespace IDs.
🪄 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0a38344b-a9d2-4036-8c47-59ada5541f84
📒 Files selected for processing (1)
wacore/src/history_sync.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 1 file
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Focused bug fix: history-sync pushname IDs are JIDs, so the prior bare-user comparison dead-coded own_pushname; the diff compares on the user part and pins JID, bare-form, and foreign-entry cases with tests.
Re-trigger cubic
There was a problem hiding this comment.
Thanks for this one, and good to have you back after #1010.
I went and checked the server side before reading the diff. In the current WA Web bundle the history sync pushname handler does t.pushnames.forEach(e => WidFactory.createUserWidOrThrow(e.id || "")), so Pushname.id really is a WID and never a bare user. whatsmeow agrees, handleHistoricalPushNames parses it with types.ParseJID. Your premise holds, the old comparison could never match. I also ran the suite on your branch, 1337 passing.
Two things I would like changed before this goes in, both small:
-
Use
Jidinstead of splitting by hand.extract_lid_mappingright below (the one you added in #1010) already doesparse::<Jid>()and readsuser_base(), anduser_base()is exactly thesplit_once(':')your helper reimplements. Going through the same path keeps the two neighbours consistent and drops the manual string work. -
Reject the wrong namespace. Right now the server part is discarded entirely, so
15550000000@lidmatchesown_user = "15550000000". The odds are tiny, but it is a silent identity mixup and it costs nothing to avoid: once you have aJid, requiringis_pn() || server == Server::Legacyis the same checkextract_lid_mappingalready makes. No perf worry here, I checked that no allocation guard covers this path and it runs once per entry, not per message.
Two optional ones:
parity_fixturestill buildsid: Some(own.to_string())in the bare form, so the streaming vs full buffer parity test never sees a real JID. Would be nice to move it over too, it is the same unrealistic fixture your PR body calls out.- whatsmeow skips entries whose pushname is
-. I looked for that sentinel in the bundle and did not find it, so it is not a WA Web rule, but if the server ever sends it for our own entry we would store-and announce it. Your call.
Good find, and the "the test that hid it" section made this very easy to review.
For what it is worth, the red Semver Checks (informational) is not yours. It is a pre existing waproto diff against the published 0.6.0 and the job says outright that it does not block the PR.
27de4f8 to
40e9242
Compare
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
Fixed in The namespace check is a good catch, and it is not only a hardening detail — for a LID-migrated account the same digits can appear as both let Some((user, server)) = id.split_once('@') else {
return id == own_user; // pre-JID bare form, still accepted
};
server == wacore_binary::DEFAULT_USER_SERVER
&& user.split(':').next().unwrap_or(user) == own_userAdded I used
|
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/history_sync.rs`:
- Around line 774-777: Update the JID matching logic in
own_pushname_matches_a_jid_id to normalize a trailing numeric .device suffix for
s.whatsapp.net IDs, alongside the existing colon-suffix normalization. Confirm
the legacy dotted PN wire format from repository WhatsApp Web references, strip
only the valid numeric device suffix, and add the normalized ID to
own_pushname_matches_a_jid_id so dotted device JIDs match the base own user.
🪄 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8474ee11-5da6-48c2-b306-d3cf81547b4c
📒 Files selected for processing (1)
wacore/src/history_sync.rs
![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: 40e9242684
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }; | ||
| // A device suffix (`user:3`) never appears on this field, but stripping it | ||
| // costs nothing and keeps the comparison total. | ||
| server == wacore_binary::DEFAULT_USER_SERVER |
There was a problem hiding this comment.
When a history-sync pushname entry is encoded as the legacy PN JID form (<own>@c.us), this check rejects it because only s.whatsapp.net is allowed. The same module still treats @c.us as the legacy phone-number namespace for history-sync mappings, so those blobs would continue to leave own_pushname unset even though the user part matches; include the legacy PN server in this comparison rather than pinning it as a non-phone namespace.
Useful? React with 👍 / 👎.
| let end = pos.checked_add(len).filter(|&e| e <= data.len())?; | ||
| let id = smoothutf8::from_utf8(data.get(pos..end)?)?; | ||
| id_match = id == own_user; | ||
| id_match = pushname_id_is(id, own_user); |
There was a problem hiding this comment.
When this newly matches real JID-form pushname entries, an entry whose pushname is "-" now flows through as own_pushname and src/history_sync.rs persists it as the device push name. That value is the history-sync missing-name sentinel (whatsmeow skips those entries), but it is non-empty here, so the presence gate can announce name="-" instead of keeping the push name unset; filter the sentinel before accepting the match.
Useful? React with 👍 / 👎.
`HistorySync.pushnames[].id` is a JID string, `own_user` is the bare user part of our JID, and the two were compared verbatim — so the entry never matched and history sync could not supply our own push name. For an account whose critical app-state sync fails to deliver `setting_pushName` that is the only remaining automatic source, and it was dead. Compare on the user part. The unit test that covered this passed because it built `id` as a bare user, which the server never sends; it now uses a JID and a second case pins that another user's entry is not taken as our own.
40e9242 to
2760359
Compare
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
Both parser findings were right, and the second one was a bug I introduced in the previous round. Fixed in
The dotted device suffix is real too. Rather than patch two more cases by hand I switched to the repository parser, which is what the first review round asked for: let Some(jid) = wacore_binary::jid::parse_jid_ref(id) else { return false };
(jid.server.is_pn_family() || jid.server == Server::Legacy) && &*jid.user == own_userI had declined that on allocation grounds. That was wrong on the facts:
The |
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/history_sync.rs`:
- Line 805: Update the matching phone-JID path around pushname assignment,
including the corresponding logic at the other reported location, to treat
pushname "-" as absent and set own_pushname to None instead of Some("-"). Add a
regression case for a matching phone JID whose pushname is "-" and assert that
the result is None.
🪄 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 45ab3e50-3aad-46cc-9821-713ce4241b9c
📒 Files selected for processing (1)
wacore/src/history_sync.rs
| let end = pos.checked_add(len).filter(|&e| e <= data.len())?; | ||
| let id = smoothutf8::from_utf8(data.get(pos..end)?)?; | ||
| id_match = id == own_user; | ||
| id_match = pushname_id_is(id, own_user); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat the "-" pushname sentinel as absent.
When a matching phone JID has pushname == "-", this path returns Some("-") as the account name. Treat the sentinel as no pushname before setting own_pushname. Add a matching phone-JID regression case that asserts None.
Also applies to: 3083-3108
🤖 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/history_sync.rs` at line 805, Update the matching phone-JID path
around pushname assignment, including the corresponding logic at the other
reported location, to treat pushname "-" as absent and set own_pushname to None
instead of Some("-"). Add a regression case for a matching phone JID whose
pushname is "-" and assert that the result is None.
|
Please review the reviews and check the fact-checks; one review failed. |
3 participants
The bug
HistorySync.pushnames[].idis a JID string (15551234567@s.whatsapp.net).extract_own_pushnamecompares it verbatim againstown_user, which is the bare user part of our own JID (process_history_sync(compressed, device.pn.to_non_ad().user, …)insrc/history_sync.rs).The two can never be equal, so
own_pushnameis alwaysNoneand history sync cannot supply our own push name.whatsmeow parses the same field with
types.ParseJID(pushname.GetID())before comparing, which is the behaviour this restores.Why it matters
For a personal account the push name has exactly one other automatic source: the
setting_pushNamemutation in thecritical_blockapp-state collection (a business account also gets it frombusiness_nameat pairing). When that collection fails to deliver it, history sync is the only remaining path — and it was dead.Without a push name the client cannot send presence at all (
features/presence.rsreturnsPresenceError::PushNameEmpty), so the account sits connected and unable to announce itself.The test that hid it
test_nct_salt_and_pushname_coexistpassed because it builtid: Some("0000000000")— a bare user, which the server never sends. It now uses a JID, and two cases are added: a JID id and the bare form both match, and another user's entry is not taken as our own.Verification
cargo test -p wacore --lib(1337 passed),cargo clippy -p wacore --all-targets -- -D warnings,cargo fmt --all --check— all clean. Fixtures use fictitious numbers per AGENTS.md.Known limitation, deliberately not addressed here
The comparison is against the PN user only. For a LID-migrated account whose entry is keyed by the LID user this still won't match; fixing that means threading the own LID through
process_history_sync, which changes the signature and felt out of scope for a comparison fix.