fix(profile-picture): WA Web parity — port PR #2614 + add id/invite/persona_id/common_gid#510
Merged
Merged
Conversation
…rsona_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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
|
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. |
There was a problem hiding this comment.
Pull request overview
This PR updates the profilePictureUrl IQ stanza to match WhatsApp Web behavior by nesting <tctoken> inside <picture>, avoiding USync-triggering LID lookups on this path, and adding support for additional WA Web-emitted <picture> attributes.
Changes:
- Add
buildTcTokenNode()to return a standalone<tctoken>BinaryNodesuitable for nesting. - Refactor
profilePictureUrl()to nest<tctoken>under<picture>, use a local-only PN→LID resolver, and accept new optional attrs (id,invite,persona_id,common_gid). - Add unit tests covering
buildTcTokenNode()behavior and cleanup semantics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Utils/tc-token-utils.ts | Adds buildTcTokenNode() helper for nested <tctoken> construction. |
| src/Socket/chats.ts | Refactors profilePictureUrl() stanza construction and adds new optional query attrs. |
| src/Signal/lid-mapping.ts | Adds getKnownLIDForPN() local-only mapping lookup (no USync). |
| src/tests/Utils/tc-token.test.ts | Adds tests for buildTcTokenNode() return/cleanup/error behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ation (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>
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>
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
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
Bring our
profilePictureUrlstanza to byte-for-byte parity with WA Web (and whatsmeow). Includes the literal port of upstream WhiskeySockets/Baileys#2614 plus 4 additional<picture>attrs that WA Web emits but we don't.Empirical evidence — triple confirmation
<picture>" + "skip USync"Client.GetProfilePictureInfocurl raw.githubusercontent.com/.../user.gopictureContentis GoContent:of the picture node (= child)Debugger.getScriptSourceWASmaxOutProfilePictureGetRequest:mergeStanzas(<picture>, <tctoken>)⇒ tctoken inside pictureThe WA Web mixin chain proves it definitively:
mergeStanzas(target, mixin)places the mixin INSIDE target.Changes
src/Signal/lid-mapping.tsgetKnownLIDForPN(pn)— port of PR WhiskeySockets#2614 (cache+disk only, no USync)src/Utils/tc-token-utils.tsbuildTcTokenNode({ … })returning the single<tctoken>BinaryNode(orundefined) for nesting. OldbuildTcTokenFromJidkept intact.src/Socket/chats.tsprofilePictureUrl— nest tctoken in<picture>, usegetKnownLIDForPN, acceptexistingId/invite/personaId/commonGidin optssrc/__tests__/Utils/tc-token.test.tsbuildTcTokenNode(72 → 78 passing)New
profilePictureUrloptsOld signature still works (all new params optional).
Stanza emitted (after)
vs before:
What's NOT touched
Explicitly out of scope — zero impact on the customizations the maintainer flagged as off-limits:
cacheMetricsIntervalmemory-leak fix in libsignalpresenceSubscribe— kept on the old helper /getLIDForPNbecause it doesn't nest tctoken inside another node (no nesting question)Risks
optsobject — signature backward-compatibleTest plan
npm run buildpassestc-token.test.ts— 78/78 passingprofilePictureUrl(knownContactJid), verify response🤖 Generated with Claude Code
Summary by cubic
Brings
profilePictureUrlto WA Web parity by nesting<tctoken>inside<picture>, addingid,invite,persona_id, andcommon_gid, and avoiding USync via a local-only LID resolver. Also unifies TC token retrieval/expiry logic to keep behavior consistent across call sites.New Features
profilePictureUrl(jid, type, timeoutMs, opts)acceptsexistingId,invite,personaId,commonGid. Backward compatible.Bug Fixes
<tctoken>is nested under<picture>; skipped for self, non-user JIDs, and invite lookups.getKnownLIDForPN, which callscheckDestroyedandtrackOperationto prevent post-destroy races.resolveTcTokenForJidto centralize token retrieval/expiry cleanup forbuildTcTokenFromJidandbuildTcTokenNode(no behavior change).Written for commit fea38ba. Summary will update on new commits.