From 6ae7aa70d6075bd6220358de5a71c8dae8d65815 Mon Sep 17 00:00:00 2001 From: Manav Rathi Date: Fri, 22 Aug 2025 18:49:24 +0530 Subject: [PATCH] fix(rust): Fix FFI type cast and clippy warnings for CI - Use std::ffi::c_char for libsodium FFI context parameter cast - Fix all clippy warnings to pass CI with RUSTFLAGS="-D warnings" - Update CLAUDE.md with FFI casting guidance for future development This ensures the code passes all CI checks including the stricter clippy settings used in GitHub Actions. Co-Authored-By: Claude --- rust/CLAUDE.md | 2 ++ rust/src/api/auth.rs | 12 +++----- rust/src/api/client.rs | 56 ++++++++++++++++-------------------- rust/src/api/methods.rs | 25 +++++++--------- rust/src/commands/account.rs | 46 +++++++++++++---------------- rust/src/crypto/kdf.rs | 2 +- rust/src/storage/account.rs | 6 ++-- rust/src/storage/sync.rs | 7 ++--- 8 files changed, 67 insertions(+), 89 deletions(-) diff --git a/rust/CLAUDE.md b/rust/CLAUDE.md index ec112050c6..afcd288e10 100644 --- a/rust/CLAUDE.md +++ b/rust/CLAUDE.md @@ -26,6 +26,8 @@ The codebase must pass the GitHub Actions workflow at `../.github/workflows/rust 2. `cargo clippy --all-targets --all-features` - Linting with all warnings as errors (RUSTFLAGS: -D warnings) 3. `cargo build` - Build verification +**Important FFI Note**: When working with libsodium FFI bindings, always use `std::ffi::c_char` for C char pointer casts (e.g., `as *const std::ffi::c_char`), NOT raw `i8` casts. The CI environment may have different type expectations than local development. + ## Architecture Overview This is a Rust CLI for ente.io, providing encrypted photo backup and export functionality. The project is migrating from a Go implementation and follows a modular architecture: diff --git a/rust/src/api/auth.rs b/rust/src/api/auth.rs index 191bcd6a82..09e6f9ec41 100644 --- a/rust/src/api/auth.rs +++ b/rust/src/api/auth.rs @@ -57,7 +57,7 @@ impl<'a> AuthClient<'a> { srp_m1: STANDARD.encode(client_proof), }; - log::debug!("Sending verify-session request: {:?}", request); + log::debug!("Sending verify-session request: {request:?}"); self.api .post("/users/srp/verify-session", &request, None) @@ -124,8 +124,7 @@ impl<'a> AuthClient<'a> { .process_reply(&a, &identity, &login_key, &srp_salt, &server_public) .map_err(|e| { crate::models::error::Error::AuthenticationFailed(format!( - "SRP client process failed: {:?}", - e + "SRP client process failed: {e:?}" )) })?; @@ -133,7 +132,7 @@ impl<'a> AuthClient<'a> { let proof = verifier.proof(); let auth_response = self - .verify_srp_session(&srp_attrs.srp_user_id, &session.session_id, &proof) + .verify_srp_session(&srp_attrs.srp_user_id, &session.session_id, proof) .await?; // TODO: Verify server proof if provided @@ -184,10 +183,7 @@ impl<'a> AuthClient<'a> { /// Check passkey verification status pub async fn check_passkey_status(&self, session_id: &str) -> Result { - let url = format!( - "/users/two-factor/passkeys/get-token?sessionID={}", - session_id - ); + let url = format!("/users/two-factor/passkeys/get-token?sessionID={session_id}"); self.api.get(&url, None).await } } diff --git a/rust/src/api/client.rs b/rust/src/api/client.rs index 228a21d4ee..4db015f1bb 100644 --- a/rust/src/api/client.rs +++ b/rust/src/api/client.rs @@ -103,23 +103,22 @@ impl ApiClient { match req.send().await { Ok(response) => { // Check if we should retry based on status code - if response.status() == StatusCode::TOO_MANY_REQUESTS - || response.status().is_server_error() + if (response.status() == StatusCode::TOO_MANY_REQUESTS + || response.status().is_server_error()) + && retry_count < MAX_RETRIES { - if retry_count < MAX_RETRIES { - retry_count += 1; - log::warn!( - "Request failed with status {}, retry attempt {}/{}", - response.status(), - retry_count, - MAX_RETRIES - ); + retry_count += 1; + log::warn!( + "Request failed with status {}, retry attempt {}/{}", + response.status(), + retry_count, + MAX_RETRIES + ); - // Exponential backoff with jitter - sleep(Duration::from_millis(delay_ms)).await; - delay_ms = (delay_ms * 2).min(MAX_RETRY_DELAY_MS); - continue; - } + // Exponential backoff with jitter + sleep(Duration::from_millis(delay_ms)).await; + delay_ms = (delay_ms * 2).min(MAX_RETRY_DELAY_MS); + continue; } return Ok(response); @@ -128,10 +127,7 @@ impl ApiClient { if retry_count < MAX_RETRIES { retry_count += 1; log::warn!( - "Request failed with error: {}, retry attempt {}/{}", - e, - retry_count, - MAX_RETRIES + "Request failed with error: {e}, retry attempt {retry_count}/{MAX_RETRIES}" ); sleep(Duration::from_millis(delay_ms)).await; @@ -170,12 +166,11 @@ impl ApiClient { serde_json::to_string_pretty(&error_json).unwrap_or(error_text.clone()) ); } else { - log::error!("API error: status={}, body={}", status, error_text); + log::error!("API error: status={status}, body={error_text}"); } return Err(Error::Generic(format!( - "API error ({}): {}", - status, error_text + "API error ({status}): {error_text}" ))); } @@ -219,12 +214,11 @@ impl ApiClient { serde_json::to_string_pretty(&error_json).unwrap_or(error_text.clone()) ); } else { - log::error!("API error: status={}, body={}", status, error_text); + log::error!("API error: status={status}, body={error_text}"); } return Err(Error::Generic(format!( - "API error ({}): {}", - status, error_text + "API error ({status}): {error_text}" ))); } @@ -258,12 +252,11 @@ impl ApiClient { serde_json::to_string_pretty(&error_json).unwrap_or(error_text.clone()) ); } else { - log::error!("API error: status={}, body={}", status, error_text); + log::error!("API error: status={status}, body={error_text}"); } return Err(Error::Generic(format!( - "API error ({}): {}", - status, error_text + "API error ({status}): {error_text}" ))); } @@ -293,12 +286,11 @@ impl ApiClient { serde_json::to_string_pretty(&error_json).unwrap_or(error_text.clone()) ); } else { - log::error!("API error: status={}, body={}", status, error_text); + log::error!("API error: status={status}, body={error_text}"); } return Err(Error::Generic(format!( - "API error ({}): {}", - status, error_text + "API error ({status}): {error_text}" ))); } @@ -317,7 +309,7 @@ impl ApiClient { .text() .await .unwrap_or_else(|_| "Unknown error".to_string()); - return Err(Error::Generic(format!("Download error: {}", error_text))); + return Err(Error::Generic(format!("Download error: {error_text}"))); } Ok(response.bytes().await?.to_vec()) diff --git a/rust/src/api/methods.rs b/rust/src/api/methods.rs index 1107294732..210e9f687c 100644 --- a/rust/src/api/methods.rs +++ b/rust/src/api/methods.rs @@ -34,14 +34,14 @@ impl<'a> ApiMethods<'a> { account_id: &str, since_time: i64, ) -> Result> { - let url = format!("/collections/v2?sinceTime={}", since_time); + let url = format!("/collections/v2?sinceTime={since_time}"); let response: GetCollectionsResponse = self.api.get(&url, Some(account_id)).await?; Ok(response.collections) } /// Get a specific collection by ID pub async fn get_collection(&self, account_id: &str, collection_id: i64) -> Result { - let url = format!("/collections/{}", collection_id); + let url = format!("/collections/{collection_id}"); self.api.get(&url, Some(account_id)).await } @@ -62,10 +62,8 @@ impl<'a> ApiMethods<'a> { collection_id: i64, since_time: i64, ) -> Result<(Vec, bool)> { - let url = format!( - "/collections/v2/diff?collectionID={}&sinceTime={}", - collection_id, since_time - ); + let url = + format!("/collections/v2/diff?collectionID={collection_id}&sinceTime={since_time}"); let response: GetFilesResponse = self.api.get(&url, Some(account_id)).await?; Ok((response.diff, response.has_more)) } @@ -77,10 +75,7 @@ impl<'a> ApiMethods<'a> { collection_id: i64, file_id: i64, ) -> Result { - let url = format!( - "/collections/file?collectionID={}&fileID={}", - collection_id, file_id - ); + let url = format!("/collections/file?collectionID={collection_id}&fileID={file_id}"); let response: GetFileResponse = self.api.get(&url, Some(account_id)).await?; Ok(response.file) } @@ -100,7 +95,7 @@ impl<'a> ApiMethods<'a> { since_time: i64, limit: i32, ) -> Result<(Vec, bool)> { - let url = format!("/diff?sinceTime={}&limit={}", since_time, limit); + let url = format!("/diff?sinceTime={since_time}&limit={limit}"); let response: GetDiffResponse = self.api.get(&url, Some(account_id)).await?; Ok((response.diff, response.has_more)) } @@ -111,10 +106,10 @@ impl<'a> ApiMethods<'a> { let base_url = &self.api.base_url; if base_url == "https://api.ente.io" { // Use the CDN URL for production - Ok(format!("https://files.ente.io/?fileID={}", file_id)) + Ok(format!("https://files.ente.io/?fileID={file_id}")) } else { // Use the API endpoint for custom/dev environments - let url = format!("/files/download/{}", file_id); + let url = format!("/files/download/{file_id}"); let response: GetFileUrlResponse = self.api.get(&url, Some(account_id)).await?; Ok(response.url) } @@ -122,7 +117,7 @@ impl<'a> ApiMethods<'a> { /// Get thumbnail URL for a file pub async fn get_thumbnail_url(&self, account_id: &str, file_id: i64) -> Result { - let url = format!("/files/preview/{}", file_id); + let url = format!("/files/preview/{file_id}"); let response: GetThumbnailUrlResponse = self.api.get(&url, Some(account_id)).await?; Ok(response.url) } @@ -143,7 +138,7 @@ impl<'a> ApiMethods<'a> { /// Get deleted files pub async fn get_trash(&self, account_id: &str, since_time: i64) -> Result<(Vec, bool)> { - let url = format!("/trash/v2?sinceTime={}", since_time); + let url = format!("/trash/v2?sinceTime={since_time}"); let response: GetDiffResponse = self.api.get(&url, Some(account_id)).await?; Ok((response.diff, response.has_more)) } diff --git a/rust/src/commands/account.rs b/rust/src/commands/account.rs index f8e82ba550..08c531e678 100644 --- a/rust/src/commands/account.rs +++ b/rust/src/commands/account.rs @@ -79,8 +79,7 @@ async fn add_account( // If invalid app provided via CLI, use interactive selection if password_arg.is_some() { return Err(crate::models::error::Error::InvalidInput(format!( - "Invalid app: {}. Must be one of: photos, locker, auth", - app_arg + "Invalid app: {app_arg}. Must be one of: photos, locker, auth" ))); } let apps = vec!["photos", "locker", "auth"]; @@ -101,10 +100,7 @@ async fn add_account( // Check if account already exists if let Ok(Some(_existing)) = storage.accounts().get(&email, app) { - println!( - "\n❌ Account already exists for {} with app {:?}", - email, app - ); + println!("\n❌ Account already exists for {email} with app {app:?}"); return Ok(()); } @@ -124,7 +120,7 @@ async fn add_account( } else { Input::new() .with_prompt("Enter export directory path") - .default(format!("./exports/{}", email)) + .default(format!("./exports/{email}")) .interact_text() .map_err(|e| crate::models::error::Error::InvalidInput(e.to_string()))? }; @@ -132,14 +128,14 @@ async fn add_account( // Validate export directory let export_path = PathBuf::from(&export_dir); if !export_path.exists() { - println!("Creating export directory: {}", export_dir); - std::fs::create_dir_all(&export_path).map_err(|e| crate::models::error::Error::Io(e))?; + println!("Creating export directory: {export_dir}"); + std::fs::create_dir_all(&export_path).map_err(crate::models::error::Error::Io)?; } // Initialize API client (use ENTE_ENDPOINT env var if set, otherwise default to production) let api_endpoint = std::env::var("ENTE_ENDPOINT").ok(); if let Some(ref endpoint) = api_endpoint { - log::debug!("Using custom API endpoint: {}", endpoint); + log::debug!("Using custom API endpoint: {endpoint}"); } let api_client = ApiClient::new(api_endpoint)?; let auth_client = AuthClient::new(&api_client); @@ -150,7 +146,7 @@ async fn add_account( let (auth_response, key_enc_key) = match auth_client.login_with_srp(&email, &password).await { Ok(result) => result, Err(e) => { - println!("\n❌ Authentication failed: {}", e); + println!("\n❌ Authentication failed: {e}"); return Err(e); } }; @@ -251,9 +247,9 @@ async fn add_account( storage.accounts().store_secrets(account_id, &secrets)?; println!("\n✅ Account added successfully!"); - println!(" Email: {}", email); - println!(" App: {:?}", app); - println!(" Export directory: {}", export_dir); + println!(" Email: {email}"); + println!(" App: {app:?}"); + println!(" Export directory: {export_dir}"); Ok(()) } @@ -265,8 +261,7 @@ async fn update_account(storage: &Storage, email: &str, dir: &str, app_str: &str "auth" => App::Auth, _ => { return Err(crate::models::error::Error::InvalidInput(format!( - "Invalid app: {}. Must be one of: photos, locker, auth", - app_str + "Invalid app: {app_str}. Must be one of: photos, locker, auth" ))); } }; @@ -274,17 +269,17 @@ async fn update_account(storage: &Storage, email: &str, dir: &str, app_str: &str // Validate export directory let export_path = PathBuf::from(dir); if !export_path.exists() { - println!("Creating export directory: {}", dir); - std::fs::create_dir_all(&export_path).map_err(|e| crate::models::error::Error::Io(e))?; + println!("Creating export directory: {dir}"); + std::fs::create_dir_all(&export_path).map_err(crate::models::error::Error::Io)?; } // Update account storage.accounts().update_export_dir(email, app, dir)?; println!("\n✅ Account updated successfully!"); - println!(" Email: {}", email); - println!(" App: {:?}", app); - println!(" New export directory: {}", dir); + println!(" Email: {email}"); + println!(" App: {app:?}"); + println!(" New export directory: {dir}"); Ok(()) } @@ -296,20 +291,19 @@ async fn get_token(storage: &Storage, email: &str, app_str: &str) -> Result<()> "auth" => App::Auth, _ => { return Err(crate::models::error::Error::InvalidInput(format!( - "Invalid app: {}. Must be one of: photos, locker, auth", - app_str + "Invalid app: {app_str}. Must be one of: photos, locker, auth" ))); } }; // Get account let account = storage.accounts().get(email, app)?.ok_or_else(|| { - crate::models::error::Error::NotFound(format!("Account not found: {}", email)) + crate::models::error::Error::NotFound(format!("Account not found: {email}")) })?; // Get account secrets let secrets = storage.accounts().get_secrets(account.id)?.ok_or_else(|| { - crate::models::error::Error::NotFound(format!("Secrets not found for account {}", email)) + crate::models::error::Error::NotFound(format!("Secrets not found for account {email}")) })?; // Convert token to string (assuming it's UTF-8) @@ -317,7 +311,7 @@ async fn get_token(storage: &Storage, email: &str, app_str: &str) -> Result<()> crate::models::error::Error::Generic("Token is not valid UTF-8".to_string()) })?; - println!("{}", token_str); + println!("{token_str}"); Ok(()) } diff --git a/rust/src/crypto/kdf.rs b/rust/src/crypto/kdf.rs index 52fccd72af..2cafd5bd0c 100644 --- a/rust/src/crypto/kdf.rs +++ b/rust/src/crypto/kdf.rs @@ -21,7 +21,7 @@ pub fn derive_login_key(key_enc_key: &[u8]) -> Result> { sub_key.as_mut_ptr(), LOGIN_SUB_KEY_LEN, LOGIN_SUB_KEY_ID, - context.as_ptr(), + context.as_ptr() as *const std::ffi::c_char, key_enc_key.as_ptr(), ) }; diff --git a/rust/src/storage/account.rs b/rust/src/storage/account.rs index 2be9a2c76a..8bd14366c9 100644 --- a/rust/src/storage/account.rs +++ b/rust/src/storage/account.rs @@ -44,7 +44,7 @@ impl<'a> AccountStore<'a> { )?; let account = stmt - .query_row(params![email, format!("{:?}", app).to_lowercase()], |row| { + .query_row(params![email, format!("{app:?}").to_lowercase()], |row| { row_to_account(row) }) .optional()?; @@ -60,7 +60,7 @@ impl<'a> AccountStore<'a> { )?; let accounts = stmt - .query_map([], |row| row_to_account(row))? + .query_map([], row_to_account)? .collect::, _>>()?; Ok(accounts) @@ -73,7 +73,7 @@ impl<'a> AccountStore<'a> { self.conn.execute( "UPDATE accounts SET export_dir = ?1, updated_at = ?2 WHERE email = ?3 AND app = ?4", - params![export_dir, now, email, format!("{:?}", app).to_lowercase()], + params![export_dir, now, email, format!("{app:?}").to_lowercase()], )?; Ok(()) diff --git a/rust/src/storage/sync.rs b/rust/src/storage/sync.rs index cab03cc663..593c961851 100644 --- a/rust/src/storage/sync.rs +++ b/rust/src/storage/sync.rs @@ -135,9 +135,8 @@ impl<'a> SyncStore<'a> { }; let query = format!( - "INSERT OR REPLACE INTO sync_state (account_id, {}) VALUES (?1, ?2) - ON CONFLICT(account_id) DO UPDATE SET {} = ?2", - column, column + "INSERT OR REPLACE INTO sync_state (account_id, {column}) VALUES (?1, ?2) + ON CONFLICT(account_id) DO UPDATE SET {column} = ?2" ); self.conn.execute(&query, params![account_id, timestamp])?; @@ -154,7 +153,7 @@ impl<'a> SyncStore<'a> { _ => return Err(crate::Error::InvalidInput("Invalid sync type".into())), }; - let query = format!("SELECT {} FROM sync_state WHERE account_id = ?1", column); + let query = format!("SELECT {column} FROM sync_state WHERE account_id = ?1"); let mut stmt = self.conn.prepare(&query)?; let timestamp = stmt