fix(pair)!: map Android PlatformType to Chrome, drop Unknown variant#601
Merged
fix(pair)!: map Android PlatformType to Chrome, drop Unknown variant#601
Conversation
PR #595 mapped `PlatformType::AndroidPhone/Tablet/Ambiguous` to the dedicated wire letters `'e'`/`'d'`/`'f'`. Empirical reproduction shows the server rejects those bytes with `400 bad-request` from any client that doesn't carry Android-side attestation, so the auto-derivation now collapses Android values to `Chrome` (`'1'`) — what real WA Web on Chrome-Android emits per `WAWebCompanionRegClientUtils.DEVICE_PLATFORM` (`info().name === "Chrome"`) and what the server accepts. The `Unknown` variant (wire byte `'0'`) is removed from the public enum. WA Web only ever produces `'0'` when `info().name` is null, which is unreachable in real browsers; the server rejects it from any non-WA-Web caller. `#[default]` now points to `OtherWebClient` (`'9'`), the catch-all WA Web emits when the browser detection returns an unknown name. Callers that need the Android letters can still opt in via `PairCodeOptions::platform_id` for contexts with legitimate Android attestation. While here, factored out a private `build_id_and_display` helper in `pair_code.rs` (the os→display step is now in one place) and merged some table-shaped tests into single parametrised loops.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
Drop test docstrings that just rephrase the test name, drop the DRY-explanation on `build_id_and_display`, and shorten the doc on `companion_web_client_type_for_platform` and the enum to keep just the attestation/server-rejection rationale.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
wacore/src/pair_code.rs (1)
733-785:⚠️ Potential issue | 🟡 MinorTighten this allow-list or the regression gets too soft.
SERVER_ACCEPT_LISTstill admits'0'and the Android letters, andKNOWN_LABELSstill admitsAndroid. So this test can keep passing even if auto-derived mapping slips back to values this PR is explicitly moving away from. For the derived path, lock it to browser labels and'1'..'9'.Suggested test tightening
- const SERVER_ACCEPT_LIST: &[u8] = b"0123456789abcdefghijklm"; - const KNOWN_LABELS: &[&str] = &[ - "Chrome", "Edge", "Firefox", "IE", "Opera", "Safari", "Android", - ]; + const DERIVED_WIRE_BYTES: &[u8] = b"123456789"; + const KNOWN_LABELS: &[&str] = &[ + "Chrome", "Edge", "Firefox", "IE", "Opera", "Safari", + ]; @@ - SERVER_ACCEPT_LIST.contains(&id.wire_byte()), - "{pt:?} wire byte {:?} outside server accept list", + DERIVED_WIRE_BYTES.contains(&id.wire_byte()), + "{pt:?} wire byte {:?} outside derived allow-list", id.wire_byte() as char, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/pair_code.rs` around lines 733 - 785, The test derive_display_uses_known_label_for_every_proto_variant is too permissive: update SERVER_ACCEPT_LIST and KNOWN_LABELS used in that test so the derived path is locked to browser labels and the wire bytes '1'..'9' only; specifically, change SERVER_ACCEPT_LIST to only allow bytes for '1' through '9' and remove non-browser entries such as "Android" from KNOWN_LABELS (keep only "Chrome", "Edge", "Firefox", "IE", "Opera", "Safari"), leaving the rest of the assertions (derive_companion_platform, props(...) and the trailing " (Linux)" check) unchanged.wacore/src/companion_reg.rs (1)
15-55: 🛠️ Refactor suggestion | 🟠 MajorMove the wire bytes onto the enum definition.
This is still a wire-facing enum with a separate
wire_byte()match, which leaves the source of truth split right where this PR is changing the public variants. DeriveWireEnumand put the byte on each variant so the wire contract can't drift from the enum.As per coding guidelines, "Every protocol enum must use
#[derive(WireEnum)]; the#[wire = \"...\"]or#[wire = NUM]attribute is the single source of truth for each variant's wire value; do not also deriveserde::Serialize/Deserializeor add#[serde(rename_all)]."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/companion_reg.rs` around lines 15 - 55, Convert this into a true wire enum: add #[derive(WireEnum)] to CompanionWebClientType, annotate each variant with the per-variant wire attribute (e.g. #[wire = "1"] for Chrome, "2" for Edge, "3" for Firefox, "4" for Ie, "5" for Opera, "6" for Safari, "7" for Electron, "8" for Uwp, "9" for OtherWebClient, "d"/"e"/"f" for the Android variants), then remove the manual wire_byte() impl so the wire mapping is single-source-of-truth on the enum variants; keep the #[default] on OtherWebClient and update any call sites that used CompanionWebClientType::wire_byte() to use the WireEnum-provided accessor (or the new canonical API).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@wacore/src/companion_reg.rs`:
- Around line 15-55: Convert this into a true wire enum: add #[derive(WireEnum)]
to CompanionWebClientType, annotate each variant with the per-variant wire
attribute (e.g. #[wire = "1"] for Chrome, "2" for Edge, "3" for Firefox, "4" for
Ie, "5" for Opera, "6" for Safari, "7" for Electron, "8" for Uwp, "9" for
OtherWebClient, "d"/"e"/"f" for the Android variants), then remove the manual
wire_byte() impl so the wire mapping is single-source-of-truth on the enum
variants; keep the #[default] on OtherWebClient and update any call sites that
used CompanionWebClientType::wire_byte() to use the WireEnum-provided accessor
(or the new canonical API).
In `@wacore/src/pair_code.rs`:
- Around line 733-785: The test
derive_display_uses_known_label_for_every_proto_variant is too permissive:
update SERVER_ACCEPT_LIST and KNOWN_LABELS used in that test so the derived path
is locked to browser labels and the wire bytes '1'..'9' only; specifically,
change SERVER_ACCEPT_LIST to only allow bytes for '1' through '9' and remove
non-browser entries such as "Android" from KNOWN_LABELS (keep only "Chrome",
"Edge", "Firefox", "IE", "Opera", "Safari"), leaving the rest of the assertions
(derive_companion_platform, props(...) and the trailing " (Linux)" check)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 20aac55d-2839-4a11-bc70-af1547169585
📒 Files selected for processing (3)
wacore/src/companion_reg.rswacore/src/pair.rswacore/src/pair_code.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
companion_platform_idforPlatformType::AndroidPhone/Tablet/Ambiguousnow producesChrome('1') +"Chrome (Android)", matching what real WA Web on Chrome-Android emits perWAWebCompanionRegClientUtils.DEVICE_PLATFORM. Empirical reproduction showed the server rejects the dedicated Android letters'e'/'d'/'f'(introduced in fix(pair): emit empirically correct companion_platform_id for Android #595) with400 bad-requestfrom any client that lacks Android-side attestation. The letters are still reachable through explicit opt-in viaPairCodeOptions::platform_idfor callers with legitimate Android signing material.CompanionWebClientType::Unknown(wire byte'0') is removed from the public enum. WA Web only emits'0'wheninfo().nameis null — unreachable in real browsers — and the server rejects it from any non-WA-Web caller.#[default]now points toOtherWebClient('9'), the catch-all WA Web actually emits when browser detection returns an unknown name. Breaking for the pre-1.0 API.pair_code.rsfactors the shared os→display step into a privatebuild_id_and_displayhelper, and a few table-shaped tests were merged into single parametrised loops.Test plan
cargo test -p wacore --lib— 644 passcargo build --workspacecargo clippy --all --tests— cleancargo fmt --all -- --check— cleanDeviceProps { platform_type: AndroidPhone, os: "Android", .. }returns alink_code_pairing_ref(no400 bad-request)