fix(audit #521): 3 P2 + 2 P3 from review (reporting token, retry DSM, orphan msmsg, __internal, any cast)#522
fix(audit #521): 3 P2 + 2 P3 from review (reporting token, retry DSM, orphan msmsg, __internal, any cast)#522rsalcara wants to merge 1 commit into
Conversation
5 findings from the release PR #521 audit applied before promoting develop → master. The release branch itself doesn't change; this PR lands the fixes on develop and the release branch will be refreshed to include them. P2 — Thread 1 (chatgpt) — reporting-token check reads outer wrap ================================================================ `messages-send.ts:1884` gated reporting-token attachment on `reportingMessage?.messageContextInfo?.messageSecret`. For Lottie stickers post-PR #519 that secret lives INSIDE the `lottieStickerMessage` wrap — the top-level read returns undefined and the token is silently skipped. Regression vs plain sticker behaviour. Fix: lift `messageContextInfo` from both locations the same way the initial-fanout DSM read does (the lift PR #519 already shipped on line ~1198): const reportingMessageSecret = reportingMessage?.lottieStickerMessage?.message ?.messageContextInfo?.messageSecret ?? reportingMessage?.messageContextInfo?.messageSecret Required adding `reportingMessage &&` to the surrounding `if` so TypeScript doesn't lose the narrowing it previously got from the inline property chain (the narrowing was the side-effect of reading `reportingMessage.messageContextInfo` in the condition itself). P2 — Thread 5 (coderabbit Major) — retry-resend DSM drops messageContextInfo ============================================================================ `messages-send.ts:1580` rebuilds the `deviceSentMessage` envelope on the retry-resend path WITHOUT carrying `messageContextInfo` along. The initial-fanout path got the lift in PR #519 (line ~1198); the retry path was missed. Concrete impact: when a companion device receives the RETRY of a Lottie sticker via DSM, our own `unwrapDeviceSentMessage` finds: - inner = messageToSend = { lottieStickerMessage: { message: { stickerMessage, messageContextInfo } } } - inner.messageContextInfo = undefined (it's nested in the wrap) - outer.messageContextInfo = undefined (the retry envelope omitted it) → messageSecret = undefined, reporting token / encrypted-edit decryption material lost on the companion's copy. Fix: same lift inline at the retry envelope build site. P2 — Thread 2 (chatgpt) — OrphanMsmsgError stub goes to Signal retry path ========================================================================= When `decryptMsmsgBotMessage` raises `OrphanMsmsgError` (cache miss), the catch in `decode-wa-message.ts` sets `messageStubType = CIPHERTEXT` and `messageStubParameters[0] = String(err.message)`. The receive handler at `messages-recv.ts:3115` checks for known stub-param strings and falls through to the Signal retry / PDO placeholder-resend path when none match — burning retry budget asking the bot for prekeys it has no business issuing, for a problem (missing CACHE entry) that a Signal retry can never fix. Fix: add a guard for `messageStubParameters[0]?.startsWith('decryptMsmsgBotMessage:')` right after the `MISSING_KEYS_ERROR_TEXT` branch. Plain ACK (no NACK, no retry) so the server considers the message delivered. The next bot reply that arrives after the outgoing-secret cache populates will decrypt cleanly. NOT a NACK MissingMessageSecret: that would tell the server to retransmit, and the retransmission will hit the same orphan state until the outgoing-side cache is populated (deferred per PR #518). P3 — Thread 4 (copilot) — `__internal` export not consumed ========================================================== `src/Utils/meta-ai-msmsg.ts` exported an `__internal` bag of helpers (`BOT_MESSAGE_INFO`, `KEY_LENGTH`, `isMeJid`, `deriveKeyAndDecrypt`, `decodeDecryptedMsmsg`, `userOnlyJid`, `isJidGroup`) that no file in the repo actually imports. Grep confirms zero call sites. Removed — the helpers stay module-private as intended. P3 — Thread 3 (copilot) — `(err as any)?.message` in auth-utils =============================================================== `src/Utils/auth-utils.ts:714` (the trace I added in PR #515 to log `transactWith rolled back` without duplicating the stack) used a plain `(err as any)?.message` cast. Tightened to `err instanceof Error ? err.message : String(err)` — no runtime change, just removes the `any` cast and gives string fallback for non-Error throws. Validation ========== * `npm run build` clean (TS narrowing fixed via explicit `reportingMessage &&` guard). * 57/57 tests pass across the new-PR suites (dsm-context-info-preservation + meta-ai-msmsg + lottie-sticker-message + error-log-utils + process-message.protocol-guard). 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. |
![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: cd355fb009
ℹ️ 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".
| const reportingMessageSecret = | ||
| reportingMessage?.lottieStickerMessage?.message?.messageContextInfo?.messageSecret ?? | ||
| reportingMessage?.messageContextInfo?.messageSecret |
There was a problem hiding this comment.
When reportingMessage is a Lottie wrapper, this new nested-secret check lets the reporting branch run, but getMessageReportingToken still reads only message.messageContextInfo?.messageSecret (src/Utils/reporting-utils.ts:316) from the same wrapped message. In that scenario it returns null, so Lottie stickers continue to be sent without the reporting token this change is intended to restore; the lifted context/secret needs to be passed into the reporting helper or handled there as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Applies five audit-driven fixes identified in the release review (PR #521) to harden msmsg handling, preserve reporting-token/messageSecret propagation for Lottie stickers, and remove minor cleanup items before promoting develop → master.
Changes:
- Lift
messageContextInfointo the DSM envelope for retry-resend paths and read nested LottiemessageContextInfowhen attaching reporting tokens. - Prevent pointless Signal retry/PDO handling for msmsg orphan failures by ACKing instead of falling through to retry logic.
- Cleanup: remove unused
__internalexport and replace(err as any)?.messagewith safer error string extraction.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Utils/meta-ai-msmsg.ts | Removes the unused __internal re-export surface. |
| src/Utils/auth-utils.ts | Replaces any-cast error message access with safe Error narrowing. |
| src/Socket/messages-send.ts | Preserves messageContextInfo in DSM retry envelopes and attempts to restore reporting-token attachment for Lottie-wrapped messages. |
| src/Socket/messages-recv.ts | Adds an ACK fast-path for msmsg orphan stub failures to avoid wasting retry budget. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const reportingMessageSecret = | ||
| reportingMessage?.lottieStickerMessage?.message?.messageContextInfo?.messageSecret ?? | ||
| reportingMessage?.messageContextInfo?.messageSecret | ||
|
|
| if (msg?.messageStubParameters?.[0]?.startsWith('decryptMsmsgBotMessage:')) { | ||
| await sendMessageAck(node) | ||
| acked = true | ||
| return |
![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
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/Socket/messages-recv.ts">
<violation number="1" location="src/Socket/messages-recv.ts:3134">
P1: The new ACK guard is too broad: it ACKs every `decryptMsmsgBotMessage:*` failure, not just orphan missing-secret errors, so malformed msmsg decrypt failures can be silently swallowed.</violation>
</file>
<file name="src/Socket/messages-send.ts">
<violation number="1" location="src/Socket/messages-send.ts:1906">
P1: The lifted `reportingMessageSecret` correctly enables the `if` guard to pass for Lottie-wrapped stickers, but `getMessageReportingToken(reportingMessage)` (called inside the subsequent `try` block) still reads only `message.messageContextInfo?.messageSecret` — which is `undefined` for Lottie wraps. This means the guard passes yet the helper returns `null`, so the reporting token is never actually attached to Lottie stickers. The lifted secret (or the nested `messageContextInfo`) needs to be passed to the reporting helper, or the helper needs the same nested-lookup logic.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // considers the message delivered; the next bot reply that arrives | ||
| // after the outgoing-secret cache populates will decrypt cleanly | ||
| // (audit thread 2 / chatgpt P2 on release PR #521). | ||
| if (msg?.messageStubParameters?.[0]?.startsWith('decryptMsmsgBotMessage:')) { |
There was a problem hiding this comment.
P1: The new ACK guard is too broad: it ACKs every decryptMsmsgBotMessage:* failure, not just orphan missing-secret errors, so malformed msmsg decrypt failures can be silently swallowed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Socket/messages-recv.ts, line 3134:
<comment>The new ACK guard is too broad: it ACKs every `decryptMsmsgBotMessage:*` failure, not just orphan missing-secret errors, so malformed msmsg decrypt failures can be silently swallowed.</comment>
<file context>
@@ -3121,6 +3121,22 @@ export const makeMessagesRecvSocket = (config: SocketConfig) => {
+ // considers the message delivered; the next bot reply that arrives
+ // after the outgoing-secret cache populates will decrypt cleanly
+ // (audit thread 2 / chatgpt P2 on release PR #521).
+ if (msg?.messageStubParameters?.[0]?.startsWith('decryptMsmsgBotMessage:')) {
+ await sendMessageAck(node)
+ acked = true
</file context>
| !isRetryResend && | ||
| reportingMessage?.messageContextInfo?.messageSecret && | ||
| reportingMessage && | ||
| reportingMessageSecret && |
There was a problem hiding this comment.
P1: The lifted reportingMessageSecret correctly enables the if guard to pass for Lottie-wrapped stickers, but getMessageReportingToken(reportingMessage) (called inside the subsequent try block) still reads only message.messageContextInfo?.messageSecret — which is undefined for Lottie wraps. This means the guard passes yet the helper returns null, so the reporting token is never actually attached to Lottie stickers. The lifted secret (or the nested messageContextInfo) needs to be passed to the reporting helper, or the helper needs the same nested-lookup logic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Socket/messages-send.ts, line 1906:
<comment>The lifted `reportingMessageSecret` correctly enables the `if` guard to pass for Lottie-wrapped stickers, but `getMessageReportingToken(reportingMessage)` (called inside the subsequent `try` block) still reads only `message.messageContextInfo?.messageSecret` — which is `undefined` for Lottie wraps. This means the guard passes yet the helper returns `null`, so the reporting token is never actually attached to Lottie stickers. The lifted secret (or the nested `messageContextInfo`) needs to be passed to the reporting helper, or the helper needs the same nested-lookup logic.</comment>
<file context>
@@ -1878,10 +1891,19 @@ export const makeMessagesSocket = (config: SocketConfig) => {
!isRetryResend &&
- reportingMessage?.messageContextInfo?.messageSecret &&
+ reportingMessage &&
+ reportingMessageSecret &&
shouldIncludeReportingToken(reportingMessage)
) {
</file context>
|
Closing in favor of applying fixes directly on release branch |
Two findings from the cubic re-review on PR #521 that the previous audit-fix commit (cherry-picked from closed PR #522) didn't cover. Issue D (P2) — test re-implemented production helper ==================================================== cubic thread 8: `dsm-context-info-preservation.test.ts` kept a local copy of `unwrapDeviceSentMessage` and asserted against it. Comment claimed "if rules change in decode-wa-message.ts these break first" but that was wrong — the assertions tested the LOCAL copy, not production. Reverting `decode-wa-message.ts` to the old `msg = msg.deviceSentMessage?.message || msg` would leave all 10 tests green. The tests were validating themselves. Fix: * `decode-wa-message.ts` — flipped the helper from module-private `const` to `export const unwrapDeviceSentMessage`. The function is a leaf utility; exporting it surfaces nothing operational beyond what tests need. * `dsm-context-info-preservation.test.ts` — deleted the re-derived copy, imported from `../../Utils/decode-wa-message`. Header comment updated to explain WHY we now import (production parity) instead of re-derive. Issue E (P3) — `if (!err)` too broad in compactError ==================================================== cubic thread 9: `error-log-utils.ts:33` short-circuited every falsy input (including `0`, `''`, `false`, `NaN`) to the literal string `'Unknown'`. Those are unusual but valid thrown values — code that does `throw 0` would get its actual value erased. Operational impact near zero (Signal Protocol throws `Error` instances), but the contract is wrong. Fix: * `error-log-utils.ts` — `if (!err)` → `if (err == null)`. Comment added explaining the nullish-only narrowing. * `error-log-utils.test.ts` — new test `preserves falsy primitives that are NOT null/undefined` pins the new contract: `compactError(0)` → `'0'`, `compactError(false)` → `'false'`, etc. Validation ========== * `npm run build` clean * 58/58 tests pass (5 suites: dsm-context-info + error-log-utils + meta-ai-msmsg + lottie-sticker-message + process-message). Was 57; +1 from the new falsy-primitives test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
5 audit findings from PR #521 (release develop→master) applied before promoting to master. PR #521 itself doesn't change directly; the release branch will be refreshed to include these once this lands on develop.
messages-send.ts:1884reporting-token check readreportingMessage?.messageContextInfoat top level. Lottie wraps move it insidelottieStickerMessage.message(PR #519). Token was silently skipped for Lottie stickers. Lift from both spots (same idiom as PR #519's DSM lift on line ~1198).OrphanMsmsgErrorfromdecryptMsmsgBotMessageproduced aCIPHERTEXTstub with parameter"decryptMsmsgBotMessage: no messageSecret for ..."that fell through to the Signal retry/PDO path atmessages-recv.ts:3115. Burned retry budget asking the bot for prekeys for a problem (missing CACHE entry) that Signal retry can't fix. Added a guard branch — plain ACK, no NACK.messages-send.ts:1580retry-resend DSM omittedmessageContextInfo. Companion devices receiving the RETRY of a Lottie sticker lost the reporting token / encrypted-edit material. Same lift the initial-fanout DSM got in PR #519.__internalexport frommeta-ai-msmsg.ts(zero consumers in the repo).(err as any)?.messageinauth-utils.ts:714→err instanceof Error ? err.message : String(err).Validation
npm run buildclean (TS narrowing on reportingMessage was lost when I extractedreportingMessageSecretto a separate variable — fixed by adding explicitreportingMessage &&to the surroundingif)Next step
After this lands on develop, refresh PR #521 (release branch) to include these fixes before merging to master.
Customizations preserved (NOT touched)
Carousel, lists, buttons, polls, view-once, biz
quality_control,useLegacyLock, TC token custom flow, LID↔PN batched, Phase 9 multi-DB,lidDbMigrated:false,cacheMetricsIntervalmemory-leak fix, schema migrations + statement cache + busy retry, recoverable-error compact logging, Meta AI msmsg decryption, Lottie sticker wrap/unwrap, DSM context info per-field merge.🤖 Generated with Claude Code
Summary by cubic
Fixes five audit findings from release PR #521. Restores reporting-token handling for Lottie stickers, prevents wasteful retries on orphan Meta AI bot replies, and cleans up minor items before promoting the release.
Bug Fixes
messageContextInfofromlottieStickerMessage.message(or top-level) and include it in retry DSM so companions keepmessageSecret/reporting token.decryptMsmsgBotMessageerrors to avoid pointless Signal retries.(err as any)?.messagewitherr instanceof Error ? err.message : String(err).Refactors
__internalexport frommeta-ai-msmsg.ts.Written for commit cd355fb. Summary will update on new commits.