feat(media): request media reupload when the CDN blob expired - #242
Merged
Conversation
Adds `client.message.requestMediaReupload()`, the round-trip WhatsApp Web runs when a media download answers 404/410 - typically old media surfaced by a history sync. A `server-error` receipt carrying an encrypted `ServerErrorReceipt` goes out, and the sender's primary device answers with a `mediaretry` notification holding a fresh `directPath`. - `@media/crypto/media-retry` seals and opens both payloads: HKDF-SHA-256 over the media key with the `WhatsApp Media Retry Notification` context, AES-256-GCM bound to the stanza id as associated data. - `@message/primitives/media-retry` owns the round-trip. A plain node query cannot serve here: the ack and the notification carry the same stanza id, so a query would resolve on the ack and drop the answer. Requests are keyed by message id, deduplicated, bounded, and expire. - The `mediaretry` notification is parsed and settled through the existing incoming pipeline; the ack it already sent is unchanged. - `getNodeBytesContent` joins the transport helpers, replacing three copies of the same optional-node-bytes expression. On success only the `directPath` changes: the media key, hashes, and length of the original message stay valid. `not_found` is a normal answer, meaning the sender no longer holds the file. Validated against the live server end to end, including an image whose CDN blob had expired 77 days earlier: the reupload returned a new `directPath` and the download verified against the original keys.
📝 WalkthroughWalkthroughAdds encrypted media retry requests and notification handling. The change includes request tracking, timeout and eviction controls, public APIs, client wiring, protocol defaults, and binary node content extraction. ChangesMedia retry flow
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🔵 Low · up to The new media reupload flow is mergeable with owner follow-up to ensure validation failures are surfaced as rejected promises; otherwise callers could fail to detect an unsuccessful reupload request. Sequence Diagram(s)sequenceDiagram
participant Client
participant WaMessageCoordinator
participant WaMediaRetryRequester
participant IncomingNotificationHandler
participant Transport
Client->>WaMessageCoordinator: requestMediaReupload(event or request)
WaMessageCoordinator->>WaMediaRetryRequester: validated retry request
WaMediaRetryRequester->>Transport: send encrypted server-error receipt
Transport-->>IncomingNotificationHandler: mediaretry notification
IncomingNotificationHandler->>WaMediaRetryRequester: handleNotification(node)
WaMediaRetryRequester-->>WaMessageCoordinator: resolve WaMediaRetryResult
WaMessageCoordinator-->>Client: return retry result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (2)
src/client/coordinators/__tests__/message-coordinator.test.ts (1)
34-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the
mediaRetryparameter to the requester contract.
unknownplusas neverremoves shape checking from every call site. IfWaMediaRetryRequester.requestorhandleNotificationchanges, the test doubles keep compiling. UsePartial<WaMediaRetryRequester>so the compiler catches contract drift.♻️ Proposed refactor
+import type { WaMediaRetryRequester } from '`@message/primitives/media-retry`' + function createCoordinator( peerDataOperation: PeerDataOperationRequester, - mediaRetry: unknown = {} + mediaRetry: Partial<WaMediaRetryRequester> = {} ): WaMessageCoordinator {🤖 Prompt for 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. In `@src/client/coordinators/__tests__/message-coordinator.test.ts` around lines 34 - 41, Update the createCoordinator helper’s mediaRetry parameter to use Partial<WaMediaRetryRequester> instead of unknown, and pass it without casting to never; preserve the default empty object while ensuring test doubles are checked against the requester contract, including request and handleNotification.src/client/coordinators/WaMessageCoordinator.ts (1)
858-875: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the implementation
asyncso validation failures reject instead of throwing synchronously.The overloads declare
Promise<WaMediaRetryResult>, but the implementation is notasyncand throws synchronously at Lines 867, 870, and 874. A caller that uses.catch()withoutawaitreceives a synchronous exception. Other public methods on this class return rejections for the same class of failure. Marking the implementationasynckeeps the documented@throwslist uniform.♻️ Proposed refactor
- public requestMediaReupload( + public async requestMediaReupload( source: WaIncomingMessageEvent | WaMediaRetryRequest, options?: { readonly timeoutMs?: number } ): Promise<WaMediaRetryResult> {🤖 Prompt for 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. In `@src/client/coordinators/WaMessageCoordinator.ts` around lines 858 - 875, Mark the public requestMediaReupload implementation as async so validation errors from the key, newsletter, and media-payload checks reject the declared Promise instead of throwing synchronously. Preserve the existing overloads and return behavior.
🤖 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/media/crypto/media-retry.ts`:
- Around line 22-29: Update the JSDoc for WaMediaRetryDecryptInput to mark
mediaKey with `@sensitive` and document that it must be encrypted at rest and must
not be passed to JSON.stringify() or console.log(). Keep the existing field
declarations and stanzaId documentation unchanged.
In `@src/message/primitives/media-retry.ts`:
- Around line 190-208: The in-flight deduplication guarantee is broken because
createMediaRetryRequester.request checks pending before awaiting
buildRequestNode; update src/message/primitives/media-retry.ts lines 190-208 so
trackPending occurs before the asynchronous build, or re-checks pending
afterward, ensuring duplicate messageId calls join the first promise. In
src/client/coordinators/WaMessageCoordinator.ts lines 811-857, retain the
documented joining behavior only if enforced by request; otherwise reword that
sentence to match the actual behavior.
In `@src/transport/node/helpers.ts`:
- Around line 164-170: Update getNodeBytesContent to return undefined when
node.content is an empty Uint8Array, preserving the existing behavior for
missing, non-byte, and non-empty content. Add tests covering the empty-byte
contract and ensure empty enc_p values are rejected by
parseMediaRetryNotification.
---
Nitpick comments:
In `@src/client/coordinators/__tests__/message-coordinator.test.ts`:
- Around line 34-41: Update the createCoordinator helper’s mediaRetry parameter
to use Partial<WaMediaRetryRequester> instead of unknown, and pass it without
casting to never; preserve the default empty object while ensuring test doubles
are checked against the requester contract, including request and
handleNotification.
In `@src/client/coordinators/WaMessageCoordinator.ts`:
- Around line 858-875: Mark the public requestMediaReupload implementation as
async so validation errors from the key, newsletter, and media-payload checks
reject the declared Promise instead of throwing synchronously. Preserve the
existing overloads and return behavior.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d1fbbbd6-6b0b-4939-ae42-afc8d8694554
📒 Files selected for processing (19)
src/client/WaClientFactory.tssrc/client/__tests__/incoming.test.tssrc/client/coordinators/WaIncomingNodeCoordinator.tssrc/client/coordinators/WaMessageCoordinator.tssrc/client/coordinators/WaMobileCoordinator.tssrc/client/coordinators/__tests__/coordinators.test.tssrc/client/coordinators/__tests__/message-coordinator-upload.test.tssrc/client/coordinators/__tests__/message-coordinator.test.tssrc/client/events/business.tssrc/client/events/incoming.tssrc/index.tssrc/media/crypto/__tests__/media-crypto.test.tssrc/media/crypto/media-retry.tssrc/message/primitives/__tests__/media-retry.test.tssrc/message/primitives/media-retry.tssrc/protocol/defaults.tssrc/protocol/notification.tssrc/transport/index.tssrc/transport/node/helpers.ts
Comment on lines
+22
to
+29
| /** Input for {@link decryptMediaRetryNotification}. */ | ||
| export interface WaMediaRetryDecryptInput { | ||
| readonly mediaKey: Uint8Array | ||
| readonly ciphertext: Uint8Array | ||
| readonly iv: Uint8Array | ||
| /** Stanza id of the original message; doubles as the AES-GCM associated data. */ | ||
| readonly stanzaId: string | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Document mediaKey as sensitive data.
WaMediaRetryDecryptInput.mediaKey contains cryptographic secret material. Add @sensitive mediaKey. Document encryption at rest. Document that callers must not use JSON.stringify() or console.log() with this value.
Proposed fix
/**
* Input for {`@link` decryptMediaRetryNotification}.
+ *
+ * `@sensitive` mediaKey
+ * The caller must encrypt `mediaKey` at rest.
+ * Do not pass `mediaKey` to `JSON.stringify()` or `console.log()`.
*/
export interface WaMediaRetryDecryptInput {As per coding guidelines, “Types containing private keys, secrets, or auth tokens must have @sensitive JSDoc listing sensitive fields and document encryption-at-rest and no JSON.stringify or console.log.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** Input for {@link decryptMediaRetryNotification}. */ | |
| export interface WaMediaRetryDecryptInput { | |
| readonly mediaKey: Uint8Array | |
| readonly ciphertext: Uint8Array | |
| readonly iv: Uint8Array | |
| /** Stanza id of the original message; doubles as the AES-GCM associated data. */ | |
| readonly stanzaId: string | |
| } | |
| /** Input for {@link decryptMediaRetryNotification}. */ | |
| export interface WaMediaRetryDecryptInput { | |
| readonly mediaKey: Uint8Array | |
| readonly ciphertext: Uint8Array | |
| readonly iv: Uint8Array | |
| /** Stanza id of the original message; doubles as the AES-GCM associated data. */ | |
| readonly stanzaId: string | |
| } |
🤖 Prompt for 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.
In `@src/media/crypto/media-retry.ts` around lines 22 - 29, Update the JSDoc for
WaMediaRetryDecryptInput to mark mediaKey with `@sensitive` and document that it
must be encrypted at rest and must not be passed to JSON.stringify() or
console.log(). Keep the existing field declarations and stanzaId documentation
unchanged.
Source: Coding guidelines
Comment on lines
+190
to
+208
| request: async (input) => { | ||
| if (!input.messageId) { | ||
| throw new Error('media reupload request requires a message id') | ||
| } | ||
| if (!input.chatJid) { | ||
| throw new Error('media reupload request requires a chat jid') | ||
| } | ||
| const inFlight = pending.get(input.messageId) | ||
| if (inFlight) { | ||
| return inFlight.promise | ||
| } | ||
|
|
||
| const node = await buildRequestNode(input) | ||
| const entry = trackPending(input) | ||
| sendNode(node).catch((error: unknown) => { | ||
| rejectPending(input.messageId, toError(error)) | ||
| }) | ||
| return entry.promise | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The in-flight deduplication guarantee does not hold, and the public JSDoc documents it as if it does. createMediaRetryRequester.request reads pending before it awaits buildRequestNode, so two concurrent calls for the same messageId both create pending entries. The second entry replaces the first under the same key, and the first caller rejects with a timeout after timeoutMs.
src/message/primitives/media-retry.ts#L190-L208: calltrackPending(input)before the async node build, or re-checkpendingafter the await, so a concurrent duplicate joins the first promise.src/client/coordinators/WaMessageCoordinator.ts#L811-L857: keep the "A second call for a message already in flight joins the first request" sentence only after the requester enforces it; otherwise reword it.
📍 Affects 2 files
src/message/primitives/media-retry.ts#L190-L208(this comment)src/client/coordinators/WaMessageCoordinator.ts#L811-L857
🤖 Prompt for 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.
In `@src/message/primitives/media-retry.ts` around lines 190 - 208, The in-flight
deduplication guarantee is broken because createMediaRetryRequester.request
checks pending before awaiting buildRequestNode; update
src/message/primitives/media-retry.ts lines 190-208 so trackPending occurs
before the asynchronous build, or re-checks pending afterward, ensuring
duplicate messageId calls join the first promise. In
src/client/coordinators/WaMessageCoordinator.ts lines 811-857, retain the
documented joining behavior only if enforced by request; otherwise reword that
sentence to match the actual behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 19 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/media/crypto/media-retry.ts">
<violation number="1" location="src/media/crypto/media-retry.ts:23">
P3: WaMediaRetryDecryptInput.mediaKey holds raw cryptographic key material. Document it with an @sensitive tag noting that callers must encrypt it at rest and must never pass it to JSON.stringify() or console.log().</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| /** Input for {@link decryptMediaRetryNotification}. */ | ||
| export interface WaMediaRetryDecryptInput { |
There was a problem hiding this comment.
P3: WaMediaRetryDecryptInput.mediaKey holds raw cryptographic key material. Document it with an @sensitive tag noting that callers must encrypt it at rest and must never pass it to JSON.stringify() or console.log().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/media/crypto/media-retry.ts, line 23:
<comment>WaMediaRetryDecryptInput.mediaKey holds raw cryptographic key material. Document it with an @sensitive tag noting that callers must encrypt it at rest and must never pass it to JSON.stringify() or console.log().</comment>
<file context>
@@ -0,0 +1,84 @@
+}
+
+/** Input for {@link decryptMediaRetryNotification}. */
+export interface WaMediaRetryDecryptInput {
+ readonly mediaKey: Uint8Array
+ readonly ciphertext: Uint8Array
</file context>
…quest Two concurrent `requestMediaReupload` calls for the same message both passed the in-flight check, because that check ran before the awaited node build. The second entry then replaced the first under the same key, so one caller never settled and a second `server-error` receipt went out. The entry is now tracked before the build. The existing dedup test only exercised the sequential path - it waited for the first receipt before calling again - so it passed throughout. The new concurrency test fails against the old code. Also from the PR review: - `parseMediaRetryNotification` rejects an empty `<enc_p>` instead of handing a zero-length ciphertext to AES-GCM - `getNodeBytesContent` documents that empty content comes back as a zero-length view, mirroring the empty string `getNodeTextContent` returns, rather than claiming `undefined` - `WaMediaRetryDecryptInput.mediaKey` carries the same do-not-log note the rest of the codebase puts on media keys Adds the coverage `requestMediaReupload` was missing: deriving a request from an incoming message event, and the newsletter / no-media guards.
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/client/coordinators/__tests__/message-coordinator.test.ts`:
- Around line 167-193: Update the requestMediaReupload test to be async and
replace both assert.throws calls with await assert.rejects, preserving the
newsletter and no-downloadable-media error assertions. Ensure
requestMediaReupload propagates event-validation failures by rejecting its
returned Promise rather than throwing synchronously.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e4229f9-91c7-46fd-aa33-a9427275ff5c
📒 Files selected for processing (5)
src/client/coordinators/__tests__/message-coordinator.test.tssrc/media/crypto/media-retry.tssrc/message/primitives/__tests__/media-retry.test.tssrc/message/primitives/media-retry.tssrc/transport/node/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/transport/node/helpers.ts
- src/message/primitives/tests/media-retry.test.ts
- src/media/crypto/media-retry.ts
- src/message/primitives/media-retry.ts
Comment on lines
+167
to
+193
| test('requestMediaReupload rejects newsletters and messages without media', () => { | ||
| const coordinator = createCoordinator(createFakePdo().requester) | ||
| const base = { | ||
| rawNode: { tag: 'message', attrs: {} }, | ||
| offline: false, | ||
| message: { imageMessage: { directPath: '/v/x', mediaKey: new Uint8Array(32) } } | ||
| } | ||
| const key = { | ||
| remoteJid: '123@newsletter', | ||
| id: 'MSG1', | ||
| fromMe: false, | ||
| isGroup: false, | ||
| isBroadcast: false, | ||
| isNewsletter: true, | ||
| senderDevice: 0 | ||
| } | ||
|
|
||
| assert.throws(() => coordinator.requestMediaReupload({ ...base, key } as never), /newsletter/) | ||
| assert.throws( | ||
| () => | ||
| coordinator.requestMediaReupload({ | ||
| ...base, | ||
| key: { ...key, isNewsletter: false }, | ||
| message: { conversation: 'hi' } | ||
| } as never), | ||
| /no downloadable media/ | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- test file ---'
sed -n '1,230p' src/client/coordinators/__tests__/message-coordinator.test.ts
printf '%s\n' '--- implementation and references ---'
rg -n -C 8 'requestMediaReupload' srcRepository: vinikjkkj/zapo
Length of output: 19299
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
test_path = Path('src/client/coordinators/__tests__/message-coordinator.test.ts')
for i, line in enumerate(test_path.read_text().splitlines(), 1):
if 150 <= i <= 200:
print(f'{i}: {line}')
PY
printf '%s\n' '--- declaration and implementation locations ---'
rg -n -C 15 'requestMediaReupload' src/clientRepository: vinikjkkj/zapo
Length of output: 19010
🏁 Script executed:
#!/bin/bash
set -eu
node - <<'JS'
const assert = require('node:assert/strict')
function requestMediaReupload(source) {
if (source.key.isNewsletter) {
throw new Error('requestMediaReupload is not supported on newsletter messages')
}
if (!source.message.imageMessage) {
throw new Error('message has no downloadable media')
}
return Promise.resolve({ result: 'success' })
}
const event = {
key: { isNewsletter: true },
message: { imageMessage: { mediaKey: new Uint8Array(32) } }
}
let synchronousError = null
try {
requestMediaReupload(event)
} catch (error) {
synchronousError = error
}
assert.match(synchronousError?.message ?? '', /newsletter/)
assert.throws(() => requestMediaReupload(event), /newsletter/)
await assert.rejects(
Promise.resolve().then(() => requestMediaReupload(event)),
/newsletter/
)
console.log('validation throws synchronously; wrapping the call in a Promise converts it to rejection')
JSRepository: vinikjkkj/zapo
Length of output: 989
🏁 Script executed:
#!/bin/bash
set -eu
node - <<'JS'
const assert = require('node:assert/strict')
;(async () => {
function requestMediaReupload(source) {
if (source.key.isNewsletter) {
throw new Error('requestMediaReupload is not supported on newsletter messages')
}
if (!source.message.imageMessage) {
throw new Error('message has no downloadable media')
}
return Promise.resolve({ result: 'success' })
}
const event = {
key: { isNewsletter: true },
message: { imageMessage: { mediaKey: new Uint8Array(32) } }
}
let synchronousError = null
try {
requestMediaReupload(event)
} catch (error) {
synchronousError = error
}
assert.match(synchronousError?.message ?? '', /newsletter/)
assert.throws(() => requestMediaReupload(event), /newsletter/)
await assert.rejects(
Promise.resolve().then(() => requestMediaReupload(event)),
/newsletter/
)
console.log('validation throws synchronously; wrapping the call in a Promise converts it to rejection')
})()
JSRepository: vinikjkkj/zapo
Length of output: 241
Assert Promise rejection for requestMediaReupload.
Make the test async and use assert.rejects. Make event-validation failures reject the returned promise.
🤖 Prompt for 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.
In `@src/client/coordinators/__tests__/message-coordinator.test.ts` around lines
167 - 193, Update the requestMediaReupload test to be async and replace both
assert.throws calls with await assert.rejects, preserving the newsletter and
no-downloadable-media error assertions. Ensure requestMediaReupload propagates
event-validation failures by rejecting its returned Promise rather than throwing
synchronously.
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.
Adds
client.message.requestMediaReupload(), the round-trip WhatsApp Web runs when a media download answers 404/410 - typically old media surfaced by a history sync. Aserver-errorreceipt carrying an encryptedServerErrorReceiptgoes out, and the sender's primary device answers with amediaretrynotification holding a freshdirectPath.@media/crypto/media-retryseals and opens both payloads: HKDF-SHA-256 over the media key with theWhatsApp Media Retry Notificationcontext, AES-256-GCM bound to the stanza id as associated data.@message/primitives/media-retryowns the round-trip. A plain node query cannot serve here: the ack and the notification carry the same stanza id, so a query would resolve on the ack and drop the answer. Requests are keyed by message id, deduplicated, bounded, and expire.mediaretrynotification is parsed and settled through the existing incoming pipeline; the ack it already sent is unchanged.getNodeBytesContentjoins the transport helpers, replacing three copies of the same optional-node-bytes expression.On success only the
directPathchanges: the media key, hashes, and length of the original message stay valid.not_foundis a normal answer, meaning the sender no longer holds the file.Validated against the live server end to end, including an image whose CDN blob had expired 77 days earlier: the reupload returned a new
directPathand the download verified against the original keys.Summary by CodeRabbit
New Features
Bug Fixes