fix(pair,props): enforce ADV HMAC verify and round-trip <prop> children - #594
Conversation
Two correctness bugs surfaced while wiring an in-process "phone simulator" in mock-server (bartender) that produces pair-success with real HMACs. pair.rs: `do_pair_crypto` discarded `hmac_bytes` and skipped the verify step. The TODO claimed QR pairing's adv_secret_key didn't reach the primary, but make_qr_data embeds it directly into the QR string and prepare_key_bundle HKDF-derives it for pair-code, so both flows do share the secret. Without the HMAC check, account_signature alone is not a backstop because its verification key comes from the same unauthenticated container. props.rs: PropsResponse::into_node only emitted attrs and silently dropped experiment_props. WA Web's parser (WASmaxInAbPropsExperimentConfigMixin) and captured prod traffic both require <prop config_code config_value/> children. The existing `test_props_response_protocol_node_round_trip` was named round-trip but omitted the experiment_props assertion, hiding the regression. Production whatsapp-rust clients are unaffected by the props change since they only consume PropsResponse; the bug was visible only to server-side reusers of wacore (mock-server).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughHMAC verification was added to pairing: Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PairUtils
participant Crypto
participant ErrorHandler
Client->>PairUtils: Receive AdvSignedDeviceIdentityHmac payload
PairUtils->>PairUtils: Extract details_bytes and hmac_bytes
PairUtils->>Crypto: Compute expected HMAC(details_bytes, prefix)
Crypto-->>PairUtils: computed_hmac
PairUtils->>Crypto: verify_slice(computed_hmac, hmac_bytes)
alt HMAC Match
Crypto-->>PairUtils: success
PairUtils-->>Client: continue pairing flow
else HMAC Mismatch
Crypto-->>PairUtils: failure
PairUtils->>ErrorHandler: create PairCryptoError(code=401, text="hmac-mismatch")
ErrorHandler-->>PairUtils: error object
PairUtils-->>Client: reject pairing
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 31s —— View job Code Review:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/iq/props.rs`:
- Around line 277-290: The serializer currently emits prop nodes for entries in
experiment_props even when config_code == 0, violating the parser invariant
(try_from_node_ref rejects config_code == 0); fix by filtering out those entries
before building nodes so wire output matches the parser rules: apply
.filter(|(code, _)| *code != 0) (or equivalent) to self.experiment_props
iterator used to build the NodeBuilder::new("prop") nodes and then pass the
filtered vec into builder.children(...).build() so no prop with config_code == 0
is emitted.
- Around line 553-604: The test
test_props_response_into_node_emits_wa_web_compliant_prop_children currently
only checks that each child has config_code and config_value but not that the
code maps to the correct value; update the assertion after building children to
collect (config_code, config_value) pairs from each child (using
child.attrs.get("config_code") and child.attrs.get("config_value")), and assert
the resulting collection equals the expected set/vec of tuples [("11262","1"),
("11103","0")] (or equivalent map equality), so each code is paired with its
correct value when PropsResponse.into_node is used.
In `@wacore/src/pair.rs`:
- Around line 611-670: The tests only cover account_type 0, missing the
hosted-account (account_type 1) branch in do_pair_crypto; add two tests that
mirror do_pair_crypto_accepts_matching_hmac and
do_pair_crypto_rejects_mismatched_hmac but target hosted accounts: either extend
build_pair_success_payload to take an account_type parameter (or add a new
helper) and ensure the synthesized AdvDeviceIdentity / AdvSignedDeviceIdentity
uses account_type: Some(1) and uses the hosted MAC input path expected by
do_pair_crypto; then call PairUtils::do_pair_crypto(&state, &payload) once with
state.adv_secret_key (expect Ok) and once with a wrong_secret (expect Err with
code 401 and text "hmac-mismatch"). Ensure the new tests reference
build_pair_success_payload (or the new helper), do_pair_crypto, and
state.adv_secret_key for clarity.
🪄 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: 0dec839e-5648-417b-9c6e-d701ea0e0f6f
📒 Files selected for processing (2)
wacore/src/iq/props.rswacore/src/pair.rs
| // Round-trip with try_from_node_ref. config_expo_key is dropped on | ||
| // both sides; extend the tuple type before adding it back. | ||
| let prop_nodes: Vec<Node> = self | ||
| .experiment_props | ||
| .into_iter() | ||
| .map(|(code, value)| { | ||
| NodeBuilder::new("prop") | ||
| .attr("config_code", code) | ||
| .attr("config_value", &*value) | ||
| .build() | ||
| }) | ||
| .collect(); | ||
|
|
||
| builder.children(prop_nodes).build() |
There was a problem hiding this comment.
Enforce config_code invariant on serialization path.
This needs to be locked down: parser rejects config_code == 0, but serializer can still emit it from experiment_props. Keep wire output consistent with the same protocol invariant.
Suggested fix
- let prop_nodes: Vec<Node> = self
+ let prop_nodes: Vec<Node> = self
.experiment_props
.into_iter()
+ .filter(|(code, _)| *code > 0)
.map(|(code, value)| {
NodeBuilder::new("prop")
.attr("config_code", code)
.attr("config_value", &*value)
.build()
})
.collect();📝 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.
| // Round-trip with try_from_node_ref. config_expo_key is dropped on | |
| // both sides; extend the tuple type before adding it back. | |
| let prop_nodes: Vec<Node> = self | |
| .experiment_props | |
| .into_iter() | |
| .map(|(code, value)| { | |
| NodeBuilder::new("prop") | |
| .attr("config_code", code) | |
| .attr("config_value", &*value) | |
| .build() | |
| }) | |
| .collect(); | |
| builder.children(prop_nodes).build() | |
| // Round-trip with try_from_node_ref. config_expo_key is dropped on | |
| // both sides; extend the tuple type before adding it back. | |
| let prop_nodes: Vec<Node> = self | |
| .experiment_props | |
| .into_iter() | |
| .filter(|(code, _)| *code > 0) | |
| .map(|(code, value)| { | |
| NodeBuilder::new("prop") | |
| .attr("config_code", code) | |
| .attr("config_value", &*value) | |
| .build() | |
| }) | |
| .collect(); | |
| builder.children(prop_nodes).build() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/iq/props.rs` around lines 277 - 290, The serializer currently
emits prop nodes for entries in experiment_props even when config_code == 0,
violating the parser invariant (try_from_node_ref rejects config_code == 0); fix
by filtering out those entries before building nodes so wire output matches the
parser rules: apply .filter(|(code, _)| *code != 0) (or equivalent) to
self.experiment_props iterator used to build the NodeBuilder::new("prop") nodes
and then pass the filtered vec into builder.children(...).build() so no prop
with config_code == 0 is emitted.
There was a problem hiding this comment.
@jlucaso1, understood — acknowledged and moving on.
Benchmark Results67 unchanged benchmark(s)
|
Address review feedback on PR #594: - props: assert (config_code, config_value) pairs preserve their original mapping, not just presence of each code. Previous version would have passed even if into_node mismapped values. - pair: cover the hosted-account branch (account_type=1) of do_pair_crypto. The HMAC and account-signature paths apply ADV_HOSTED_PREFIX_ACCOUNT_SIGNATURE under that branch; without these tests a regression in the prefix wiring would be silent. build_pair_success_payload gains an `is_hosted` parameter and mirrors the verifier's hosted/E2EE branching.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wacore/src/iq/props.rs (1)
279-288:⚠️ Potential issue | 🟠 MajorFilter out invalid
config_codevalues before emitting<prop>nodes.This still serializes
config_code == 0, whiletry_from_node_ref()only accepts codes> 0. That makes the new round-trip lossy again and can put invalid props on the wire.Suggested fix
let prop_nodes: Vec<Node> = self .experiment_props .into_iter() + .filter(|(code, _)| *code > 0) .map(|(code, value)| { NodeBuilder::new("prop") .attr("config_code", code) .attr("config_value", &*value) .build() }) .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/iq/props.rs` around lines 279 - 288, The code currently serializes entries from self.experiment_props including invalid config_code values (e.g., 0) even though try_from_node_ref() only accepts codes > 0; update the creation of prop_nodes to filter out invalid codes before mapping (e.g., use .into_iter().filter(|(code, _)| *code > 0) or equivalent) so only entries with config_code > 0 are turned into NodeBuilder::new("prop")... nodes, preventing invalid props from being emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wacore/src/iq/props.rs`:
- Around line 279-288: The code currently serializes entries from
self.experiment_props including invalid config_code values (e.g., 0) even though
try_from_node_ref() only accepts codes > 0; update the creation of prop_nodes to
filter out invalid codes before mapping (e.g., use .into_iter().filter(|(code,
_)| *code > 0) or equivalent) so only entries with config_code > 0 are turned
into NodeBuilder::new("prop")... nodes, preventing invalid props from being
emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 64d8b976-dc4f-4bc4-869d-0a84310f13b8
📒 Files selected for processing (2)
wacore/src/iq/props.rswacore/src/pair.rs
The mock server (post-MockPhone-migration) no longer auto-pairs — its legacy timer is off by default. External e2e harnesses that only have a WebSocket to bartender must POST scanned QR strings to the new `/admin/mock-phone/scan-qr` endpoint, the out-of-process equivalent of bartender's in-process `spawn_qr_autoresponder`. `spawn_qr_autoresponder_http` listens for `Event::PairingQrCode` on a dedicated `ChannelEventHandler` (sharing the test's event_rx would steal events from `wait_for_event` since async_channel is MPMC), derives the admin URL from `MOCK_SERVER_URL` (scheme + path swap), and POSTs the QR via the existing UreqHttpClient (already configured with `danger-skip-tls-verify` for the self-signed mock cert).
![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: f45cb1545f
ℹ️ 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".
| Ok(resp) => { | ||
| eprintln!( | ||
| "qr-autoresponder: admin POST returned status {}: {}", | ||
| resp.status_code, | ||
| String::from_utf8_lossy(&resp.body) | ||
| ); | ||
| return; | ||
| } | ||
| Err(e) => { | ||
| eprintln!("qr-autoresponder: admin POST transport error: {e}"); | ||
| return; |
There was a problem hiding this comment.
The autoresponder exits permanently on the first non-2xx response or transport error, so any transient failure from /admin/mock-phone/scan-qr leaves the client unpaired and causes TestClient::connect* to time out even though later QR events could succeed. This makes the full e2e suite flaky under brief mock-server startup or network hiccups; the loop should keep listening/retrying until a successful POST (or channel close).
Useful? React with 👍 / 👎.
1 participant
Summary
Two correctness bugs in
wacore, surfaced while wiring an in-process"phone simulator" in mock-server (bartender) that produces
<pair-success>with real HMACs.
1.
wacore/src/pair.rs— ADV HMAC verification was skippedPairUtils::do_pair_cryptoparsed the HMAC bytes into_hmac_bytesandnever called
mac.verify_slice(...). The previous TODO claimed QRpairing's
adv_secret_keycouldn't reach the primary, but in fact:PairUtils::make_qr_dataembeds the 32-byteadv_secret_keydirectlyin the QR string (5th field), so any peer that scans the QR knows it.
pair_code::prepare_key_bundleHKDF-derives the sameadv_secretfrom the DH bundle, so the pair-code flow shares it too.
Without verification,
accountSignature(ed25519) alone is not abackstop — its verification key comes from the same untrusted
AdvSignedDeviceIdentityblob, so an attacker could mint a freshkeypair and self-sign. WA Web's
WAWebHandlePairSuccess.js:88-97doesthe HMAC check and calls
logoutAfterValidationFail()on mismatch;WhatsApp Android (
FHU.A01in the apk decompile) likewise.The fix is
mac.verify_slice(hmac_bytes)?returning a typedPairCryptoError { code: 401, text: "hmac-mismatch" }. Two regressiontests added:
do_pair_crypto_accepts_matching_hmacanddo_pair_crypto_rejects_mismatched_hmac.2.
wacore/src/iq/props.rs—PropsResponse::into_nodedropped childrenPropsResponse::into_nodeonly emitted attrs (ab_key,hash,refresh,refresh_id,delta_update) and never iteratedexperiment_props,producing an empty
<props/>tag.Empirical wire format (captured prod log + WA Web JS):
WA Web parser (
WASmaxInAbPropsGetExperimentConfigResponseSuccess.js:55)calls
mapChildrenWithTag(props, "prop", 0, infinity, …)to read everychild via
WASmaxInAbPropsExperimentConfigMixin(which expectsconfig_codeint >=1,config_valuestring, optionalconfig_expo_key).The fix iterates
experiment_propsand emits one<prop config_code … config_value …/>per entry.config_expo_keyis dropped on both sidesof the round-trip (intentional, matches the lightweight tuple type
introduced in #553); extending to preserve it is a separate change.
The existing
test_props_response_protocol_node_round_tripclaimed toround-trip but omitted the
experiment_propsassertion — the omissionthat hid the regression. The assertion is now added, and a new test
(
test_props_response_into_node_emits_wa_web_compliant_prop_children)checks the wire structure against the WA Web schema explicitly.
Impact on production whatsapp-rust
<pair-success>containers.Real WA server traffic is unaffected (it has the secret and produces
matching HMACs).
PropsResponse, neverserialize one back, so this is a no-op for the production client.
Server-side reusers of wacore (mock-server, hypothetical proxies)
now get the correct wire format.
Test plan
cargo test -p wacore --lib— 613 passed (was 611 before the 2 new tests)cargo test -p wacore --lib props— 11 passed including the new round-trip assertioncargo test -p wacore --lib pair::tests— 17 passed including new HMAC-pass and HMAC-fail testscargo clippy -p wacore --all-targets— cleancargo test -p whatsapp-mock-server --test group_privacy_token_e2e(with this branch path-patched) goes from 1/2 to 2/2 passing, confirming the props fix unblocks the AB-prop-gatedadd_participantsprivacy attachment.