Complete Android parity and security audit (#111)

crates/storage/src/mobile_home.rs

@@ -24,7 +24,7 @@ use crate::{
     mobile_authentication::{
         MobileAuthentication, MobileRepositoryOperation, MobileRepositoryOperationError,
     },
-    mobile_private_storage::{PrivateStorage, PrivateStorageError, exclusive_lock},
+    mobile_private_storage::{PrivateStorage, PrivateStorageError, runtime_lock},
     repository::{Repository, RepositoryError},
     secret_store::{
         NativeSecretBackend, SecretCachePolicy, SecretProtectionPolicy, SecretStore,
@@ -582,8 +582,7 @@ impl MobileHomeStorage {
                 cap_std::ambient_authority(),
             )
             .map_err(|_| MobileHomeError::from_private(PrivateStorageError::Io))?;
-            let lock =
-                exclusive_lock(&root, "runtime.lock").map_err(MobileHomeError::from_private)?;
+            let lock = runtime_lock(&root).map_err(MobileHomeError::from_private)?;
             let config = private
                 .storage
                 .configuration()

crates/storage/src/mobile_home/android_tests.rs

@@ -3,6 +3,7 @@ use crate::{
     authentication::SystemAuthenticationClock,
     git::{GitCredential, GitFetchTransport, GitRemoteCredentialProvider, GitSmartHttpTransport},
     mobile_onboarding::android::tests::{Backend, home_fixture},
+    mobile_private_storage::exclusive_lock,
     repository::SecretBytes,
 };
 use std::{fs, sync::mpsc, thread, time::Duration};
@@ -66,7 +67,7 @@ fn private_home_keeps_transient_private_storage_retry_guidance() {
     )
     .err()
     .unwrap();
-    assert_eq!(error.kind(), MobileHomeErrorKind::Configuration);
+    assert_eq!(error.kind(), MobileHomeErrorKind::Conflict);
     assert!(error.detail().contains("Retry later"));
     assert!(
         !error

crates/storage/src/mobile_onboarding/android.rs

@@ -538,7 +538,7 @@ impl AndroidOnboarding {
     }
     fn lock(&self) -> Result<fs::File, MobileOnboardingError> {
         let dir = Dir::open_ambient_dir(&self.root, ambient_authority()).map_err(io_error)?;
-        crate::mobile_private_storage::exclusive_lock(&dir, "runtime.lock").map_err(private_error)
+        crate::mobile_private_storage::runtime_lock(&dir).map_err(private_error)
     }
     fn begin(
         &self,
@@ -878,7 +878,11 @@ fn io_error(error: io::Error) -> MobileOnboardingError {
 fn private_error(error: PrivateStorageError) -> MobileOnboardingError {
     MobileOnboardingError {
         kind: MobileOnboardingErrorKind::Configuration,
-        title: "Setup Recovery Required",
+        title: if error == PrivateStorageError::Busy {
+            "Private Storage Busy"
+        } else {
+            "Setup Recovery Required"
+        },
         detail: error.detail().into(),
     }
 }
@@ -912,6 +916,16 @@ pub(crate) mod tests {
     };
     use std::collections::BTreeMap;
     use std::sync::atomic::{AtomicBool, Ordering};
+    #[test]
+    fn private_contention_does_not_claim_setup_recovery() {
+        let busy = private_error(PrivateStorageError::Busy);
+        assert_eq!(busy.title(), "Private Storage Busy");
+        assert_eq!(busy.detail(), PrivateStorageError::Busy.detail());
+        assert_eq!(
+            private_error(PrivateStorageError::CorruptLayout).title(),
+            "Setup Recovery Required"
+        );
+    }
     const ALICE: &str = "7E5C5241B25F6FFAAD717EBFA132DCB2DC23AE30";
     const BOB: &str = "B37027B56FC406BD3F6A622B2AC03492B992D06F";
     fn password() -> Option<SecretBytes> {

crates/storage/src/mobile_private_storage.rs

@@ -55,7 +55,7 @@ impl PrivateStorageError {
             Self::AccessDenied => "Private storage access was denied. Existing data was retained.",
             Self::ReadOnly => "Private storage is read-only. Existing data was retained.",
             Self::StorageFull => "Free device storage and retry. Existing data was retained.",
-            Self::Busy => "Another process is initializing private storage. Retry later.",
+            Self::Busy => "Another private storage operation is in progress. Retry later.",
             Self::Io => "Private storage could not be accessed. Existing data was retained.",
             Self::CorruptLayout => {
                 "Storage layout metadata is damaged. Do not reset the repository."
@@ -179,7 +179,7 @@ impl PrivateStorage {
         let os_cache = Dir::open_ambient_dir(&roots.cache, ambient_authority())?;
         let data = private_directory(&no_backup, "ironstorage")?;
         let cache = private_directory(&os_cache, "ironstorage")?;
-        let _initialization = exclusive_lock(&data, "runtime.lock")?;
+        let _initialization = runtime_lock(&data)?;
         let layout = match regular_bytes(&data, MARKER, 16 * 1024)? {
             Some(bytes) => toml::from_str::<Layout>(
                 std::str::from_utf8(&bytes).map_err(|_| PrivateStorageError::CorruptLayout)?,
@@ -321,11 +321,18 @@ impl PrivateStorage {
         clock: Arc<dyn crate::authentication::AuthenticationClock>,
     ) -> Result<Arc<MobileAuthentication>, MobileAuthenticationError> {
         let recovery = |error: PrivateStorageError| {
-            MobileAuthenticationError::new(
-                MobileAuthenticationErrorKind::Configuration,
-                "Private Storage Recovery Required",
-                error.detail(),
-            )
+            let (kind, title) = if error == PrivateStorageError::Busy {
+                (
+                    MobileAuthenticationErrorKind::Conflict,
+                    "Private Storage Busy",
+                )
+            } else {
+                (
+                    MobileAuthenticationErrorKind::Configuration,
+                    "Private Storage Recovery Required",
+                )
+            };
+            MobileAuthenticationError::new(kind, title, error.detail())
         };
         // Retain the native backend's encrypted transactions across the FFI callback adapter.
         let backend: Arc<dyn SecretStoreBackend> = match self
@@ -344,7 +351,11 @@ impl PrivateStorage {
         let config = self
             .configuration()
             .map_err(recovery)?
-            .ok_or_else(|| recovery(PrivateStorageError::ConfigurationMissingWithData))?;
+            .ok_or_else(|| MobileAuthenticationError::new(
+                MobileAuthenticationErrorKind::Configuration,
+                "Set Up Password Store",
+                "Complete local or HTTPS password-store setup before opening protected settings or entries.",
+            ))?;
         if let Some(current) = owner.as_ref() {
             if current.matches_configuration(&config)? {
                 return Ok(Arc::clone(current));
@@ -468,7 +479,7 @@ impl PrivateStorage {
         &self,
         backend: &dyn SecretStoreBackend,
     ) -> Result<(), PrivateStorageError> {
-        let _lock = exclusive_lock(&self.data, "runtime.lock")?;
+        let _lock = runtime_lock(&self.data)?;
         let bytes = regular_bytes(&self.data, MARKER, 16 * 1024)?
             .ok_or(PrivateStorageError::CorruptLayout)?;
         let mut layout: Layout = toml::from_str(
@@ -553,7 +564,7 @@ impl PrivateStorage {
         &self,
         action: impl FnOnce(&Dir) -> Result<T, PrivateStorageError>,
     ) -> Result<T, PrivateStorageError> {
-        let initialization = exclusive_lock(&self.data, "runtime.lock")?;
+        let initialization = runtime_lock(&self.data)?;
         let scratch = open_directory(&self.data, "scratch")?;
         let name = format!("transaction-{:016x}", rand::random::<u64>());
         let dir = private_directory(&scratch, &name)?;
@@ -634,6 +645,19 @@ pub(crate) fn exclusive_lock(parent: &Dir, name: &str) -> Result<fs::File, Priva
     crate::file_lock::try_lock(&file)?;
     Ok(file)
 }
+
+pub(crate) fn runtime_lock(parent: &Dir) -> Result<fs::File, PrivateStorageError> {
+    // Startup's binding/setup/Home reads share this gate; absorb brief overlap, not long operations.
+    let deadline = std::time::Instant::now() + std::time::Duration::from_secs(1);
+    loop {
+        match exclusive_lock(parent, "runtime.lock") {
+            Err(PrivateStorageError::Busy) if std::time::Instant::now() < deadline => {
+                std::thread::sleep(std::time::Duration::from_millis(10));
+            }
+            result => return result,
+        }
+    }
+}
 fn save_layout(parent: &Dir, layout: &Layout) -> Result<(), PrivateStorageError> {
     let bytes = toml::to_string(layout).map_err(|_| PrivateStorageError::CorruptLayout)?;
     let mut temporary = TempFile::new(parent)?;
@@ -1066,6 +1090,94 @@ mod tests {
                 .ok_or(SecretStoreError::Missing)
         }
     }
+    #[test]
+    fn authentication_distinguishes_first_run_from_missing_configuration_with_data() {
+        let temp = tempfile::tempdir().unwrap();
+        let storage = PrivateStorage::open(roots(temp.path())).unwrap();
+        let backend = Arc::new(BindingBackend::default());
+        let clock = Arc::new(crate::authentication::SystemAuthenticationClock::default());
+        let error = storage
+            .authentication(backend.clone(), clock.clone())
+            .err()
+            .unwrap();
+        assert_eq!(error.kind(), MobileAuthenticationErrorKind::Configuration);
+        assert_eq!(error.title(), "Set Up Password Store");
+        assert!(backend.0.lock().unwrap().is_none());
+        assert_eq!(storage.status().recovery, None);
+        configured(&storage);
+        let ciphertext = fs::read(storage.root.join("vault/mail.gpg")).unwrap();
+        fs::remove_file(storage.root.join("config.toml")).unwrap();
+        let error = storage
+            .authentication(backend.clone(), clock)
+            .err()
+            .unwrap();
+        assert_eq!(error.title(), "Private Storage Recovery Required");
+        assert_eq!(
+            error.detail(),
+            PrivateStorageError::ConfigurationMissingWithData.detail()
+        );
+        assert_eq!(
+            storage.status().recovery,
+            Some(PrivateStorageError::ConfigurationMissingWithData)
+        );
+        assert_eq!(
+            fs::read(storage.root.join("vault/mail.gpg")).unwrap(),
+            ciphertext
+        );
+        assert!(backend.0.lock().unwrap().is_none());
+    }
+
+    #[test]
+    fn startup_waits_for_a_short_lived_runtime_owner_without_losing_data() {
+        let temp = tempfile::tempdir().unwrap();
+        let storage = PrivateStorage::open(roots(temp.path())).unwrap();
+        configured(&storage);
+        let ciphertext = fs::read(storage.root.join("vault/mail.gpg")).unwrap();
+        let lock = exclusive_lock(&storage.data, "runtime.lock").unwrap();
+        let release = std::thread::spawn(move || {
+            std::thread::sleep(std::time::Duration::from_millis(50));
+            drop(lock);
+        });
+        let backend = Arc::new(BindingBackend::default());
+        let owner = storage
+            .authentication(
+                backend,
+                Arc::new(crate::authentication::SystemAuthenticationClock::default()),
+            )
+            .unwrap();
+        release.join().unwrap();
+        assert!(!owner.state().unwrap().unlocked());
+        assert_eq!(
+            fs::read(storage.root.join("vault/mail.gpg")).unwrap(),
+            ciphertext
+        );
+        assert_eq!(storage.status().recovery, None);
+    }
+
+    #[test]
+    fn authentication_contention_is_retryable_without_claiming_data_recovery() {
+        let temp = tempfile::tempdir().unwrap();
+        let storage = PrivateStorage::open(roots(temp.path())).unwrap();
+        configured(&storage);
+        let backend = Arc::new(BindingBackend::default());
+        let clock = Arc::new(crate::authentication::SystemAuthenticationClock::default());
+        let lock = exclusive_lock(&storage.data, "runtime.lock").unwrap();
+        let error = storage
+            .authentication(backend.clone(), clock.clone())
+            .err()
+            .unwrap();
+        assert_eq!(error.kind(), MobileAuthenticationErrorKind::Conflict);
+        assert_eq!(error.title(), "Private Storage Busy");
+        assert_eq!(error.detail(), PrivateStorageError::Busy.detail());
+        assert!(backend.0.lock().unwrap().is_none());
+        assert_eq!(storage.status().recovery, None);
+        drop(lock);
+        let owner = storage.authentication(backend, clock).unwrap();
+        assert!(!owner.state().unwrap().unlocked());
+        assert!(storage.status().configured);
+        assert_eq!(storage.status().recovery, None);
+    }
+
     #[test]
     fn restored_or_mismatched_keystore_binding_is_a_typed_non_destructive_recovery() {
         let temp = tempfile::tempdir().unwrap();