fix: nest profile picture tctoken and avoid usync on lookup#2614
Open
gusquadri wants to merge 3 commits into
Open
fix: nest profile picture tctoken and avoid usync on lookup#2614gusquadri wants to merge 3 commits into
gusquadri wants to merge 3 commits into
Conversation
Contributor
|
Thanks for opening this pull request and contributing to the project! The next step is for the maintainers to review your changes. If everything looks good, it will be approved and merged into the main branch. In the meantime, anyone in the community is encouraged to test this pull request and provide feedback. ✅ How to confirm it worksIf you’ve tested this PR, please comment below with: This helps us speed up the review and merge process. 📦 To test this PR locally:If you encounter any issues or have feedback, feel free to comment as well. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds LIDMappingStore.getKnownLIDForPN for single-PN LID resolution (validation, cache check, targeted DB fetch, and cache update) and wires it into makeChatsSocket; profilePictureUrl is refactored to build the picture node up front and use the resolver when creating WhatsApp tctoken payloads. ChangesPN-to-LID Resolver Integration
Sequence DiagramsequenceDiagram
participant profilePictureUrl as profilePictureUrl
participant LIDMappingStore
participant Cache as pn:/lid: Cache
participant DB as lid-mapping DB
participant tctoken as buildTcTokenFromJid
profilePictureUrl->>LIDMappingStore: getKnownLIDForPN(pn)
LIDMappingStore->>LIDMappingStore: validate & decode PN
LIDMappingStore->>Cache: check pn:<user> mapping
alt cache hit
Cache-->>LIDMappingStore: cached lid:<user>
else cache miss
LIDMappingStore->>DB: fetch lid-mapping for PN user
DB-->>LIDMappingStore: mapping result
LIDMappingStore->>Cache: update pn: & lid: caches
end
LIDMappingStore-->>profilePictureUrl: formatted LID JID
profilePictureUrl->>tctoken: buildTcTokenFromJid(jid, getKnownLIDForPN)
tctoken-->>profilePictureUrl: token nodes
profilePictureUrl->>profilePictureUrl: assign to pictureNode.content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Signal/lid-mapping.ts`:
- Around line 121-126: The code reads stored = await
this.keys.get('lid-mapping', [pnUser]) and unguardedly sets lidUser =
stored[pnUser] then caches and formats it; if stored[pnUser] is non-string we
may cache invalid JIDs. Update the block that handles the result from keys.get
(the stored variable and lidUser) to check typeof lidUser === 'string' before
calling this.mappingCache.set(`pn:${pnUser}`, lidUser) or
this.mappingCache.set(`lid:${lidUser}`, pnUser) and before using lidUser for any
JID formatting—mirror the same guard used in getLIDsForPNs to bail out or skip
caching when lidUser is not a string.
🪄 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: CHILL
Plan: Pro
Run ID: 517a657d-8490-4c0b-b8f6-d89f9d171f80
📒 Files selected for processing (2)
src/Signal/lid-mapping.tssrc/Socket/chats.ts
3 tasks
rsalcara
added a commit
to rsalcara/InfiniteAPI
that referenced
this pull request
Jun 7, 2026
PR #510 cubic review (Thread 3, P3) flagged that `buildTcTokenFromJid` and `buildTcTokenNode` carried byte-for-byte identical retrieval / expiry / opportunistic-cleanup pipelines — only the return shape differed (sibling array vs single node). Extracted that pipeline into a private `resolveTcTokenForJid(...)` helper so the two public builders become thin adapters around a single source of truth. Behavioral contract preserved (verified against the existing 78 tests): - Both public builders keep their exact public signatures. - `buildTcTokenFromJid` still mutates `baseContent` in place when a token exists, and still returns `baseContent.length > 0 ? baseContent : undefined` on every "no token" branch — same legacy behavior the presenceSubscribe call site relies on. - `buildTcTokenNode` still returns a single `BinaryNode` or `undefined` — same shape profilePictureUrl uses for nesting under `<picture>`. - The opportunistic expired-token wipe (with senderTimestamp preservation) runs exactly once, in the helper, with the same write shape as before. Missing-token entries are still NOT wiped. - Key-store errors are still swallowed inside the helper; both public builders fall back to their no-token branch on throw. Why the two builders stay as separate public surfaces (instead of one): - `presenceSubscribe` builds a `<presence>` whose content is the legacy sibling-array shape; it cares about preserving `baseContent` when no token exists. Forcing it to the single-node shape would require every call site to special-case `undefined`. - `profilePictureUrl` (port of PR WhiskeySockets#2614) needs the token nested as a CHILD of `<picture>`. It MUST get a single node, not an array. So the API split is meaningful; only the internal implementation was duplicated. The helper kills the duplication without changing the surface. Test impact: 0 changes to the test suite, 78/78 still pass. Same numbers for the LID-mapping suite (15) and identity-change suite (23) that indirectly exercise the same signal-store paths — 116/116 in the combined run. Out of scope (intentionally NOT touched): - Carousel, lists, buttons, polls, view-once, biz quality_control, useLegacyLock, the broader TC token issuance flow (only retrieval helpers refactored), LID↔PN batched, Phase 9 multi-DB, lidDbMigrated:false, the cacheMetricsInterval memory-leak fix, schema migrations + statement cache + busy retry. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
rsalcara
added a commit
to rsalcara/InfiniteAPI
that referenced
this pull request
Jun 7, 2026
…dd id/invite/persona_id/common_gid (#510) * fix(profile-picture): WA Web parity — nest tctoken + add id/invite/persona_id/common_gid Port of upstream WhiskeySockets#2614 + adicional gaps identified by comparing our `profilePictureUrl` against WA Web (live source via CDP) and whatsmeow source code. Brings our `<iq xmlns="w:profile:picture">` stanza to byte-for-byte parity with the official client behavior. What changes: 1. New `LIDMappingStore.getKnownLIDForPN(pn)` (port of PR WhiskeySockets#2614, literal). Returns the LID for a PN **only if locally known** (cache or store) — never triggers USync. Opportunistic call sites (profile-picture, etc.) use this so the failure-to-resolve path does NOT fire a USync round-trip. USync- on-profile-picture is not a behavior WA Web or whatsmeow emit; doing it makes our traffic profile stand out and may serve as a ban signal. 2. New `buildTcTokenNode({ authState, jid, getLIDForPN })` helper in tc-token-utils.ts. Same expiry / opportunistic-cleanup semantics as the pre-existing `buildTcTokenFromJid`, but returns the single <tctoken> BinaryNode (or undefined) instead of pushing into a content array. Callers that need the token as a nested CHILD of another node use this; the older `buildTcTokenFromJid` is kept intact for `presenceSubscribe` which uses the sibling-array shape. 3. `profilePictureUrl` refactored to match WA Web's `WASmaxOutProfilePictureGetRequest` mixin composition: - tctoken is now nested as a CHILD of <picture> (was a sibling — verified wrong against WA Web JS source: mergeStanzas(<picture>, <tctoken>) places tctoken inside picture, NOT alongside it). - tctoken resolver swapped to getKnownLIDForPN (no USync, PR WhiskeySockets#2614). - New optional params accepted in `opts`: * existingId → <picture id="..."> for 304-NotModified caching (matches WA Web pictureId + whatsmeow ExistingID). Saves a CDN re-fetch when the picture hasn't changed. * invite → <picture invite="..."> for group-invite-link preview (fetch a group's picture without joining). When set, we skip the tctoken (the invite IS the authorization). * personaId → <picture persona_id="..."> for Meta AI bot personas. * commonGid → <picture common_gid="..."> required when the target's privacy is "My contacts" and we share a group but are not in their contacts — without it the server returns 401/403. Matches WA Web. - Same callers using the old signature work unchanged (all new params are in an optional `opts` object). 4. `presenceSubscribe` is intentionally NOT changed — it doesn't wrap the tctoken inside a `<picture>` (no nesting question), and a USync miss during presence-subscribe is far less common (we usually know the LID by the time we subscribe). Kept on `buildTcTokenFromJid` + `getLIDForPN`. Empirical validation — triple-confirmed before applying: a) PR WhiskeySockets#2614 description (`fix: nest profile picture tctoken and avoid usync on lookup`) plus cubic + coderabbit summaries. b) whatsmeow's `Client.GetProfilePictureInfo` in user.go — pictureContent is the `Content` of the picture node (Go semantics: child). c) WA Web's live JS bundle, extracted via Chrome DevTools Protocol (`Debugger.getScriptSource`). The mixin chain in `WASmaxOutProfilePictureGetRequest` is: smax("picture", attrs) → optionalMerge(mergeAddRequestMixin, ...) → optionalMerge(mergeTCTokenMixin, ...) → optionalMerge(mergeAvatarMixin, ...) where `mergeStanzas(target, mixin)` places the mixin INSIDE target. The tctoken is undeniably a child of <picture>. Tests: - Existing 72 tests in `tc-token.test.ts` continue to pass (no signature changes to `buildTcTokenFromJid`). - 6 new tests for `buildTcTokenNode` (valid token, missing, expired + cleanup, no-wipe-when-missing, error swallow, return shape). - Full TC token suite: 78/78 passing. Out of scope (intentionally NOT touched): - Carousel, lists, buttons, polls, view-once, biz quality_control, useLegacyLock, the broader TC token issuance flow, LID↔PN batched, Phase 9 multi-DB, lidDbMigrated:false, the cacheMetricsInterval memory-leak fix, schema migrations + statement cache + busy retry, presence- subscribe path (covered separately above). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(lid-mapping): wrap getKnownLIDForPN in checkDestroyed + trackOperation (PR #510 review) PR #510 review caught a P2 contract violation: every other public method on `LIDMappingStore` calls `this.checkDestroyed()` at entry and runs its body inside `this.trackOperation(...)`. The new `getKnownLIDForPN` (added in this PR as part of the WA Web profile-picture parity port) was missing both guards. Why this matters: - Without `checkDestroyed()`: the method runs after `destroy()` has been called, when `this.keys` may already have been torn down. The first `await this.keys.get(...)` then races UAF on the underlying signal key store. - Without `trackOperation()`: `destroy()` does not wait for this method's in-flight I/O before clearing the mapping cache. A concurrent call can observe a half-cleared cache (a `mappingCache.set(...)` against a Map that `destroy()` is about to `clear()`) and silently return stale data rather than failing fast. Both threads (copilot + cubic) flagged the same issue, cubic with confidence 9. The fix is identical to the suggestion both reviewers provided: wrap the body in `this.trackOperation(async () => { … })` preceded by `this.checkDestroyed()`. Caller-side impact: none. `buildTcTokenNode` (the sole call site) has a try/catch that swallows any throw — so the only effect a post-destroy call has is "tctoken not attached" (same as the existing miss path), not a crash. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(tc-token): extract resolveTcTokenForJid helper (PR #510 review) PR #510 cubic review (Thread 3, P3) flagged that `buildTcTokenFromJid` and `buildTcTokenNode` carried byte-for-byte identical retrieval / expiry / opportunistic-cleanup pipelines — only the return shape differed (sibling array vs single node). Extracted that pipeline into a private `resolveTcTokenForJid(...)` helper so the two public builders become thin adapters around a single source of truth. Behavioral contract preserved (verified against the existing 78 tests): - Both public builders keep their exact public signatures. - `buildTcTokenFromJid` still mutates `baseContent` in place when a token exists, and still returns `baseContent.length > 0 ? baseContent : undefined` on every "no token" branch — same legacy behavior the presenceSubscribe call site relies on. - `buildTcTokenNode` still returns a single `BinaryNode` or `undefined` — same shape profilePictureUrl uses for nesting under `<picture>`. - The opportunistic expired-token wipe (with senderTimestamp preservation) runs exactly once, in the helper, with the same write shape as before. Missing-token entries are still NOT wiped. - Key-store errors are still swallowed inside the helper; both public builders fall back to their no-token branch on throw. Why the two builders stay as separate public surfaces (instead of one): - `presenceSubscribe` builds a `<presence>` whose content is the legacy sibling-array shape; it cares about preserving `baseContent` when no token exists. Forcing it to the single-node shape would require every call site to special-case `undefined`. - `profilePictureUrl` (port of PR WhiskeySockets#2614) needs the token nested as a CHILD of `<picture>`. It MUST get a single node, not an array. So the API split is meaningful; only the internal implementation was duplicated. The helper kills the duplication without changing the surface. Test impact: 0 changes to the test suite, 78/78 still pass. Same numbers for the LID-mapping suite (15) and identity-change suite (23) that indirectly exercise the same signal-store paths — 116/116 in the combined run. Out of scope (intentionally NOT touched): - Carousel, lists, buttons, polls, view-once, biz quality_control, useLegacyLock, the broader TC token issuance flow (only retrieval helpers refactored), LID↔PN batched, Phase 9 multi-DB, lidDbMigrated:false, the cacheMetricsInterval memory-leak fix, schema migrations + statement cache + busy retry. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
This PR is stale because it has been open for 14 days with no activity. Remove the stale label or comment or this will be closed in 14 days |
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
2 participants
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.
tctokeninside the<picture>node, matching WA Web and whatsmeowSummary by cubic
Nest the profile-picture tctoken inside the
<picture>node and avoid PN→LID USync when only fetching a profile photo URL. Matches WA Web/whatsmeowbehavior and reduces potential ban risk.<picture>in profile picture URL queries.getKnownLIDForPN(cached PN→LID, no USync) to build the tctoken; accept only non-empty stored LIDs and attach the token only when the mapping is locally known.Written for commit 2d91a47. Summary will update on new commits.
Summary by CodeRabbit
New Features
Refactor