fix(lid_pn): WA Web compliant signal address for Hosted JIDs - #605
Merged
Conversation
`resolve_encryption_jid` previously fell through Hosted JIDs untouched,
leaving Signal session keys at `{user}@hosted` while WA Web's
`SignalAddress.toString()` (`WAWeb/Signal/Address.js`) keys them at
`{lid_user}@hosted.lid` once a LID mapping is known. Same shape as the
existing PN → LID upgrade — a single `lid_pn_cache` lookup mirrored
across Hosted → HostedLid.
Branches:
Lid / HostedLid → already canonical, returned as-is.
Pn → upgrade to Lid when mapping known.
Hosted → upgrade to HostedLid when mapping known.
Hot path is unchanged: PN/LID dispatch costs the same as before; the
new Hosted branch is dormant for current send paths because they
filter `is_hosted` out before reaching the resolver, but the logic is
in place for any future hosted support and matches WA Web's intent.
Also pins down the post-#391 wire encoding for direct-constructed
Hosted/HostedLid JIDs in a regression test — `Jid::new(_, Hosted)`
defaults `agent=0`, but `server_to_domain_type` derives the correct
byte (128/129) from the server enum regardless.
|
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
WalkthroughI changed resolve_encryption_jid to derive the upgraded server namespace from the incoming JID’s server, consult the lid_pn cache by user, and reconstruct a LID-style Jid on cache hit (preserving device/agent/integrator); no-op on cache miss. Added encoder unit test for Hosted/HostedLid AD_JID domain-type encoding. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/lid_pn.rs`:
- Around line 307-308: Update the documentation comment that currently reads
"misformed JID" in the hosted-device note to use the correct word "malformed"
instead; locate the doc comment containing the phrase "hosted; we preserve
whatever device the input carries so a misformed JID is still routable rather
than silently dropped." and replace "misformed" with "malformed" to fix the
typo.
- Around line 566-583: The test
test_resolve_encryption_jid_hosted_with_lid_upgrades_to_hosted_lid currently
only verifies device=99 which could mask regressions; add an additional
assertion case (or duplicate the test logic) using a different hosted.device
value (e.g., device=7) when creating Jid::new(user, Server::Hosted) and call
client.resolve_encryption_jid(&hosted).await, then assert resolved.user == lid,
resolved.server == Server::HostedLid, and resolved.device == 7 to ensure the
input device is preserved and not hardcoded to 99.
🪄 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: 65c1c303-1116-4815-979b-4db688e1b626
📒 Files selected for processing (2)
src/client/lid_pn.rswacore/binary/src/encoder.rs
Single match maps Pn → Lid and Hosted → HostedLid; cache lookup runs once with target.clone() as the no-mapping fallback. Drops the upgrade_to_lid_namespace helper (one caller) and trims the doc to the WA Web reference. Same allocation profile (one Jid clone on the fallback path, one CompactString conversion on upgrade — both unavoidable given the Option<String> return from lid_pn_cache).
Loop over [99, 7] in test_resolve_encryption_jid_hosted_with_lid_upgrades_to_hosted_lid to prove the input device is preserved, not coerced to 99. Same fixture, same upgrade path; the asymmetric value catches a regression that hardcodes :99 (which would still pass on the original :99 case).
![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: f0a0ee4d15
ℹ️ 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".
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/client/lid_pn.rs (1)
538-556:⚠️ Potential issue | 🟡 MinorAdd one non-99 Hosted-device upgrade case to harden this contract.
Right now Line 550 only validates
device = 99. Add one more case (for exampledevice = 7) so future regressions that pin device values can’t slip through.Suggested test addition
#[tokio::test] async fn test_resolve_encryption_jid_hosted_with_lid_upgrades_to_hosted_lid() { @@ let mut hosted = Jid::new(user, Server::Hosted); hosted.device = 99; let resolved = client.resolve_encryption_jid(&hosted).await; @@ assert_eq!(resolved.server, Server::HostedLid); assert_eq!(resolved.device, 99); + + let mut hosted_non_default = Jid::new(user, Server::Hosted); + hosted_non_default.device = 7; + let resolved_non_default = client.resolve_encryption_jid(&hosted_non_default).await; + assert_eq!(resolved_non_default.user, lid); + assert_eq!(resolved_non_default.server, Server::HostedLid); + assert_eq!(resolved_non_default.device, 7); }Based on learnings, WA Web address derivation is device-scoped (
:deviceis preserved when device != 0), so non-default device preservation should be explicitly locked by tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/lid_pn.rs` around lines 538 - 556, Add a second non-99 device case to the test_resolve_encryption_jid_hosted_with_lid_upgrades_to_hosted_lid test: after creating the client, mapping lid->user and asserting device 99 upgrades to Server::HostedLid, also create another Jid::new(user, Server::Hosted) with device = 7 (or another non-zero/non-99 value), call client.resolve_encryption_jid(&hosted) and assert resolved.user == lid, resolved.server == Server::HostedLid and resolved.device == 7; use the same helpers (create_test_client, add_lid_pn_mapping, resolve_encryption_jid) so the contract that non-default devices are preserved is explicitly verified.
🤖 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/client/lid_pn.rs`:
- Around line 538-556: Add a second non-99 device case to the
test_resolve_encryption_jid_hosted_with_lid_upgrades_to_hosted_lid test: after
creating the client, mapping lid->user and asserting device 99 upgrades to
Server::HostedLid, also create another Jid::new(user, Server::Hosted) with
device = 7 (or another non-zero/non-99 value), call
client.resolve_encryption_jid(&hosted) and assert resolved.user == lid,
resolved.server == Server::HostedLid and resolved.device == 7; use the same
helpers (create_test_client, add_lid_pn_mapping, resolve_encryption_jid) so the
contract that non-default devices are preserved is explicitly verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f27890b6-6a10-4c05-b1ac-a7f4a64bccd3
📒 Files selected for processing (2)
src/client/lid_pn.rswacore/binary/src/encoder.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/lid_pn.rs`:
- Around line 549-559: The test loop constructing Jid and calling
client.resolve_encryption_jid currently only asserts device round-trips; extend
the assertions to also verify that the agent and integrator fields are
preserved. After obtaining resolved from client.resolve_encryption_jid(&hosted),
add assertions that resolved.agent == hosted.agent and resolved.integrator ==
hosted.integrator (or the expected preserved values) alongside the existing
user/server/device asserts so the full Jid contract (user, server, device,
agent, integrator) is locked in the test.
🪄 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: 56be7e94-454e-439d-8ddc-9401b7ad9cca
📒 Files selected for processing (1)
src/client/lid_pn.rs
Set non-default agent (0xAB) and integrator (0xBEEF) on the input and assert both round-trip alongside the existing user/server/device checks. Closes the gap where a regression that drops or rewrites these fields would slip through with all-zero defaults.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
3 tasks
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.
Background
Audit of the Hosted/HostedLid path triggered by a question on the encoder `domain_type` bug (#391) surfaced a deeper Signal-address divergence. WA Web's `SignalAddress.toString()` in `WAWeb/Signal/Address.js`:
```js
if (this.wid.isHosted()) {
if (bizHostedDevicesEnabled()) {
if (t !== ":99") throw "Hosted jid with wrong device id";
var n = asUserWidOrThrow(this.wid),
a = !n.isLid() && !n.isHostedLid() && n.isUser(),
i = a ? getCurrentLid(n) : n;
return i == null
? [this.wid.user, t, "@HosteD"].join("")
: [i.user, t, "@hosted.lid"].join("");
}
throw "Unexpected hosted jid";
}
```
Same pattern for non-hosted JIDs lower in the function: prefer LID-keyed addresses, fall back to the original namespace if no LID mapping is known.
What was wrong
`resolve_encryption_jid` only handled the PN → LID upgrade. Hosted JIDs fell through the catch-all `_ => target.clone()`, so a hosted device whose LID was known was keyed locally at `{user}@hosted` while WA Web would key it at `{lid_user}@hosted.lid`.
Practical impact today: nil — current send paths filter `is_hosted()` before reaching the resolver, so the divergent key was never built or looked up. The gap was dormant but real, and any future hosted support would have inherited an incompatible keyspace.
Fix
`resolve_encryption_jid` mirrors WA Web in a single match:
```rust
let lid_server = match target.server {
Server::Pn => Server::Lid,
Server::Hosted => Server::HostedLid,
_ => return target.clone(),
};
match self.lid_pn_cache.get_current_lid(&target.user).await {
Some(lid_user) => Jid { user: lid_user.into(), server: lid_server, .. },
None => target.clone(),
}
```
The Hosted upgrade reuses the same `lid_pn_cache.get_current_lid()` lookup as the PN upgrade. PN/LID dispatch cost is unchanged.
Allocation profile
The wasted `String` is a property of `lid_pn_cache.get_current_lid() -> Option`. Eliminating it requires changing `LidPnEntry.lid: String → CompactString` cross-crate; out of scope here, dormant in hot paths today (LID groups don't go through the upgrade — devices already are LID).
Tests
Plus a wire-encoding regression in `wacore/binary/src/encoder.rs`: direct-constructed Hosted JIDs (`Jid::new(_, Server::Hosted)` defaults `agent=0`) now emit the correct `domain_type` byte (128 / 129) because the encoder derives it from the server enum, not from `agent`. Pre-#391 the same input would have written `0`.
What's not in this PR
Test plan