Manage encrypted group image using Blossom #337
Conversation
WalkthroughAdds an image_pointer column and threads image metadata through DB/models; implements encrypted group image upload/retrieval using Blossom and local caching; reworks media module exposing only encryption/errors and replaces encryption API; introduces legacy media_old implementation; updates dependencies and expands Whitenoise errors and fields. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Caller
participant W as Whitenoise
participant FS as FileSystem
participant ENC as media::encryption
participant B as Blossom
participant DB as Database
rect #EBF5FF
note right of W: update_group_image flow
User->>W: update_group_image(account, group_id, image_path)
W->>FS: read(image_path)
FS-->>W: bytes
W->>ENC: encrypt_data(bytes, key)
ENC-->>W: ciphertext, nonce[12]
W->>B: upload_blob(ciphertext)
B-->>W: blob descriptor (sha256/url)
W->>DB: store image_hash, image_key, image_nonce
DB-->>W: ok
W-->>User: ok
end
sequenceDiagram
autonumber
actor User as Caller
participant W as Whitenoise
participant DB as Database
participant FS as FileSystem
participant B as Blossom
participant ENC as media::encryption
rect #FFF5EB
note right of W: get_group_image flow
User->>W: get_group_image(account, group_id)
W->>DB: fetch GroupInformation (image_pointer?)
DB-->>W: record
alt pointer exists
W->>FS: read(pointer)
FS-->>W: bytes
W-->>User: bytes
else no pointer
W->>DB: fetch image_hash/key/nonce
DB-->>W: {hash,key,nonce}
W->>B: get_blob(hash)
B-->>W: ciphertext
W->>ENC: decrypt_data(ciphertext, key, nonce)
ENC-->>W: bytes
W->>FS: write(cache_path, bytes)
W->>DB: update image_pointer
W-->>User: bytes
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/media/encryption.rs (3)
21-35: Nonce parameter is ignored; fix API-contract and security semantics.You accept
noncebut generate a new one, which breaks determinism and caller expectations. Also shadows the param with a localnonce.Minimal fix—use provided nonce (must be 12 bytes) and rename the local var:
pub fn encrypt_file( data: &[u8], key: &[u8; 32], - nonce: &[u8], + nonce: &[u8], ) -> Result<(Vec<u8>, Vec<u8>), MediaError> { let cipher = ChaCha20Poly1305::new(Key::from_slice(key)); - let mut nonce_bytes = [0u8; 12]; - rand::rng().fill_bytes(&mut nonce_bytes); - let nonce = Nonce::from_slice(&nonce_bytes); + if nonce.len() != 12 { + return Err(MediaError::Encryption("Nonce must be 12 bytes".to_string())); + } + let mut nonce_bytes = [0u8; 12]; + nonce_bytes.copy_from_slice(nonce); + let chacha_nonce = Nonce::from_slice(&nonce_bytes); - cipher - .encrypt(nonce, data) + cipher + .encrypt(chacha_nonce, data) .map(|encrypted| (encrypted, nonce_bytes.to_vec())) .map_err(|e| MediaError::Encryption(e.to_string())) }Optional (preferred) follow-up: change signature to
nonce: &[u8; 12]and drop runtime check.
47-53: Tighten types and avoid implicit conversions indecrypt_file.Use fixed-size key and nonce types for compile-time safety; avoid
.into()on slices.-#[allow(dead_code)] -pub fn decrypt_file(data: &[u8], key: &[u8], nonce: &[u8]) -> Result<Vec<u8>, MediaError> { - let cipher = ChaCha20Poly1305::new(Key::from_slice(key)); - cipher - .decrypt(nonce.into(), data) +#[allow(dead_code)] +pub fn decrypt_file(data: &[u8], key: &[u8; 32], nonce: &[u8; 12]) -> Result<Vec<u8>, MediaError> { + let cipher = ChaCha20Poly1305::new(Key::from_slice(key)); + let chacha_nonce = Nonce::from_slice(nonce); + cipher + .decrypt(chacha_nonce, data) .map_err(|e| MediaError::Decryption(e.to_string())) }
60-73: Extend test to assert caller-provided nonce is used.This would have caught the bug above.
#[tokio::test] async fn test_encrypt_file() { let keys = Keys::generate(); let data = b"test data"; - let nonce = b"random_bytes"; + let nonce = b"random_bytes"; // 12 bytes - let encrypted = encrypt_file(data, &keys.secret_key().to_secret_bytes(), nonce).unwrap(); + let encrypted = encrypt_file(data, &keys.secret_key().to_secret_bytes(), nonce).unwrap(); + assert_eq!(encrypted.1, nonce, "Encrypt should use the provided nonce");src/media/blossom.rs (2)
147-156: Don’t set Content-Length manually; let reqwest compute it.Manually setting Content-Length is error-prone and may be rejected by some proxies. Reqwest sets it automatically for Vec bodies.
- let response = self - .client - .put(format!("{}/upload", self.url)) - .header("Content-Length", file.len()) - .header("Content-Type", "application/octet-stream") - .header("Authorization", auth_header) - .body(file) + let response = self + .client + .put(format!("{}/upload", self.url)) + .header("Content-Type", "application/octet-stream") + .header("Authorization", auth_header) + .body(file) .send() .await?;
243-251: Don’t set Content-Length manually in upload_media either.- let response = self - .client - .put(format!("{}/media", self.url)) - .header("Content-Length", file.len()) - .header("Content-Type", content_type) - .header("Authorization", auth_header) - .body(file) + let response = self + .client + .put(format!("{}/media", self.url)) + .header("Content-Type", content_type) + .header("Authorization", auth_header) + .body(file) .send() .await?;src/whitenoise/database/group_information.rs (1)
91-99: Selecting the BLOB in all lookups will degrade performance.Both single and batch queries include
group_image; this pulls large payloads unnecessarily.
- Introduce a “without blob” row type and queries excluding
group_imagefor common paths.- Provide a dedicated
get_group_imagefor when the bytes are needed.
🧹 Nitpick comments (17)
src/media_old/errors.rs (1)
28-39: Add transparent wrappers for common error sources.These will simplify error propagation from HTTP and I/O in media/blossom code.
#[error("Database operation failed: {0}")] Database(#[from] sqlx::Error), + #[error(transparent)] + Http(#[from] reqwest::Error), + + #[error(transparent)] + Io(#[from] std::io::Error),src/media/encryption.rs (1)
75-91: Adjust decrypt test after type tightening (if applied).Pass fixed-size key/nonce types directly and keep the round-trip assertion.
- let nonce = b"random_bytes"; - let encrypted = encrypt_file(data, &keys.secret_key().to_secret_bytes(), nonce).unwrap(); + let nonce = b"random_bytes"; + let encrypted = encrypt_file(data, &keys.secret_key().to_secret_bytes(), nonce).unwrap(); let decrypted = decrypt_file( &encrypted.0, - &keys.secret_key().to_secret_bytes(), - &encrypted.1, + &keys.secret_key().to_secret_bytes(), + encrypted.1.as_slice().try_into().expect("nonce len"), ) .unwrap();src/whitenoise/groups.rs (2)
114-114: Typo in debug log.“Succefully” → “Successfully”.
- tracing::debug!("Succefully fetched the key packages of members"); + tracing::debug!("Successfully fetched the key packages of members");
196-203: Confirmgroup_imageparameter order and future wiring.
Noneoccupies the newgroup_imageslot; verify allcreate_for_groupinvocations include this argument in the correct position and plan to integrate the encrypted Blossom image once available.src/media/mod.rs (1)
2-3: Public API surface changed; verify downstream breakage or re-export MediaError.You removed prior media re-exports. If external crates depended on
media::MediaErroror similar, this is a breaking change. Either bump semver appropriately or re-export the error to preserve API.Apply to keep the common error type available:
pub mod blossom; pub mod encryption; pub mod errors; + +// Preserve common public surface +pub use errors::MediaError;src/media/blossom.rs (2)
253-260: Propagate server “X-Reason” on failure for better diagnostics.Current error drops server-provided reason; return it when available.
if !response.status().is_success() { - tracing::error!(target: "whitenoise::nostr_manager::blossom","Media upload failed: {:?}",response); - return Err(format!("Media upload failed with status: {}", response.status()).into()); + let status = response.status(); + let reason = response + .headers() + .get("X-Reason") + .and_then(|v| v.to_str().ok()) + .unwrap_or("Unknown error"); + tracing::error!(target: "whitenoise::nostr_manager::blossom","Media upload failed: {} - {}", status, reason); + return Err(format!("Media upload failed: {} - {}", status, reason).into()); }Apply the same pattern to upload() (Line 157-164) and delete() (Line 200-207).
303-307: Avoid borrowing a temporary String for headers.Pass a String directly; eliminates a needless temporary &str.
- .header("X-Content-Length", content_length.to_string().as_str()) + .header("X-Content-Length", content_length.to_string())Apply similarly in check_media_requirements (Line 371).
src/whitenoise/group_information.rs (2)
66-83: Creation only sets image on first insert; no update path.If a group exists and the image changes later,
find_or_create_*ignores newgroup_image.Add an explicit
update_group_image(mls_group_id, image)method to persist changes post-create. I can draft it if desired.
242-261: Add a test that covers image round-trip.Current tests don’t validate
group_imagepersistence.I can add a test that creates with
Some(vec![1,2,3])and asserts it’s returned unchanged.src/whitenoise/database/group_information.rs (2)
187-211: Validate image size before insert to prevent oversized rows.Enforce a server-side limit (e.g., 1–2 MiB) and reject larger images.
You can add a guard before binding:
- If
group_image.as_ref().map(|b| b.len()) > Some(LIMIT), return aWhitenoiseError::Validation.
213-283: Missing test for group_image persistence.Add a test that inserts with
Some(bytes)and verifies retrieval returns the same bytes.Here’s a minimal test to add to this module:
#[tokio::test] async fn test_group_image_round_trip() { let (whitenoise, _d, _l) = create_mock_whitenoise().await; let group_id = GroupId::from_slice(&[42; 32]); let img = vec![0xde, 0xad, 0xbe, 0xef]; let (created, created_new) = GroupInformation::find_or_create_by_mls_group_id( &group_id, Some(GroupType::Group), Some(img.clone()), &whitenoise.database, ).await.unwrap(); assert!(created_new); let fetched = GroupInformation::find_by_mls_group_id(&group_id, &whitenoise.database) .await.unwrap(); assert_eq!(fetched.group_image, Some(img)); }src/media_old/mod.rs (2)
51-56: Missing import for hashing original fileYou’ll need Sha256 here (see later diffs). Also, the prelude import is fine.
Apply:
use ::image::GenericImageView; use nostr_mls::prelude::*; +use sha2::{Digest, Sha256};
214-223: Prefer typed error overStringfor IMETA generationReturning
Result<Vec<String>, MediaError>removes stringly-typed errors and avoids extrato_string()calls at the callsite.If you agree, I can provide a follow-up patch to change the return type and adjust call sites.
src/media_old/blossom.rs (4)
289-306: Minor: avoidto_string().as_str()on temporaries for headersPassing
&strfrom a temporary works but is awkward. Prefer ownedStringor pre-build aHeaderValue.- .header("X-Content-Length", content_length.to_string().as_str()) + .header("X-Content-Length", content_length.to_string())Apply similarly in
check_media_requirements.
62-78: Reuse a persistent reqwest::Client with timeoutsYou instantiate a new client per call. Prefer a single client in
BlossomClientwith sensible timeouts and connection pooling.#[derive(Clone, Debug)] pub struct BlossomClient { /// Base URL of the Blossom server pub url: String, + client: reqwest::Client, } @@ - pub fn new(url: &str) -> Self { - BlossomClient { - url: url.to_string(), - } - } + pub fn new(url: &str) -> Self { + let client = reqwest::Client::builder() + .tcp_keepalive(std::time::Duration::from_secs(60)) + .pool_idle_timeout(std::time::Duration::from_secs(90)) + .timeout(std::time::Duration::from_secs(30)) + .build() + .expect("failed to build reqwest client"); + BlossomClient { url: url.to_string(), client } + }Then replace local
let client = reqwest::Client::new();withlet client = &self.client;in methods.
123-166: Consider preflight checks before uploadYou already provide
check_upload_requirements. Running it before PUT lets you fail fast on size/type limits and return the server-provided reason.I can wire this in with graceful fallback if the server doesn’t support HEAD.
168-209: Add a DELETE test to cover auth + error mappingThere’s no test for
delete. Add a mock asserting Authorization header is present and verify non-2xx maps to error as expected.I can draft the test case using
mockitosimilar to the upload tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
db_migrations/0012_add_group_image_to_group_information.sql(1 hunks)src/lib.rs(1 hunks)src/media/blossom.rs(7 hunks)src/media/encryption.rs(3 hunks)src/media/mod.rs(1 hunks)src/media_old/blossom.rs(1 hunks)src/media_old/encryption.rs(1 hunks)src/media_old/errors.rs(1 hunks)src/media_old/mod.rs(1 hunks)src/whitenoise/database/group_information.rs(16 hunks)src/whitenoise/group_information.rs(12 hunks)src/whitenoise/groups.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/media_old/errors.rs (1)
src/media_old/cache.rs (2)
sqlx(77-77)sqlx(114-114)
src/media_old/mod.rs (3)
src/media_old/sanitizer.rs (2)
sanitize_media(290-322)create_test_file(341-347)src/media_old/encryption.rs (1)
encrypt_file(21-31)src/media_old/cache.rs (3)
add_to_cache(38-97)fetch_cached_file(109-133)delete_cached_file(145-169)
src/whitenoise/group_information.rs (1)
src/whitenoise/database/group_information.rs (1)
find_or_create_by_mls_group_id(116-136)
src/whitenoise/database/group_information.rs (1)
src/whitenoise/database/accounts.rs (1)
row(45-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (macos-14, native)
- GitHub Check: check (macos-latest, native)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (7)
src/lib.rs (1)
7-7: No ambiguousMediaErrorimports found. Verified that no file imports bothcrate::media::errors::MediaErrorandcrate::media_old::errors::MediaErrorin the same scope.src/media/blossom.rs (1)
179-211: Verify delete endpoint path is correct.Using DELETE {base}/{sha256} assumes Blossom’s delete route matches this shape. If spec requires “/delete/{sha256}” or different, this will 404.
Would you like me to add/delete tests with mockito to lock the expected path?
src/media_old/encryption.rs (1)
21-31: LGTM on encryption flow and error mapping.Nonce generation, AEAD usage, and tests look correct after the key/OsRng tweaks.
src/media_old/mod.rs (3)
134-138: Nice: IMETA tag includes nonce and algoGood inclusion; interoperable with clients expecting decryption metadata.
252-333: Tests for IMETA generation look solidCoverage includes generic files, valid images, and invalid images with error assertion.
226-237: Keepimg.to_rgba8()for blurhash and removeprintln!Blurhash’s
encodeexpects RGBA (4 bytes per pixel), soimg.to_rgba8()is correct. You can still replace or remove theprintln!in favor of structured logging.Likely an incorrect or invalid review comment.
src/media_old/blossom.rs (1)
330-394: Good error mapping usingX-ReasonHEAD preflight error parsing looks solid and ergonomic.
| // Generate IMETA values | ||
| let mut imeta_values = | ||
| generate_imeta_tag_values(&uploaded_file, &blob_descriptor, &media_file.file_hash) | ||
| .map_err(|e| MediaError::Metadata(e.to_string()))?; | ||
|
|
||
| // Add nonce to the IMETA tag | ||
| let nonce_hex = hex::encode(nonce); | ||
| imeta_values.push(format!("decryption-nonce {}", nonce_hex)); | ||
| imeta_values.push("encryption-algorithm chacha20-poly1305".to_string()); |
There was a problem hiding this comment.
Use the precomputed original SHA256 for IMETA; current code ties x to cached (now sanitized) hash
After the change above, media_file.file_hash will reflect the sanitized file. IMETA must retain the original hash.
-// Generate IMETA values
-let mut imeta_values =
- generate_imeta_tag_values(&uploaded_file, &blob_descriptor, &media_file.file_hash)
- .map_err(|e| MediaError::Metadata(e.to_string()))?;
+// Generate IMETA values (use original file SHA256)
+let mut imeta_values =
+ generate_imeta_tag_values(&uploaded_file, &blob_descriptor, &original_sha256)
+ .map_err(MediaError::Metadata)?;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/media_old/mod.rs around lines 123 to 131, IMETA is being generated with
media_file.file_hash which now contains the sanitized/cached hash; replace that
with the precomputed original SHA256 value (the variable that holds the original
file hash in scope — e.g., uploaded_file.original_sha256 or
blob_descriptor.original_hash) when calling generate_imeta_tag_values and when
composing IMETA entries so IMETA retains the original hash; leave the nonce and
algorithm pushes unchanged.
src/media/blossom.rs
Outdated
| pub fn new(url: &str) -> Self { | ||
| BlossomClient { | ||
| url: url.to_string(), | ||
| client: reqwest::Client::new(), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeouts and UA to the persistent reqwest client.
Network calls lack timeouts; requests could hang indefinitely and consume executors.
use base64::{engine::general_purpose::STANDARD, Engine};
use nostr_sdk::prelude::*;
use serde::{Deserialize, Serialize};
use sha2::{Digest, Sha256};
+use std::time::Duration;
...
pub fn new(url: &str) -> Self {
BlossomClient {
url: url.to_string(),
- client: reqwest::Client::new(),
+ client: reqwest::Client::builder()
+ .user_agent("whitenoise-blossom/1.x")
+ .connect_timeout(Duration::from_secs(10))
+ .timeout(Duration::from_secs(30))
+ .build()
+ .expect("failed to build reqwest client"),
}
}📝 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.
| pub fn new(url: &str) -> Self { | |
| BlossomClient { | |
| url: url.to_string(), | |
| client: reqwest::Client::new(), | |
| } | |
| use base64::{engine::general_purpose::STANDARD, Engine}; | |
| use nostr_sdk::prelude::*; | |
| use serde::{Deserialize, Serialize}; | |
| use sha2::{Digest, Sha256}; | |
| use std::time::Duration; | |
| ... | |
| pub fn new(url: &str) -> Self { | |
| BlossomClient { | |
| url: url.to_string(), | |
| client: reqwest::Client::builder() | |
| .user_agent("whitenoise-blossom/1.x") | |
| .connect_timeout(Duration::from_secs(10)) | |
| .timeout(Duration::from_secs(30)) | |
| .build() | |
| .expect("failed to build reqwest client"), | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/media/blossom.rs around lines 75 to 79, the persistent reqwest client is
built without timeouts or a User-Agent which can cause requests to hang and be
hard to trace; update the new() constructor to build reqwest::Client with
sensible connect and request timeouts (e.g., connect_timeout and timeout) and
set a descriptive default User-Agent header (or use ClientBuilder::user_agent)
so every request has time limits and identifiable UA; ensure these values are
configurable via constants or env vars and replace Client::new() with
Client::builder()...build() accordingly.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/whitenoise/accounts.rs (1)
216-223: Docstring is outdated (mentions removedserverparam).The method no longer accepts a
serverargument; update the docs to match the new signature and note it useswhitenoise.blossom./// Uploads an image file to a Blossom server and returns the URL. /// /// # Arguments /// * `file_path` - Path to the image file to upload /// * `image_type` - Image type (JPEG, PNG, etc.) -/// * `server` - Blossom server URL /// * `whitenoise` - Whitenoise instance for accessing account keys
♻️ Duplicate comments (7)
src/media_old/mod.rs (7)
66-71: Docs referencewnarg that isn’t in the signatureRemove
wnfrom arguments in docs; list injected deps explicitly.-/// * `wn` - The Whitenoise state +/// * `db` - Database handle +/// * `data_dir` - Root data directory for cache +/// * `blossom_client` - Blossom client for upload/delete +/// * `exporter_key` - 32-byte key for ChaCha20-Poly1305
102-109: Use injected Blossom client, not globalsReplace
wn.nostr.blossomwith the parameter.- let (blob_descriptor, keys) = wn - .nostr - .blossom - .upload(encrypted_file_data) + let (blob_descriptor, keys) = blossom_client + .upload(encrypted_file_data) .await .map_err(|e| MediaError::Upload(e.to_string()))?;
110-121: Security: caching unsanitized bytes; unwrap on path; globals againCache sanitized bytes; pass
data_diranddbparams; avoid.unwrap().- // Add the file to the local cache + // Add the sanitized file to the local cache let media_file = cache::add_to_cache( - &uploaded_file.data, + &sanitized_file.data, group, &active_account.pubkey.to_string(), Some(blob_descriptor.url.clone()), Some(keys.secret_key().to_secret_hex()), Some(sanitized_file.metadata), - wn.data_dir.to_str().unwrap(), - &wn.database, + data_dir, + db, ) .await?;
123-131: IMETA should use precomputed original SHA256, not cached sanitized hashSwitch to
original_sha256. Also minor error mapping tidy.- let mut imeta_values = - generate_imeta_tag_values(&uploaded_file, &blob_descriptor, &media_file.file_hash) - .map_err(|e| MediaError::Metadata(e.to_string()))?; + let mut imeta_values = + generate_imeta_tag_values(&uploaded_file, &blob_descriptor, &original_sha256) + .map_err(MediaError::Metadata)?;
140-186: Remote delete likely needs Blossom blob SHA256, not local file hashThe server delete usually keys off the server’s blob SHA256; we only have local hash. Persist
blob_descriptor.sha256in cache and use it here.- blossom_client - .delete(file_hash, &nostr_key) + // Use remote SHA256 stored at cache time + let remote_sha256 = cached_media_file + .media_file + .blossom_sha256 + .as_deref() + .ok_or_else(|| MediaError::Delete("Missing Blossom SHA256".into()))?; + blossom_client + .delete(remote_sha256, &nostr_key) .await .map_err(|e| MediaError::Delete(e.to_string()))?;Follow-up:
- Add
blossom_sha256 TEXTtomedia_fileswith a migration.- Populate it in
cache::add_to_cacheright after upload.I can draft the migration and cache changes if desired.
76-94: Doesn’t compile: uses undefinedwn; sync Mutex in async; hidden global depsThe function depends on globals (
wn.*), locks a std::sync::Mutex in async, and can panic on unwrap. Inject deps explicitly and avoid locking here.-pub async fn add_media_file( - group: &group_types::Group, - uploaded_file: FileUpload, -) -> Result<UploadedMedia, MediaError> { +pub async fn add_media_file( + group: &group_types::Group, + uploaded_file: FileUpload, + db: &Database, + data_dir: &str, + blossom_client: &blossom::BlossomClient, + exporter_key: [u8; 32], +) -> Result<UploadedMedia, MediaError> { let active_account = Account::get_active() .await .map_err(|_| MediaError::NoActiveAccount)?; - // Get the raw secret key bytes - let exporter_secret: group_types::GroupExporterSecret; - let nostr_mls_guard = wn.nostr_mls.lock().unwrap(); - if let Some(nostr_mls) = nostr_mls_guard.as_ref() { - exporter_secret = nostr_mls - .exporter_secret(&group.mls_group_id) - .map_err(|e| MediaError::NostrMLS(e.to_string()))?; - } else { - return Err(MediaError::NostrMLSNotInitialized); - } + // exporter_key is provided by caller; avoid locking inside async
95-101: Compute “original” SHA256 once; encrypt sanitized bytes with injected keyThis preserves IMETA’s original hash while never storing unsanitized bytes.
+ // Compute original file SHA256 for IMETA + let original_sha256 = { + use sha2::{Digest, Sha256}; + let mut h = Sha256::new(); + h.update(&uploaded_file.data); + format!("{:x}", h.finalize()) + }; // Sanitize the file let sanitized_file = sanitize_media(&uploaded_file)?; // Encrypt the file - let (encrypted_file_data, nonce) = - encryption::encrypt_file(&sanitized_file.data, &exporter_secret.secret)?; + let (encrypted_file_data, nonce) = + encryption::encrypt_file(&sanitized_file.data, &exporter_key)?;
🧹 Nitpick comments (24)
justfile (1)
28-31: Harden test image dir setup/cleanupPre-create is good. For cleanup, remove all contents (incl. dotfiles) without glob pitfalls.
test: - mkdir -p ./dev/data/images + mkdir -p ./dev/data/images cargo test --all-features --all-targets cargo test --all-features --doc - rm -rf ./dev/data/images/* + find ./dev/data/images -mindepth 1 -deletesrc/media_old/mod.rs (2)
15-17: Docs: “Original files are cached” contradicts Security sectionWe should cache sanitized bytes, not originals. Update the docs to reflect this.
-//! 4. Original files are cached locally +//! 4. Sanitized files are cached locally
224-242: Avoid println! in library; prefer tracing; consider sanitized bytes for metadataUse logging instead of stdout; optional: generate image metadata from sanitized bytes to avoid EXIF surprises.
- if file.mime_type.starts_with("image/") { - println!("Image file detected"); + if file.mime_type.starts_with("image/") { + tracing::debug!("Image file detected");Please confirm the blurhash API variant you intend to use (RGBA vs RGB). If the crate expects RGB, convert with to_rgb8() and adjust the buffer accordingly.
db_migrations/0012_add_group_image_to_group_information.sql (1)
3-4: Cap pointer length to prevent oversized valuesAdd a CHECK to bound
image_pointerlength (e.g., 1024). Keeps rows sane and prevents accidental large blobs/URIs.ALTER TABLE group_information - ADD COLUMN image_pointer TEXT; + ADD COLUMN image_pointer TEXT + CHECK (image_pointer IS NULL OR length(image_pointer) <= 1024);Cargo.toml (2)
24-24: Unify image crate version; avoid dual versionsYou depend on image 0.24 and dev-depend on 0.25 (renamed). Prefer a single version to cut compile time and API skew.
-image = "0.24" +image = { version = "0.25", features = ["png"] } @@ -image_extern = { package = "image", version = "0.25", features = ["png"] } +# Remove if not explicitly needed by testsAlso applies to: 78-78
72-72: Avoid exact pin on base64ct unless requiredExact “=1.7.3” blocks patch updates. Relax unless you need strict pinning.
-base64ct = "=1.7.3" +base64ct = "1.7.3"src/whitenoise/group_information.rs (2)
61-71: Update docs to includeimage_pointerargumentDocument the new parameter in
create_for_group./// # Arguments /// * `mls_group_id` - The MLS group ID /// * `group_type` - Optional explicit group type. If None, will be inferred from participant count +/// * `image_pointer` - Optional pointer to the group image (e.g., local path or blob ref) /// * `participant_count` - Total number of participants including the creator (used for inference if group_type is None) /// * `whitenoise` - Reference to the Whitenoise instance for database operations
75-83: Semantics: pointer only applied on insert?
find_or_create_by_mls_group_idignoresimage_pointerwhen the row exists. Confirm this is intended; otherwise add an explicit “set/update image pointer” API for existing groups.I can add
update_image_pointer(mls_group_id, pointer)with tests if you want.src/whitenoise/accounts.rs (1)
234-243: Prefer propagating Blossom errors directly instead of erasing the type.Avoid wrapping with
Other(anyhow!(...)); keep the original error type for better handling/logging. IfFrom<BlossomError> for WhitenoiseErrorexists (as suggested by the PR), use?. Otherwise, at least map viaWhitenoiseError::from.- let descriptor = whitenoise - .blossom - .upload_blob( - data, - Some(image_type.mime_type().to_string()), - None, - Some(&keys), - ) - .await - .map_err(|err| WhitenoiseError::Other(anyhow::anyhow!(err)))?; + let descriptor = whitenoise + .blossom + .upload_blob( + data, + Some(image_type.mime_type().to_string()), + None, + Some(&keys), + ) + .await?;If
From<BlossomError>is not implemented, use:- .await?; + .await + .map_err(WhitenoiseError::from)?;src/media/encryption.rs (3)
18-19: Update doc comment to reflect new return type.Returns now (Vec, [u8; 12]); docs still say (Vec, Vec).
-/// * `Ok((Vec<u8>, Vec<u8>))` - The encrypted data and nonce +/// * `Ok((Vec<u8>, [u8; 12]))` - The encrypted data and nonce
52-64: Rename tests for consistency with API.Names still say “file”. Optional, but keeps tests aligned with function names.
-async fn test_encrypt_file() { +async fn test_encrypt_data() {
66-81: Add a decryption negative test.Test with wrong key/nonce to ensure MediaError::Decryption is returned (no panics).
If you want, I can add a unit test that mutates one nonce byte and asserts decryption failure.
src/whitenoise/database/group_information.rs (4)
116-136: Creation path threads image_pointer through; one behavioral note.find_or_create ignores provided group_type when the row exists, which is fine, but consider logging if caller passes a different type to avoid confusion.
167-185: Dynamic IN clause is safe; minor readability tweak optional.Placeholders construction is correct. Using itertools::repeat_n or join on vec!["?"; n] can read clearer.
- let placeholders = "?,".repeat(id_bytes.len()); - let placeholders = placeholders.trim_end_matches(','); + let placeholders = std::iter::repeat("?") + .take(id_bytes.len()) + .collect::<Vec<_>>() + .join(", ");
187-193: Comment says private but function is pub(crate).Either make it private or adjust the comment.
- // Private helper method for creating and persisting new records - pub(crate) async fn insert_new( + // Helper for creating and persisting new records + pub(crate) async fn insert_new(
223-305: Tests: add assertions for image_pointer round-trip.Currently always passing None. Add a case with Some("...") and assert it’s stored and returned.
I can add a new test: create with Some("foo"), fetch by ID, assert image_pointer == Some("foo").
src/whitenoise/groups.rs (8)
5-7: AvoidHashtype ambiguity; alias or drop the import.Two different Hash types are in scope. To prevent confusion and accidental shadowing, alias the
nwcone (or drop it if unused).-use nwc::nostr::hashes::Hash; +use nwc::nostr::hashes::Hash as NwcHash;If it’s not required, remove the import instead.
503-507: Generate a true symmetric key, not a Nostr secret key.Using
Keys::generate().secret_key()works but couples semantics to Nostr. Prefer generating 32 random bytes directly for the AEAD key.- let secret_key = Keys::generate().secret_key().to_secret_bytes(); + // Generate a 32-byte symmetric key + let mut secret_key = [0u8; 32]; + rand_core::OsRng.fill_bytes(&mut secret_key);Additional imports (outside the selected range):
use rand_core::{OsRng, RngCore};
513-517: Drop redundant map_err; let?convertBlossomErrorintoWhitenoiseError.
From<BlossomError> for WhitenoiseErrorexists, so?is sufficient.- .await - .map_err(BlossomError::from)?; + .await?;
518-521: Lower log level and avoid leaking local file paths in info logs.Use debug level or redact the absolute path to reduce PII/noise.
- tracing::info!( - "groups::update_group_image: Uploaded blob to blossom for image in {image_path}" - ); + tracing::debug!("groups::update_group_image: blob uploaded for image");
535-538: Cache miss resilience: fall back if cached file is missing/corrupted.If
fs::read(image_pointer)fails (deleted or partial file), consider fetching from Blossom instead of erroring.Example inline fallback:
match fs::read(&image_pointer).await { Ok(bytes) => Ok(bytes), Err(e) => { tracing::warn!("groups::get_group_image: cache read failed: {e}"); // fall through to remote fetch path // (refactor the remote-fetch-and-cache logic into a helper and call it here) } }
551-555: Drop redundant map_err here as well.Same rationale as upload: rely on
?.- .await - .map_err(BlossomError::from)?; + .await?;
678-680: Add assertion forimage_noncetoo.Complete the trio to catch nonce wiring regressions.
assert_eq!(group.image_key, config.image_key); + assert_eq!(group.image_nonce, config.image_nonce);
1120-1176: Strengthentest_group_imagewith cache verification.After the first
get_group_imagecall, assert that the DB pointer is set and the cached file exists.let group_info = GroupInformation::get_by_mls_group_id(&group.mls_group_id, &whitenoise).await.unwrap(); assert!(group_info.image_pointer.is_some()); let cached = group_info.image_pointer.unwrap(); assert!(tokio::fs::try_exists(&cached).await.unwrap(), "cached image file missing");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.toml(2 hunks)db_migrations/0012_add_group_image_to_group_information.sql(1 hunks)justfile(1 hunks)src/lib.rs(1 hunks)src/media/encryption.rs(5 hunks)src/media/mod.rs(1 hunks)src/media_old/encryption.rs(1 hunks)src/media_old/errors.rs(1 hunks)src/media_old/mod.rs(1 hunks)src/nostr_manager/mod.rs(0 hunks)src/whitenoise/accounts.rs(3 hunks)src/whitenoise/database/group_information.rs(16 hunks)src/whitenoise/error.rs(4 hunks)src/whitenoise/group_information.rs(12 hunks)src/whitenoise/groups.rs(7 hunks)src/whitenoise/mod.rs(9 hunks)
💤 Files with no reviewable changes (1)
- src/nostr_manager/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/media_old/encryption.rs
- src/media_old/errors.rs
- src/lib.rs
🧰 Additional context used
📓 Path-based instructions (1)
Cargo.toml
📄 CodeRabbit inference engine (.cursor/rules/mls-protocol-docs.mdc)
Use the rust-nostr (nostr-mls) crate via Cargo.toml for MLS integration
Files:
Cargo.toml
🧠 Learnings (7)
📚 Learning: 2025-08-07T12:20:52.502Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/format-lint-test.mdc:0-0
Timestamp: 2025-08-07T12:20:52.502Z
Learning: Always use the `just precommit` command to run all three (clippy, formatting, and tests).
Applied to files:
justfile
📚 Learning: 2025-08-07T12:20:52.502Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/format-lint-test.mdc:0-0
Timestamp: 2025-08-07T12:20:52.502Z
Learning: Always use the `just check-clippy` and `just check-fmt` commands to check for linter errors and formatting issues. Do not run `cargo clippy` or `cargo fmt` directly.
Applied to files:
justfile
📚 Learning: 2025-08-24T05:58:25.044Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/integration-test.mdc:0-0
Timestamp: 2025-08-24T05:58:25.044Z
Learning: Applies to src/bin/integration_test.rs : Ensure the `integration-tests` Cargo feature is enabled when running the integration test (handled by `just int-test`).
Applied to files:
justfile
📚 Learning: 2025-08-24T05:58:25.044Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/integration-test.mdc:0-0
Timestamp: 2025-08-24T05:58:25.044Z
Learning: Applies to src/bin/integration_test.rs : Run the integration test binary only via the `just int-test` command (do not invoke it directly).
Applied to files:
justfile
📚 Learning: 2025-08-17T19:34:30.333Z
Learnt from: jmcorgan
PR: parres-hq/whitenoise#318
File: src/whitenoise/accounts/messages.rs:193-195
Timestamp: 2025-08-17T19:34:30.333Z
Learning: In the whitenoise codebase, `Account::create_nostr_mls()` returns `Result<NostrMls<NostrMlsSqliteStorage>, AccountError>`, and `WhitenoiseError` has an `Account(#[from] AccountError)` variant that automatically converts. When handling MLS creation errors, use the `?` operator to let `AccountError` convert naturally to `WhitenoiseError::Account` rather than manually mapping to `WhitenoiseError::Configuration`.
Applied to files:
src/whitenoise/error.rssrc/whitenoise/groups.rs
📚 Learning: 2025-08-29T10:59:20.701Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/mls-protocol-docs.mdc:0-0
Timestamp: 2025-08-29T10:59:20.701Z
Learning: Applies to Cargo.toml : Use the rust-nostr (nostr-mls) crate via Cargo.toml for MLS integration
Applied to files:
Cargo.tomlsrc/whitenoise/groups.rs
📚 Learning: 2025-08-24T05:58:25.044Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/integration-test.mdc:0-0
Timestamp: 2025-08-24T05:58:25.044Z
Learning: Applies to src/bin/integration_test.rs : Do not run the integration test with custom log/data directories; use the directories configured by `just int-test`.
Applied to files:
src/whitenoise/mod.rs
🧬 Code graph analysis (5)
src/whitenoise/accounts.rs (1)
src/whitenoise/mod.rs (2)
create_mock_whitenoise(389-449)create_random_png(589-601)
src/whitenoise/groups.rs (7)
src/media/encryption.rs (2)
decrypt_data(40-45)encrypt_data(20-28)src/whitenoise/error.rs (2)
from(128-130)from(135-137)src/whitenoise/accounts.rs (3)
new(50-69)create_nostr_mls(248-255)create_nostr_mls(886-888)src/whitenoise/mod.rs (4)
new(56-70)create_mock_whitenoise(389-449)create_nostr_group_config_data(546-556)create_random_png(589-601)src/whitenoise/secrets_store.rs (1)
new(47-51)src/whitenoise/group_information.rs (1)
get_by_mls_group_id(86-98)src/whitenoise/database/group_information.rs (1)
insert_new(188-214)
src/media_old/mod.rs (3)
src/media_old/sanitizer.rs (1)
sanitize_media(290-322)src/media_old/encryption.rs (1)
encrypt_file(21-31)src/media_old/cache.rs (3)
add_to_cache(38-97)fetch_cached_file(109-133)delete_cached_file(145-169)
src/whitenoise/group_information.rs (1)
src/whitenoise/database/group_information.rs (1)
find_or_create_by_mls_group_id(116-136)
src/whitenoise/mod.rs (1)
src/whitenoise/accounts.rs (2)
data_dir(882-884)new(50-69)
🪛 GitHub Actions: CI
Cargo.toml
[error] 1-1: Command 'cargo check --all-targets --all-features' failed: failed to read '/Users/runner/work/whitenoise/rust-nostr/rfs/nostr-blossom/Cargo.toml' (No such file or directory).
🔇 Additional comments (18)
Cargo.toml (1)
65-65: Tokio version is available and semver-compatible
Tokio 1.44 is published (current max is 1.47.1) and follows SemVer; no change needed.src/whitenoise/group_information.rs (3)
47-47: Switch to pointer: good moveReplacing in-struct BLOB with
image_pointer: Option<String>reduces heavy clones and I/O.
90-96: PassingNonefor pointer on reads is appropriateFetching/creating with default type and no image pointer looks correct.
Also applies to: 127-133
311-317: Tests updated for new signatureAdding the extra argument in tests keeps them compiling and ensures idempotency behavior remains unchanged.
Also applies to: 387-393
src/whitenoise/accounts.rs (1)
896-898: Imports for hashing look good.src/whitenoise/mod.rs (2)
144-149: Creatinggroup_imagesstorage directory early is good.
182-183:blossomfield wiring looks correct.src/media/mod.rs (1)
1-2: No downstream references to removed media APIs Verified thatmedia::MediaError,media::blossom,add_media_file, anddelete_media_filearen’t used outside of the legacysrc/media_olddirectory (which isn’t part of the build).src/whitenoise/error.rs (2)
120-122: Good: unify media errors at top-level via From.This aligns media::errors::MediaError with WhitenoiseError ergonomics and simplifies ? usage.
39-41: GroupImageNotFound variant looks good.Clear, specific, and likely to be surfaced to API callers. Ensure it’s used where appropriate.
Would you like me to scan the repo for places where missing images should map to GroupImageNotFound and open TODOs?
src/whitenoise/database/group_information.rs (3)
31-49: Row mapping includes image_pointer correctly.SELECT/FromRow wiring for image_pointer is consistent.
91-99: SELECT adds image_pointer; good.Query columns match FromRow fields including image_pointer.
188-214: UPSERT now persists image_pointer; good.INSERT … ON CONFLICT updates image_pointer and updated_at; RETURNING matches FromRow.
src/whitenoise/groups.rs (5)
523-527: Image metadata update looks correct.Setting hash, key, and nonce matches the encryption flow.
575-581: Upsert viainsert_newis fine; confirm this is the intended API for updates.
insert_newperforms an upsert. If a dedicated “update pointer” method exists, prefer that for clarity.
665-674: Group creation test changes look good.Asserting
image_hashandimage_keymatches config is appropriate.
991-998: LGTM: optional image fields correctly useOption<Option<_>>in updates.This matches partial-update semantics in
NostrGroupDataUpdate.
206-207: Requested expanded call-site contexts for verification.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Cargo.toml (1)
44-55: Fix the commented local path for nostr-blossom.The path points to rfs/nostr-blossom; prior CI failures showed the crate lives under crates/nostr-blossom.
-# nostr-blossom = { version = "0.43", path="../rust-nostr/rfs/nostr-blossom" } +# nostr-blossom = { version = "0.43", path="../rust-nostr/crates/nostr-blossom" }
🧹 Nitpick comments (2)
Cargo.toml (2)
32-42: Align Nostr crates to crates.io or the official upstream
Cargo.lock confirms nostr-mls, nostr-mls-sqlite-storage, nwc, nostr-blossom and nostr-sdk are all sourced from the personal delcin-raj/nostr fork (rev fe2c68c8038da58642fcade64bc3aa866ae2b7f6). Switch these to the published crates.io v0.43 releases or the official upstream Git repo, or add a comment explaining why the fork is needed and plan a path back upstream.
78-78: Align image crate versionsBump the primary
imagedependency in Cargo.toml from 0.24 to 0.25 (withfeatures = ["png"]) so both prod and dev alias use the same version and avoid duplicate compilation. Optionally remove theimage_externalias and update tests to import fromimagedirectly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
Cargo.toml(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
Cargo.toml
📄 CodeRabbit inference engine (.cursor/rules/mls-protocol-docs.mdc)
Use the rust-nostr (nostr-mls) crate via Cargo.toml for MLS integration
Files:
Cargo.toml
🧠 Learnings (1)
📚 Learning: 2025-08-29T10:59:20.701Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/mls-protocol-docs.mdc:0-0
Timestamp: 2025-08-29T10:59:20.701Z
Learning: Applies to Cargo.toml : Use the rust-nostr (nostr-mls) crate via Cargo.toml for MLS integration
Applied to files:
Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (macos-14, native)
- GitHub Check: check (ubuntu-latest, native)
- GitHub Check: check (macos-latest, native)
🔇 Additional comments (1)
Cargo.toml (1)
32-33: MLS guideline satisfied.Using rust-nostr’s nostr-mls via Cargo.toml is in place.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/whitenoise/error.rs (1)
127-131: Preserve error chaining for boxed dyn Error.Stringifying the error drops its source/backtrace. Wrap it directly into anyhow.
- WhitenoiseError::Other(anyhow::anyhow!(err.to_string())) + WhitenoiseError::Other(anyhow::Error::new(err))
♻️ Duplicate comments (2)
src/whitenoise/error.rs (2)
117-122: Make Blossom variant transparent and add #[from] to keep the error chain and drop the manual impl.Using
#[error(transparent)]forwards Display/source fromBlossomError. Adding#[from]enables?without a separateFromimpl and preserves context.- #[error("Blossom error: {0}")] - Blossom(Box<BlossomError>), + #[error(transparent)] + Blossom(#[from] Box<BlossomError>), #[error("Media error: {0}")] Media(#[from] MediaError),
132-139: Remove manual From; rely on the variant #[from].Redundant once the variant carries
#[from]. It also ensuressource()is wired by thiserror.-// To avoid clippy error WhitenoiseError exceeds 160bytes -impl From<BlossomError> for WhitenoiseError { - fn from(value: BlossomError) -> Self { - WhitenoiseError::Blossom(Box::new(value)) - } -}
🧹 Nitpick comments (2)
src/whitenoise/error.rs (2)
39-41: New not-found variant is fine; consider consolidation later.
GroupImageNotFoundis clear. Optionally, consider a genericNotFound { resource: &'static str }to reduce enum growth if more are added.
140-146: Enrich InvalidSha256 with context (optional).Including the observed length (or reason) helps diagnostics without logs.
#[derive(Error, Debug)] pub enum BlossomError { #[error("Blossom Client error: {0}")] Client(#[from] nostr_blossom::error::Error), - #[error("Invalid sha256 to locate the blob")] - InvalidSha256, + #[error("Invalid sha256 to locate the blob; expected 64 hex chars, got {got}")] + InvalidSha256 { got: usize }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/media/encryption.rs(5 hunks)src/whitenoise/accounts.rs(3 hunks)src/whitenoise/error.rs(4 hunks)src/whitenoise/mod.rs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/whitenoise/mod.rs
- src/media/encryption.rs
- src/whitenoise/accounts.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-17T19:34:30.333Z
Learnt from: jmcorgan
PR: parres-hq/whitenoise#318
File: src/whitenoise/accounts/messages.rs:193-195
Timestamp: 2025-08-17T19:34:30.333Z
Learning: In the whitenoise codebase, `Account::create_nostr_mls()` returns `Result<NostrMls<NostrMlsSqliteStorage>, AccountError>`, and `WhitenoiseError` has an `Account(#[from] AccountError)` variant that automatically converts. When handling MLS creation errors, use the `?` operator to let `AccountError` convert naturally to `WhitenoiseError::Account` rather than manually mapping to `WhitenoiseError::Configuration`.
Applied to files:
src/whitenoise/error.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (macos-14, native)
- GitHub Check: check (macos-latest, native)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (1)
src/whitenoise/error.rs (1)
4-4: MediaError plumbed correctly.Import looks correct and aligns with the new
Media(#[from] MediaError)variant.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/whitenoise/mod.rs (1)
345-351: Compilation error: Url not in scopedefault_blossom() calls Url::parse but url::Url isn’t imported. Minimal in-place fix:
fn default_blossom() -> BlossomClient { if cfg!(debug_assertions) { - BlossomClient::new(Url::parse("http://localhost:3000").unwrap()) + BlossomClient::new(url::Url::parse("http://localhost:3000").unwrap()) } else { - BlossomClient::new(Url::parse("https://blossom.primal.net/").unwrap()) + BlossomClient::new(url::Url::parse("https://blossom.primal.net/").unwrap()) } }Alternatively, add
use url::Url;at the top.
🧹 Nitpick comments (6)
src/whitenoise/mod.rs (3)
107-108: Storing BlossomClient on the struct looks fineAssuming BlossomClient is cheap to clone; otherwise consider Arc if cloning gets expensive.
144-149: Dir creation for group_images is correct; minor message nitThe context message says “data directory” while creating the group_images subdir. Optional: make the error mention group_images for clarity.
- .with_context(|| format!("Failed to create data directory: {:?}", data_dir)) + .with_context(|| format!("Failed to create group_images directory under: {:?}", data_dir))
589-605: Use the constructed path when saving; avoid recomputingYou construct path then recompute it in save(). Use the variable for consistency.
- rand_image - .save(format!("./dev/data/images/{}.png", filename)) - .unwrap(); + rand_image.save(&path).unwrap();src/whitenoise/groups.rs (3)
496-529: Gatekeeping and key generation considerations for update_group_image
- Consider validating that account is a group member (and possibly admin) before uploading to Blossom to fail fast with a clearer error if MLS update later rejects the commit.
- Using Keys::generate() only to get 32 random bytes works, but a direct 32-byte random (e.g., rand::rng().random()) avoids generating an unused keypair.
+ // Optional: fail fast if caller isn't a member/admin (prevents wasted upload) + // let nostr_mls = Account::create_nostr_mls(account.pubkey, &self.config.data_dir)?; + // ensure membership/admin here if needed. - let secret_key = Keys::generate().secret_key().to_secret_bytes(); + // Any 32 random bytes work as a symmetric key: + let secret_key: [u8; 32] = ::rand::rng().random();
531-590: Resilience: fall back to Blossom if cached file is missingIf image_pointer exists but the file was deleted, fs::read() will error. Consider catching NotFound and falling back to fetch/decrypt/cache logic instead of failing.
1124-1181: End-to-end test for encrypted group image is solid
- Validates upload, retrieval, decryption, and cache consistency across members.
- Minor nit: logs use info level; consider debug to reduce noise in CI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.toml(2 hunks)src/media/encryption.rs(5 hunks)src/whitenoise/groups.rs(7 hunks)src/whitenoise/mod.rs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- src/media/encryption.rs
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-02T17:30:34.154Z
Learnt from: delcin-raj
PR: parres-hq/whitenoise#337
File: src/whitenoise/groups.rs:567-573
Timestamp: 2025-09-02T17:30:34.154Z
Learning: The `group_images` directory is created during Whitenoise initialization in `src/whitenoise/mod.rs` at line 144 using `std::fs::create_dir_all(data_dir.join("group_images"))`, so no additional directory creation is needed before writing cached group images in the `get_group_image` method.
Applied to files:
src/whitenoise/mod.rssrc/whitenoise/groups.rs
📚 Learning: 2025-09-02T17:30:34.154Z
Learnt from: delcin-raj
PR: parres-hq/whitenoise#337
File: src/whitenoise/groups.rs:567-573
Timestamp: 2025-09-02T17:30:34.154Z
Learning: The `group_images` directory is created during Whitenoise initialization in `src/whitenoise/mod.rs`, so no additional directory creation is needed before writing cached group images in `get_group_image` method.
Applied to files:
src/whitenoise/mod.rssrc/whitenoise/groups.rs
📚 Learning: 2025-08-24T05:58:25.044Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/integration-test.mdc:0-0
Timestamp: 2025-08-24T05:58:25.044Z
Learning: Applies to src/bin/integration_test.rs : Do not run the integration test with custom log/data directories; use the directories configured by `just int-test`.
Applied to files:
src/whitenoise/mod.rs
📚 Learning: 2025-08-17T19:34:30.333Z
Learnt from: jmcorgan
PR: parres-hq/whitenoise#318
File: src/whitenoise/accounts/messages.rs:193-195
Timestamp: 2025-08-17T19:34:30.333Z
Learning: In the whitenoise codebase, `Account::create_nostr_mls()` returns `Result<NostrMls<NostrMlsSqliteStorage>, AccountError>`, and `WhitenoiseError` has an `Account(#[from] AccountError)` variant that automatically converts. When handling MLS creation errors, use the `?` operator to let `AccountError` convert naturally to `WhitenoiseError::Account` rather than manually mapping to `WhitenoiseError::Configuration`.
Applied to files:
src/whitenoise/groups.rs
📚 Learning: 2025-08-29T10:59:20.701Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/mls-protocol-docs.mdc:0-0
Timestamp: 2025-08-29T10:59:20.701Z
Learning: Applies to Cargo.toml : Use the rust-nostr (nostr-mls) crate via Cargo.toml for MLS integration
Applied to files:
src/whitenoise/groups.rs
🧬 Code graph analysis (2)
src/whitenoise/mod.rs (2)
src/whitenoise/accounts.rs (2)
data_dir(881-883)new(50-69)src/whitenoise/database/mod.rs (1)
new(52-96)
src/whitenoise/groups.rs (6)
src/media/encryption.rs (2)
decrypt_data(40-46)encrypt_data(20-28)src/whitenoise/error.rs (2)
from(128-130)from(135-137)src/whitenoise/mod.rs (4)
new(56-70)create_mock_whitenoise(389-449)create_nostr_group_config_data(546-556)create_random_png(589-605)src/whitenoise/accounts.rs (3)
new(50-69)create_nostr_mls(247-254)create_nostr_mls(885-887)src/whitenoise/group_information.rs (1)
get_by_mls_group_id(86-98)src/whitenoise/database/group_information.rs (1)
insert_new(188-214)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (ubuntu-latest, native)
- GitHub Check: check (macos-14, native)
- GitHub Check: check (macos-latest, native)
🔇 Additional comments (13)
src/whitenoise/mod.rs (6)
6-6: Import OKBrings BlossomClient into scope as needed for the new field/initializer.
182-183: Initializer for blossom fieldGood to initialize once here from a helper.
357-359: Test imports OKimage_extern and rand usage are fine for test helpers.
396-396: Creating group_images in mock setup is appropriateNecessary because the mock builder doesn’t run initialize_whitenoise().
445-446: Mock uses same blossom defaultConsistent with production helper.
550-553: Test config image fieldsMatches the new schema (image_hash/key/nonce).
src/whitenoise/groups.rs (7)
5-9: Imports for hashing and async fs look fineSha256Hash and tokio::fs are appropriate for the Blossom/get_blob and caching path.
11-21: Encryption API usage wired correctlyencrypt_data/decrypt_data imports align with the new media module API.
201-208: Passing image_pointer: None into create_for_groupMatches the new signature; pointer will be set after first fetch/cache.
552-577: Cache path choice is reasonableUsing sha256 hex as filename under data_dir/group_images is deterministic and avoids collisions.
579-586: Upsert via insert_new is acceptableinsert_new with ON CONFLICT updates image_pointer cleanly; no separate update method needed.
669-677: Assertions cover image metadata propagationVerifies name/description and image fields; good coverage of the config→group flow.
Also applies to: 682-684
995-1032: Nested Option usage in NostrGroupDataUpdateMatches the struct’s Option<Option<_>> fields; assertions correspond correctly.
There was a problem hiding this comment.
Unless you think these are going to be reused very soon, I think i would just delete all these old files that you don't need. we can always dig them back up via git.
There was a problem hiding this comment.
Yes, that's the best practice
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn update_group_image( |
There was a problem hiding this comment.
Don't we need to update the group_information pointer here too?
| [dev-dependencies] | ||
| mockito = "1.2" | ||
| tempfile = "3.19.1" | ||
| image_extern = { package = "image", version = "0.25", features = ["png"] } |
There was a problem hiding this comment.
We'll need to support jpg, jpeg, png, webp, and gif at a minimum.
There was a problem hiding this comment.
This is used only for testing.
I used it to randomly generate PNG files that are encrypted and uploaded to blossom.
There was a problem hiding this comment.
This is used only for testing.
I used it to randomly generate PNG files that are encrypted and uploaded to blossom.
| -- Migration: Store local file pointer for the group image (no inline BLOB) | ||
|
|
||
| ALTER TABLE group_information | ||
| ADD COLUMN image_pointer TEXT; |
There was a problem hiding this comment.
I think the name image_pointer is a bit confusing, even though conceptually it can be thought as a pointer I would use something like image_path just to try to make it clearer that we're talking about a path in the filesystem rather than a memory-address of sorts
There was a problem hiding this comment.
I'm not completely sure if it's the best idea but maybe we could use the already existing media_files table to store the information related to the actual image (which already has the file_path) and here just add a foreign key reference to it?
| // Fetch and encrypt image bytes | ||
| let image_bytes = fs::read(image_path).await?; | ||
| let secret_key = Keys::generate().secret_key().to_secret_bytes(); | ||
|
|
||
| let (encrypted_bytes, nonce_bytes) = encrypt_data(&image_bytes, &secret_key)?; | ||
|
|
||
| // upload to blossom | ||
| let account_keys = self | ||
| .secrets_store | ||
| .get_nostr_keys_for_pubkey(&account.pubkey)?; | ||
| let blob_descriptor = self | ||
| .blossom | ||
| .upload_blob(encrypted_bytes, None, None, Some(&account_keys)) | ||
| .await | ||
| .map_err(BlossomError::from)?; | ||
|
|
||
| tracing::info!( | ||
| "groups::update_group_image: Uploaded blob to blossom for image in {image_path}" | ||
| ); |
There was a problem hiding this comment.
This could probably be extracted to an upload_image or upload_media method, what do you think?
| let nostr_mls = Account::create_nostr_mls(account.pubkey, &self.config.data_dir)?; | ||
| let group_info = GroupInformation::get_by_mls_group_id(group_id, self).await?; | ||
| match group_info.image_pointer { | ||
| Some(image_pointer) => { | ||
| let image_bytes = fs::read(image_pointer).await?; | ||
| Ok(image_bytes) | ||
| } |
There was a problem hiding this comment.
When we're on the first branch (Some(image_pointer)) we don't really need to have instanced the nostr_mls object. We should move that to the None branch
| let image_hash_array: [u8; 32] = image_hash | ||
| .try_into() | ||
| .map_err(|_| BlossomError::InvalidSha256)?; | ||
| let sha256 = Sha256Hash::from_byte_array(image_hash_array); | ||
|
|
||
| // Fetch and decrypt the encrypted bytes from blossom | ||
| let encrypted_bytes = self | ||
| .blossom | ||
| .get_blob(sha256, None, None, Option::<&Keys>::None) | ||
| .await | ||
| .map_err(BlossomError::from)?; | ||
|
|
||
| tracing::info!( | ||
| "groups::get_group_image: Fetched blob from blossom for hash {image_hash_array:?}" | ||
| ); | ||
|
|
||
| let image_key = group.image_key.ok_or(WhitenoiseError::GroupImageNotFound)?; | ||
| let image_nonce = group | ||
| .image_nonce | ||
| .ok_or(WhitenoiseError::GroupImageNotFound)?; | ||
|
|
||
| let decrypted_bytes = decrypt_data(&encrypted_bytes, &image_key, &image_nonce)?; | ||
|
|
||
| // Save the decrypted files locally | ||
| let image_pointer = self | ||
| .config | ||
| .data_dir | ||
| .join("group_images") | ||
| .join(sha256.to_string()); | ||
| fs::write(image_pointer.clone(), &decrypted_bytes).await?; |
There was a problem hiding this comment.
This could be extracted to a 'download_file' or 'download_image' method or something like that
Summary by CodeRabbit
New Features
Error Handling
Database
Chores