fix(media): use ciphertext-compatible Blossom defaults#852
Open
mubarakcoded wants to merge 1 commit into
Open
Conversation
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit 75a90d1 on Jul 13, 2026 3:26pm UTC. |
WalkthroughChangesThe PR replaces the single default encrypted-media Blossom endpoint with an ordered endpoint list, embeds that policy into new groups, documents migration and configuration behavior, and preserves bounded privacy-filtered server rejection reasons for failed uploads. Encrypted media upload behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 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/README.md`:
- Around line 60-63: Document the endpoint contracts separately: in
crates/marmot-app/README.md (lines 60-63), state that ordered endpoint selection
and failover apply to encrypted-media uploads, while encrypted group-image
uploads use only the primary ciphertext-compatible endpoint; in
crates/cli/CHANGELOG.md (lines 20-22), revise the release note to describe the
same distinct behaviors.
In `@crates/marmot-app/src/media/blossom.rs`:
- Around line 110-135: Update privacy_safe_server_reason to reject any URL
scheme pattern, including ws://, wss://, ftp://, file://, and other
scheme-prefixed URLs, rather than only checking HTTP and HTTPS. Preserve the
existing normalization and sensitive-token filters while ensuring relay URLs
cannot pass through into AppError messages.
In `@crates/marmot-app/tests/relay_runtime.rs`:
- Around line 4102-4115: The default endpoint assertion in the relay runtime
test must account for MARMOT_ENCRYPTED_MEDIA_BLOB_ENDPOINTS overrides. Update
the expected endpoints to use the effective configured endpoint list, or isolate
this test from the override while preserving its admin-only/full-replacement
coverage; do not hard-code DEFAULT_BLOSSOM_SERVER_URLS when an override is
active.
🪄 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: 74c66598-e150-4e3d-9666-0f26d09e02a5
📒 Files selected for processing (9)
crates/cli/CHANGELOG.mdcrates/cli/README.mdcrates/marmot-app/README.mdcrates/marmot-app/src/client/mod.rscrates/marmot-app/src/lib.rscrates/marmot-app/src/media/blossom.rscrates/marmot-app/src/media/mod.rscrates/marmot-app/src/media/tests.rscrates/marmot-app/tests/relay_runtime.rs
Comment on lines
+60
to
+63
| Encrypted media and encrypted group images are uploaded as opaque `application/octet-stream` blobs. A compatible | ||
| Blossom server must accept arbitrary binary data rather than only recognizable image, audio, or video payloads. New | ||
| groups use the ordered built-in ciphertext-compatible endpoint list unless the host build supplies | ||
| `MARMOT_ENCRYPTED_MEDIA_BLOB_ENDPOINTS`; upload attempts follow that order. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the two endpoint-selection contracts separately.
The current documentation conflates ordered encrypted-media failover with single-endpoint encrypted group-image upload.
crates/marmot-app/README.md#L60-L63: state that ordered endpoint selection applies to encrypted media, while group-image upload uses the primary ciphertext-compatible endpoint.crates/cli/CHANGELOG.md#L20-L22: revise the release note to match those distinct behaviors.
📍 Affects 2 files
crates/marmot-app/README.md#L60-L63(this comment)crates/cli/CHANGELOG.md#L20-L22
🤖 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-app/README.md` around lines 60 - 63, Document the endpoint
contracts separately: in crates/marmot-app/README.md (lines 60-63), state that
ordered endpoint selection and failover apply to encrypted-media uploads, while
encrypted group-image uploads use only the primary ciphertext-compatible
endpoint; in crates/cli/CHANGELOG.md (lines 20-22), revise the release note to
describe the same distinct behaviors.
Comment on lines
+110
to
+135
| fn privacy_safe_server_reason(reason: &str) -> Option<String> { | ||
| let reason = reason.split_whitespace().collect::<Vec<_>>().join(" "); | ||
| if reason.is_empty() | ||
| || reason.len() > 256 | ||
| || !reason | ||
| .chars() | ||
| .all(|character| character.is_ascii() && !character.is_ascii_control()) | ||
| { | ||
| return None; | ||
| } | ||
| let lowercase = reason.to_ascii_lowercase(); | ||
| if ["http://", "https://", "nostr:", "npub1", "nsec1"] | ||
| .iter() | ||
| .any(|needle| lowercase.contains(needle)) | ||
| { | ||
| return None; | ||
| } | ||
| if reason.split_ascii_whitespace().any(|token| { | ||
| let token = token.trim_matches(|character: char| !character.is_ascii_alphanumeric()); | ||
| (token.len() >= 32 && token.bytes().all(|byte| byte.is_ascii_hexdigit())) | ||
| || (token.len() >= 48 && token.bytes().all(|byte| byte.is_ascii_alphanumeric())) | ||
| }) { | ||
| return None; | ||
| } | ||
| Some(reason) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
privacy_safe_server_reason misses ws:///wss:// (and other) URL schemes.
The scheme-substring filter only checks "http://"/"https://", so a hostile/compromised Blossom server can still smuggle a wss://… (or ftp://, file://, etc.) relay-shaped URL through X-Reason or the body into the resulting AppError — undercutting this helper's own stated goal of keeping "blob hashes, pubkeys, URLs" out of app errors, and the repo guideline to never log relay URLs.
As per coding guidelines, **/*.rs: "Keep tracing and logging privacy-safe: use explicit crate/module target and method fields, aggregate values only, and never log account IDs, group IDs, message IDs, relay URLs, public keys, payloads, ciphertext, plaintext, or key material."
🛡️ Proposed fix: filter any URL scheme, not just http(s)
let lowercase = reason.to_ascii_lowercase();
- if ["http://", "https://", "nostr:", "npub1", "nsec1"]
+ if ["://", "nostr:", "npub1", "nsec1"]
.iter()
.any(|needle| lowercase.contains(needle))
{
return None;
}📝 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
| fn privacy_safe_server_reason(reason: &str) -> Option<String> { | |
| let reason = reason.split_whitespace().collect::<Vec<_>>().join(" "); | |
| if reason.is_empty() | |
| || reason.len() > 256 | |
| || !reason | |
| .chars() | |
| .all(|character| character.is_ascii() && !character.is_ascii_control()) | |
| { | |
| return None; | |
| } | |
| let lowercase = reason.to_ascii_lowercase(); | |
| if ["http://", "https://", "nostr:", "npub1", "nsec1"] | |
| .iter() | |
| .any(|needle| lowercase.contains(needle)) | |
| { | |
| return None; | |
| } | |
| if reason.split_ascii_whitespace().any(|token| { | |
| let token = token.trim_matches(|character: char| !character.is_ascii_alphanumeric()); | |
| (token.len() >= 32 && token.bytes().all(|byte| byte.is_ascii_hexdigit())) | |
| || (token.len() >= 48 && token.bytes().all(|byte| byte.is_ascii_alphanumeric())) | |
| }) { | |
| return None; | |
| } | |
| Some(reason) | |
| } | |
| fn privacy_safe_server_reason(reason: &str) -> Option<String> { | |
| let reason = reason.split_whitespace().collect::<Vec<_>>().join(" "); | |
| if reason.is_empty() | |
| || reason.len() > 256 | |
| || !reason | |
| .chars() | |
| .all(|character| character.is_ascii() && !character.is_ascii_control()) | |
| { | |
| return None; | |
| } | |
| let lowercase = reason.to_ascii_lowercase(); | |
| if ["://", "nostr:", "npub1", "nsec1"] | |
| .iter() | |
| .any(|needle| lowercase.contains(needle)) | |
| { | |
| return None; | |
| } | |
| if reason.split_ascii_whitespace().any(|token| { | |
| let token = token.trim_matches(|character: char| !character.is_ascii_alphanumeric()); | |
| (token.len() >= 32 && token.bytes().all(|byte| byte.is_ascii_hexdigit())) | |
| || (token.len() >= 48 && token.bytes().all(|byte| byte.is_ascii_alphanumeric())) | |
| }) { | |
| return None; | |
| } | |
| Some(reason) | |
| } |
🤖 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-app/src/media/blossom.rs` around lines 110 - 135, Update
privacy_safe_server_reason to reject any URL scheme pattern, including ws://,
wss://, ftp://, file://, and other scheme-prefixed URLs, rather than only
checking HTTP and HTTPS. Preserve the existing normalization and sensitive-token
filters while ensuring relay URLs cannot pass through into AppError messages.
Source: Coding guidelines
Comment on lines
+4102
to
+4115
| let initial_group = app.group("alice", &group_id_hex).unwrap().unwrap(); | ||
| let initial_endpoints = initial_group | ||
| .encrypted_media | ||
| .default_blob_endpoints | ||
| .iter() | ||
| .map(|endpoint| endpoint.base_url.as_str()) | ||
| .collect::<Vec<_>>(); | ||
| assert_eq!( | ||
| initial_endpoints, | ||
| marmot_app::DEFAULT_BLOSSOM_SERVER_URLS | ||
| .iter() | ||
| .map(|endpoint| format!("{endpoint}/")) | ||
| .collect::<Vec<_>>() | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the default-endpoint assertion honor supported overrides.
MARMOT_ENCRYPTED_MEDIA_BLOB_ENDPOINTS can replace the built-in list, but this test always expects DEFAULT_BLOSSOM_SERVER_URLS. A build using that supported override will fail before testing admin-only/full-replacement behavior. Either isolate the test from the override or assert the effective configured endpoint list.
🤖 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-app/tests/relay_runtime.rs` around lines 4102 - 4115, The
default endpoint assertion in the relay runtime test must account for
MARMOT_ENCRYPTED_MEDIA_BLOB_ENDPOINTS overrides. Update the expected endpoints
to use the effective configured endpoint list, or isolate this test from the
override while preserving its admin-only/full-replacement coverage; do not
hard-code DEFAULT_BLOSSOM_SERVER_URLS when an override is active.
dannym-arx
approved these changes
Jul 17, 2026
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
application/octet-streamciphertextRoot cause
MDK encrypts media before upload, so the Blossom request body is intentionally indistinguishable from random bytes and is sent as
application/octet-stream. The previous built-in endpoint,https://blossom.primal.net, rejects that payload with HTTP 415. Android and iOS both delegate endpoint selection to MDK/the group's encrypted-media component, so the shared fix belongs here.Existing groups
The defaults apply when creating new groups. Upgrading MDK does not rewrite signed state for existing groups; an active group admin must call
replace_encrypted_media_blob_endpointsto migrate a group whose embedded endpoints reject encrypted blobs.Verification
cargo test -p marmot-app: 323 unit, 11 audit-log, and 68 relay-runtime tests passedjust fast-ci: formatting, naming gate, workspace checks, feature checks, and Clippy all passedapplication/octet-stream; successful probes were deleted immediatelyCloses #851
Summary by CodeRabbit
Bug Fixes
Documentation