fix(pair-code): correct link_code_pairing_nonce byte and close WA Web stage-2 gaps - #976
Conversation
The companion_hello IQ sent a `link_code_pairing_nonce` of `b"0"` (byte
0x30, the ASCII digit). The captured WA Web bundle (Alt/DeviceLinkingIq.js)
sends `new Uint8Array(1)` — a single 0x00 byte — and whatsmeow sends
`[]byte{0}`, so the previous "matching whatsmeow/baileys" comment was
inaccurate for whatsmeow. Emit the zero byte to match WA Web wire behavior.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EdZLyCWkYyEwPtgtdE6DFo
…dling Bring the phone-number linking companion flow in line with WA Web's DeviceLinkingApi / handleAltDeviceLinkingNotification: - Dispatch link_code_companion_reg notifications on the child `stage` attribute instead of assuming primary_hello. - Verify the primary_hello `link_code_pairing_ref` matches the ref cached from companion_hello (WA Web InvalidRefError); ignore mismatches. - Enforce the ~180s code-validity window (WA Web OldCodeError). - Allow up to 3 primary_hello attempts per code, re-deriving fresh key material each time, instead of completing after the first (WA Web MaxPrimaryHelloError, T=3). The state is retained after sending companion_finish; the terminal transition still happens on pair-success. - Handle the refresh_code stage: surface a new Event::PairingCodeRefresh (with force_manual) when the ref matches the outstanding flow, plus a Bot::on_pair_code_refresh convenience handler. Adds regression tests driving the top-level handler for each guard (ref mismatch, expired code, attempt cap, valid in-window retry proceeds) and for refresh_code dispatch/ignore and unknown-stage handling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdZLyCWkYyEwPtgtdE6DFo
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds PairingCodeRefresh handling end to end: event types, pair-code state and stage-2 guards, bot registration, and the stage-1 nonce wire-format change with matching tests. ChangesPairing code refresh and stage-2 retry handling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 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 |
There was a problem hiding this comment.
1 issue found across 4 files
Confidence score: 3/5
- In
src/pair_code.rs, the retry counter appears to increment before validatingprimary_helloreference/expiry, so stale or unrelated notifications can consume legitimate pair-code attempts and cause premature pairing failure for real users; move the increment to occur only after those checks pass (and add a regression test for stale-notification handling) before merging.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
… retry cap Review follow-up (#976): the per-code attempt counter was bumped before the ref and expiry checks, so a stale/foreign or late `primary_hello` — neither of which triggers a `companion_finish` — could silently exhaust the 3-try budget and cause the genuine attempt to be rejected as MaxPrimaryHelloError. WA Web increments first and shares that window; we now validate (matching ref + unexpired code) before spending a slot. Also tightens the pair-code comments to explain "why" per AGENTS.md, and adds regression coverage: the ref-mismatch and expired-code paths assert the counter stays at 0, plus a test that several stale mismatched hellos don't block the subsequent valid one from reaching stage 2. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdZLyCWkYyEwPtgtdE6DFo
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 `@src/pair_code.rs`:
- Around line 356-363: The retry budget in
PairCodeUtils::handle_primary_hello_attempts is being incremented before
checking the cap, so a rejected attempt still mutates state and can push the
counter past the limit. Reorder the logic to check primary_hello_attempt_count
against PairCodeUtils::max_primary_hello_attempts() before incrementing, and
only increment when the attempt is still allowed; keep the existing warn! and
false return path for exhausted retries.
🪄 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: d03c0a98-ef54-4fab-9fc0-de0f17f21c88
📒 Files selected for processing (2)
src/pair_code.rswacore/src/pair_code.rs
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ounter Review follow-up (#976): the attempt counter was bumped before the cap comparison, so a rejected over-cap notification still mutated state and let the counter grow unbounded past the limit. Compare against the max first (`>=`) and only increment when the attempt is still allowed, keeping the counter bounded at max. Accept/reject decisions are unchanged (3 processed, 4+ rejected); added an assertion that a rejected over-cap attempt leaves the counter at max. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdZLyCWkYyEwPtgtdE6DFo
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: This PR modifies core pairing protocol logic: nonce byte change, stage-2 notification dispatch with attempt/expiry gates, and a new event type. Even with zero AI issues, the blast radius of a bug in pairing flow (auth, retry, or state machine) is high and requires human judgment.
Re-trigger cubic
…race Adversarial review follow-up (#976). The `<notification>` transport dispatches each stanza on its own detached task (client/node_io.rs), so two `primary_hello` for the same code can run concurrently. The earlier switch from `mem::take` (atomic first-wins) to a non-consuming clone (needed for retries) dropped that atomicity: both notifications could pass the guard, derive a *different* random adv_secret, and race `SetAdvSecretKey` (last-write-wins) while sending divergent `companion_finish` bundles — leaving the persisted secret out of sync with the bundle the server acts on, so pair-success HMAC verification fails. Hold the `pair_code_state` lock across the whole of `handle_primary_hello` (derive → persist → send) so concurrent notifications are processed sequentially, matching WA Web's single-threaded model; the persisted adv_secret then always corresponds to the last companion_finish sent. `async_lock::Mutex` is async-aware, so holding it across the spawn_blocking PBKDF2 + IQ send is safe. Also, for exact WA Web parity, stamp `code_generation_ts` before companion_hello (WA Web `startAltLinkingFlow`) instead of after the stage-1 round-trip, and add a regression test that an absent `force_manual_refresh` maps to `force_manual: false`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdZLyCWkYyEwPtgtdE6DFo
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Review follow-up (#976). Now that code_generation_ts is stamped before the stage-1 round-trip (WA Web parity), the ~180s expiry the server (and handle_primary_hello) enforce is measured from that earlier instant. The PairingCode event, dispatched after stage 1, was still advertising the full code_validity(), so a consumer's countdown would outlast the real window by the stage-1 elapsed time and show a code as valid after it would already be rejected. Advertise the remaining window (code_validity - elapsed) instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdZLyCWkYyEwPtgtdE6DFo
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Changes to critical pairing logic: nonce byte, stage dispatch, ref validation, expiry, attempt limits, and new event/handler. Requires human review for correctness and security.
Re-trigger cubic
…anionOs WhatsApp's server validates companion_platform_display and rejects a non-OS string with bad-request — so a consumer that overrides DeviceProps::os for branding (e.g. "Veloz") could pair by QR but never by phone code. Empirically probed against the live server (fresh-device companion_hello, stage 1 is pre-auth): real OS names pass (Linux/Mac/macOS/Mac OS/Ubuntu/Fedora/Windows), non-OS strings and empty are rejected; the browser half and platform_id are validated too but always come from CompanionWebClientType, and bad-request is additionally returned on per-number rate-limiting (~5/number) — noted so callers don't misread it. Replace the free-form os passthrough with a typed CompanionOs enum (the single source of truth for the accepted set) + from_hint() that classifies the free-form DeviceProps::os, collapsing anything unrecognized to Linux (always accepted). QR / device-name branding is untouched — companion_platform_display is pair-code-only. Updates the two tests that encoded the old "Mac" passthrough (now "Mac OS") and adds alias-table + "Veloz"->Linux regressions. Builds on the nonce fix in PR #976.
What
Makes phone-number pair-code linking (
link_code_companion_reg) behave against the WhatsApp server, cross-checked against the captured WhatsApp Web bundle (WAWeb/Alt/DeviceLinking*.js,WA/Smax/*CompanionHello/Finish/RefreshCode*.js).The full crypto path (PBKDF2
2<<16, AES-CTR ephemeral wrap, HKDF arg order for the bundle key +adv_secret, GCM tag placement, the pair-success HMAC) was audited against the bundle and already matched WA Web — the changes below cover the deviations and the hardening found in an adversarial review.1.
link_code_pairing_noncewire bytecompanion_hellosent the nonce asb"0"(ASCII0x30). WA Web'sAlt/DeviceLinkingIq.jssendsnew Uint8Array(1)— a single0x00byte — and whatsmeow sends[]byte{0}. Now emits the zero byte.2. Stage-2 notification handling
handle_pair_code_notificationassumed every notification was aprimary_helloand marked the flowCompletedafter the first. Brought in line with WA WebDeviceLinkingApi.handlePrimaryHelloInternal/handleAltDeviceLinkingNotification:staget[0].attrs.stageprimary_hellovsrefresh_code(unknown → ignore)link_code_pairing_refInvalidRefErrorprimary_hellowhose ref ≠ the ref cached fromcompanion_helloOldCodeError(I=180)code_generation_ts(beforecompanion_hello, matchingstartAltLinkingFlow) and drops an out-of-windowprimary_helloMaxPrimaryHelloError(T=3)refresh_codestagerefreshAltLinkingCode/forceManualRefreshEvent::PairingCodeRefresh { force_manual }(ref-gated) +Bot::on_pair_code_refreshhelper3. Concurrency + event hardening (adversarial-review follow-ups)
notificationstanzas on concurrent detached tasks, so twoprimary_hellofor the same code could each derive a different randomadv_secretand raceSetAdvSecretKey(last-write-wins), desyncing the persisted secret from thecompanion_finishthe server acts on → pair-success HMAC failure. Thepair_code_statelock is now held across the whole of stage 2 (derive → persist → send), making concurrent notifications sequential — matching WA Web's single-threaded model.code_generation_tsnow starts before stage 1, thePairingCodeevent advertises the remaining window (not the fullcode_validity()), so a consumer's countdown can't outlast the real expiry.Testing
handle_pair_code_notification: ref mismatch (rejected, no retry slot spent, state preserved), expired code (rejected, no slot), over-cap 4th attempt (rejected, counter bounded at max), a valid in-window retry reaching stage 2, stale mismatches not blocking the valid one,refresh_codedispatch/ignore + absent-force_manualdefault, and unknown-stage ignore. The nonce byte is guarded by the wacorecompanion_hello_iq_shapetest.cargo test -p whatsapp-rust --lib pair_code(11) andcargo test -p wacore --lib pair_code(40) green;cargo fmt --all --checkandcargo clippy -p whatsapp-rust -p wacore --testsclean.Not in scope
No replication of WA Web's QPL markers or telemetry pings; no change to the QR pairing path or the pair-success crypto.