feat: cstoken (NCT) privacy token fallback + proto update - #433
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces NCT (Nonce-based Content Token) salt support for privacy token management, including database schema additions, history sync extraction, deferred tc-token issuance with cs-token fallback generation, an outgoing node waiter system, app-state mutation handling, and comprehensive E2E test coverage. Changes
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 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 |
Bencher Report
|
@claude review |
|
Claude finished @jlucaso1's task in 2m 45s —— View job Code Review: cstoken (NCT) privacy token fallback + proto update
Overall this is a well-structured PR with clear WA Web cross-references, good test coverage, and clean separation across crate boundaries. A few observations: Duplicate LID resolution in
|
![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 (1)
storages/sqlite-storage/src/sqlite_store.rs (1)
55-76: 🧹 Nitpick | 🔵 TrivialConsider replacing
DeviceRowtuple with a named struct.This positional tuple is now very wide; a
#[derive(Queryable)]struct would reduce schema-order coupling and make future column additions safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storages/sqlite-storage/src/sqlite_store.rs` around lines 55 - 76, The DeviceRow type is a large positional tuple; replace it with a named struct (e.g., struct DeviceRow { id: i32, name: String, ... }) that derives Queryable (and maybe Debug/Clone) so fields are accessed by name instead of position. Update any usages of DeviceRow in sqlite_store.rs (constructors, pattern matches, mapping from query rows, and return types) to use the new struct field names, and adjust query deserialization to derive or implement Queryable for the struct to preserve compatibility with Diesel or your DB layer.
🤖 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 2634-2643: The app-state branch is persisting Some(vec![]) which
should be treated as absent; update the conditional handling of m.action_value
-> nct_salt_sync_action -> salt to reject empty salts (check salt.is_empty())
and only call
self.persistence_manager.process_command(DeviceCommand::SetNctSalt(Some(salt.clone())))
when salt is non-empty; if salt is empty, log a warning (similar to the existing
warn! for missing salt) and do not persist so the history-sync path can later
backfill a real salt.
In `@storages/sqlite-storage/migrations/2026-03-23-000000_add_nct_salt/down.sql`:
- Line 36: Replace the rollback INSERT that uses "INSERT INTO device SELECT *
FROM device_backup;" with an INSERT ... SELECT that specifies explicit column
names for both target and source (e.g., "INSERT INTO device (col1, col2, ...)
SELECT col1, col2, ... FROM device_backup") so the migration remains correct if
column order or schema changes; update the column list to match the current
device table columns and ensure the same order is used in the SELECT from
device_backup.
In `@wacore/src/history_sync.rs`:
- Around line 260-269: The test uses a phone-like value "5511999990000" in the
Pushname.id and the process_history_sync call; replace both occurrences with an
obviously synthetic identifier (e.g., "0000000000" or "example-jid-1") to avoid
PII. Locate the pushnames construction (wa::Pushname with id) and the
process_history_sync::<fn(Bytes)> invocation and update the id string and the
second argument to the same clearly fictitious value so the test remains
functionally identical but contains no phone-shaped data.
In `@wacore/src/store/device.rs`:
- Around line 161-165: The doc comment expands "NCT" as "Neuro-Computed Token"
which is misleading; update the comment above the field (the block that mentions
NCT, cstoken = HMAC-SHA256(salt, recipient_lid), WAWebNctSaltSync and the
#[serde(default)] attribute) to remove the expansion and instead use a neutral
phrase like "NCT salt" or the protocol's canonical term throughout the comment
while preserving the remaining technical details (usage, source, and fallback
behavior).
---
Outside diff comments:
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 55-76: The DeviceRow type is a large positional tuple; replace it
with a named struct (e.g., struct DeviceRow { id: i32, name: String, ... }) that
derives Queryable (and maybe Debug/Clone) so fields are accessed by name instead
of position. Update any usages of DeviceRow in sqlite_store.rs (constructors,
pattern matches, mapping from query rows, and return types) to use the new
struct field names, and adjust query deserialization to derive or implement
Queryable for the struct to preserve compatibility with Diesel or your DB layer.
🪄 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: acdaf5e1-9e47-4857-9a3b-7dad07c6cd83
📒 Files selected for processing (16)
src/client.rssrc/features/chat_actions.rssrc/features/polls.rssrc/history_sync.rssrc/send.rsstorages/sqlite-storage/migrations/2026-03-23-000000_add_nct_salt/down.sqlstorages/sqlite-storage/migrations/2026-03-23-000000_add_nct_salt/up.sqlstorages/sqlite-storage/src/schema.rsstorages/sqlite-storage/src/sqlite_store.rswacore/noise/src/handshake.rswacore/src/history_sync.rswacore/src/iq/tctoken.rswacore/src/store/commands.rswacore/src/store/device.rswaproto/src/whatsapp.protowaproto/src/whatsapp.rs
Update proto to WhatsApp Web 2.3000.1035617621 (from wppconnect wa-proto) and implement the cstoken fallback for first-contact messaging. When no tctoken exists for a recipient, whatsapp-rust now computes cstoken = HMAC-SHA256(nct_salt, recipient_lid) matching WA Web's genCsTokenBody in MsgCreateFanoutStanza.js. NCT salt is received via two paths: - History sync (HistorySync.nctSalt field 19) during initial pairing - App state sync (NctSaltSyncAction field 80) for ongoing updates This addresses error 463 for first-message and post-restriction scenarios.
…ut, known-answer test - Resolve LID once in maybe_include_tc_token and reuse for tctoken lookup and cstoken HMAC (was resolved twice via lid_pn_cache) - Use Jid::new(user, "lid") consistently for HMAC input to strip device suffix, matching WA Web's accountLid.toString() - Add known-answer HMAC-SHA256 test to catch accidental algorithm changes (e.g., swapped key/data arguments)
…hetic test data - Reject empty salt bytes in nct_salt_sync handler (prevents persisting Some(vec![]) which would block history sync backfill) - Use explicit column names in down migration INSERT instead of SELECT * - Replace phone-shaped test data with synthetic "0000000000" - Remove misleading "Neuro-Computed Token" expansion from doc comment
e1f5489 to
2d86cd8
Compare
|
@claude review |
|
Claude finished @jlucaso1's task in 2m 48s —— View job Code Review: cstoken (NCT) privacy token fallback + proto update
Well-structured PR with clean separation across crate boundaries. The cstoken computation is correct, the fallback chain is clear, and test coverage is solid (including a known-answer HMAC test). Here are my findings: Looks Good
Minor Observations (non-blocking)
VerdictClean implementation that follows the crate architecture well. The cstoken HMAC input format is consistent, empty salts are properly rejected, and the fallback chain matches the documented WA Web behavior. The observations above are minor hardening suggestions, not blockers. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/send.rs (1)
1141-1149:⚠️ Potential issue | 🟠 MajorDon't advance
sender_timestampbefore the refresh actually succeeds.This write happens before both
send_node()andissue_tc_token_after_send(). If the send fails, the cached entry still looks freshly re-issued, so later sends in the same bucket skip the refresh even though the recipient never got a new tc token. Move thesender_timestampupdate into the successful post-send store path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/send.rs` around lines 1141 - 1149, The current logic calls backend.put_tc_token to update TcTokenEntry.sender_timestamp when should_send_new_tc_token(...) is true before performing send_node() and issue_tc_token_after_send(), which can mark the token as refreshed even if the send fails; change this by removing the pre-send update and instead update the cached TcTokenEntry.sender_timestamp only after a successful send and after issue_tc_token_after_send() completes (i.e., in the post-send store path), using the same TcTokenEntry shape and backend.put_tc_token call so that should_send_new_tc_token and subsequent sends behave correctly.
♻️ Duplicate comments (2)
src/history_sync.rs (1)
261-268:⚠️ Potential issue | 🔴 CriticalPrevent stale history-sync salt from overwriting newer mutation state.
Line 261 triggers an unconditional write. If
nct_salt_syncclear/replace happens first, this can resurrect stale salt and break token correctness. Make this an atomic, precedence-aware command (history-sync should not overwrite a newer mutation-derived value).Suggested direction
- self.persistence_manager - .process_command(wacore::store::commands::DeviceCommand::SetNctSalt(Some( - salt, - ))) - .await; + self.persistence_manager + .process_command( + wacore::store::commands::DeviceCommand::SetNctSaltFromHistorySync { salt }, + ) + .await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/history_sync.rs` around lines 261 - 268, The current code unconditionally writes sync_result.nct_salt via self.persistence_manager.process_command(DeviceCommand::SetNctSalt(...)), which can resurrect stale history-sync salt over a newer mutation-derived value; change this to a precedence-aware update by either (a) reading the current device salt/metadata first (via the persistence manager query) and only issuing DeviceCommand::SetNctSalt when the stored value is absent or older than the history-sync value, or (b) add a new atomic command variant (e.g., DeviceCommand::SetNctSaltIfNotNewer or similar) implemented in the persistence layer that compares timestamps/sequence numbers and only sets when it will not overwrite a newer mutation-derived salt; update the call site to use that conditional/atomic command instead of the unconditional SetNctSalt.src/send.rs (1)
1136-1140:⚠️ Potential issue | 🟠 MajorTreat zero-length tc tokens as missing.
An empty cached or newly issued token still enters the valid-token branch, attaches
<tctoken>, and suppresses the cstoken fallback. Persisting empty responses here makes that state sticky across later sends. Gate the branch on!entry.token.is_empty()and skip storing empty tokens.Also applies to: 1196-1205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/send.rs` around lines 1136 - 1140, The cached-token branch treats zero-length tokens as valid; change the match arm for Some(entry) to also require !entry.token.is_empty() (i.e., Some(entry) if !is_tc_token_expired(entry.token_timestamp) && !entry.token.is_empty()) before pushing build_tc_token_node into extra_nodes, and likewise update the code path that persists newly issued tokens (the branch around the second block noted) to skip writing/storing empty entry.token values so empty tokens are never saved or used.
🤖 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 290-292: cleanup_connection_state() currently leaves
transport-scoped SentNodeWaiter entries live; grab the sent_node_waiters mutex
inside cleanup_connection_state() and cancel/resolve all pending SentNodeWaiter
instances (and clear the Vec) so they cannot match against a later connection,
and decrement/reset sent_node_waiter_count accordingly; apply the same pattern
wherever connection teardown logic runs (areas referencing sent_node_waiters /
sent_node_waiter_count / SentNodeWaiter) to ensure each waiter is
notified/faulted and removed on disconnect.
In `@tests/e2e/tests/privacy_tokens.rs`:
- Around line 372-389: The salt-negative assertions are using the PN JID from
TestClient::jid() (jid_a) which can be unresolved; instead obtain the
recipient's LID via the TestClient API (call client_b.client.get_lid(...) or the
existing get_lid helper for client_b) and assert against the sent node using
that LID—e.g., resolve the LID for the recipient before checking the sent node
stored in `sent` and replace has_child(&sent, "tctoken") / has_child(&sent,
"cstoken") checks to use the resolved LID so the test fails on missing salt
rather than missing identity; apply the same LID-based change to the other
occurrences mentioned (the blocks around the other line ranges).
In `@wacore/src/iq/tctoken.rs`:
- Around line 524-572: Tests for compute_cs_token use device-qualified numeric
LIDs that resemble real phone numbers; replace those with synthetic bare LIDs to
match production input shape and avoid PII. Update occurrences in tests
test_compute_cs_token_deterministic, test_compute_cs_token_different_lids,
test_compute_cs_token_different_salts, and test_compute_cs_token_known_answer to
use fictitious bare JIDs (e.g. "alice@lid" and "bob@lid" or "user@lid") instead
of values like "100000000000001:67@lid" or "236395184570386@lid"; ensure the
expected value in test_compute_cs_token_known_answer is recomputed to match the
new salt+lid combination, and keep build_cs_token_node test unchanged except for
using synthetic input if needed.
---
Outside diff comments:
In `@src/send.rs`:
- Around line 1141-1149: The current logic calls backend.put_tc_token to update
TcTokenEntry.sender_timestamp when should_send_new_tc_token(...) is true before
performing send_node() and issue_tc_token_after_send(), which can mark the token
as refreshed even if the send fails; change this by removing the pre-send update
and instead update the cached TcTokenEntry.sender_timestamp only after a
successful send and after issue_tc_token_after_send() completes (i.e., in the
post-send store path), using the same TcTokenEntry shape and
backend.put_tc_token call so that should_send_new_tc_token and subsequent sends
behave correctly.
---
Duplicate comments:
In `@src/history_sync.rs`:
- Around line 261-268: The current code unconditionally writes
sync_result.nct_salt via
self.persistence_manager.process_command(DeviceCommand::SetNctSalt(...)), which
can resurrect stale history-sync salt over a newer mutation-derived value;
change this to a precedence-aware update by either (a) reading the current
device salt/metadata first (via the persistence manager query) and only issuing
DeviceCommand::SetNctSalt when the stored value is absent or older than the
history-sync value, or (b) add a new atomic command variant (e.g.,
DeviceCommand::SetNctSaltIfNotNewer or similar) implemented in the persistence
layer that compares timestamps/sequence numbers and only sets when it will not
overwrite a newer mutation-derived salt; update the call site to use that
conditional/atomic command instead of the unconditional SetNctSalt.
In `@src/send.rs`:
- Around line 1136-1140: The cached-token branch treats zero-length tokens as
valid; change the match arm for Some(entry) to also require
!entry.token.is_empty() (i.e., Some(entry) if
!is_tc_token_expired(entry.token_timestamp) && !entry.token.is_empty()) before
pushing build_tc_token_node into extra_nodes, and likewise update the code path
that persists newly issued tokens (the branch around the second block noted) to
skip writing/storing empty entry.token values so empty tokens are never saved or
used.
🪄 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: 7fc437a0-d902-4c83-a7ee-5b317a743cbb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
src/client.rssrc/history_sync.rssrc/send.rsstorages/sqlite-storage/migrations/2026-03-23-000000_add_nct_salt/down.sqlstorages/sqlite-storage/migrations/2026-03-23-000000_add_nct_salt/up.sqlstorages/sqlite-storage/src/schema.rsstorages/sqlite-storage/src/sqlite_store.rstests/e2e/Cargo.tomltests/e2e/src/lib.rstests/e2e/tests/privacy_tokens.rswacore/src/history_sync.rswacore/src/iq/tctoken.rswacore/src/store/commands.rswacore/src/store/device.rs
| /// Waiters for raw outgoing nodes before encryption. | ||
| sent_node_waiters: std::sync::Mutex<Vec<SentNodeWaiter>>, | ||
| sent_node_waiter_count: AtomicUsize, |
There was a problem hiding this comment.
Cancel pending sent-node waiters on disconnect.
These waiters are transport-scoped, but cleanup_connection_state() never drops them. If the socket dies before a match arrives, the receiver can stay pending across reconnects and then resolve against a later stanza from the wrong connection.
Suggested cleanup
async fn cleanup_connection_state(&self) {
+ {
+ let mut waiters = self
+ .sent_node_waiters
+ .lock()
+ .unwrap_or_else(|poisoned| poisoned.into_inner());
+ if !waiters.is_empty() {
+ waiters.clear(); // dropping senders cancels receivers
+ self.sent_node_waiter_count.store(0, Ordering::Release);
+ }
+ }
+
self.is_logged_in.store(false, Ordering::Relaxed);
self.is_ready.store(false, Ordering::Relaxed);Also applies to: 593-594, 2994-3011, 3037-3055, 3208-3210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client.rs` around lines 290 - 292, cleanup_connection_state() currently
leaves transport-scoped SentNodeWaiter entries live; grab the sent_node_waiters
mutex inside cleanup_connection_state() and cancel/resolve all pending
SentNodeWaiter instances (and clear the Vec) so they cannot match against a
later connection, and decrement/reset sent_node_waiter_count accordingly; apply
the same pattern wherever connection teardown logic runs (areas referencing
sent_node_waiters / sent_node_waiter_count / SentNodeWaiter) to ensure each
waiter is notified/faulted and removed on disconnect.
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 52s —— View job Code Review: cstoken (NCT) privacy token fallback + proto update
This is a well-executed PR. The previous two reviews already caught the main issues (empty salt persistence, empty tctoken suppressing cstoken, history sync TOCTOU, PII in tests), and those have all been addressed. The current state of the code is solid. Here's what I found in this pass: Addressed from prior reviews (confirmed fixed)
Remaining observations1.
In practice this is unlikely to cause issues because: (a) history sync typically only delivers during initial pairing, not on reconnect, and (b) if the server cleared the salt, it probably won't re-deliver it via history sync. But it's worth noting as a semantic gap. If you want belt-and-suspenders, you could persist 2.
3. Clean extraction that persists PN→LID mappings from both 4. Benchmark alerts are noise The 2 Bencher alerts ( Minor nit
VerdictThe implementation is clean and production-ready. All prior review findings have been addressed. The fallback chain ( |
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 32s —— View job Code Review: cstoken (NCT) privacy token fallback + proto update
Fourth pass — all prior review findings have been addressed. The code is in good shape. Here's my assessment of the current state: Confirmed Fixed (from prior reviews)
One Minor Remaining Issue (non-blocking)
The main send path ( Ok(Some(entry)) if !is_tc_token_expired(entry.token_timestamp) => Some(entry.token),Since
|
![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: 99c3d72267
ℹ️ 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 should_issue_after_send = | ||
| should_send_new_tc_token(existing.as_ref().and_then(|entry| entry.sender_timestamp)); |
There was a problem hiding this comment.
should_issue_after_send is computed only from sender_timestamp, so an expired cached token can suppress reissuance if the sender timestamp is still in the current bucket. In that case the code falls through to cstoken/no-token and never calls IssuePrivacyTokensSpec until the next bucket boundary (up to 7 days), leaving an expired tc-token in storage and breaking tc-token-gated flows during that window (e.g., presence/profile-picture access or tc-only scenarios). Expired/empty tokens should force immediate reissue regardless of sender bucket state.
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/send.rs`:
- Around line 1085-1090: The code currently calls
mark_tc_token_used_after_send(&token_key).await regardless of whether
issue_tc_token_after_send(...) actually succeeded; change the flow so that
mark_tc_token_used_after_send is only invoked when issue_tc_token_after_send
returned a successful, non-empty issuance (or alternatively let
store_issued_tc_tokens() be responsible for stamping sender_timestamp only on
success). Concretely, check the result/value returned by
issue_tc_token_after_send (or the issued token container) and gate the call to
mark_tc_token_used_after_send on that success; apply the same guard to the other
similar blocks that call issue_tc_token_after_send and
mark_tc_token_used_after_send.
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 567-568: The code always sets nct_salt_sync_seen = false when
hydrating state (using row.nct_salt), which loses the persisted
“authoritative-clear seen” information and lets SetNctSaltFromHistorySync
resurrect stale salts; update the storage/schema to persist a separate
seen/source bit (or tri-state) (e.g. add/persist row.nct_salt_sync_seen or
row.nct_salt_state), ensure SetNctSalt/SetNctSalt(None) correctly writes that
bit/state, and in sqlite_store.rs hydrate nct_salt_sync_seen from the DB row
instead of hard-coding false so the load path preserves the prior seen/source
value.
🪄 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: e341b84a-12d4-43c9-8535-662ddd7a93eb
📒 Files selected for processing (9)
src/client.rssrc/features/contacts.rssrc/history_sync.rssrc/send.rsstorages/sqlite-storage/src/sqlite_store.rstests/e2e/tests/privacy_tokens.rswacore/src/iq/tctoken.rswacore/src/store/commands.rswacore/src/store/device.rs
| nct_salt: row.nct_salt, | ||
| nct_salt_sync_seen: false, |
There was a problem hiding this comment.
Persist the authoritative-clear state across restarts.
This load path always recreates nct_salt_sync_seen = false. If a prior SetNctSalt(None) is already reflected in SQLite and the app-state patch is not replayed after restart, the device becomes indistinguishable from “never saw app-state”, so SetNctSaltFromHistorySync can resurrect a stale salt. Persist a separate seen/source bit (or a tri-state field) and hydrate it here instead of hard-coding false.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@storages/sqlite-storage/src/sqlite_store.rs` around lines 567 - 568, The code
always sets nct_salt_sync_seen = false when hydrating state (using
row.nct_salt), which loses the persisted “authoritative-clear seen” information
and lets SetNctSaltFromHistorySync resurrect stale salts; update the
storage/schema to persist a separate seen/source bit (or tri-state) (e.g.
add/persist row.nct_salt_sync_seen or row.nct_salt_state), ensure
SetNctSalt/SetNctSalt(None) correctly writes that bit/state, and in
sqlite_store.rs hydrate nct_salt_sync_seen from the DB row instead of
hard-coding false so the load path preserves the prior seen/source value.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/send.rs (1)
1085-1090:⚠️ Potential issue | 🟠 MajorDon't advance
sender_timestampunless the refresh actually succeeded.
issue_tc_token_after_send()returns early on IQ failure, andstore_issued_tc_tokens()already drops empty tokens, but the cached entry still gets stamped whenever a refresh was scheduled. One transient failure will therefore suppress retries for the rest of the bucket and leave the peer without a fresh tc-token. Return a success flag from the refresh path and gatemark_tc_token_used_after_send()on it.Patch sketch
- if should_issue_tc_token_after_send { - self.issue_tc_token_after_send(&tc_issue_target).await; - } - if let Some(token_key) = used_cached_tc_token_key { + let refresh_succeeded = if should_issue_tc_token_after_send { + self.issue_tc_token_after_send(&tc_issue_target).await + } else { + false + }; + if refresh_succeeded && let Some(token_key) = used_cached_tc_token_key { self.mark_tc_token_used_after_send(&token_key).await; }- async fn issue_tc_token_after_send(&self, to: &Jid) { + async fn issue_tc_token_after_send(&self, to: &Jid) -> bool { use wacore::iq::tctoken::IssuePrivacyTokensSpec; let to_lid = self.resolve_to_lid_jid(to).await; let Ok(response) = self .execute(IssuePrivacyTokensSpec::new(std::slice::from_ref(&to_lid))) .await else { log::debug!(target: "Client/TcToken", "Failed to issue tc_token for {}", to_lid); - return; + return false; }; - self.store_issued_tc_tokens(&response.tokens).await; + self.store_issued_tc_tokens(&response.tokens).await } - async fn store_issued_tc_tokens(&self, tokens: &[wacore::iq::tctoken::ReceivedTcToken]) { + async fn store_issued_tc_tokens( + &self, + tokens: &[wacore::iq::tctoken::ReceivedTcToken], + ) -> bool { use wacore::store::traits::TcTokenEntry; if tokens.is_empty() { - return; + return false; } let backend = self.persistence_manager.backend(); let now = wacore::time::now_secs(); + let mut stored_any = false; for received in tokens { if received.token.is_empty() { log::warn!(target: "Client/TcToken", "Server returned empty tc_token for {}, skipping", received.jid); continue; } @@ let store_jid = received.jid.user.clone(); if let Err(e) = backend.put_tc_token(&store_jid, &entry).await { log::warn!(target: "Client/TcToken", "Failed to store issued tc_token: {e}"); + } else { + stored_any = true; } } + stored_any }Also applies to: 1179-1247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/send.rs` around lines 1085 - 1090, The refresh path is stamping cached entries even when the TC refresh failed; change issue_tc_token_after_send to return a bool (or Result) indicating success and only call mark_tc_token_used_after_send(&token_key).await when that success flag is true; keep should_issue_tc_token_after_send/used_cached_tc_token_key logic but gate the mark on the returned success, and update both refresh locations (the blocks around issue_tc_token_after_send/mark_tc_token_used_after_send, including the other occurrence covering the 1179-1247 region); no other behavior changes—store_issued_tc_tokens can remain as-is.storages/sqlite-storage/src/sqlite_store.rs (1)
567-568:⚠️ Potential issue | 🟠 MajorPersist the authoritative-clear bit across restarts.
nct_salt_sync_seenis reconstructed asfalsehere, so a priorSetNctSalt(None)becomes indistinguishable from “never saw app-state” after restart. That letsSetNctSaltFromHistorySyncresurrect a stale salt even though app-state is supposed to stay authoritative. Please persist a separate seen/source bit (or a tri-state field) and hydrate it instead of hard-codingfalse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storages/sqlite-storage/src/sqlite_store.rs` around lines 567 - 568, The code currently reconstructs nct_salt_sync_seen as false when hydrating rows, losing the prior "authoritative-clear" state; update the storage schema to persist a separate seen/source flag (or tri-state) for the salt, read that persisted column instead of hard-coding false in the hydrate path (the struct field nct_salt_sync_seen), and adjust the logic in SetNctSalt(None) / SetNctSaltFromHistorySync handlers to honor the persisted flag so a prior explicit clear remains authoritative across restarts; ensure all references to nct_salt and nct_salt_sync_seen are updated to use the new persisted bit/tri-state and migrate existing rows appropriately.
🤖 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 3005-3010: Cleanup must not unconditionally store(0) into
sent_node_waiter_count because wait_for_sent_node() increments the counter
before locking sent_node_waiters, which can race with cleanup and leave the
counter stale. Instead, when clearing or draining the sent_node_waiters vector
(inside the mutex protected block), compute how many entries you removed and
decrement sent_node_waiter_count accordingly (e.g. fetch_sub(removed_len,
Ordering::Release)) rather than store(0); locate the cleanup logic around
sent_node_waiters and sent_node_waiter_count and replace the unconditional
store(0) with a fetch_sub of the removed count so send_node(),
wait_for_sent_node(), and resolve_sent_node_waiters() remain consistent.
- Around line 1063-1065: cleanup_connection_state currently clears sent-node
waiters but does not cancel transport-scoped node waiters; add a call to a new
helper (e.g., clear_node_waiters) from cleanup_connection_state so that
node_waiters are locked, counted, cleared, and node_waiter_count is decremented
by the number cleared (use lock().unwrap_or_else(|p| p.into_inner()) and
fetch_update with saturating_sub). Update clear_sent_node_waiters to mirror the
same pattern for consistency and ensure wait_for_node cannot survive a teardown
by referencing the node_waiters and node_waiter_count fields in the helper and
invoking it from cleanup_connection_state alongside clear_sent_node_waiters and
is_logged_in.store(false, Ordering::Relaxed).
In `@src/features/contacts.rs`:
- Around line 36-45: The code currently filters invalid jid/lid pairs at the
callsite, but the invariant should live inside the persistence API so all
writers are protected; add validation logic at the start of
Client::add_lid_pn_mapping (or implement a shared validator function used by
that method) to assert/judge that the provided jid.is_pn() and lid.is_lid() are
true and return a descriptive error if not, and update callers (e.g., sites that
call add_lid_pn_mapping from usync and pair paths) to handle the returned error
instead of relying on local guards.
- Around line 48-52: The log::warn! call emitting "jid" and "lid" in
src/features/contacts.rs should not print raw identifiers; replace the direct
jid and lid interpolation with redacted or hashed representations (e.g., call a
helper like redact_jid(jid) and redact_id(lid) or compute a short hash) and use
those redacted values in the log::warn! invocation so the warning still conveys
context without leaking user-identifying data.
In `@tests/e2e/tests/privacy_tokens.rs`:
- Around line 960-1007: Add explicit assertions that "cstoken" is not present on
the tctoken-only endpoints: after you obtain denied_node and allowed_node, call
has_descendant(&denied_node, "cstoken") and has_descendant(&allowed_node,
"cstoken") and assert they are false (e.g. assert!(!has_descendant(...,
"cstoken"))). Place these checks alongside the existing tctoken assertions in
the denied and allowed branches (using the same denied_node and allowed_node
variables) so the test fails if a cstoken is ever attached to
profile-picture/presence IQs.
In `@wacore/src/store/commands.rs`:
- Around line 60-63: The DeviceCommand::SetNctSaltFromHistorySync arm currently
accepts and stores empty salts (Some(vec![])), violating the contract that empty
salts should be ignored; change the handler in commands.rs so that when matching
DeviceCommand::SetNctSaltFromHistorySync(salt) you only set device.nct_salt =
Some(salt) if salt is non-empty and device.nct_salt_sync_seen is false and
device.nct_salt.is_none(); otherwise leave nct_salt unchanged and still
mark/handle nct_salt_sync_seen as appropriate. Update or add a unit test that
invokes the SetNctSaltFromHistorySync command with an empty Vec and asserts that
device.nct_salt remains None (regression test for the empty-salt case).
---
Duplicate comments:
In `@src/send.rs`:
- Around line 1085-1090: The refresh path is stamping cached entries even when
the TC refresh failed; change issue_tc_token_after_send to return a bool (or
Result) indicating success and only call
mark_tc_token_used_after_send(&token_key).await when that success flag is true;
keep should_issue_tc_token_after_send/used_cached_tc_token_key logic but gate
the mark on the returned success, and update both refresh locations (the blocks
around issue_tc_token_after_send/mark_tc_token_used_after_send, including the
other occurrence covering the 1179-1247 region); no other behavior
changes—store_issued_tc_tokens can remain as-is.
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 567-568: The code currently reconstructs nct_salt_sync_seen as
false when hydrating rows, losing the prior "authoritative-clear" state; update
the storage schema to persist a separate seen/source flag (or tri-state) for the
salt, read that persisted column instead of hard-coding false in the hydrate
path (the struct field nct_salt_sync_seen), and adjust the logic in
SetNctSalt(None) / SetNctSaltFromHistorySync handlers to honor the persisted
flag so a prior explicit clear remains authoritative across restarts; ensure all
references to nct_salt and nct_salt_sync_seen are updated to use the new
persisted bit/tri-state and migrate existing rows appropriately.
🪄 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: f419c22b-cc43-4987-a511-0a89ef4f3826
📒 Files selected for processing (9)
src/client.rssrc/features/contacts.rssrc/history_sync.rssrc/send.rsstorages/sqlite-storage/src/sqlite_store.rstests/e2e/tests/privacy_tokens.rswacore/src/iq/tctoken.rswacore/src/store/commands.rswacore/src/store/device.rs
| if !jid.is_pn() || !lid.is_lid() { | ||
| continue; | ||
| } | ||
| if let Err(err) = self | ||
| .client | ||
| .add_lid_pn_mapping( | ||
| &lid.user, | ||
| &jid.user, | ||
| crate::lid_pn_cache::LearningSource::Usync, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Enforce PN/LID validation in the persistence API, not only at this callsite.
This helper filters invalid pairs, but other write paths (src/usync.rs:38-46, src/pair.rs:250-256) persist without the same guard. Move/duplicate this invariant into Client::add_lid_pn_mapping (or a single shared validator) so cache integrity is consistent regardless of caller.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/contacts.rs` around lines 36 - 45, The code currently filters
invalid jid/lid pairs at the callsite, but the invariant should live inside the
persistence API so all writers are protected; add validation logic at the start
of Client::add_lid_pn_mapping (or implement a shared validator function used by
that method) to assert/judge that the provided jid.is_pn() and lid.is_lid() are
true and return a descriptive error if not, and update callers (e.g., sites that
call add_lid_pn_mapping from usync and pair paths) to handle the returned error
instead of relying on local guards.
| log::warn!( | ||
| "Failed to persist usync LID mapping {} -> {}: {err}", | ||
| jid, | ||
| lid | ||
| ); |
There was a problem hiding this comment.
Redact JIDs in warning logs to avoid identifier leakage.
Line 49 logs full jid/lid values on failure. These identifiers can contain user-linked data and should not be emitted in plain logs.
Suggested fix
- log::warn!(
- "Failed to persist usync LID mapping {} -> {}: {err}",
- jid,
- lid
- );
+ log::warn!("Failed to persist usync LID mapping (jid/lid redacted): {err}");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log::warn!( | |
| "Failed to persist usync LID mapping {} -> {}: {err}", | |
| jid, | |
| lid | |
| ); | |
| log::warn!("Failed to persist usync LID mapping (jid/lid redacted): {err}"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/contacts.rs` around lines 48 - 52, The log::warn! call emitting
"jid" and "lid" in src/features/contacts.rs should not print raw identifiers;
replace the direct jid and lid interpolation with redacted or hashed
representations (e.g., call a helper like redact_jid(jid) and redact_id(lid) or
compute a short hash) and use those redacted values in the log::warn! invocation
so the warning still conveys context without leaking user-identifying data.
- Fix race in clear_sent_node_waiters: use fetch_sub(count) instead of store(0) to avoid counter desync with concurrent wait_for_sent_node - Drain node_waiters on disconnect: add clear_node_waiters() called from cleanup_connection_state so incoming-node waiters don't survive teardown - Gate sender_timestamp mark on issuance success: match WA Web's TcTokenChatAction.js which only updates tcTokenSenderTimestamp after issuePrivacyToken resolves, allowing retry on next send if IQ failed - Reject empty salt in SetNctSaltFromHistorySync command handler - Assert cstoken absence on profile picture and presence IQ tests
node_waiters are cross-connection — callers may register a waiter before an action whose response arrives on a subsequent connection (e.g. after 515 reconnect). Clearing them in cleanup_connection_state caused the connection e2e tests to fail because the PairSuccess/Connected event flow spans the initial connect + 515 reconnect cycle. sent_node_waiters are correctly cleared since they match pre-encryption outgoing stanzas which are transport-scoped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cb0f7f563
ℹ️ 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".
| props_hash: row.props_hash, | ||
| next_pre_key_id: row.next_pre_key_id as u32, | ||
| nct_salt: row.nct_salt, | ||
| nct_salt_sync_seen: false, |
There was a problem hiding this comment.
nct_salt_sync_seen is always reloaded as false, so a previously authoritative remove (SetNctSalt(None)) is forgotten after process restart. Because SetNctSaltFromHistorySync only checks !nct_salt_sync_seen && nct_salt.is_none(), any later history-sync payload containing nct_salt will repopulate a salt that had been explicitly removed, causing stale cstoken behavior until another syncd mutation arrives. Persisting a remove tombstone (or equivalent) is needed to keep the “do not resurrect after remove” guarantee across restarts.
Useful? React with 👍 / 👎.
1 participant
Summary
Update the proto to WhatsApp Web
2.3000.1035617621and add thecstoken/ NCT fallback path for privacy-token-gated 1:1 messaging.This completes the send-side privacy-token chain to match WA Web:
Where:
tctoken: server-issued trusted-contact token from prior interactioncstoken:HMAC-SHA256(nct_salt, recipient_account_lid)using the bareuser@lidWhy this matters
WhatsApp's privacy-token rollout gates access to sensitive endpoints behind proof of relationship. Without a valid token, the server rejects requests with
463.tctokenwas already implemented. This PR adds the missingcstokenfallback so first-contact messaging can succeed when the server has provisionednct_salt, even before any trusted-contact token exists.Scope
waproto: proto update addsNctSaltSyncActionandHistorySync.nctSaltwacore/store: persistnct_saltwacore/history_sync: parseHistorySync.nctSaltsrc/client.rs: handlenct_salt_syncset/remove mutationssrc/send.rs: attachcstokenwhen no usabletctokenexists and the recipient LID can be resolvedsqlite-storage: addnct_saltmigration/schema persistencetests/e2e: add coverage for provisioned/unprovisioned salt flows and token-gated endpointsImportant behavior / precedence
nct_salt_syncis authoritative.cstokenfallback on later sends.Endpoint behavior
tctoken, elsecstokenifnct_salt+ recipient LID are available, else no token / server may return463tctoken-gatedtctoken-gatedE2E coverage added
463cstokencstokennct_saltsurvives reconnect and still enables first contactcstokenafter PN -> LID resolutiontctoken-only reply path remains valid whencstokenis disabledtctoken-gatedLive availability still depends on server AB props / rollout (
wa_nct_token_*). Accounts that are not provisioned withnct_saltwill still follow the no-salt /463branch.Validation
cargo test -p wacore --libcargo test -p whatsapp-rust --libcargo test -p e2e-tests --no-run -qnct_saltvia server rolloutSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests