feat(noise): implement Noise_IK + XXfallback for WA-Web parity - #598
Conversation
NoiseState::new previously hashed any protocol_name not exactly 32 bytes, including the canonical 28-byte unpadded forms. Spec requires zero-padding for names <= HASHLEN and SHA256 only for names > HASHLEN. Hardens against silent regression where a caller passes the unpadded form "Noise_XX_25519_AESGCM_SHA256" expecting WhatsApp Web's pre-padded h0 — it would have computed a different hash and surfaced as a generic MAC failure deep inside DecryptAndHash. The unused to_array helper goes away now that the only caller is gone.
Locks the chaining-key/hash state right after NoiseHandshake::new for the WhatsApp XX pattern + WA_CONN_HEADER prologue. The expected value is hand-computed via SHA256(name_padded || prologue) so any future drift in either the pattern constant, the padding behavior, or the prologue header will surface as a focused test failure instead of a generic AEAD MAC error deep inside the handshake. Also asserts salt == raw 32-byte pattern bytes, which only holds when the zero-padding branch (not the SHA256 branch) is taken.
Drops the single-pattern view and introduces three named constants: NOISE_PATTERN_XX, NOISE_PATTERN_IK, NOISE_PATTERN_XXFALLBACK Mirrors WA Web's three handshake protocol_names (M, w, A in WAWebOpenChatSocket) so subsequent commits can wire IK and XXfallback without weaving raw byte literals through the handshake code. Length-locking unit tests guard against accidental loss of the four-byte zero pad on XX/IK (which would silently change h0) and against accidental padding of XXfallback (which is intentionally 36 bytes so Noise hashes it). Pre-1.0 API change: NOISE_START_PATTERN is removed without an alias. Three in-tree call-sites updated.
Adds the persistence skeleton needed to enable Noise IK on reconnect:
- Device.server_cert_chain: Option<CachedServerCertChain> with
serde(default) for backwards-compat with pre-existing serialized
devices.
- CachedNoiseCert / CachedServerCertChain: minimal cached form with
just `key` plus `not_before` / `not_after`. Mirrors the JSON shape
WA Web persists in waNoiseInfo.certificateChainBuffer (signatures
and issuer_serial dropped — they were already validated on reception
and aren't needed for IK or expiry checks).
- DeviceCommand::SetServerCertChain / ClearServerCertChain. Two
variants (one struct field) so the cache always moves atomically.
- SQLite column `server_cert_chain BLOB NULL` with bincode-encoded
payload. New nullable column, no data migration.
The handshake itself does not yet populate or consume this field —
that is wired in the next commit when the XX outcome is reshaped and
the IK / XXfallback state machines land. This commit is the storage
landing pad.
No encryption-at-rest. WA Web does encrypt this blob via DbEncKey, but
the existing Device row already stores the noise_key private key and
identity_key in the clear; layering encryption only on the cert chain
would be inconsistent. Treat at-rest encryption as a separate hardening
pass over the whole Device.
Replaces the single HandshakeState with three pattern-specific machines
that mirror WA Web's handshake topology in WAWebOpenChatSocket:
XxHandshakeState — Noise XX, used on first connect / pairing
IkHandshakeState — Noise IK, used on reconnect with cached
server static (1-RTT, ships 0-RTT login
payload in clientHello)
XxFallbackHandshakeState — Noise XXfallback, recovery path when the
server rejects IK by replying with a
ServerHello carrying static != null. The
ephemeral already on the wire is reused.
XxHandshakeOutcome bundles the resulting cipher pair with the freshly
verified VerifiedServerCertChain (intermediate + leaf, key + validity
window) so the orchestration layer can persist it for the next IK
attempt. CachedServerCertChain in wacore::store::device gains a
From<VerifiedServerCertChain> impl so the boundary between the noise
crate (no serde dep) and the storage crate stays clean.
IkServerHelloOutcome::Continue / Fallback drives the dispatch in code
that holds an IkHandshakeState: Continue completes immediately;
Fallback hands carryover state (ephemeral, static, payload, raw
serverHello bytes) to XxFallbackHandshakeState::from_ik_failure
without losing the message that's already in flight.
End-to-end coverage with a self-contained TestResponder:
xx_handshake_round_trip_completes
ik_handshake_round_trip_continue
ik_to_xx_fallback_round_trip
ik_with_wrong_server_static_fails_at_decrypt
Existing src/handshake.rs still selects only XX — pattern selection
based on cached state lands in the next commit.
Pre-1.0 API change: HandshakeState removed; HandshakeUtils gains
build_ik_client_hello and parse_server_hello_body. NOISE_PATTERN_XX is
now baked into XxHandshakeState::new (callers no longer pass it).
do_handshake now mirrors WAWebOpenChatSocket's pattern selection:
- no cached cert chain → XX
- per-process IK failure → XX (counter K, threshold 1, matches WA Web)
- cert chain expired → XX (validates leaf + intermediate not_after)
- otherwise → IK with cached leaf.key
After IK rejection (serverHello carries static != null), the in-flight
ephemeral is handed to XxFallbackHandshakeState so the recovery costs
zero extra RTT — the response we just got from the server is consumed
as the XXfallback ServerHello. Mirrors WA Web's `U/V` -> `H` chain.
Invalidation policy distinguishes:
- is_transient (timeout / disconnect / transport) → cache preserved,
counter unchanged
- is_crypto_fatal (Core handshake error) → cache cleared via
DeviceCommand::ClearServerCertChain, counter incremented; next
connect falls back to XX
Successful XX or XX-fallback persists the freshly verified chain via
SetServerCertChain. Successful IK Continue does NOT re-persist (the
on-disk chain just proved itself by completing the handshake).
A new Client.ik_handshake_failures: AtomicU32 holds the per-process
counter. Resets to 0 on any successful handshake. Not persisted
(WA Web behavior — survives only the process lifetime).
Tests cover: select_pattern{no_cache, valid_cache, after_one_failure,
expired_leaf, expired_intermediate} and the is_transient /
is_crypto_fatal classification used by the invalidation policy.
The shared send_first_handshake_message helper introduced in the previous commit already routes both XX and IK through the same build_handshake_header() call, so edge_routing + WA_CONN_HEADER are applied identically. This test pins that invariant. Worth pinning because: the wire-side server validates the prologue when it re-derives h0 for transcript MAC checks. If a future change diverges the two paths (e.g. someone adds a header to one without the other), the symptom is a generic AEAD failure deep inside DecryptAndHash on the server side. This test surfaces that as a focused failure instead.
…estration
Adds tests/handshake_integration.rs which stands up an in-process Noise
responder using the same primitives wacore-noise exposes. No dependency
on the external mock server — these tests run as part of the standard
cargo test --workspace sweep.
The harness wires:
- CaptureTransport: implements Transport, buffers all client→server
bytes for inspection
- InProcessServer: holds a server keypair + cert chain bytes and
serves XX, IK-accept, or IK-reject paths inline
- PersistenceManager backed by InMemoryBackend (no SQLite)
- Per-test AtomicU32 IK-failure counter
Three scenarios covered:
cold_start_xx_then_cached_ik_reconnect
Cold start: device has no cache → orchestrator picks XX, completes,
persists the freshly-validated cert chain. Second call to
do_handshake (with the same PM) sees the cache and picks IK; the IK
ClientHello bytes are inspected to confirm presence of `static` and
`payload` fields (IK shape, not XX).
ik_rejected_recovers_via_xxfallback_and_repopulates_cache
Server replies with `static != null`, exercising the IK→XXfallback
transition. Asserts the orchestration completes in one round trip
(one ClientHello + one ServerHello + one ClientFinish, no second
ClientHello) and that the cert chain is re-persisted.
ik_with_stale_cache_invalidates_and_increments_counter
Pre-seeds a cache with a leaf.key the server doesn't own; server
replies with garbage payload that triggers AEAD MAC failure on the
client during decrypt of the cert. Asserts the error is classified
as crypto-fatal, the counter is incremented to 1, and the cache is
cleared via DeviceCommand::ClearServerCertChain.
Adds a Noise Handshake Patterns section to protocol_architecture.md
covering:
- the three patterns and when each fires
- the select_pattern flow (counter threshold, cache presence,
cert validity window)
- the invalidation policy table mapping error class to cache /
counter mutation
- the cached cert chain shape that lives on Device
- the [socket] log lines emitted at each phase, matched verbatim
against WA Web's WAWebOpenChatSocket so cross-implementation
debugging just works
The log lines themselves were added in the previous orchestration
commit. The on-the-wire behavior is unchanged.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements runtime handshake selection between XX, IK, and XXfallback driven by a per-process IK failure counter and persisted cached server cert chains; adds schema/persistence, handshake refactor into IK/XX/XXfallback states, crypto-fatal classification, cache invalidation commands, and integration/unit tests. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant PM as PersistenceManager
participant HO as HandshakeOrchestrator
participant NP as NoiseProtocol
participant DB as SQLiteStorage
Client->>HO: do_handshake(persistence_manager, ik_handshake_failures)
HO->>PM: snapshot device state
PM->>DB: load device.server_cert_chain
DB-->>PM: cert_chain or None
PM-->>HO: Device + cached certs
alt IK viable (counter < threshold AND certs valid AND registered)
HO->>NP: initiate IK (encrypted static + payload)
NP-->>HO: IkServerHelloOutcome::Continue OR IkServerHelloOutcome::Fallback
alt IK success
HO->>PM: (conditionally) SetServerCertChain(verified_chain)
HO-->>Client: reset ik_handshake_failures
else IK rejected (crypto-fatal)
HO->>PM: ClearServerCertChain
HO-->>Client: increment ik_handshake_failures
HO->>NP: run XxFallback / XX
NP-->>HO: XX cipher + verified_chain
HO->>PM: (conditionally) SetServerCertChain(verified_chain)
end
else IK blocked (counter >= threshold OR not registered OR certs invalid)
HO->>NP: initiate XX
NP-->>HO: XX cipher + verified_chain
HO->>PM: (conditionally) SetServerCertChain(verified_chain)
end
HO-->>Client: NoiseSocket (ciphers)
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 |
![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: 70cad9d0e1
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/noise/src/handshake.rs (1)
137-205:⚠️ Potential issue | 🔴 CriticalBlocker: this is not actually verifying the server cert chain.
This only checks serial linkage and that
leaf.keymatches the decrypted static. The leaf/intermediate signatures are never verified, and the validity window is never enforced. An active MITM can mint a fake chain around its own static, pass XX/XXfallback, and poison the cached IK key. We can't ship the cache-on-success flow until this function does full signature and time validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/noise/src/handshake.rs` around lines 137 - 205, verify_server_cert currently only checks serial linkage and key equality; update it to perform full cryptographic and temporal validation: decode and extract the signature fields from CertChain.intermediate and .leaf, verify the intermediate certificate's signature using the known global issuer public key (the WA issuer corresponding to WA_CERT_ISSUER_SERIAL), then verify the leaf certificate's signature using the intermediate public key extracted from intermediate_details.key(); also enforce the validity windows by checking intermediate_details.not_before()/not_after() and leaf_details.not_before()/not_after() against the current system time (e.g., UTC now) and return HandshakeError::CertVerification on any failure. Ensure these checks are done before returning VerifiedServerCertChain and before any caching of the derived keys (symbols: verify_server_cert, CertChain, intermediate_details, leaf_details, intermediate_key, WA_CERT_ISSUER_SERIAL, VerifiedServerCertChain).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent_docs/protocol_architecture.md`:
- Around line 174-183: Update the fenced log block so the opening fence is
tagged with a language (e.g., change ``` to ```text) so the snippet containing
lines like "[socket] doFullHandshake: openChatSocket send hello",
"resumeNoiseHandshake started", "resumeNoiseHandshake failed:
serverStaticCiphertext not null — doFallbackHandshake continuing handshake with
given server hello", and "continueFullHandshakeCore client finish and deriving
secrets" is fenced as a language-specific code block to satisfy markdownlint
MD040.
- Around line 166-170: The paragraph claiming "full protobuf signatures + issuer
serial are validated at reception and then dropped" overstates current behavior;
update the text around CachedServerCertChain and the reference to
PrefsInfoStore.js:setCertificateChain to remove or qualify any assertion that
cert-chain signatures (e.g., Ed25519 verification) are performed on
reception—instead state that only the reduced fields ({ key: [u8;32],
not_before: i64, not_after: i64 }) are persisted and that full cert-chain
signature verification (including Ed25519) is not implemented here and remains a
follow-up for IK trust checks.
In `@src/handshake.rs`:
- Around line 49-54: The current is_crypto_fatal() treats any Core(_) as
crypto-fatal; change it to only treat the real auth/decryption/stale-key Core
error variants as fatal. Replace matches!(self, Self::Core(_)) with code that
matches Self::Core(inner) and then matches inner against the specific Core enum
variants that represent authentication/decryption/stale-key failures (for
example DecryptFailure, VerifyFailed, StaleServerStatic, InvalidServerCert — use
the actual variant names from the Core enum in your codebase), returning true
only for those and false otherwise.
- Around line 304-307: The branch handling transport_events.recv() currently
maps Err(_) to HandshakeError::Timeout which hides producer teardown; change the
Err(_) arm in the match (the one handling transport_events.recv()) to return a
distinct error like HandshakeError::StreamClosed (or StreamEnd) instead of
Timeout, and add that new variant to the HandshakeError enum (implement
Display/From/any existing conversions consistent with other variants) so callers
can distinguish a closed event stream from a timeout; reference TransportEvent,
HandshakeError, and transport_events.recv() when making the change.
In
`@storages/sqlite-storage/migrations/2026-04-26-000000_add_server_cert_chain/down.sql`:
- Line 1: The down migration currently uses "ALTER TABLE device DROP COLUMN
server_cert_chain" which will fail on SQLite < 3.35.0; update the migration to
be compatible by either documenting the minimum SQLite version (add a visible
note to README/Cargo.toml/docs) or replace the DROP COLUMN with the existing
backward-compatible workaround used in earlier migrations: create a temporary
table for "device" without the server_cert_chain column, copy data from the
original into the temp table selecting only the retained columns, drop the
original "device" table, and rename the temp table to "device" so the rollback
works on older SQLite versions; reference the migration SQL in this file and
mirror the pattern from the 2026-03-12 / 2026-02-05 migrations.
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 591-599: The current decode pipeline for server_cert_chain treats
bincode decode failures as fatal (producing StoreError::Serialization) which
aborts CoreDevice loading; instead, catch decode errors, log a warning with the
error and context, and degrade to None so a corrupt cache blob doesn't stop
startup. Replace the map/map_err/transpose chain around server_cert_chain and
bincode::serde::decode_from_slice with logic that attempts to decode (using
bincode::serde::decode_from_slice(bytes, ...)), on Ok return Some(chain), on Err
log the error (e.g. log::warn! or the crate's logger) and return None; ensure
the overall expression yields Option<...> (not a Result::Err) so no StoreError
is returned for corrupt cache data.
In `@tests/handshake_integration.rs`:
- Around line 299-307: The tests use pm() which constructs a PersistenceManager
with InMemoryBackend, so they never exercise the SQLite persistence/migration
path; update tests to add at least one integration test that creates a
PersistenceManager backed by the SQLite backend (instead of
wacore::store::InMemoryBackend) so the server_cert_chain is written to disk and
then create a fresh PersistenceManager::new(...) pointed at the same SQLite DB
to verify the server_cert_chain survives a restart/migration; locate pm() and
add or add a new helper (e.g., pm_sqlite or a test case) that uses the sqlite
backend implementation, write/read assertions against
PersistenceManager::get_server_cert_chain (or the code path that checks cached
server_cert_chain) to ensure a real DB round-trip and migration are exercised.
In `@wacore/noise/src/lib.rs`:
- Around line 27-30: The documentation example is not copy-pasteable because it
references NOISE_PATTERN_XX and WA_CONN_HEADER without importing or qualifying
them; update the example to include the necessary use/import lines (e.g., bring
NOISE_PATTERN_XX and WA_CONN_HEADER into scope) or fully qualify those constants
so the snippet compiles, and ensure NoiseHandshake and HandshakeUtils are also
imported (or fully qualified) so the example can be run as-is.
In `@wacore/src/store/commands.rs`:
- Around line 118-123: The test clear_server_cert_chain_drops_field is mutating
Device directly; instead seed the state via the command path: send
DeviceCommand::SetServerCertChain (with dummy_chain()) through
PersistenceManager::process_command (or the same helper used elsewhere to apply
commands), then send DeviceCommand::ClearServerCertChain via process_command and
assert the cleared state by calling get_device_snapshot() (or the standard
snapshot helper) to verify server_cert_chain is None; remove direct
Device::new() and direct assignment to device.server_cert_chain to keep a single
mutation path.
---
Outside diff comments:
In `@wacore/noise/src/handshake.rs`:
- Around line 137-205: verify_server_cert currently only checks serial linkage
and key equality; update it to perform full cryptographic and temporal
validation: decode and extract the signature fields from CertChain.intermediate
and .leaf, verify the intermediate certificate's signature using the known
global issuer public key (the WA issuer corresponding to WA_CERT_ISSUER_SERIAL),
then verify the leaf certificate's signature using the intermediate public key
extracted from intermediate_details.key(); also enforce the validity windows by
checking intermediate_details.not_before()/not_after() and
leaf_details.not_before()/not_after() against the current system time (e.g., UTC
now) and return HandshakeError::CertVerification on any failure. Ensure these
checks are done before returning VerifiedServerCertChain and before any caching
of the derived keys (symbols: verify_server_cert, CertChain,
intermediate_details, leaf_details, intermediate_key, WA_CERT_ISSUER_SERIAL,
VerifiedServerCertChain).
🪄 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: 54ecc0d2-6ce6-4e41-9eaa-82641d506ec2
📒 Files selected for processing (17)
agent_docs/protocol_architecture.mdsrc/client.rssrc/handshake.rsstorages/sqlite-storage/migrations/2026-04-26-000000_add_server_cert_chain/down.sqlstorages/sqlite-storage/migrations/2026-04-26-000000_add_server_cert_chain/up.sqlstorages/sqlite-storage/src/schema.rsstorages/sqlite-storage/src/sqlite_store.rstests/handshake_integration.rswacore/binary/src/consts.rswacore/noise/src/handshake.rswacore/noise/src/lib.rswacore/noise/src/state.rswacore/src/handshake/mod.rswacore/src/store/commands.rswacore/src/store/device.rswacore/src/store/mod.rswacore/tests/noise_handshake_test.rs
is_crypto_fatal previously matched any Core(_), including programmer- side bugs (own-encode failures, generic crypto-provider errors, HKDF impossibilities, counter exhaustion in a single handshake). Those have nothing to do with a stale cached server static, so clearing the cache in response would have masked real defects. Narrowed to the variants that genuinely indicate "the static we used to derive ee/se isn't the right one": AEAD decrypt fail, ciphertext-too-short, structurally bad ServerHello, cert verification fail. Adds HandshakeError::StreamClosed for the case where the events channel's producer side is dropped — distinct from a real timeout because nothing further will ever arrive on the channel. Previously mapped to Timeout, which masked transport teardown. Both still classify as transient (they don't invalidate the cache). Tests adjusted: handshake_error_classification now also asserts StreamClosed transience and explicitly contrasts the cache-invalidating Core variants with the programmer-side ones (Crypto(String) bug case).
…g load A corrupt cert-chain BLOB (truncated row, format change between versions, on-disk corruption) used to abort device loading with StoreError::Serialization, which prevented the whole client from starting. The cert chain is a perf optimization — Noise IK can be skipped harmlessly in favor of a fresh XX next connect — so a decode failure must not be load-bearing. Now wraps the bincode decode in and_then: on Err it logs a warning with row size and the underlying error, then returns None for the field. The next handshake will populate the cache again from a clean XX outcome. Also adds a SQLite roundtrip test (test_server_cert_chain_survives_save_load_roundtrip) that exercises the bincode encode -> BLOB column -> bincode decode path that the in-memory backend integration tests don't reach. Uses a shared-cache memdb with two concurrent stores to simulate a process restart against the same on-disk state.
Two small fixes from the PR review:
- wacore/noise/src/lib.rs: the WhatsApp example was using
NOISE_PATTERN_XX and WA_CONN_HEADER without imports, so it wasn't
copy-pasteable. Adds the missing wacore_binary::consts use line.
- wacore/src/store/commands.rs: clear_server_cert_chain_drops_field
was seeding state via direct field assignment. Switched to seeding
via DeviceCommand::SetServerCertChain so the test exercises the
same single mutation surface used in production. Behaviorally the
same (apply_command_to_device on this variant compiles to a field
assignment), but cleaner as a regression target.
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 `@src/handshake.rs`:
- Around line 204-217: The current error-handling clears the cached server cert
and increments ik_handshake_failures whenever pattern == HandshakePattern::Ik
and e.is_crypto_fatal(), but this also runs when an IK flow already pivoted to
XX (see run_ik_handshake() and IkServerHelloOutcome::Fallback), which wrongly
poisons the cache for post-fallback failures; change the logic so you only treat
the error as a pre-fallback IK crypto-fatal: have run_ik_handshake() (or the
caller) indicate whether the failure occurred before or after
IkServerHelloOutcome::Fallback (e.g., return an outcome enum or set a boolean),
and then only call ik_handshake_failures.fetch_add(1, Ordering::AcqRel) and
persistence_manager.process_command(DeviceCommand::ClearServerCertChain).await
when the failure is confirmed to be pre-fallback and e.is_crypto_fatal() while
pattern == HandshakePattern::Ik.
- Around line 121-135: select_pattern currently only checks certificate
expirations and allows a future-dated cached chain to take the IK path; update
the validity checks in select_pattern to also reject chains where
chain.leaf.not_before or chain.intermediate.not_before are in the future
(compare now_secs < not_before) so both leaf and intermediate must be within
their not_before..not_after windows before returning
HandshakePattern::Ik(chain.leaf.key); keep the existing ik_failures and
server_cert_chain checks and return HandshakePattern::Xx on any failure, and add
a regression test alongside the expired-cert tests that covers a future-dated
cert chain to ensure the IK path is not selected.
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 2911-2999: The test
test_server_cert_chain_survives_save_load_roundtrip currently covers happy-path
save/load and clearing-to-NULL but omits the corrupt-blob recovery branch in
load_device_data_for_device; update the test to seed an invalid/corrupted BLOB
for server_cert_chain (using the existing shared-cache DB opened via
SqliteStore::new_for_device and the save_device_data_for_device or direct SQL on
the same connection) then reopen or call load_device_data_for_device and assert
that the loader returns Ok(Some(device)) with device.server_cert_chain == None,
verifying the warning-and-degrade path is exercised.
🪄 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: 28795398-170d-4b8a-9423-959fab896aa3
📒 Files selected for processing (5)
agent_docs/protocol_architecture.mdsrc/handshake.rsstorages/sqlite-storage/src/sqlite_store.rswacore/noise/src/lib.rswacore/src/store/commands.rs
| #[tokio::test] | ||
| async fn test_server_cert_chain_survives_save_load_roundtrip() { | ||
| use portable_atomic::AtomicU64; | ||
| use std::sync::atomic::Ordering; | ||
| use wacore::store::device::{CachedNoiseCert, CachedServerCertChain}; | ||
|
|
||
| static COUNTER: AtomicU64 = AtomicU64::new(200); | ||
| let id = COUNTER.fetch_add(1, Ordering::Relaxed); | ||
| // shared-cache so a second SqliteStore opened on the same name | ||
| // sees the same on-disk state — the closest we can get to a real | ||
| // process restart inside a single test run. | ||
| let db_name = format!( | ||
| "file:memdb_certchain_{}_{}?mode=memory&cache=shared", | ||
| std::process::id(), | ||
| id | ||
| ); | ||
|
|
||
| let device_id = 7; | ||
| let chain = CachedServerCertChain { | ||
| intermediate: CachedNoiseCert { | ||
| key: [0xAB; 32], | ||
| not_before: 1_700_000_000, | ||
| not_after: 1_900_000_000, | ||
| }, | ||
| leaf: CachedNoiseCert { | ||
| key: [0xCD; 32], | ||
| not_before: 1_700_000_500, | ||
| not_after: 1_899_999_500, | ||
| }, | ||
| }; | ||
|
|
||
| // First store: create + populate. Keep it alive until after the | ||
| // second store opens — `cache=shared` only persists the in-memory | ||
| // database while at least one connection is open. Dropping the | ||
| // first store would also drop the schema before the second can | ||
| // see it. | ||
| let _writer = SqliteStore::new_for_device(&db_name, device_id) | ||
| .await | ||
| .expect("create store"); | ||
| _writer.create_new_device().await.expect("create device"); | ||
|
|
||
| let mut device = _writer | ||
| .load_device_data_for_device(device_id) | ||
| .await | ||
| .expect("load") | ||
| .expect("device should exist after create"); | ||
| device.server_cert_chain = Some(chain.clone()); | ||
| _writer | ||
| .save_device_data_for_device(device_id, &device) | ||
| .await | ||
| .expect("save with cert chain"); | ||
|
|
||
| // Second store on the SAME shared-cache db: this exercises the | ||
| // exact path a fresh-process load would take — schema migration | ||
| // already applied, BLOB column present, and the bincode-encoded | ||
| // chain decoded by the load path. | ||
| let store = SqliteStore::new_for_device(&db_name, device_id) | ||
| .await | ||
| .expect("reopen store"); | ||
| let loaded = store | ||
| .load_device_data_for_device(device_id) | ||
| .await | ||
| .expect("load") | ||
| .expect("device should exist after reopen"); | ||
| assert_eq!( | ||
| loaded.server_cert_chain.as_ref(), | ||
| Some(&chain), | ||
| "server_cert_chain must survive a save/load roundtrip" | ||
| ); | ||
|
|
||
| // Sanity: clearing the chain and saving leaves the column as NULL, | ||
| // not as an empty serialized struct. | ||
| let mut device = loaded; | ||
| device.server_cert_chain = None; | ||
| store | ||
| .save_device_data_for_device(device_id, &device) | ||
| .await | ||
| .expect("save with cleared cert chain"); | ||
|
|
||
| let reloaded = store | ||
| .load_device_data_for_device(device_id) | ||
| .await | ||
| .expect("reload") | ||
| .expect("device should exist"); | ||
| assert!( | ||
| reloaded.server_cert_chain.is_none(), | ||
| "cleared chain must round-trip as None" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add coverage for the corrupt-blob recovery path.
This test proves roundtrip and clear-to-NULL, but it never hits the new load_device_data_for_device() branch that warns and degrades an invalid server_cert_chain blob to None. That’s the behavior protecting startup now, and it can regress without this file noticing. Seed a bad blob and assert load still succeeds with server_cert_chain == None.
🤖 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 2911 - 2999, The
test test_server_cert_chain_survives_save_load_roundtrip currently covers
happy-path save/load and clearing-to-NULL but omits the corrupt-blob recovery
branch in load_device_data_for_device; update the test to seed an
invalid/corrupted BLOB for server_cert_chain (using the existing shared-cache DB
opened via SqliteStore::new_for_device and the save_device_data_for_device or
direct SQL on the same connection) then reopen or call
load_device_data_for_device and assert that the loader returns Ok(Some(device))
with device.server_cert_chain == None, verifying the warning-and-degrade path is
exercised.
|
@claude review nitpicky also check the advantages pratically of this implementation |
|
Claude encountered an error —— View job Nitpicky Review + Practical Advantages Analysis
|
|
@claude review nitpicky also check the advantages pratically of this implementation |
|
Claude finished @jlucaso1's task in 4m 44s —— View job Nitpicky Review + Practical Advantages Analysis
I read every changed file end-to-end. This is a well-structured PR with clean state machine decomposition, thorough tests, and careful alignment with WA Web's JS. The coderabbit and codex reviews already caught some real issues (several addressed in the follow-up commits). Below are my remaining findings, ordered by severity, followed by the practical advantages analysis you asked for. Remaining Issues1.
|
| Location | Note |
|---|---|
src/handshake.rs:19 |
NOISE_HANDSHAKE_RESPONSE_TIMEOUT — 20s is generous. WA Web uses ~15s for the handshake timeout. Not wrong, just potentially slower to surface a dead connection. |
wacore/noise/src/handshake.rs:706-707 |
Comment "Skips signatures since verify_server_cert only checks shape + key bytes" — accurate and honest, but worth noting in the public doc that this is the current trust model. The protocol_architecture.md at line 174-180 now does this well after the follow-up commits. |
tests/handshake_integration.rs:159 |
let _ = sent; — dead binding. The Arc is accessible via transport.sent. Harmless but reads oddly; could be removed. |
wacore/noise/src/handshake.rs:388 |
HandshakeError::CertVerification(format!("Error parsing server hello: {e}")) — wrapping a parse error inside CertVerification is semantically off. It's really an IncompleteResponse or ProtoDecode. But changing it would alter the is_crypto_fatal() classification, so best left as-is unless the error taxonomy gets a broader cleanup. |
src/handshake.rs:86-89 |
IncompleteResponse, InvalidLength, InvalidKeyLength, and ProtoDecode are classified as crypto-fatal. This is conservative (reasonable) but broader than "the cached static is bad" — a genuinely malformed protobuf from a server bug would clear the cache unnecessarily. The doc at line 86 acknowledges this ("Server sent a structurally invalid response"). Acceptable trade-off. |
Practical Advantages
This is the core question: what does this PR actually buy in practice?
1. Reconnect latency: ~50% reduction (1 RTT saved)
The dominant real-world scenario is reconnection after a brief disconnect (network switch, sleep/wake, server-side load rebalance). Before this PR, every reconnect was XX (1.5 RTT). After:
| Scenario | Before | After | Saving |
|---|---|---|---|
| WiFi→mobile handoff | 1.5 RTT (~150-300ms) | 1 RTT (~100-200ms) | ~50-100ms |
| Sleep/wake reconnect | 1.5 RTT | 1 RTT | ~50-100ms |
| Server rebalance | 1.5 RTT | 1 RTT (or 1.5 if server rotated) | ~50-100ms typical |
| Cold start / first pair | 1.5 RTT | 1.5 RTT (still XX) | none |
The 0-RTT login payload in IK's ClientHello means the server can begin processing the auth token while the handshake finishes, further reducing perceived latency.
2. Bandwidth: one fewer round trip = one fewer frame
Each handshake message is ~200-400 bytes on the wire. Saving the ClientFinish frame (which XX requires but IK doesn't) saves ~300 bytes per reconnect. Negligible individually, but it adds up for a library handling thousands of connections (e.g., a multi-account gateway).
3. Server fingerprint parity
WA's server-side heuristics track whether a client speaks IK on reconnect. An XX-only client looks like a fresh installation or a re-paired device to the server's rate limiters and anti-abuse systems. Matching the official client's handshake pattern reduces the risk of triggering server-side throttling or bans. This is arguably the most practically important benefit — it's not about speed, it's about behavioral camouflage.
4. Graceful degradation via XXfallback
When WA rotates server keys (which they do periodically), an IK-only implementation would fail and require a full XX from scratch. XXfallback recovers in the same connection without dropping the TCP session, reusing the already-sent ephemeral. This means:
- No user-visible reconnect flicker
- No race with incoming messages during the reconnect gap
- No need for the application to implement retry logic for key rotation
5. Architectural preparedness
The state machine decomposition (XxHandshakeState / IkHandshakeState / XxFallbackHandshakeState) with shared process_xx_server_hello_into is clean and opens the door for:
- Future IKpsk2 support (WA has signals of this in newer JS builds)
- Cert chain Ed25519 verification (acknowledged follow-up)
- Custom handshake policies (e.g., always-XX for debugging)
Summary of practical value
For a single-session desktop/mobile client, the main benefit is faster reconnects (~50-100ms saved) and better server behavior parity. For a multi-session gateway library, the compound savings across thousands of connections are meaningful. The XXfallback path eliminates a class of "silent failure during key rotation" bugs that would otherwise require application-level retry logic. The code quality (state machines, thorough error classification, matching WA Web's log lines) makes the handshake layer significantly more maintainable going forward.
Overall: solid PR. The future-dated cert check (issue #1) is the only thing I'd want fixed before merge. Everything else is minor or tracked follow-up.
Mirrors WAWebProcessCertificate.Certificate.js:35 (`if (a && ...)`): the cert chain is only persisted once the device has reached the registered/paired state. Previously we persisted after every successful XX, so a cold-start that displayed a QR but was killed before scanning left the DB with a chain but no `pn` — the next connect picked IK, the server rejected the unregistered identity, the disconnect classified as transient, and the client retried IK forever. Two layers: - `should_persist_cert_chain` skips the SetServerCertChain command when `!device.is_registered()`, matching WA Web exactly. - `select_pattern` returns Xx whenever `!device.is_registered()`, even with a valid cached chain, defending against pre-fix DBs. `Device::is_registered()` mirrors WAWebUserPrefsMultiDevice.isRegistered (`!!(m() && getMaybeMeDevicePn())`) and reuses the same `pn`-based signal already used by `get_client_payload` to choose the login vs registration payload.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tests/handshake_integration.rs (1)
299-308:⚠️ Potential issue | 🟠 MajorThese tests never exercise the SQLite persistence path.
The
pm()helper usesInMemoryBackend, so all assertions aboutserver_cert_chainstay in RAM. If the new column, migration, or DB round-trip has a bug, this suite still passes. You need at least one test that creates aPersistenceManagerbacked by SQLite, writes a cert chain, creates a freshPersistenceManagerpointed at the same file, and verifies the chain survives the restart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/handshake_integration.rs` around lines 299 - 308, The helper pm() only uses InMemoryBackend so tests never exercise SQLite persistence; add a new test that uses the SQLite backend implementation to verify DB round-trips: create a temporary file for the DB, instantiate a PersistenceManager with the SQLite backend (instead of InMemoryBackend), write a server_cert_chain via the PersistenceManager API, drop/close that PersistenceManager, then create a fresh PersistenceManager pointed at the same file and assert the server_cert_chain is preserved; update or add a test function that mirrors the existing assertions but uses the SQLite backend to catch migration/column/round-trip regressions (reference pm(), PersistenceManager, and InMemoryBackend to locate the current helper and replace/augment it).src/handshake.rs (1)
138-144:⚠️ Potential issue | 🟠 MajorAdd
not_beforevalidation to prevent future-dated cert chains from selecting IK.The
select_patternfunction only rejects certs wherenow_secs >= not_after, but ignoresnot_before. A cached chain dated for the future (wherenow_secs < not_before) incorrectly triggers the IK path. Both leaf and intermediate must be within their full validity window.Fix
- if now_secs >= chain.leaf.not_after || now_secs >= chain.intermediate.not_after { + if now_secs < chain.leaf.not_before + || now_secs >= chain.leaf.not_after + || now_secs < chain.intermediate.not_before + || now_secs >= chain.intermediate.not_after + { return HandshakePattern::Xx; }Add a test case to
select_pattern_*tests covering future-dated certs alongside the existing expired-cert tests. Tests currently setnot_before = 1_700_000_000and passnow_secs = 1_800_000_000, so they never exercise the future-dated scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handshake.rs` around lines 138 - 144, The select_pattern logic (in src/handshake.rs, around the block returning HandshakePattern::Ik) only checks not_after and must be extended to also reject chains where now_secs is before not_before for both chain.leaf and chain.intermediate; change the conditional that currently tests now_secs >= leaf.not_after || now_secs >= intermediate.not_after to instead validate that now_secs is within [not_before, not_after] for both leaf and intermediate before returning HandshakePattern::Ik(chain.leaf.key). Update or add unit tests (the select_pattern_* tests) to include a future-dated-cert scenario by setting not_before > now_secs and asserting the function does not choose IK (mirror the existing expired-cert tests), ensuring test timestamps exercise both expired and future-dated cases.
🤖 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/handshake.rs`:
- Around line 222-238: The current logic invalidates cached server certs for any
HandshakePattern::Ik(_) crypto-fatal error, but run_ik_handshake can pivot to
XXfallback so we must only clear cache for pre-fallback IK failures; change
run_ik_handshake to return a granular error (e.g., enum IkHandshakeError {
PreFallback(HandshakeError), PostFallback(HandshakeError) } or similar), update
callers to match on IkHandshakeError, and in the Err arm only increment
ik_handshake_failures and call
persistence_manager.process_command(DeviceCommand::ClearServerCertChain).await
when the error is IkHandshakeError::PreFallback(e) and e.is_crypto_fatal();
leave PostFallback failures untouched.
- Around line 385-402: The test helper cached_chain currently hardcodes
not_before to 1_700_000_000, preventing tests from creating future-dated or
varying validity windows; change cached_chain to accept configurable not_before
values (either a single not_before param or separate intermediate_not_before and
leaf_not_before) and propagate those into the CachedNoiseCert fields (refer to
CachedServerCertChain and CachedNoiseCert in cached_chain), then update all test
callers to pass the desired not_before values (or use a helper overload/default)
so tests can exercise different date scenarios.
In `@tests/handshake_integration.rs`:
- Around line 279-297: The polling loop in wait_for_send currently sleeps 5ms up
to a 3s timeout which can slow CI debugging; modify wait_for_send (and its local
vars start, timeout, transport) to use a bounded iteration counter (e.g.,
compute max_iters = (timeout / poll_interval)) and increment a counter each loop
so you can break with a clear panic earlier, and add a debug log inside the loop
that prints the current iteration and transport.sent.len() every N iterations
(or reduce timeout to 1s) to provide faster feedback in CI.
- Around line 154-161: Remove the redundant suppressor by deleting the `let _ =
sent;` line: `sent` is not otherwise needed because `transport` already stores
an Arc clone of it (constructed via `CaptureTransport { sent: Arc::clone(&sent)
}`), so keep the `sent` variable only to create the `CaptureTransport` and then
remove the unnecessary `let _ = sent;` binding that was only silencing a
warning.
---
Duplicate comments:
In `@src/handshake.rs`:
- Around line 138-144: The select_pattern logic (in src/handshake.rs, around the
block returning HandshakePattern::Ik) only checks not_after and must be extended
to also reject chains where now_secs is before not_before for both chain.leaf
and chain.intermediate; change the conditional that currently tests now_secs >=
leaf.not_after || now_secs >= intermediate.not_after to instead validate that
now_secs is within [not_before, not_after] for both leaf and intermediate before
returning HandshakePattern::Ik(chain.leaf.key). Update or add unit tests (the
select_pattern_* tests) to include a future-dated-cert scenario by setting
not_before > now_secs and asserting the function does not choose IK (mirror the
existing expired-cert tests), ensuring test timestamps exercise both expired and
future-dated cases.
In `@tests/handshake_integration.rs`:
- Around line 299-308: The helper pm() only uses InMemoryBackend so tests never
exercise SQLite persistence; add a new test that uses the SQLite backend
implementation to verify DB round-trips: create a temporary file for the DB,
instantiate a PersistenceManager with the SQLite backend (instead of
InMemoryBackend), write a server_cert_chain via the PersistenceManager API,
drop/close that PersistenceManager, then create a fresh PersistenceManager
pointed at the same file and assert the server_cert_chain is preserved; update
or add a test function that mirrors the existing assertions but uses the SQLite
backend to catch migration/column/round-trip regressions (reference pm(),
PersistenceManager, and InMemoryBackend to locate the current helper and
replace/augment it).
🪄 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: 06828b86-7354-4a17-a36d-8066ca04719f
📒 Files selected for processing (3)
src/handshake.rstests/handshake_integration.rswacore/src/store/device.rs
| fn cached_chain( | ||
| leaf_key: [u8; 32], | ||
| leaf_not_after: i64, | ||
| intermediate_not_after: i64, | ||
| ) -> CachedServerCertChain { | ||
| CachedServerCertChain { | ||
| intermediate: CachedNoiseCert { | ||
| key: [0xCC; 32], | ||
| not_before: 1_700_000_000, | ||
| not_after: intermediate_not_after, | ||
| }, | ||
| leaf: CachedNoiseCert { | ||
| key: leaf_key, | ||
| not_before: 1_700_000_000, | ||
| not_after: leaf_not_after, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test helper cached_chain hardcodes not_before to a fixed value.
The helper sets not_before: 1_700_000_000 for both certs, which means the tests can't easily cover future-dated scenarios without creating a separate helper or passing not_before as a parameter.
Consider making not_before configurable
fn cached_chain(
leaf_key: [u8; 32],
+ leaf_not_before: i64,
leaf_not_after: i64,
+ intermediate_not_before: i64,
intermediate_not_after: i64,
) -> CachedServerCertChain {
CachedServerCertChain {
intermediate: CachedNoiseCert {
key: [0xCC; 32],
- not_before: 1_700_000_000,
+ not_before: intermediate_not_before,
not_after: intermediate_not_after,
},
leaf: CachedNoiseCert {
key: leaf_key,
- not_before: 1_700_000_000,
+ not_before: leaf_not_before,
not_after: leaf_not_after,
},
}
}📝 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.
| fn cached_chain( | |
| leaf_key: [u8; 32], | |
| leaf_not_after: i64, | |
| intermediate_not_after: i64, | |
| ) -> CachedServerCertChain { | |
| CachedServerCertChain { | |
| intermediate: CachedNoiseCert { | |
| key: [0xCC; 32], | |
| not_before: 1_700_000_000, | |
| not_after: intermediate_not_after, | |
| }, | |
| leaf: CachedNoiseCert { | |
| key: leaf_key, | |
| not_before: 1_700_000_000, | |
| not_after: leaf_not_after, | |
| }, | |
| } | |
| } | |
| fn cached_chain( | |
| leaf_key: [u8; 32], | |
| leaf_not_before: i64, | |
| leaf_not_after: i64, | |
| intermediate_not_before: i64, | |
| intermediate_not_after: i64, | |
| ) -> CachedServerCertChain { | |
| CachedServerCertChain { | |
| intermediate: CachedNoiseCert { | |
| key: [0xCC; 32], | |
| not_before: intermediate_not_before, | |
| not_after: intermediate_not_after, | |
| }, | |
| leaf: CachedNoiseCert { | |
| key: leaf_key, | |
| not_before: leaf_not_before, | |
| not_after: leaf_not_after, | |
| }, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/handshake.rs` around lines 385 - 402, The test helper cached_chain
currently hardcodes not_before to 1_700_000_000, preventing tests from creating
future-dated or varying validity windows; change cached_chain to accept
configurable not_before values (either a single not_before param or separate
intermediate_not_before and leaf_not_before) and propagate those into the
CachedNoiseCert fields (refer to CachedServerCertChain and CachedNoiseCert in
cached_chain), then update all test callers to pass the desired not_before
values (or use a helper overload/default) so tests can exercise different date
scenarios.
| /// Waits up to ~3s for the captured-sent buffer to reach `min_count` | ||
| /// entries. Polls every 5 ms; tight enough for unit tests. | ||
| async fn wait_for_send(transport: &Arc<CaptureTransport>, min_count: usize) { | ||
| let start = wacore::time::Instant::now(); | ||
| let timeout = Duration::from_secs(3); | ||
| loop { | ||
| if transport.sent.lock().unwrap().len() >= min_count { | ||
| return; | ||
| } | ||
| if start.elapsed() >= timeout { | ||
| panic!( | ||
| "transport did not produce {} sends within deadline (got {})", | ||
| min_count, | ||
| transport.sent.lock().unwrap().len() | ||
| ); | ||
| } | ||
| tokio::time::sleep(Duration::from_millis(5)).await; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Polling loop is acceptable for tests but consider a bounded iteration count.
The wait_for_send function polls every 5ms with a 3-second timeout. This works, but if something goes wrong, developers will wait the full 3 seconds before seeing a failure. Consider adding a debug log inside the loop or reducing the timeout for faster feedback in CI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/handshake_integration.rs` around lines 279 - 297, The polling loop in
wait_for_send currently sleeps 5ms up to a 3s timeout which can slow CI
debugging; modify wait_for_send (and its local vars start, timeout, transport)
to use a bounded iteration counter (e.g., compute max_iters = (timeout /
poll_interval)) and increment a counter each loop so you can break with a clear
panic earlier, and add a debug log inside the loop that prints the current
iteration and transport.sent.len() every N iterations (or reduce timeout to 1s)
to provide faster feedback in CI.
…, dedup Addresses self-review nits before merge: - `select_pattern`: also gate on `not_before` for both leaf and intermediate, so a backwards-clock-skewed device (RTC reset, frozen container clock) refuses IK against a not-yet-valid cert. The `not_after` check alone left a window where `now < not_before < not_after` and IK would proceed. WA Web has the same weakness; this is defense-in-depth, not a divergence. Two new unit tests cover both certs. - New integration test `ik_continue_does_not_overwrite_cached_chain`: pre-seeds a sentinel `not_after` and asserts it survives an IK Continue. Captures the invariant that `do_handshake` issues `SetServerCertChain` only on XX/XX-fallback, never on IK Continue. - New integration test `xx_after_pair_success_persists_cert_chain`: drives the full WA Web post-pair flow — XX-1 unpaired (no persist), pair-success populates `pn`, XX-2 paired (persist now). The registration gate that landed in ce22c51 had no integration coverage for the second leg until now. - Dedup `build_cert_chain_bytes`: moved to `wacore_noise::test_util` behind a `test-util` feature; consumed by both the unit tests in `wacore-noise` and the integration tests in `whatsapp-rust`. - Realigned `select_pattern` doc comment with the actual `>= threshold` check (was reading "<").
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/noise/src/handshake.rs (1)
137-205:⚠️ Potential issue | 🔴 CriticalVerify the certificate signatures before trusting or caching this chain.
This path never checks the intermediate signature against
WA_CERT_PUB_KEYor the leaf signature against the intermediate key. Right now any attacker who can complete the XX transcript with their own static can mint a self-consistentCertChainand pass this function. That breaks the authentication boundary for both fresh XX and XXfallback.
♻️ Duplicate comments (2)
tests/handshake_integration.rs (1)
259-268:⚠️ Potential issue | 🟠 MajorAdd one SQLite-backed reconnect test.
These helpers only build
PersistenceManageronInMemoryBackend, so the newserver_cert_chaincolumn and migration never get exercised. This PR changes persisted schema; right now a broken SQLite round-trip can still leave this whole suite green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/handshake_integration.rs` around lines 259 - 268, The current pm() helper always constructs a PersistenceManager with InMemoryBackend so the new SQLite migration and server_cert_chain column never get exercised; add a new helper (or extend pm()) to build a PersistenceManager backed by a real SQLite backend (e.g. using wacore/sqlite backend or the crate's SQLite implementation) and write a new reconnect integration test that uses that SQLite-backed PersistenceManager to exercise the migration path and round-trip persistence; reference the PersistenceManager::new, InMemoryBackend::new, and the server_cert_chain schema change so the test opens a temporary SQLite file, constructs Arc<dyn Backend> for the SQLite backend, initializes PersistenceManager with it, performs the reconnect scenario, and asserts the server_cert_chain column/migration applied correctly.src/handshake.rs (1)
56-102:⚠️ Potential issue | 🟠 MajorDon't treat malformed
ServerHelloshapes as cache-poisoning by default.
ProtoDecode,IncompleteResponse,InvalidLength, andInvalidKeyLengthstill clearserver_cert_chainand bumpik_handshake_failures. Those errors prove the reply was malformed, not that the cached static is stale. One bad frame can still force the next reconnect down XX and hide the real transport/server issue. Keep cache invalidation limited to authenticated decrypt/cert-mismatch cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handshake.rs` around lines 56 - 102, In is_crypto_fatal(), stop classifying malformed/server-hello-shape errors as crypto-fatal: remove Core::ProtoDecode(_), Core::IncompleteResponse, Core::InvalidLength { .. }, and the top-level Core::InvalidKeyLength from the true arm and instead treat them as non-crypto-fatal (false). Leave only authenticated-decrypt and cert-mismatch cases as true (e.g., Core::Noise(NoiseError::Decrypt(_)), Core::Noise(NoiseError::CiphertextTooShort), Core::Noise(NoiseError::InvalidKeyLength { .. }) and Core::CertVerification(_)); ensure all other Core variants (including Core::ProtoDecode, Core::IncompleteResponse, Core::InvalidLength, Core::InvalidKeyLength, Core::Proto(_), Core::Crypto(_), and the remaining Noise::... variants) return false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/handshake.rs`:
- Around line 56-102: In is_crypto_fatal(), stop classifying
malformed/server-hello-shape errors as crypto-fatal: remove
Core::ProtoDecode(_), Core::IncompleteResponse, Core::InvalidLength { .. }, and
the top-level Core::InvalidKeyLength from the true arm and instead treat them as
non-crypto-fatal (false). Leave only authenticated-decrypt and cert-mismatch
cases as true (e.g., Core::Noise(NoiseError::Decrypt(_)),
Core::Noise(NoiseError::CiphertextTooShort),
Core::Noise(NoiseError::InvalidKeyLength { .. }) and Core::CertVerification(_));
ensure all other Core variants (including Core::ProtoDecode,
Core::IncompleteResponse, Core::InvalidLength, Core::InvalidKeyLength,
Core::Proto(_), Core::Crypto(_), and the remaining Noise::... variants) return
false.
In `@tests/handshake_integration.rs`:
- Around line 259-268: The current pm() helper always constructs a
PersistenceManager with InMemoryBackend so the new SQLite migration and
server_cert_chain column never get exercised; add a new helper (or extend pm())
to build a PersistenceManager backed by a real SQLite backend (e.g. using
wacore/sqlite backend or the crate's SQLite implementation) and write a new
reconnect integration test that uses that SQLite-backed PersistenceManager to
exercise the migration path and round-trip persistence; reference the
PersistenceManager::new, InMemoryBackend::new, and the server_cert_chain schema
change so the test opens a temporary SQLite file, constructs Arc<dyn Backend>
for the SQLite backend, initializes PersistenceManager with it, performs the
reconnect scenario, and asserts the server_cert_chain column/migration applied
correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f6db6a92-a018-4dcb-a4b0-61a68a115483
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlsrc/handshake.rstests/handshake_integration.rswacore/noise/Cargo.tomlwacore/noise/src/handshake.rswacore/noise/src/lib.rswacore/noise/src/test_util.rs
`do_handshake`'s post-failure invalidation gate previously fired for any crypto-fatal error returned by `run_ik_handshake`, including failures that surfaced AFTER the IK→XXfallback pivot. By the time the server has replied with `static.is_some()` and we entered the XXfallback branch, the IK ClientHello has been processed end-to-end on the server side and the cache is no longer the implicated party. Subsequent failures (e.g. malformed XXfallback ServerHello, AEAD failure on the new server static, transport error mid-finish) are XXfallback / wire issues; clearing the chain and bumping `ik_handshake_failures` for those events is wrong. `run_ik_handshake` now writes through a `&mut bool fallback_taken` the moment we enter the Fallback arm — before any operation that could fail — and the orchestrator gates invalidation on `!fallback_taken && ...`. Adds `post_xxfallback_failure_does_not_invalidate_ik_cache`: pre-seeds a sentinel `not_after`, drives a responder that emits a fallback-shaped ServerHello with garbage AEAD payloads, asserts the failure surfaces as crypto-fatal, the cached chain is preserved, and the counter stays at 0. Cleanup: removed the redundant `let _ = sent;` warning suppressor in `new_transport_pair`; the binding can be inlined cleanly now.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Replaces the prior "exported but unused, worth a follow-up" framing on WA_CERT_PUB_KEY with an explicit non-goal statement at its declaration. Three reasons we are not gating the handshake on signature verification: 1. The in-tree mock server cannot produce signatures under the real issuer key, so verification would block the entire e2e suite or force a fragile danger-skip-cert-verify feature gate. 2. whatsmeow ships the same gap; we are not claiming stronger guarantees than the upstream Go reference. 3. The cert chain travels inside the Noise AEAD-protected handshake, so passive substitution is impossible. Server-static-key compromise is outside this implementation's threat model. The constant stays exported (removing it would be a SemVer break for downstream pinners) but the doc-comment now warns future contributors not to wire it into verify_server_cert without also shipping a non-load-bearing escape hatch for the e2e crate.
|
@claude review carefully everything |
|
Claude finished @jlucaso1's task in 3m 23s —— View job Careful Full Review of Noise_IK + XXfallback PR
I read every changed file end-to-end (~2900 lines across 21 files). This is a well-structured PR — the state machine decomposition is clean, the Below are my remaining findings, ordered by severity. Remaining Issues1.
|
| Area | Assessment |
|---|---|
| State machine decomposition | Clean separation of XxHandshakeState / IkHandshakeState / XxFallbackHandshakeState with shared process_xx_server_hello_into. Easy to reason about, easy to extend. |
fallback_taken gate (commit 71a8702) |
Correctly solved the post-pivot cache-invalidation bug from CodeRabbit's feedback. The &mut bool approach is simple and effective. |
Registration gate (commit ce22c51) |
should_persist_cert_chain + is_registered() prevent the unpaired-then-restart infinite loop. The two integration tests (unpaired_xx_does_not_persist_cert_chain, xx_after_pair_success_persists_cert_chain) nail the exact regression scenario. |
| Error taxonomy | is_transient() / is_crypto_fatal() with exhaustive match arms is good for maintainability — adding a new HandshakeError variant forces a classification decision. |
IK Continue no-overwrite test (ik_continue_does_not_overwrite_cached_chain) |
Uses a sentinel not_after value to detect spurious SetServerCertChain — clever and non-fragile. |
| Noise § 5.2 conformance | The zero-padding fix for protocol_name <= 32 with unit tests pinning h0 against a hand-computed reference value is thorough. |
Shared test util (wacore-noise/src/test_util.rs + test-util feature) |
Good pattern — avoids duplicating cert-chain builders across crate boundaries. |
| Integration test architecture | The InProcessServer + CaptureTransport + async_channel pattern avoids any network dependency while testing the full orchestration path. Well-structured. |
| Documentation | protocol_architecture.md additions (invalidation matrix, log lines, cert storage shape) are high-signal and match the code. |
Nitpicks (Non-Blocking)
| Location | Note |
|---|---|
wacore/noise/src/handshake.rs:407-408 |
The CertVerification(format!("Error parsing server hello: {e}")) wrapping of a parse error is semantically off — it's really a protobuf parse failure, not a cert issue. But changing it would alter is_crypto_fatal() classification, so leave it unless the error taxonomy gets a broader cleanup. |
wacore/noise/src/handshake.rs:220-221 |
not_before() as i64 / not_after() as i64 — the u64 → i64 cast is safe for realistic Unix timestamps but could silently wrap for pathological values. A try_into() would be more defensive. Very low priority. |
tests/handshake_integration.rs:239-254 |
The wait_for_send polling loop (5ms sleep, 3s timeout) is fine for tests but could be replaced with a tokio::sync::Notify for instant wakeup. Current approach works and is simple. |
src/handshake.rs:19 |
NOISE_HANDSHAKE_RESPONSE_TIMEOUT = 20s — generous. WA Web uses ~15s. Not wrong, just slower to detect a dead connection. |
wacore/noise/src/handshake.rs:609 |
let _cert_plaintext = noise.decrypt(&cert_payload)?; — In the IK Continue path, the decrypted cert plaintext is thrown away. This is correct (the cached chain is authoritative), but worth noting that the cert is still decrypted purely for transcript authentication. A comment would help future readers understand why. |
Summary
This is a solid, well-tested PR that correctly implements the XX → IK → XXfallback pattern selection with appropriate cache invalidation semantics. The follow-up commits (particularly the fallback_taken gate, the registration-gated persistence, and the not_before check) addressed the most important issues from prior reviews.
Remaining actionable items (all minor):
- Fix the doc-comment on
verify_server_cert(says "they were just checked" about signatures that aren't verified) - Add a corrupt-blob recovery test for the SQLite decode path
- Optionally: add a future-dated cert test to
select_patterntests for completeness
None of these are merge-blocking. The architecture, correctness, and test coverage are strong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/handshake.rs (1)
83-89:⚠️ Potential issue | 🟠 MajorDon’t clear the IK cache on parse/shape errors.
ProtoDecode,IncompleteResponse, andInvalidLengthtell us the server hello was malformed; they do not prove the cached static is stale. Clearingserver_cert_chainfor those cases turns one bad or truncated response into a forced XX on the next reconnect. Keep the crypto-fatal bucket to actual auth/decrypt/cert-mismatch failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handshake.rs` around lines 83 - 89, The check that classifies parse/shape errors as crypto-fatal is too broad: remove Core::ProtoDecode(_), Core::IncompleteResponse, and Core::InvalidLength { .. } from the branch that triggers clearing the IK/cache (the block currently listing Core::IncompleteResponse | Core::InvalidLength { .. } | Core::InvalidKeyLength | Core::ProtoDecode(_) => true). Update the match/conditional in handshake.rs so only genuine crypto/auth/decrypt/certificate-mismatch failures (e.g., variants indicating authentication failure, decryption failure, or explicit cert mismatch) return true and cause server_cert_chain to be cleared; leave parsing/truncation errors to return false so the cached static is preserved.tests/handshake_integration.rs (1)
257-265:⚠️ Potential issue | 🟠 MajorThese integration tests still never hit the SQLite path.
pm()is backed byInMemoryBackend, so everyserver_cert_chainassertion stays in RAM. That means the new column, migration, and restart round-trip can break while this suite still goes green. Add one SQLite-backed reconnect test that writes the chain, creates a freshPersistenceManageron the same DB, and verifies the cache survives restart. That’s the practical safety net for this PR’s migration story.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/handshake_integration.rs` around lines 257 - 265, The tests use pm() which currently constructs an InMemoryBackend so the SQLite migration path is never exercised; change or supplement the test to add an integration test that uses a SQLite-backed backend: create a temporary sqlite file, build a PersistenceManager via PersistenceManager::new(backend) pointing at that file, write the server_cert_chain (use the same APIs the test already uses), drop the manager, recreate a fresh PersistenceManager::new against the same SQLite file, and assert the server_cert_chain value is preserved across restart. Locate pm(), the PersistenceManager::new calls, and the server_cert_chain assertions in the handshake_integration.rs test and implement the SQLite-backed variant there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/handshake_integration.rs`:
- Around line 632-637: The comment is misleading: pairing success is modeled by
calling PersistenceManager::process_command with
wacore::store::DeviceCommand::SetId(...) rather than mutating pn directly;
update the comment above the process_command(...) call to state that
pair-success is represented by invoking
pm.process_command(DeviceCommand::SetId(...)) and that direct mutation of pn is
prohibited outside tests, referencing process_command and DeviceCommand::SetId
to guide readers.
---
Duplicate comments:
In `@src/handshake.rs`:
- Around line 83-89: The check that classifies parse/shape errors as
crypto-fatal is too broad: remove Core::ProtoDecode(_),
Core::IncompleteResponse, and Core::InvalidLength { .. } from the branch that
triggers clearing the IK/cache (the block currently listing
Core::IncompleteResponse | Core::InvalidLength { .. } | Core::InvalidKeyLength |
Core::ProtoDecode(_) => true). Update the match/conditional in handshake.rs so
only genuine crypto/auth/decrypt/certificate-mismatch failures (e.g., variants
indicating authentication failure, decryption failure, or explicit cert
mismatch) return true and cause server_cert_chain to be cleared; leave
parsing/truncation errors to return false so the cached static is preserved.
In `@tests/handshake_integration.rs`:
- Around line 257-265: The tests use pm() which currently constructs an
InMemoryBackend so the SQLite migration path is never exercised; change or
supplement the test to add an integration test that uses a SQLite-backed
backend: create a temporary sqlite file, build a PersistenceManager via
PersistenceManager::new(backend) pointing at that file, write the
server_cert_chain (use the same APIs the test already uses), drop the manager,
recreate a fresh PersistenceManager::new against the same SQLite file, and
assert the server_cert_chain value is preserved across restart. Locate pm(), the
PersistenceManager::new calls, and the server_cert_chain assertions in the
handshake_integration.rs test and implement the SQLite-backed variant there.
🪄 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: 6f6aab33-1618-4be8-976d-bc7102046b3f
📒 Files selected for processing (2)
src/handshake.rstests/handshake_integration.rs
Sweep over the comments added during this PR cycle. Drops paraphrased JS-source citations and multi-paragraph regression rationales that restate code or test names; keeps only the WA Web file:line refs that are non-obvious wire-protocol pointers and the assert messages that load-bear at failure time. Net -149 lines.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
Brings the Noise handshake stack to functional parity with WhatsApp Web's
WAWebOpenChatSocket: cold-start XX, IK on reconnect with cached server static, and XXfallback when the server rejects an in-flight IK. Saves ~1 RTT per reconnection and ships a 0-RTT login payload, matching the official client.Before this change, the client was XX-only on every connect (same as whatsmeow / Baileys). After this change:
serverStaticPub→ IK (1 RTT + 0-RTT payload)static != null→ XXfallback (1 RTT, reuses already-sent ephemeral)Why parity matters
Reverse-engineered from
docs/captured-js/WAWeb/Open/ChatSocket.js:73-275,WAWeb/Process/Certificate.js,WAWeb/User/PrefsInfoStore.js. The dispatcher logic, persistence shape, log lines, and counter semantics all mirror the JS flow byte-for-byte where possible (table inagent_docs/protocol_architecture.md).Commits (each independently green:
cargo fmt + clippy --all --tests + test --workspace --exclude e2e-tests)fix(noise)!protocol_name <= 32per Noise § 5.2 (was hashing — would silently break unpadded names)test(noise)h0for XX withWA_CONN_HEADERagainst hand-computed referencerefactor(consts)!NOISE_PATTERN_XX/_IK/_XXFALLBACK, dropNOISE_START_PATTERNfeat(store)CachedServerCertChainonDevice+DeviceCommand::Set/ClearServerCertChain+ SQLite migrationrefactor(noise)!XxHandshakeState/IkHandshakeState/XxFallbackHandshakeState+IkServerHelloOutcomeenum + 4 unit tests with self-responderfeat(handshake)Client.ik_handshake_failures: AtomicU32, invalidation policy distinguishing transient vs crypto-fataltest(handshake)h0derivation matches)test(handshake)docs(handshake)protocol_architecture.mdBreaking changes (pre-1.0)
wacore::handshake::HandshakeStateremoved; replace withXxHandshakeState. Constructor no longer takes a pattern arg (XX is implicit).wacore::handshake::HandshakeState::finishreturnsXxHandshakeOutcomeinstead of(NoiseCipher, NoiseCipher).wacore_binary::consts::NOISE_START_PATTERNremoved; useNOISE_PATTERN_XX.whatsapp_rust::handshake::do_handshakesignature: now takes&PersistenceManagerand&AtomicU32(counter) instead of&Device.2026-04-26-000000_add_server_cert_chain(adds nullableserver_cert_chain BLOBcolumn).Test plan
cargo fmt --allcargo clippy --all --tests(zero warnings)cargo test --workspace --exclude e2e-tests— 13 new tests, all passing alongside the existing 1300+static + payload(not just ephemeral)server_cert_chainis repopulated on XX/XX-fallback success and untouched on IK Continue successIntentional non-goal: Ed25519 cert chain signature verification
WA_CERT_PUB_KEYis exported inwacore/noise/src/handshake.rsbut never used to verify the intermediate cert's Ed25519 signature, and this stays that way. Trade-offs:bartender) cannot produce signatures under the real Meta-controlled issuer key. Enabling verification would block the entire e2e suite or force a fragiledanger-skip-cert-verifyfeature gate.The constant is kept exported (SemVer) and a doc-comment at its definition records the rationale so future contributors don't reopen the discussion. CodeRabbit flagged this as a blocker; we are not blocking on it.
Minor divergences from WA Web (intentional)
ik_handshake_failuresonly on crypto-fatal errors; WA Web bumps on any failure with positive network status. Our heuristic avoids losing the IK cache to a wifi blip.WAWebUserPrefsScreenLock.getScreenLockEnabled()) — N/A for a Rust library; storage encryption is an orthogonal concern.Followups worth considering
Devicerow in SQLite (touchesnoise_key.privKeyetc., orthogonal hardening).