feat: add Sonar-compatible sticker support - #875
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesSonar sticker integration
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/storage-sqlite/src/timeline.rs (1)
797-805: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle
NULLvalues when parsingtags_json.If
tags_jsonin themessage_timelinetable can beNULL(e.g., for rows created before the schema migration), usingrow.get::<_, String>will return arusqlite::Error::InvalidColumnTypeand fail the query. Consider reading it as anOption<String>.🐛 Proposed fix
Ok(( row.get::<_, String>(0)?, row.get::<_, String>(1)?, row.get::<_, String>(2)?, row.get::<_, i64>(3)?, - row.get::<_, String>(4)?, + row.get::<_, Option<String>>(4)?, row.get::<_, i64>(5)?, row.get::<_, i64>(6)?, ))🤖 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 `@crates/storage-sqlite/src/timeline.rs` around lines 797 - 805, Update the row-mapping tuple in the timeline query to read the nullable tags_json column as Option<String> instead of String, preserving successful parsing for existing non-NULL values and allowing legacy NULL rows without InvalidColumnType errors.
🤖 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 `@crates/marmot-app/src/relay_plane/mod.rs`:
- Around line 381-393: Update the relay connection loop in fetch_public_events
so failures from add_relay, connection timeout, or connect_relay are handled per
relay without propagating via ?. Log or otherwise record each individual
failure, then continue iterating through relay_urls so successfully connected
relays can still be used for fetching.
- Around line 381-393: Update the relay connection loop in fetch_public_events
so failures from add_relay, connection timeout, or connect_relay do not
propagate through ?. Handle each relay independently, optionally logging the
failure, and continue processing the remaining relay_urls so fetching proceeds
with all successfully connected relays.
In `@crates/marmot-app/src/stickers.rs`:
- Around line 1030-1048: Update the PNG parsing loop around the acTL handling to
count each fcTL chunk as an actual APNG frame, enforce the existing frame limit
against that count, and require exactly one valid acTL declaration whose frame
count matches the observed fcTL total before accepting the image. Do not rely
solely on the declared frames value.
- Around line 459-528: Update the sticker import flow surrounding the upload
loop and pack publication to persist a staged import intent before the first
external side effect. Advance that intent after each successful upload, signing,
publication, and install-operation step, and add compensation or resumable
recovery for failures after any step has completed so no orphaned assets or
published packs remain without durable state.
- Around line 239-247: Update the sticker listing flow around
desired_installed_sticker_packs and sticker_packs so installed_only is evaluated
against the desired installed coordinate set rather than the remote base
projection. Pass that desired set into storage filtering, while preserving the
existing search behavior and applying limit semantics to the resulting
desired-state-filtered packs.
- Around line 324-325: In the sync flow around sync_sticker_packs, call
refresh_installed_base(self, &context) before flush_sticker_outbox(self,
&context). Ensure the outbox replay and resulting publication are regenerated
from the refreshed remote winner so pending kind-10031 operations are not
cleared against a stale local base.
- Around line 454-456: In the imported sticker loop, add a
MAX_STICKER_ASSET_BYTES validation for imported_sticker.bytes before
inspect_image and upload processing. Reject oversized sticker data using the
existing error-handling pattern, while preserving the current inspection and
upload flow for buffers within the limit.
In `@crates/storage-sqlite/src/chat_list.rs`:
- Around line 902-904: Handle nullable tags_json consistently across
crates/storage-sqlite/src/chat_list.rs:902-904,
crates/storage-sqlite/src/timeline.rs:797-805, and
crates/storage-sqlite/src/timeline.rs:2418-2420: read column 4 as
Option<String>, preserve the row tuple mapping at timeline.rs:797-805, and make
the chat-list and timeline tag parsing fall back to an empty vector when the
value is None or invalid JSON.
- Around line 902-904: Update the tags parsing in the row-conversion function
around the tags field to read tags_json as an Option<String>, then fall back to
the same default used by chat_list_row_from_row when it is NULL before
deserializing. Preserve the existing FromSqlConversionFailure mapping for
invalid JSON.
In `@crates/storage-sqlite/src/timeline.rs`:
- Around line 2418-2420: Update the tags parsing in the row-mapping code around
the tags field to read the column as Option<String> instead of String, then fall
back to the existing default tags value when it is NULL before deserializing.
Preserve the current serde_json error-to-rusqlite conversion for present values.
---
Outside diff comments:
In `@crates/storage-sqlite/src/timeline.rs`:
- Around line 797-805: Update the row-mapping tuple in the timeline query to
read the nullable tags_json column as Option<String> instead of String,
preserving successful parsing for existing non-NULL values and allowing legacy
NULL rows without InvalidColumnType errors.
🪄 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: CHILL
Plan: Pro
Run ID: 89219b8a-5074-46b1-bd4f-1b8faebd7fc2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
Cargo.tomlcrates/marmot-app/Cargo.tomlcrates/marmot-app/src/client/audit.rscrates/marmot-app/src/client/mod.rscrates/marmot-app/src/client/projection.rscrates/marmot-app/src/client/push.rscrates/marmot-app/src/error.rscrates/marmot-app/src/lib.rscrates/marmot-app/src/media/blossom.rscrates/marmot-app/src/media/mod.rscrates/marmot-app/src/messages/intents.rscrates/marmot-app/src/notifications.rscrates/marmot-app/src/relay_plane/mod.rscrates/marmot-app/src/runtime/mod.rscrates/marmot-app/src/stickers.rscrates/marmot-app/src/tests.rscrates/marmot-uniffi/src/commands/message.rscrates/marmot-uniffi/src/commands/mod.rscrates/marmot-uniffi/src/commands/sticker.rscrates/marmot-uniffi/src/conversions/chat_list.rscrates/marmot-uniffi/src/conversions/message.rscrates/marmot-uniffi/src/conversions/mod.rscrates/marmot-uniffi/src/conversions/notification.rscrates/marmot-uniffi/src/conversions/sticker.rscrates/marmot-uniffi/src/conversions/timeline.rscrates/marmot-uniffi/src/errors.rscrates/marmot-uniffi/src/lib.rscrates/marmot-uniffi/tests/smoke.rscrates/storage-sqlite/src/chat_list.rscrates/storage-sqlite/src/lib.rscrates/storage-sqlite/src/migrations.rscrates/storage-sqlite/src/migrations/0026_sonar_stickers.rscrates/storage-sqlite/src/stickers.rscrates/storage-sqlite/src/timeline.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/storage-sqlite/src/stickers.rs (1)
180-192: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEscape SQL
LIKEmetacharacters to preserve literal search semantics.The installed path uses literal
contains, while this path treats%and_as wildcards. The same query therefore returns different matches depending oninstalled_only.Proposed fix
- let pattern = format!("%{}%", search.to_ascii_lowercase()); + let escaped = search + .replace('!', "!!") + .replace('%', "!%") + .replace('_', "!_"); + let pattern = format!("%{escaped}%"); ... - WHERE lower(title) LIKE ?1 - OR lower(COALESCE(description, '')) LIKE ?1 + WHERE lower(title) LIKE ?1 ESCAPE '!' + OR lower(COALESCE(description, '')) LIKE ?1 ESCAPE '!'🤖 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 `@crates/storage-sqlite/src/stickers.rs` around lines 180 - 192, Update the searched branch of the sticker-pack lookup around the search pattern and SQL in the coordinates query to escape LIKE metacharacters (`%`, `_`, and the escape character) before binding the value, then add the matching ESCAPE clause. Preserve case-insensitive contains semantics so installed and non-installed searches return equivalent literal matches.
🤖 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.
Outside diff comments:
In `@crates/storage-sqlite/src/stickers.rs`:
- Around line 180-192: Update the searched branch of the sticker-pack lookup
around the search pattern and SQL in the coordinates query to escape LIKE
metacharacters (`%`, `_`, and the escape character) before binding the value,
then add the matching ESCAPE clause. Preserve case-insensitive contains
semantics so installed and non-installed searches return equivalent literal
matches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02f27817-87f0-483f-b20b-9ade3aa669c6
📒 Files selected for processing (3)
crates/marmot-app/src/relay_plane/mod.rscrates/marmot-app/src/stickers.rscrates/storage-sqlite/src/stickers.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/marmot-app/src/relay_plane/mod.rs
- crates/marmot-app/src/stickers.rs
|
Addressed the remaining non-inline CodeRabbit feedback in 9b8f3cb: sticker-pack searches now escape SQLite LIKE metacharacters ( |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
cb64f17 to
e0e12be
Compare
|
@vincenzopalazzo I think we need to do more protocol level work before we can get to a PR to add this to MDK. ideally we would have a NIP PR up on the NIPs repo. Do you want to do that work starting from the doc you have (https://github.com/hedwig-corp/bitchat-to-sonar/blob/162ac26c88b351dfa3f35d8f098c00a18ae46114/docs/SONAR-STICKERS.md#identifiers)? Or do you want to start something new? |
|
Hey @erskingardner talking with fiatjaf in DM he direct me to open a couple of PR on nostr-protocol/nips#2410 and nostr-protocol/registry-of-kinds#4 based on the docs that I have on sonar website Are these enough? Or were you referring to something deeper? |
Automated multi-agent auditFinal auditHigh — GIF frame geometry bypasses sticker dimension limits
Validate every frame’s nonzero dimensions and pixel count, and require its bounds to fit within the logical screen. Add adversarial tests for oversized and out-of-canvas descriptors. Medium — Pack links disclose user interest to attacker-selected relaysThe parser accepts relay hints embedded in Ignore untrusted link-provided hints and fetch through configured relays. If interoperability requires hints, an alternative is explicit user approval or a trusted-relay allowlist; relay sanitization alone does not address this privacy leak. Need, design, and alternativesTyped sticker references, hash-pinned historical assets, encrypted per-account projections, and an outbox/rebase model address a legitimate need and fit the existing architecture. The principal alternative—resolving mutable pack state without preserving hash-exact asset history—would weaken message integrity, so the chosen persistence design is justified. Local Signal import is also reasonable, provided the image-validation boundary is completed as described above. The advisory WebP frame-count concern is not actionable: Advisory audit of head |
|
@vincenzopalazzo want to rebase this and add some details about the NIP in the comments? would love to get it into the apps. |
Nightly deep audit
Advisory deep pass; no approval, merge, push, checkout, or PR-code execution was performed. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Health remediation applied for stale head
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/marmot-uniffi/src/conversions/timeline.rs (1)
112-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd conversion tests for the new sticker fields.
The current application test checks event encoding only. It does not execute either FFI conversion. Add valid kind-9 fixtures for
TimelineReplyPreviewFfiandTimelineMessageRecordFfi. Assert thatstickercontains the coordinate, shortcode, and hash. Also assert that non-sticker kinds remainNone.Also applies to: 226-240
🤖 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 `@crates/marmot-uniffi/src/conversions/timeline.rs` around lines 112 - 119, Add conversion tests for sticker handling in the conversion paths producing TimelineReplyPreviewFfi and TimelineMessageRecordFfi. Create valid kind-9 fixtures and assert sticker preserves the coordinate, shortcode, and hash; add non-sticker fixtures asserting sticker is None, covering the sticker_ref_from_tags conversion.crates/storage-sqlite/src/migrations/0047_sticker_asset_history.rs (1)
8-26: 🩺 Stability & Availability | 🔵 TrivialPlan a retention policy for
app_sticker_assets.Each republished pack version adds one row per changed shortcode. Only pack deletion removes rows, through the FK cascade. For discovered packs that the user never installs, this table grows without bound in the per-account database. Consider a retention pass that keeps assets referenced by retained messages plus the current pack version, and prunes the rest. The PR objectives list unbounded persistence of discovered packs as an open concern.
🤖 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 `@crates/storage-sqlite/src/migrations/0047_sticker_asset_history.rs` around lines 8 - 26, Add a retention policy for app_sticker_assets that periodically removes obsolete assets while preserving assets referenced by retained messages and those belonging to the current version of each pack. Ensure the cleanup also covers discovered packs that were never installed, without removing assets still needed by retained data or active packs.
🤖 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 `@Cargo.toml`:
- Line 122: The sonar-stickers dependency declaration must be changed so Cargo
can resolve its manifest: replace the current git revision in the dependency
entry with a registry package, a repository exposing a root Cargo.toml, or a
vendored crate source, while preserving the existing nostr and signal-import
features.
In `@crates/storage-sqlite/src/chat_list.rs`:
- Around line 1105-1110: Update the chat-list migration or completeness check
around the INSERT projection containing last_message_tags_json so existing rows
with messages and a NULL last_message_tags_json are rebuilt. Add validation for
this NULL state, or introduce an explicit projection migration, while preserving
the existing rebuild behavior for rows whose tag data is already populated.
In `@crates/storage-sqlite/src/migrations/0046_sonar_stickers.rs`:
- Around line 40-41: Remove the UNIQUE constraint from the
idx_app_stickers_pack_hash definition in the 0046 migration, replacing it with a
regular index on pack_coordinate and sha256 so packs may reuse an image across
shortcodes. Preserve hash lookup performance without changing sticker_for_ref or
the insertion behavior in replace_sticker_pack_if_newer.
---
Nitpick comments:
In `@crates/marmot-uniffi/src/conversions/timeline.rs`:
- Around line 112-119: Add conversion tests for sticker handling in the
conversion paths producing TimelineReplyPreviewFfi and TimelineMessageRecordFfi.
Create valid kind-9 fixtures and assert sticker preserves the coordinate,
shortcode, and hash; add non-sticker fixtures asserting sticker is None,
covering the sticker_ref_from_tags conversion.
In `@crates/storage-sqlite/src/migrations/0047_sticker_asset_history.rs`:
- Around line 8-26: Add a retention policy for app_sticker_assets that
periodically removes obsolete assets while preserving assets referenced by
retained messages and those belonging to the current version of each pack.
Ensure the cleanup also covers discovered packs that were never installed,
without removing assets still needed by retained data or active packs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0003d8fd-9917-46de-adbe-1cb8a6a36469
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
Cargo.tomlcrates/marmot-app/Cargo.tomlcrates/marmot-app/src/client/audit.rscrates/marmot-app/src/client/mod.rscrates/marmot-app/src/client/projection.rscrates/marmot-app/src/client/push.rscrates/marmot-app/src/error.rscrates/marmot-app/src/lib.rscrates/marmot-app/src/media/blossom.rscrates/marmot-app/src/media/mod.rscrates/marmot-app/src/messages/intents.rscrates/marmot-app/src/notifications.rscrates/marmot-app/src/relay_plane/mod.rscrates/marmot-app/src/runtime/mod.rscrates/marmot-app/src/stickers.rscrates/marmot-app/src/tests.rscrates/marmot-uniffi/src/commands/message.rscrates/marmot-uniffi/src/commands/mod.rscrates/marmot-uniffi/src/commands/sticker.rscrates/marmot-uniffi/src/conversions/chat_list.rscrates/marmot-uniffi/src/conversions/message.rscrates/marmot-uniffi/src/conversions/mod.rscrates/marmot-uniffi/src/conversions/notification.rscrates/marmot-uniffi/src/conversions/sticker.rscrates/marmot-uniffi/src/conversions/timeline.rscrates/marmot-uniffi/src/errors.rscrates/marmot-uniffi/src/lib.rscrates/storage-sqlite/src/chat_list.rscrates/storage-sqlite/src/lib.rscrates/storage-sqlite/src/migrations.rscrates/storage-sqlite/src/migrations/0046_sonar_stickers.rscrates/storage-sqlite/src/migrations/0047_sticker_asset_history.rscrates/storage-sqlite/src/stickers.rscrates/storage-sqlite/src/timeline.rs
🚧 Files skipped from review as they are similar to previous changes (23)
- crates/marmot-app/src/client/push.rs
- crates/marmot-app/src/client/audit.rs
- crates/marmot-app/src/error.rs
- crates/marmot-app/src/messages/intents.rs
- crates/marmot-app/Cargo.toml
- crates/marmot-app/src/media/mod.rs
- crates/marmot-app/src/notifications.rs
- crates/marmot-app/src/media/blossom.rs
- crates/marmot-uniffi/src/commands/message.rs
- crates/marmot-app/src/client/projection.rs
- crates/marmot-uniffi/src/conversions/chat_list.rs
- crates/marmot-uniffi/src/conversions/mod.rs
- crates/marmot-app/src/relay_plane/mod.rs
- crates/marmot-app/src/lib.rs
- crates/storage-sqlite/src/lib.rs
- crates/marmot-uniffi/src/conversions/notification.rs
- crates/marmot-app/src/client/mod.rs
- crates/marmot-uniffi/src/commands/sticker.rs
- crates/storage-sqlite/src/timeline.rs
- crates/marmot-uniffi/src/conversions/message.rs
- crates/marmot-app/src/stickers.rs
- crates/marmot-uniffi/src/commands/mod.rs
- crates/marmot-uniffi/src/conversions/sticker.rs
- Drop UNIQUE constraint on idx_app_stickers_pack_hash so packs can reuse the same image across multiple shortcodes without ingestion failure. - Add a chat-list completeness check that rebuilds rows with a last message but NULL last_message_tags_json, fixing stale projection state after migration 0046. - Add tests for both fixes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/storage-sqlite/src/stickers.rs (2)
61-159: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound persisted sticker-pack retention.
replace_sticker_pack_if_newerretains historical assets for accepted packs. The listing limit only bounds reads. It does not bound writes.A stream of valid unique packs and assets can exhaust the per-account SQLite database. Retain assets required by message references, but evict uninstalled and unreferenced data under a quota, or enforce a bounded ingestion policy before persistence. Add a quota regression test.
🤖 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 `@crates/storage-sqlite/src/stickers.rs` around lines 61 - 159, Add a bounded retention policy to replace_sticker_pack_if_newer so accepted packs cannot grow the per-account database without limit. After the transactional replacement, evict uninstalled sticker-pack assets and packs that are not required by message references until storage is within the configured quota, preserving all referenced data. Add a regression test that ingests enough unique packs/assets to exceed the quota and verifies unreferenced data is removed.
368-384: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not clear operations outside the published snapshot.
A local install operation can be enqueued after an outbox event is created and before its publication commit succeeds. Clearing the pending-operation set after the publication wins removes that newer local intent even though the published event does not contain it.
Store an operation sequence or operation identifiers with each outbox snapshot. Delete only the operations included in that snapshot in the same transaction. Add a regression test for this interleaving.
🤖 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 `@crates/storage-sqlite/src/stickers.rs` around lines 368 - 384, The commit_installed_sticker_publication flow currently deletes all pending install operations, so it must instead delete only the operation sequence or identifiers captured in the corresponding published outbox snapshot. Extend the snapshot and replace_installed_sticker_packs_tx transaction flow to carry those identifiers, and perform the selective deletion atomically with the publication update. Add a regression test covering an operation added after snapshot creation but before commit, verifying that the newer operation remains pending.
🤖 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.
Outside diff comments:
In `@crates/storage-sqlite/src/stickers.rs`:
- Around line 61-159: Add a bounded retention policy to
replace_sticker_pack_if_newer so accepted packs cannot grow the per-account
database without limit. After the transactional replacement, evict uninstalled
sticker-pack assets and packs that are not required by message references until
storage is within the configured quota, preserving all referenced data. Add a
regression test that ingests enough unique packs/assets to exceed the quota and
verifies unreferenced data is removed.
- Around line 368-384: The commit_installed_sticker_publication flow currently
deletes all pending install operations, so it must instead delete only the
operation sequence or identifiers captured in the corresponding published outbox
snapshot. Extend the snapshot and replace_installed_sticker_packs_tx transaction
flow to carry those identifiers, and perform the selective deletion atomically
with the publication update. Add a regression test covering an operation added
after snapshot creation but before commit, verifying that the newer operation
remains pending.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 14722397-9fc5-4288-be80-e2a7b321594f
📒 Files selected for processing (4)
crates/storage-sqlite/src/chat_list.rscrates/storage-sqlite/src/chat_list/tests.rscrates/storage-sqlite/src/migrations/0046_sonar_stickers.rscrates/storage-sqlite/src/stickers.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/storage-sqlite/src/migrations/0046_sonar_stickers.rs
Datawav
left a comment
Datawav
left a comment
There was a problem hiding this comment.
Automated review
Outcome: three actionable defects remain.
-
Medium — Historical sticker assets can be lost.
crates/storage-sqlite/src/stickers.rs:69-71,111-155returns when an older pack loses replacement ordering, before recording its immutable assets; both ingestion paths use this API (crates/marmot-app/src/stickers.rs:713-743). Exact-hash misses trigger relay resolution only when no current pack exists (:332-355). Consequently, an older message can permanently renderStickerNotFoundwhen a newer pack version arrived first. Record assets from every valid pack event independently of current-pack replacement, and perform bounded historical relay resolution whenever the exact(coordinate, shortcode, hash)asset is absent. -
Medium — Signal import publishes assets before deterministic validation completes.
crates/marmot-app/src/stickers.rs:543-557uploads each plaintext sticker before validating later stickers; metadata and final pack construction follow at:558-599. A malformed later sticker or invalid pack therefore returns an error after earlier assets have been durably uploaded to Blossom. Validate all assets and construct a side-effect-free upload plan before the first upload; retain explicit handling for unavoidable partial failures during upload or later publication. -
Medium — Animated frame bounds are not validated.
crates/marmot-app/src/stickers.rs:1227-1238limits only reported canvas dimensions; GIF parsing ignores descriptor offsets and dimensions at:1346-1373, while WebP parsing merely countsANMFchunks at:1427-1465. Oversized, overflowing, or out-of-canvas frames can pass validation and cause downstream decoder failures or disproportionate allocation. Parse every GIF descriptor and WebPANMFrectangle, rejecting zero-sized, overflowing, over-limit, or out-of-canvas frames.
| self.connection.with_transaction(|| { | ||
| let conn = self.lock()?; | ||
| let existing = sticker_pack_version_tx(&conn, &pack.coordinate)?; | ||
| if !replacement_wins(existing.as_ref(), &pack.version) { |
There was a problem hiding this comment.
Returning before the asset inserts means a valid older replacement event contributes none of its immutable sticker assets. If a newer pack version arrived first, messages referencing an asset that exists only in the older version can remain StickerNotFound; the fetch path also skips relay resolution whenever any current pack exists. Persist assets from every valid pack event independently of whether it wins replacement ordering, and allow bounded historical resolution when the exact (coordinate, shortcode, hash) is absent.
| if sha256_hex(&imported_sticker.bytes) != imported_sticker.sha256 { | ||
| return Err(invalid_sticker("Signal sticker hash mismatch")); | ||
| } | ||
| let url = upload_blossom_blob( |
There was a problem hiding this comment.
This starts a durable Blossom upload before the remaining stickers and final pack metadata have been validated. If a later image, hash, sticker constructor, address, or pack constructor fails, previously uploaded blobs are orphaned even though the import returns an error. Validate every asset and construct the complete pack/upload plan without side effects before the first upload; still handle partial failures during the upload/publication phase explicitly.
| || inspected.height == 0 | ||
| || inspected.width > MAX_STICKER_DIMENSION | ||
| || inspected.height > MAX_STICKER_DIMENSION | ||
| || u64::from(inspected.width) * u64::from(inspected.height) > MAX_STICKER_PIXELS |
There was a problem hiding this comment.
These checks constrain only the reported canvas, not individual animation frames. The GIF parser skips descriptor offsets and dimensions, and the WebP parser only counts ANMF chunks, so zero-sized, overflowing, over-limit, or out-of-canvas frames can pass validation and trigger excessive allocation or decoder failures downstream. Parse and validate every GIF descriptor and WebP ANMF rectangle against the canvas and configured limits.
|
PR health repair is blocked: this exact head conflicts with current |
Datawav
left a comment
Datawav
left a comment
There was a problem hiding this comment.
Nightly deep review
Deep audit — marmot-protocol/mdk#875 at 301f4450007f5eb741fbf8cfa2935e7d3985f1ef
- High — Relay-controlled discovery causes unbounded permanent database growth
Evidence:
sync_sticker_packsfetches recent kind-30031 events and persists every valid pack, without requiring installation, an existing message reference, or prior user intent:crates/marmot-app/src/stickers.rs:402-415,725-745.replace_sticker_pack_if_newerpermanently adds each distinct(coordinate, shortcode, sha256)toapp_sticker_assets:crates/storage-sqlite/src/stickers.rs:66-72,116-156.app_sticker_assetsretains every attacker-selected hash. Its only deletion path is cascading deletion of the parent pack, but this patch provides no pack deletion or history-pruning path:crates/storage-sqlite/src/migrations/0047_sticker_asset_history.rs:7-20.MAX_DISCOVERY_PACKSand per-pack limits bound one synchronization response, not cumulative storage. An attacker can continually publish fresh coordinates, use multiple authors, or publish winning replacements with fresh hashes.
Trigger and impact:
- Repeated synchronization against a relay carrying attacker-created valid packs causes the per-account encrypted SQLite database to grow indefinitely.
- The same account can be attacked repeatedly across launches; the in-memory mutation lock does not bound durable state.
- Eventually this can exhaust device storage or make migrations, backups, queries, and account startup increasingly expensive.
Why a design change is needed:
- A per-request limit cannot enforce a durable-state invariant.
- A per-author quota alone is insufficient because relay identities are cheap.
- Simply limiting history per coordinate conflicts with the requirement to resolve immutable references from old messages.
Required design:
- Separate bounded discovery metadata from durable, user-relevant state.
- Persist packs durably only when installed, explicitly fetched, or referenced by retained message history; otherwise keep discovery in a globally bounded LRU/cache.
- Add transactional global byte/row limits and garbage collection. Protect installed packs, pending outbox/install operations, current pack versions, and assets reachable from retained message references.
- Permit pruned historical assets to be recovered through bounded exact-reference fetching, as described in finding 2.
Alternatives:
- A strict global row/byte quota with deterministic eviction is acceptable if eviction preserves the protected roots above.
- Persisting all discovery remains viable only if it uses a separately bounded cache database that can be safely discarded.
Regression coverage:
- Synchronize more than the configured durable quota across fresh coordinates and replacement hashes; assert row and byte bounds remain enforced.
- Verify installed packs, pending publications, and assets referenced by retained messages survive eviction.
- Verify quota checks and ingestion occur in one transaction so concurrent fetches cannot overshoot the bound.
- Medium — Historical sticker resolution fails after a newer pack version is stored
Evidence:
replace_sticker_pack_if_newerreturns before inserting assets whenever the incoming event loses NIP-01 replacement ordering:crates/storage-sqlite/src/stickers.rs:69-71.- Historical asset insertion happens only while replacing the current pack:
crates/storage-sqlite/src/stickers.rs:111-156. - Both relay ingestion paths route every event through this replacement-only method:
crates/marmot-app/src/stickers.rs:713-743. fetch_sticker_assetperforms an on-demand relay fetch only when the entire pack is absent. If the current pack exists but the requested(shortcode, hash)does not, it immediately returnsStickerNotFound:crates/marmot-app/src/stickers.rs:332-355.
Trigger and impact:
- A device receives the newest replacement event before an older valid version, or first encounters an old message after caching the current pack.
- The old event cannot contribute its immutable asset mapping, and the exact-hash miss does not trigger historical resolution.
- Valid older messages consequently lose their stickers even when relays return the signed historical event.
Why both paths must change:
- Ingesting assets from all events fixes out-of-order bulk synchronization but not an existing-pack exact-hash miss.
- Fetching on every miss without retaining historical mappings would repeatedly perform network work and still depend on relay ordering.
Required design:
- Split pack ingestion into two independently enforced operations:
- Insert validated immutable asset mappings from every accepted pack event using conflict-safe semantics.
- Replace
app_sticker_packsandapp_stickersonly whenreplacement_wins.
- On an exact asset miss, perform a bounded coordinate-specific historical fetch even when a newer current pack exists, ingest all valid returned versions, and retry the exact lookup.
- Apply durable history bounds together with finding 1; old mappings should be retained when reachable and otherwise safely refetchable.
Alternatives:
- A dedicated historical-assets cache is suitable if exact references remain keyed by coordinate, shortcode, and plaintext hash.
- Storing full historical pack versions is unnecessary if only immutable asset metadata is required for rendering.
Regression coverage:
- Store a newer version first, then ingest an older version containing a different hash for the same shortcode. Assert the older exact reference resolves while the newer pack remains current.
- Start with only the newer version stored, request the old exact hash, return both versions from the relay, and assert bounded recovery succeeds.
- Exercise equal-timestamp event-ID ordering to ensure asset ingestion is independent of replacement convergence.
- Medium — Signal import uploads plaintext assets before deterministic validation completes
Evidence:
- Each sticker is uploaded inside the same loop that validates subsequent stickers:
crates/marmot-app/src/stickers.rs:543-557. - Sticker metadata construction follows each upload, and cover selection, pack address construction, final
StickerPack::new, signing, and event publication happen only after all uploads:crates/marmot-app/src/stickers.rs:558-609.
Trigger and impact:
- The first sticker is valid and uploaded, but a later sticker fails size, image, hash, or metadata validation.
- Final cover/pack construction or signing can also fail after every plaintext asset has been uploaded.
- The API returns an import failure while remotely durable Blossom content remains for a pack that was never successfully constructed or published. Retrying may repeat uploads.
Why restructuring is needed:
- Local validation failures are fully avoidable before the first irreversible network side effect.
- Moving validation earlier cannot make the complete distributed operation transactional: later upload, signing, or relay failures can still leave partial uploads.
Required design:
- First validate every asset, hash, image property, shortcode, metadata field, cover relationship, address, title/description, and final pack shape without network side effects.
- Produce a validated upload plan, then upload assets and substitute returned URLs into the final publication model.
- Explicitly define recovery for failures after uploads begin: idempotent content-addressed reuse, supported Blossom deletion/rollback, or durable resumable import state.
Alternatives:
- Content-addressed idempotent uploads can make retry reuse the same blobs, reducing duplicates, but they do not remove orphaned plaintext.
- A server supporting authenticated staging and commit/abort would provide a stronger publication boundary, at greater protocol complexity.
Regression coverage:
- Use a recording uploader or loopback Blossom server with a valid first sticker and malformed second sticker; assert validation fails with zero upload requests.
- Inject failure during a later upload, signing, and relay publication; verify the documented resume/cleanup behavior and that retries do not create uncontrolled additional state.
- Medium — Animated GIF and WebP frame rectangles bypass dimension bounds
Evidence:
- The global validation applies limits only to dimensions returned by the format inspector:
crates/marmot-app/src/stickers.rs:1227-1238. - GIF parsing counts image descriptors but does not validate each descriptor’s left, top, width, or height fields:
crates/marmot-app/src/stickers.rs:1346-1373. - WebP parsing counts
ANMFchunks without validating frame offsets and dimensions against the canvas:crates/marmot-app/src/stickers.rs:1427-1465.
Trigger and impact:
- A file declares an acceptable canvas but contains a zero-sized, oversized, overflowing, or out-of-canvas animation frame rectangle.
- It passes the patch’s validation despite the claimed exact dimension/frame enforcement.
- Downstream platform decoders may reject it, allocate disproportionate resources, or exhibit decoder-specific behavior, creating inconsistent rendering and avoidable resource risk.
Required design:
- Parse every GIF image descriptor and WebP
ANMFframe rectangle. - Reject zero dimensions, arithmetic overflow, dimensions above the configured per-axis/pixel limits, and rectangles extending beyond the declared canvas.
- Use checked arithmetic for all offset-plus-size and width-times-height calculations.
Alternatives:
- Fully decoding with a hardened image library under explicit memory, pixel, and frame budgets is safer than maintaining partial format parsers, but increases dependency and runtime cost.
- Retaining custom parsing is reasonable only with complete structural bounds for every frame-bearing chunk.
Regression coverage:
- Add GIF and WebP fixtures with oversized frames inside small canvases, out-of-canvas offsets, zero dimensions, maximum-value overflow, and boundary-valid rectangles.
- Assert malformed inputs are rejected before upload or rendering.
Reconciliation notes:
- The proposed Signal query-string credential leak is not supported by the supplied patch. The patch passes the URL to the pinned SDK, but no evidence shows that the SDK requests or logs that original URL; accepting a query is therefore a compatibility concern, not a demonstrated disclosure.
- Exact-reference send authorization is present: the outgoing sticker must already match a stored coordinate, shortcode, and plaintext hash.
- Per-account mutation locking and the installed-list outbox/rebase design address same-process ordering and stale snapshot convergence. No additional concrete fork/convergence defect was established from the supplied patches.
check_evidence.runsis empty, so the bundle provides no successful build, test, clippy, migration, or concurrency-run evidence.
| continue; | ||
| } | ||
| if storage.replace_sticker_pack_if_newer(&stored)? { | ||
| updated += 1; |
There was a problem hiding this comment.
High: This persists every valid discovered pack, while MAX_DISCOVERY_PACKS only limits one synchronization response. Relays can continually supply fresh coordinates or replacement hashes, and app_sticker_assets has no global row/byte quota or pruning path, so the encrypted database can grow without bound across syncs and launches. Keep uninstalled discovery in a bounded cache, or enforce transactional global limits and deterministic GC while protecting installed packs, pending operations, current versions, and assets referenced by retained messages.
| &sticker_ref.plaintext_sha256, | ||
| )?; | ||
| if stored.is_none() && storage.sticker_pack(&coordinate)?.is_none() { | ||
| // A received sticker may reference a valid pack outside the recent |
There was a problem hiding this comment.
No description provided.
| return Err(invalid_sticker("Signal sticker hash mismatch")); | ||
| } | ||
| let url = upload_blossom_blob( | ||
| server, |
There was a problem hiding this comment.
Medium: Uploading inside this loop creates an irreversible network side effect before validation of later stickers and before cover, address, final pack construction, and signing can succeed. A malformed later sticker therefore returns an import error after plaintext assets have already been uploaded, and retries may repeat the uploads. Validate the complete pack into a side-effect-free upload plan first, then upload; also define idempotent reuse, cleanup, or resumable state for failures after uploading begins.
| )); | ||
| } | ||
| b"ANMF" => frames = frames.saturating_add(1), | ||
| _ => {} |
There was a problem hiding this comment.
Medium: ANMF chunks are only counted; their frame offsets and dimensions are never parsed or checked against the declared canvas. The GIF path similarly skips image-descriptor rectangle fields. An animation can therefore advertise a small valid canvas while containing zero-sized, oversized, overflowing, or out-of-canvas frames. Parse every frame rectangle and use checked arithmetic to reject zero dimensions, per-axis/pixel-limit violations, overflow, and offset + size beyond the canvas.
3 participants
Summary
Security and privacy
Verification
Summary by CodeRabbit