From 4cbf029eceff873bd9de25e3698a6752acd99c27 Mon Sep 17 00:00:00 2001 From: Maximiliano Sandoval Date: Tue, 1 Sep 2026 21:25:48 +0200 Subject: [PATCH 1/2] Run cargo +nightly fmt --- cli/src/main.rs | 8 +++---- client/src/file/api/mod.rs | 4 ++-- client/src/secret.rs | 3 ++- client/tests/file_unlocked_keyring.rs | 10 ++++---- server/src/capability.rs | 11 +++++---- server/src/collection/mod.rs | 3 ++- server/src/collection/tests.rs | 3 ++- server/src/gnome/internal.rs | 9 +++++--- server/src/gnome/prompter.rs | 3 ++- server/src/gnome/secret_exchange.rs | 5 ++-- server/src/pam_listener/mod.rs | 3 ++- server/src/plasma/prompter.rs | 3 ++- server/src/prompt/tests.rs | 6 +++-- server/src/service/mod.rs | 33 +++++++++++++++++---------- server/src/service/tests.rs | 8 +++---- server/src/tests.rs | 14 +++++++----- 16 files changed, 77 insertions(+), 49 deletions(-) diff --git a/cli/src/main.rs b/cli/src/main.rs index 1f2d0a907..5e90e41be 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -422,8 +422,8 @@ impl Commands { "Only one of application ID or keyring can be specified at a time.", )); } - // We get the secret first from the app-id, then if the --keyring is set, we try - // to use the --secret variable. + // We get the secret first from the app-id, then if the --keyring is + // set, we try to use the --secret variable. let (secret, path) = if let Some(app_id) = &args.app_id { let default_collection = service.default_collection().await?; let secret = if let Some(item) = default_collection @@ -438,8 +438,8 @@ impl Commands { )); }; - // That is the path used by libsecret/oo7, how does it work with kwallet for - // example? + // That is the path used by libsecret/oo7, how does it work with + // kwallet for example? let path = home().map(|mut path| { path.push(".var/app"); path.push(app_id.to_string()); diff --git a/client/src/file/api/mod.rs b/client/src/file/api/mod.rs index 165aedf39..5a21019b5 100644 --- a/client/src/file/api/mod.rs +++ b/client/src/file/api/mod.rs @@ -322,8 +322,8 @@ impl Keyring { pub fn validate_secret(&self, secret: &Secret) -> Result { let key = self.derive_key(secret)?; - // If there are no items, we can't validate (empty keyrings are valid with any - // password) + // If there are no items, we can't validate (empty keyrings are valid + // with any password) if self.items.is_empty() { return Ok(true); } diff --git a/client/src/secret.rs b/client/src/secret.rs index 783eea431..dda6294fc 100644 --- a/client/src/secret.rs +++ b/client/src/secret.rs @@ -35,7 +35,8 @@ impl FromStr for ContentType { type Err = String; fn from_str(s: &str) -> Result { - // MIME types may include parameters, which are irrelevant to ContentType. + // MIME types may include parameters, which are irrelevant to + // ContentType. let media_type = s .split_once(';') .map_or(s, |(media_type, _)| media_type) diff --git a/client/tests/file_unlocked_keyring.rs b/client/tests/file_unlocked_keyring.rs index b383eb70e..85873663b 100644 --- a/client/tests/file_unlocked_keyring.rs +++ b/client/tests/file_unlocked_keyring.rs @@ -535,8 +535,8 @@ async fn delete_broken_items() -> Result<(), Error> { let keyring_path = v1_dir.join("default.keyring"); fs::copy(&fixture_path, &keyring_path).await?; - // 1) Load with the correct password and add several valid items. This ensures - // valid_items > broken_items that we'll add later. + // 1) Load with the correct password and add several valid items. This + // ensures valid_items > broken_items that we'll add later. let keyring = UnlockedKeyring::load(&keyring_path, Some(Secret::blob("test"))).await?; for i in 0..VALID_TO_ADD { keyring @@ -1201,7 +1201,8 @@ async fn partially_corrupted_keyring_error() -> Result<(), Error> { let key = key.as_ref().as_ref().to_vec(); drop(keyring); - // Load_unchecked with wrong password and add 3 broken items (more than valid) + // Load_unchecked with wrong password and add 3 broken items (more than + // valid) let wrong_secret = Secret::from("wrong-password-long-enough".as_bytes()); let keyring = unsafe { UnlockedKeyring::load_unchecked(&keyring_path, wrong_secret).await? }; keyring @@ -1329,7 +1330,8 @@ async fn set_attributes() -> Result<(), Error> { // Write the updated item back to the keyring at index 0 keyring.replace_item_index(0, &item).await?; - // Now retrieve the item again from the keyring to verify the changes persisted + // Now retrieve the item again from the keyring to verify the changes + // persisted let mut items = keyring.items().await?; assert_eq!(items.len(), 1); let item = items.remove(0); diff --git a/server/src/capability.rs b/server/src/capability.rs index 593840fc9..a5fbea388 100644 --- a/server/src/capability.rs +++ b/server/src/capability.rs @@ -100,7 +100,8 @@ pub fn drop_unnecessary_capabilities() -> Result<(), rustix::io::Errno> { tracing::warn!("Failed to set PR_SET_KEEPCAPS"); } - // Requires CAP_SETPCAP -> must run before reducing the bounding set drops it + // Requires CAP_SETPCAP -> must run before reducing the bounding set + // drops it if let Err(err) = set_bounding_set(CapabilitySet::IPC_LOCK) { tracing::debug!("Could not set bounding set (may not be supported): {}", err); } @@ -126,8 +127,9 @@ pub fn drop_unnecessary_capabilities() -> Result<(), rustix::io::Errno> { )?; } CapabilityState::None => { - // CAP_IPC_LOCK bypasses RLIMIT_MEMLOCK, but is not required when the - // process has a sufficient limit. Always try mlockall below. + // CAP_IPC_LOCK bypasses RLIMIT_MEMLOCK, but is not required when + // the process has a sufficient limit. Always try + // mlockall below. tracing::warn!("No process capabilities; relying on RLIMIT_MEMLOCK to lock memory"); } CapabilityState::Partial => { @@ -139,7 +141,8 @@ pub fn drop_unnecessary_capabilities() -> Result<(), rustix::io::Errno> { ); } - // Clear bounding set if we have CAP_SETPCAP (do this before dropping caps) + // Clear bounding set if we have CAP_SETPCAP (do this before + // dropping caps) if caps.effective.contains(CapabilitySet::SETPCAP) && let Err(err) = set_bounding_set(CapabilitySet::IPC_LOCK) { diff --git a/server/src/collection/mod.rs b/server/src/collection/mod.rs index 72d32ad06..a65564096 100644 --- a/server/src/collection/mod.rs +++ b/server/src/collection/mod.rs @@ -589,7 +589,8 @@ impl Collection { let unlocked = match unlock_result { Ok(unlocked) => unlocked, Err(err) => { - // Reload the locked keyring from disk before returning error + // Reload the locked keyring from disk before + // returning error if let Some(path) = keyring_path && let Ok(reloaded) = oo7::file::LockedKeyring::load(&path).await { diff --git a/server/src/collection/tests.rs b/server/src/collection/tests.rs index a2dba90f5..7524d2f05 100644 --- a/server/src/collection/tests.rs +++ b/server/src/collection/tests.rs @@ -48,7 +48,8 @@ async fn create_item_plain() -> Result<(), Box> { async fn create_item_encrypted() -> Result<(), Box> { let setup = TestServiceSetup::encrypted_session(true).await?; - // Create an encrypted item using the helper (automatically handles encryption) + // Create an encrypted item using the helper (automatically handles + // encryption) let item = setup .create_item( "Test Encrypted Item", diff --git a/server/src/gnome/internal.rs b/server/src/gnome/internal.rs index 62b6bfd0c..5ab57cfa1 100644 --- a/server/src/gnome/internal.rs +++ b/server/src/gnome/internal.rs @@ -368,7 +368,8 @@ mod tests { ) .await?; - // Verify the password was changed by locking and unlocking with new password + // Verify the password was changed by locking and unlocking with new + // password setup .service_api .lock(std::slice::from_ref(&collection_path), None) @@ -428,7 +429,8 @@ mod tests { // Wait for prompt to complete tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; - // Verify the password was changed by locking and unlocking with new password + // Verify the password was changed by locking and unlocking with new + // password setup .service_api .lock(std::slice::from_ref(&collection_path), None) @@ -463,7 +465,8 @@ mod tests { let collection_path: zbus::zvariant::OwnedObjectPath = default_collection.inner().path().to_owned().into(); - // Create an item first so that the unlock validation has something to validate + // Create an item first so that the unlock validation has something to + // validate let dbus_secret = setup.create_dbus_secret("item-secret")?; let mut attributes = std::collections::HashMap::new(); diff --git a/server/src/gnome/prompter.rs b/server/src/gnome/prompter.rs index 018d5d8b1..e8feca4d7 100644 --- a/server/src/gnome/prompter.rs +++ b/server/src/gnome/prompter.rs @@ -412,7 +412,8 @@ impl GNOMEPrompterCallback { Some(raw_secret) }; - // Handle each role differently based on what validation/preparation is needed + // Handle each role differently based on what validation/preparation is + // needed match prompt.role() { PromptRole::Unlock => { if prompt.on_unlock_collection(secret).await? { diff --git a/server/src/gnome/secret_exchange.rs b/server/src/gnome/secret_exchange.rs index 2a545658e..e9cd2d15f 100644 --- a/server/src/gnome/secret_exchange.rs +++ b/server/src/gnome/secret_exchange.rs @@ -45,8 +45,9 @@ pub fn handshake(private_key: &Key, exchange: &str) -> Result Option { let decoded = decode(exchange)?; - // If we cancel an ongoing prompt call, the final exchange won't have the secret - // or IV. The following is to avoid `Option::unwrap()` on a `None` value + // If we cancel an ongoing prompt call, the final exchange won't have the + // secret or IV. The following is to avoid `Option::unwrap()` on a + // `None` value let secret = decoded.get(SECRET)?; // AES ciphertext must be a multiple of 16 bytes (block size) diff --git a/server/src/pam_listener/mod.rs b/server/src/pam_listener/mod.rs index 79f231cf8..a4c3c8564 100644 --- a/server/src/pam_listener/mod.rs +++ b/server/src/pam_listener/mod.rs @@ -398,7 +398,8 @@ impl PamListener { tracing::info!("Successfully changed password for collection: {}", path); changed_count += 1; - // Re-lock the collection if it was locked before we unlocked it + // Re-lock the collection if it was locked before we + // unlocked it drop(keyring_guard); if was_locked { if let Err(e) = collection.set_locked(true, None).await { diff --git a/server/src/plasma/prompter.rs b/server/src/plasma/prompter.rs index 1c8de2fe4..8da89f2af 100644 --- a/server/src/plasma/prompter.rs +++ b/server/src/plasma/prompter.rs @@ -213,7 +213,8 @@ impl PlasmaPrompterCallback { prompt: &Prompt, secret: Option, ) -> Result { - // Handle each role differently based on what validation/preparation is needed + // Handle each role differently based on what validation/preparation is + // needed match prompt.role() { PromptRole::Unlock => { if prompt.on_unlock_collection(secret).await? { diff --git a/server/src/prompt/tests.rs b/server/src/prompt/tests.rs index 2a7948903..f0ff76396 100644 --- a/server/src/prompt/tests.rs +++ b/server/src/prompt/tests.rs @@ -30,7 +30,8 @@ async fn prompt_called_twice_error_impl( // Give the prompt a moment to register the callback tokio::time::sleep(tokio::time::Duration::from_millis(50)).await; - // Second call to prompt() should fail with "callback is ongoing already" error + // Second call to prompt() should fail with "callback is ongoing already" + // error assert!( prompt.prompt(None).await.is_err(), "Second call to prompt() should fail" @@ -66,7 +67,8 @@ async fn prompt_not_found_error() -> Result<(), Box> { assert!(!prompt_path.is_empty(), "Should have a prompt path"); - // Remove the prompt from the service before MockPrompter tries to process it + // Remove the prompt from the service before MockPrompter tries to process + // it setup.server.remove_prompt(&prompt_path).await; // Manually serve a callback to trigger the error path diff --git a/server/src/service/mod.rs b/server/src/service/mod.rs index 9f8c524ee..c2c7b08fb 100644 --- a/server/src/service/mod.rs +++ b/server/src/service/mod.rs @@ -112,8 +112,9 @@ impl Service { } else { #[cfg(any(test, feature = "test-util"))] { - // For p2p test connections, use a dummy sender since p2p connections - // don't have a bus to assign unique names + // For p2p test connections, use a dummy sender since p2p + // connections don't have a bus to assign unique + // names UniqueName::try_from(":p2p.test").unwrap() } #[cfg(not(any(test, feature = "test-util")))] @@ -273,7 +274,8 @@ impl Service { // The prompter will handle secret validation // Here we just perform the unlock operation - // First, check for pending migrations (without holding collections lock) + // First, check for pending migrations (without holding + // collections lock) for object in ¬_unlocked { let collection = { let collections = service.collections.lock().await; @@ -281,7 +283,8 @@ impl Service { }; if let Some(collection) = collection { - // Check if this collection has a pending migration by name + // Check if this collection has a pending migration by + // name let migration_opt = { let pending = service.pending_migrations.lock().await; pending.get(collection.name()).cloned() @@ -294,7 +297,8 @@ impl Service { migration_name ); - // Attempt migration with the provided secret (no locks held) + // Attempt migration with the provided secret (no + // locks held) match migration.migrate(&service.data_dir, secret.as_ref()).await { Ok(unlocked_keyring) => { tracing::info!( @@ -421,7 +425,8 @@ impl Service { match item.get_secret(session.clone()).await { Ok((secret,)) => { secrets.insert(item.path().clone().into(), secret); - // To avoid iterating through all the remaining collections, if the + // To avoid iterating through all the remaining + // collections, if the // items secrets are already retrieved. if secrets.len() == items.len() { break 'outer; @@ -884,8 +889,8 @@ impl Service { } } - // Migration failed or no secret - create locked placeholder and register for - // pending migration + // Migration failed or no secret - create locked placeholder and + // register for pending migration tracing::debug!( "Creating locked placeholder for KWallet keyring '{name}', will migrate on unlock", ); @@ -1223,9 +1228,11 @@ impl Service { break; } else if let Some(item) = collection.item_from_path(&resolved).await { found = true; - // If collection is locked, can't perform any item lock/unlock operations + // If collection is locked, can't perform any item + // lock/unlock operations if collection_locked { - // Unlocking an item when collection is locked requires unlocking collection + // Unlocking an item when collection is locked requires + // unlocking collection if !locked { with_prompt.push(resolved.clone()); } else { @@ -1421,7 +1428,8 @@ impl Service { let signal_emitter = self.signal_emitter(service_path)?; Service::collection_created(&signal_emitter, &collection_path).await?; - // Emit PropertiesChanged for Collections property to invalidate client cache + // Emit PropertiesChanged for Collections property to invalidate client + // cache self.collections_changed(&signal_emitter).await?; tracing::info!( @@ -1538,7 +1546,8 @@ impl Service { let label = migration.label(); let alias = migration.alias(); - // Create a collection for this migrated keyring with unique label and alias + // Create a collection for this migrated keyring with unique + // label and alias let (unique_label, unique_alias) = { let collections = self.collections.lock().await; Self::make_unique_label_and_alias(&collections, label, alias) diff --git a/server/src/service/tests.rs b/server/src/service/tests.rs index dac152b23..393310597 100644 --- a/server/src/service/tests.rs +++ b/server/src/service/tests.rs @@ -720,8 +720,8 @@ async fn lock_item_in_unlocked_collection() -> Result<(), Box Result<(), Box Result<(), Box> { let setup = TestServiceSetup::plain_session(true).await?; - // Try to complete collection creation with a prompt path that has no pending - // collection + // Try to complete collection creation with a prompt path that has no + // pending collection let fake_prompt_path = ObjectPath::try_from("/org/freedesktop/secrets/prompt/p999").unwrap(); let secret = Secret::from("test-password-long-enough"); diff --git a/server/src/tests.rs b/server/src/tests.rs index 0e1a0395b..6ed6a0e8a 100644 --- a/server/src/tests.rs +++ b/server/src/tests.rs @@ -454,8 +454,8 @@ impl MockPrompterService { // Small delay to ensure callback is fully registered tokio::time::sleep(tokio::time::Duration::from_millis(10)).await; - // Call PromptReady directly without building a proxy (avoids introspection - // issues in p2p) + // Call PromptReady directly without building a proxy (avoids + // introspection issues in p2p) tracing::debug!( "MockPrompter: calling PromptReady with None on {}", callback_path @@ -493,8 +493,8 @@ impl MockPrompterService { callback, type_ ); - // This is called by GNOMEPrompterCallback.prompter_init() with the server's - // exchange + // This is called by GNOMEPrompterCallback.prompter_init() with the + // server's exchange let callback_path = callback.to_owned(); let unlock_password = self.unlock_password.clone(); let should_accept = self.should_accept.clone(); @@ -788,7 +788,8 @@ impl MockPrompterServicePlasma { break; } - // Wait for Retry signal before sending next secret from the queue + // Wait for Retry signal before sending next secret from the + // queue signal_stream.next().await; } }); @@ -816,7 +817,8 @@ impl MockPrompterServicePlasma { "MockPrompterServicePlasma: create_collection_prompt called for {}", request ); - // Behavior is identical for both prompts. Visualization would be different. + // Behavior is identical for both prompts. Visualization would be + // different. self.unlock_collection_prompt( request, window_id, From 4504038655256f11b5fb7b45c8b967c639d3c7a0 Mon Sep 17 00:00:00 2001 From: Maximiliano Sandoval Date: Tue, 1 Sep 2026 21:06:47 +0200 Subject: [PATCH 2/2] pam: Use CStr instead of NUL terminated byte slice --- pam/src/lib.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/pam/src/lib.rs b/pam/src/lib.rs index ff150031e..4e693b906 100644 --- a/pam/src/lib.rs +++ b/pam/src/lib.rs @@ -23,7 +23,7 @@ use crate::{ socket::send_secret_to_daemon, }; -const STASHED_PASSWORD_KEY: &[u8] = b"oo7_stashed_password\0"; +const STASHED_PASSWORD_KEY: &CStr = c"oo7_stashed_password"; /// Get the username unsafe fn get_user(pamh: *mut pam_handle_t) -> Result { @@ -164,7 +164,7 @@ pub unsafe extern "C" fn pam_sm_authenticate( let ret = unsafe { ffi::pam_set_data( pamh, - STASHED_PASSWORD_KEY.as_ptr() as *const std::os::raw::c_char, + STASHED_PASSWORD_KEY.as_ptr(), password_ptr, Some(cleanup_password), ) @@ -238,13 +238,7 @@ pub unsafe extern "C" fn pam_sm_open_session( } let mut password_ptr: *const std::os::raw::c_void = std::ptr::null(); - let ret = unsafe { - ffi::pam_get_data( - pamh, - STASHED_PASSWORD_KEY.as_ptr() as *const std::os::raw::c_char, - &mut password_ptr, - ) - }; + let ret = unsafe { ffi::pam_get_data(pamh, STASHED_PASSWORD_KEY.as_ptr(), &mut password_ptr) }; if ret != PAM_SUCCESS || password_ptr.is_null() { tracing::debug!("No stashed password found in session");