feat!: sticker pack sending support - #454
Conversation
Add proto-level helpers for creating and sending sticker pack messages, following the same pattern as album messages (no dedicated send method). New modules: - wacore/src/zip.rs: minimal store-only ZIP writer (no deps) - wacore/src/webp.rs: animated WebP detection via RIFF/VP8X parsing - wacore/src/sticker_pack.rs: create_sticker_pack_zip(), build_sticker_pack_message(), types Breaking changes: - Client::upload() now takes UploadOptions as third parameter - MediaType enum gains StickerPackThumbnail variant and #[non_exhaustive]
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds sticker-pack creation (ZIP + proto message), WebP animation detection, an in-memory ZIP writer, key-aware media encryption via new UploadOptions, re-exports Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Pack as StickerPackModule
participant Zip as ZipWriter
participant CoreUpload as wacore::upload
participant CDN as UploadService
Client->>Pack: create_sticker_pack(stickers, metadata, cover)
Pack->>Zip: add files / finish -> zip_bytes
Zip-->>Pack: zip_bytes
Pack->>CoreUpload: encrypt_media_with_key(zip_bytes, media_key?)
CoreUpload->>CDN: upload encrypted payload
CDN-->>CoreUpload: UploadResponse
CoreUpload-->>Pack: MediaUploadInfo (From<UploadResponse>)
Pack-->>Client: StickerPackMessage (with upload info)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 37s —— View job Code Review: Sticker Pack Sending Support
Overall this is a well-structured PR that follows the existing codebase patterns (builder pattern, proto-level helpers without dedicated send methods, Bug: Silent fallback on invalid media key length
let key_ref = options
.media_key
.as_ref()
.and_then(|k| <&[u8; 32]>::try_from(k.as_slice()).ok());If the caller passes a Potential overflow in WebP chunk parser
offset += 8 + chunk_size + (chunk_size & 1);
offset = match offset.checked_add(8 + chunk_size + (chunk_size & 1)) {
Some(next) => next,
None => break,
};Low severity since this is likely 64-bit only, but good defensive practice for a parser that handles untrusted input. Minor: Unnecessary allocation in dedup hash set
let mut seen_hashes = std::collections::HashSet::new();
for input in stickers {
let hash = Sha256::digest(input.data);
// ...
if seen_hashes.insert(hash.to_vec()) {
let hash: [u8; 32] = Sha256::digest(input.data).into();Verify:
|
![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: abd6d3d150
ℹ️ 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".
| put_u32(&mut self.buf, e.size); // compressed | ||
| put_u32(&mut self.buf, e.size); // uncompressed | ||
| put_u16(&mut self.buf, e.name.len() as u16); | ||
| put_zero_u16s(&mut self.buf, 5); // extra len, comment len, disk start, internal attrs |
There was a problem hiding this comment.
The central-directory record writes put_zero_u16s(..., 5) after the filename length, but ZIP format only has four 16-bit fields there (extra length, comment length, disk start, internal attrs). This extra zero shifts the encoded external attrs and relative offset fields by 2 bytes, so readers parse incorrect local-header offsets and treat the archive as corrupt, which breaks sticker-pack ZIP consumption for multi-entry archives.
Useful? React with 👍 / 👎.
| let key_ref = options | ||
| .media_key | ||
| .as_ref() | ||
| .and_then(|k| <&[u8; 32]>::try_from(k.as_slice()).ok()); |
There was a problem hiding this comment.
If UploadOptions.media_key is not exactly 32 bytes, try_from(...).ok() silently discards it and falls back to generating a fresh key. That makes callers believe key reuse succeeded when it did not; in sticker-pack flows this can produce thumbnail uploads encrypted with a different key than the one referenced in the final message, causing downstream decryption failures. This should return an explicit error instead of silently changing behavior.
Useful? React with 👍 / 👎.
Bencher Report![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/upload.rs`:
- Around line 294-298: The code silently drops an invalid-length
UploadOptions.media_key by using .ok(), causing unintended fallback to a new
key; instead, validate the media_key length and return an explicit error when it
isn't exactly 32 bytes so caller intent is preserved. In the block that builds
key_ref from UploadOptions.media_key (the variable key_ref and the conversion
using <&[u8; 32]>::try_from), check if media_key.is_some() and if try_from fails
then return Err(...) (or propagate a specific error type) rather than calling
.ok(), and only call wacore::upload::encrypt_media_with_key(&data, media_type,
key_ref) when key_ref is valid or when media_key was absent and a fresh key is
acceptable.
In `@wacore/src/sticker_pack.rs`:
- Around line 206-207: The code hardcodes thumbnail_height and thumbnail_width
to 252 in the sticker pack proto (in the
build_sticker_pack_message/MediaUploadInfo area), which can produce incorrect
metadata if the actual thumbnail size differs; update MediaUploadInfo (or the
build_sticker_pack_message function signature) to accept thumbnail_width and
thumbnail_height as parameters and populate the proto from those fields, or if
you prefer not to change the API add a clear doc/comment on
MediaUploadInfo/build_sticker_pack_message stating callers must supply a 252×252
thumbnail and that sizes will not be validated or adjusted.
In `@wacore/src/webp.rs`:
- Around line 34-36: The arithmetic offset += 8 + chunk_size + (chunk_size & 1)
can overflow for crafted large chunk_size; replace this with checked or
saturating arithmetic on offset and chunk_size (e.g., use checked_add or
saturating_add when summing 8, chunk_size, and (chunk_size & 1)) and return an
Err or handle the overflow if addition returns None, ensuring you update the
same offset variable safely (refer to offset and chunk_size in the parsing loop
/ function that computes chunk offsets).
In `@wacore/src/zip.rs`:
- Around line 45-46: Add a doc-comment to the ZipWriter type explaining that
sizes are stored as u32 (ZIP32) and that the current casts (data.len() as u32
and self.buf.len() as u32 in the write logic) will truncate values > 4GiB;
mention this is acceptable for small sticker WebP use but warn against reusing
ZipWriter for larger archives or suggest adding checks/assertions if larger
inputs may be expected. Ensure the comment references ZipWriter and the
size/offset casts so maintainers see the limitation when editing that code.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30acc5e5-1645-4f0c-9380-36c373c5fe9c
📒 Files selected for processing (11)
src/lib.rssrc/upload.rstests/e2e/tests/media.rstests/e2e/tests/newsletter.rswacore/src/download.rswacore/src/lib.rswacore/src/send.rswacore/src/sticker_pack.rswacore/src/upload.rswacore/src/webp.rswacore/src/zip.rs
| let size = data.len() as u32; | ||
| let offset = self.buf.len() as u32; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting ZIP32 size limitations.
The as u32 casts on data.len() and buf.len() will silently truncate values exceeding 4GB. While this is a ZIP32 format limitation and acceptable for the sticker pack use case (small WebP files), a brief doc comment on ZipWriter noting this constraint would help prevent misuse if the struct is later reused for other purposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/zip.rs` around lines 45 - 46, Add a doc-comment to the ZipWriter
type explaining that sizes are stored as u32 (ZIP32) and that the current casts
(data.len() as u32 and self.buf.len() as u32 in the write logic) will truncate
values > 4GiB; mention this is acceptable for small sticker WebP use but warn
against reusing ZipWriter for larger archives or suggest adding
checks/assertions if larger inputs may be expected. Ensure the comment
references ZipWriter and the size/offset casts so maintainers see the limitation
when editing that code.
- Error on invalid media_key length instead of silent fallback - Use checked_add in WebP chunk parser to prevent overflow - Use [u8; 32] instead of Vec<u8> in dedup HashSet to avoid allocations - Document 252x252 thumbnail requirement on build_sticker_pack_message
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 27s —— View job Code Review: Sticker Pack Sending Support
Well-structured PR that follows existing codebase patterns nicely. The fix commit (b1a55be) addressed the previous review's top findings (media_key validation, WebP overflow). One remaining bug and a few minor items: Bug: Central directory has extra u16 field (ZIP corruption)
put_zero_u16s(&mut self.buf, 5); // extra len, comment len, disk start, internal attrsThe comment lists 4 fields but writes 5 zero u16s. Per the ZIP spec (APPNOTE.TXT §4.3.12), after
The extra 2 bytes shift Should be: put_zero_u16s(&mut self.buf, 4); // extra len, comment len, disk start, internal attrsVerify:
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
wacore/src/webp.rs (1)
34-36:⚠️ Potential issue | 🟡 MinorThe chunk-advance expression can still overflow before
checked_addruns.
offset.checked_add(8 + chunk_size + (chunk_size & 1))only checks the final add. On 32-bit targets, the inner8 + chunk_size + ...can still wrap or panic first on crafted input.In Rust on 32-bit targets, can the subexpression in `offset.checked_add(8 + chunk_size + (chunk_size & 1))` overflow before `checked_add` is invoked, and what is the correct fully-checked pattern for this case?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/webp.rs` around lines 34 - 36, The expression offset.checked_add(8 + chunk_size + (chunk_size & 1)) can overflow in the inner arithmetic on narrower targets; replace it with a chain of checked_add calls to perform each addition safely (e.g. call offset.checked_add(8) then .and_then(|v| v.checked_add(chunk_size)) then .and_then(|v| v.checked_add(chunk_size & 1))) and use the resulting Option to match Some(next) / None to break; reference the variables offset, chunk_size and the checked_add usage to locate and update the code.
🤖 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/upload.rs`:
- Around line 263-268: UploadOptions currently derives Debug and will leak the
raw media_key; remove Debug from the derive list on the UploadOptions struct and
add a manual impl std::fmt::Debug for UploadOptions that prints the other fields
normally but redacts media_key (e.g., show "Some(<redacted>)" or "None").
Reference: the UploadOptions struct and its media_key field—keep Default and
Clone derives, drop Debug, and implement Debug to avoid printing the raw key.
In `@wacore/src/sticker_pack.rs`:
- Around line 181-210: The function build_sticker_pack_message currently mixes
data from StickerPackMetadata, StickerPackZipResult, and MediaUploadInfo without
verification—fix by enforcing consistency before building the proto: in
build_sticker_pack_message compute/validate that zip_upload.file_length equals
zip_result.zip_bytes.len() and that zip_upload.file_sha256 matches the SHA-256
of zip_result.zip_bytes (or alternatively change the function signature to
accept a single aggregated struct that pairs a StickerPackZipResult with its
MediaUploadInfo); also validate thumb_upload.direct_path/sha256 against the
tray/thumbnail bytes in StickerPackZipResult (and confirm metadata.pack_id
matches the pack identity inside zip_result if present), and return an error (or
panic) if any check fails so mismatched sources cannot produce an inconsistent
Message.
- Around line 128-148: The code creates a ZIP entry name using pack_id directly
in create_sticker_pack_zip (tray_icon_file_name = format!("{pack_id}.webp")),
which allows path traversal or invalid entry names; fix it by validating and
sanitizing pack_id before using it: ensure pack_id is non-empty, within a
reasonable length, and contains only a safe character set (e.g., alphanumeric,
dash, underscore) or replace/encode any path separators, dots, or control
characters (reject or canonicalize values containing "..", '/', '\\', or ASCII
control codes) and then build tray_icon_file_name from the sanitized value;
apply the same validation when constructing any other ZIP entry names and return
an error (bail!) on invalid pack_id.
In `@wacore/src/webp.rs`:
- Around line 23-27: The VP8X branch reads data[offset + 8] without ensuring the
VP8X chunk actually contains the 10-byte payload; add a guard that the parsed
chunk_size for the VP8X chunk is at least 10 (VP8X payload length) and that
offset + 8 is within data bounds before accessing data[offset + 8]; update the
condition that checks fourcc == b"VP8X" to require chunk_size >= 10 and keep the
existing bounds check so the animation flag read (data[offset + 8]) only occurs
when the full VP8X payload is present.
---
Duplicate comments:
In `@wacore/src/webp.rs`:
- Around line 34-36: The expression offset.checked_add(8 + chunk_size +
(chunk_size & 1)) can overflow in the inner arithmetic on narrower targets;
replace it with a chain of checked_add calls to perform each addition safely
(e.g. call offset.checked_add(8) then .and_then(|v| v.checked_add(chunk_size))
then .and_then(|v| v.checked_add(chunk_size & 1))) and use the resulting Option
to match Some(next) / None to break; reference the variables offset, chunk_size
and the checked_add usage to locate and update the code.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5efd666b-c061-4727-b134-4e5e184313bc
📒 Files selected for processing (3)
src/upload.rswacore/src/sticker_pack.rswacore/src/webp.rs
| pub fn build_sticker_pack_message( | ||
| zip_result: &StickerPackZipResult, | ||
| zip_upload: &MediaUploadInfo, | ||
| thumb_upload: &MediaUploadInfo, | ||
| metadata: StickerPackMetadata, | ||
| ) -> wa::Message { | ||
| use wa::message::sticker_pack_message::StickerPackOrigin; | ||
|
|
||
| let pack_msg = wa::message::StickerPackMessage { | ||
| sticker_pack_id: Some(metadata.pack_id), | ||
| name: Some(metadata.name), | ||
| publisher: Some(metadata.publisher), | ||
| stickers: zip_result.stickers.clone(), | ||
| file_length: Some(zip_upload.file_length), | ||
| file_sha256: Some(zip_upload.file_sha256.clone()), | ||
| file_enc_sha256: Some(zip_upload.file_enc_sha256.clone()), | ||
| media_key: Some(zip_upload.media_key.clone()), | ||
| direct_path: Some(zip_upload.direct_path.clone()), | ||
| caption: None, | ||
| context_info: None, | ||
| pack_description: metadata.description, | ||
| media_key_timestamp: Some(zip_upload.media_key_timestamp), | ||
| tray_icon_file_name: Some(zip_result.tray_icon_file_name.clone()), | ||
| thumbnail_direct_path: Some(thumb_upload.direct_path.clone()), | ||
| thumbnail_sha256: Some(thumb_upload.file_sha256.clone()), | ||
| thumbnail_enc_sha256: Some(thumb_upload.file_enc_sha256.clone()), | ||
| thumbnail_height: Some(252), | ||
| thumbnail_width: Some(252), | ||
| image_data_hash: None, | ||
| sticker_pack_size: Some(zip_result.zip_bytes.len() as u64), |
There was a problem hiding this comment.
Avoid building the proto from three independent pack sources.
This function pulls pack identity from metadata, tray-icon/sticker data from zip_result, and CDN metadata from zip_upload without any consistency check. A mismatched call can emit a proto whose hashes/direct path point to one ZIP while the sticker list or size describe another.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/sticker_pack.rs` around lines 181 - 210, The function
build_sticker_pack_message currently mixes data from StickerPackMetadata,
StickerPackZipResult, and MediaUploadInfo without verification—fix by enforcing
consistency before building the proto: in build_sticker_pack_message
compute/validate that zip_upload.file_length equals zip_result.zip_bytes.len()
and that zip_upload.file_sha256 matches the SHA-256 of zip_result.zip_bytes (or
alternatively change the function signature to accept a single aggregated struct
that pairs a StickerPackZipResult with its MediaUploadInfo); also validate
thumb_upload.direct_path/sha256 against the tray/thumbnail bytes in
StickerPackZipResult (and confirm metadata.pack_id matches the pack identity
inside zip_result if present), and return an error (or panic) if any check fails
so mismatched sources cannot produce an inconsistent Message.
put_zero_u16s wrote 5 zero u16s instead of 4 after file_name_length, shifting external_file_attributes and local_header_offset by 2 bytes. Added a test that validates byte-level offsets through the central directory to prevent regressions.
- Redact media_key in UploadOptions Debug impl - Validate pack_id to prevent path traversal in ZIP entry names - Require VP8X chunk_size >= 10 before reading animation flag - Chain checked_add calls to prevent inner arithmetic overflow
Remove fields WA Web doesn't set on outgoing sticker packs: media_key_timestamp, thumbnail_height/width, sticker_pack_origin, image_data_hash. Add caption field to StickerPackMetadata. Verified against GenerateStickerPackMessageProto.js.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02572b26f6
ℹ️ 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".
| file_length: Some(zip_upload.file_length), | ||
| file_sha256: Some(zip_upload.file_sha256.clone()), | ||
| file_enc_sha256: Some(zip_upload.file_enc_sha256.clone()), | ||
| media_key: Some(zip_upload.media_key.clone()), |
There was a problem hiding this comment.
StickerPackMessage carries a single media_key, but this builder always takes it from zip_upload and never verifies thumb_upload.media_key is the same key. If callers upload the thumbnail with Default::default() (fresh key) instead of reusing the ZIP key, the message will include thumbnail hashes/direct path for bytes encrypted under a different key than recipients derive from media_key, causing thumbnail decryption failures. Please fail fast on key mismatch (or make this function return Result) to prevent silently constructing inconsistent messages.
Useful? React with 👍 / 👎.
1 participant
Summary
wacore/src/zip.rs(store-only ZIP writer),wacore/src/webp.rs(animated detection),wacore/src/sticker_pack.rs(helpers + types)StickerPackThumbnailMediaType andDownloadableimpl forStickerPackMessageUploadOptionswith optionalmedia_keyfor shared-key uploads (sticker pack thumbnail reuses ZIP's key)GenerateStickerPackMessageProto.jsexactlyBreaking changes
Client::upload()now takesUploadOptionsas third parameter (useDefault::default()for existing behavior)MediaTypegainsStickerPackThumbnailvariant and#[non_exhaustive]WA Web parity
All protocol details verified against captured WhatsApp Web JS:
"WhatsApp Sticker Pack Keys"CreateMediaKeys.js:62"WhatsApp Sticker Pack Thumbnail Keys"CreateMediaKeys.js:63-64"sticker-pack"/"thumbnail-sticker-pack"MmsMediaTypes.js:40-41/mms/sticker-pack//mms/thumbnail-sticker-packClientFormatHashUrl.js:42"sticker_pack"JobsCommon.js:220-221Prep.js:271-274,308,334GenerateStickerPackMessageProto.jsUsage
Test plan
cargo clippy --all --testscleancargo fmt --all --checkcleanSummary by CodeRabbit
New Features
Tests