fix(security): self-repair locked .secret_key on Windows (OPENHUMAN-TAURI-GN) (#2061)

.github/workflows/test-reusable.yml

@@ -99,6 +99,36 @@ jobs:
       - name: Test core crate (openhuman)
         run: cargo test -p openhuman
 
+  rust-core-tests-windows:
+    if: inputs.run_rust_core
+    name: Rust Core Tests (Windows — secrets ACL)
+    runs-on: windows-latest
+    timeout-minutes: 20
+    env:
+      CARGO_INCREMENTAL: '0'
+      SCCACHE_GHA_ENABLED: 'true'
+      RUSTC_WRAPPER: sccache
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v5
+        with:
+          ref: ${{ inputs.ref }}
+          fetch-depth: 1
+          submodules: recursive
+      - name: Cache Rust build artifacts
+        uses: Swatinem/rust-cache@v2
+        with:
+          workspaces: . -> target
+          cache-on-failure: true
+          key: core-windows
+      - name: Install sccache
+        uses: mozilla-actions/sccache-action@v0.0.9
+      - name: Run Windows-specific secrets tests
+        # Runs the full security::secrets suite including all #[cfg(windows)]
+        # tests: self-repair ACL path (OPENHUMAN-TAURI-GN), domain-qualified
+        # icacls username, is_permission_error, repair_windows_acl.
+        run: cargo test -p openhuman -- security::secrets --nocapture
+
   rust-tauri-tests:
     if: inputs.run_rust_tauri
     name: Rust Tauri Shell Tests

src/openhuman/security/secrets.rs

@@ -188,7 +188,32 @@ impl SecretStore {
         }
 
         if self.key_path.exists() {
-            let hex_key = read_key_file_with_retry(&self.key_path).with_context(|| {
+            let read_result = read_key_file_with_retry(&self.key_path);
+
+            // On Windows a previous bad icacls invocation may have stripped the
+            // inherited ACEs from %APPDATA% without granting the current user
+            // explicit access, leaving the file permanently unreadable. Attempt
+            // a one-shot ACL repair via `icacls /reset` before giving up.
+            #[cfg(windows)]
+            let read_result = if let Err(ref e) = read_result {
+                if is_permission_error(e) {
+                    log::warn!(
+                        "[security] PermissionDenied reading key file '{}'; \
+                         attempting icacls /reset self-repair",
+                        self.key_path.display()
+                    );
+                    repair_windows_acl(&self.key_path);
+                    // Single retry regardless of whether repair reported success —
+                    // icacls /reset may partially restore access even on a non-zero exit.
+                    read_key_file_with_retry(&self.key_path)
+                } else {
+                    read_result
+                }
+            } else {
+                read_result
+            };
+
+            let hex_key = read_result.with_context(|| {
                 let mut msg = format!(
                     "Failed to read secret key file at {}",
                     self.key_path.display()
@@ -229,35 +254,60 @@ impl SecretStore {
             }
             #[cfg(windows)]
             {
-                // On Windows, use icacls to restrict permissions to current user only
+                // On Windows, use icacls to restrict permissions to current user only.
+                // We use USERDOMAIN\USERNAME so the account is resolved correctly on
+                // domain-joined and AAD-joined machines (bare USERNAME is ambiguous and
+                // may refer to a local account that doesn't match the signed-in user).
                 let username = std::env::var("USERNAME").unwrap_or_default();
-                let Some(grant_arg) = build_windows_icacls_grant_arg(&username) else {
+                let userdomain = std::env::var("USERDOMAIN").unwrap_or_default();
+                let computername = std::env::var("COMPUTERNAME").unwrap_or_default();
+                let qualified_username =
+                    qualify_windows_username(&username, &userdomain, &computername);
+                let Some(grant_arg) = build_windows_icacls_grant_arg(&qualified_username) else {
                     log::warn!(
-                        "USERNAME environment variable is empty; \
+                        "[security] USERNAME/USERDOMAIN environment variables are empty; \
                          cannot restrict key file permissions via icacls"
                     );
                     cache_key(&cache_key_path, &key);
                     return Ok(key);
                 };
 
-                match std::process::Command::new("icacls")
+                let icacls_ok = match std::process::Command::new("icacls")
                     .arg(&self.key_path)
                     .args(["/inheritance:r", "/grant:r"])
-                    .arg(grant_arg)
+                    .arg(&grant_arg)
                     .output()
                 {
-                    Ok(o) if !o.status.success() => {
+                    Ok(o) if o.status.success() => {
+                        log::debug!("[security] key file permissions restricted via icacls");
+                        true
+                    }
+                    Ok(o) => {
                         log::warn!(
-                            "Failed to set key file permissions via icacls (exit code {:?})",
-                            o.status.code()
+                            "[security] icacls exited {:?} for account '{}'; \
+                             restoring inherited ACLs so the file remains readable",
+                            o.status.code(),
+                            grant_arg,
                         );
+                        false
                     }
                     Err(e) => {
-                        log::warn!("Could not set key file permissions: {e}");
-                    }
-                    _ => {
-                        log::debug!("Key file permissions restricted via icacls");
+                        log::warn!(
+                            "[security] could not run icacls: {e}; \
+                                    restoring inherited ACLs"
+                        );
+                        false
                     }
+                };
+                // If the icacls grant command failed, the `/inheritance:r` flag may have
+                // already stripped the inherited ACEs that let the current user read the
+                // file. Explicitly reset to restore inheritance so the file is always
+                // readable — a slightly weaker ACL is preferable to a locked-out user.
+                if !icacls_ok {
+                    let _ = std::process::Command::new("icacls")
+                        .arg(&self.key_path)
+                        .args(["/reset"])
+                        .output();
                 }
             }
 
@@ -342,6 +392,97 @@ fn read_key_file_with_retry(path: &Path) -> std::io::Result<String> {
     Err(last_err.unwrap_or_else(|| std::io::Error::other("read_to_string failed")))
 }
 
+/// Returns `true` when an `std::io::Error` is a permanent permission/access
+/// denial rather than a transient sharing violation.
+#[cfg(windows)]
+fn is_permission_error(e: &std::io::Error) -> bool {
+    matches!(e.kind(), std::io::ErrorKind::PermissionDenied) || e.raw_os_error() == Some(5)
+    // ERROR_ACCESS_DENIED
+}
+
+/// Attempt to repair a locked key file by running `icacls /reset` on it.
+///
+/// Attempt to repair a locked key file.
+///
+/// Two-step process:
+///   1. `icacls /reset` — removes all explicit ACEs and re-enables ACL
+///      inheritance from the parent directory.
+///   2. `icacls /grant:r <DOMAIN\USER>:F` — explicit grant for the current
+///      user as a belt-and-suspenders fallback for environments (e.g. CI
+///      temp dirs) where the parent's inheritance chain may not include the
+///      runner account.
+///
+/// Returns `true` if the file is actually readable after the repair attempt,
+/// regardless of which step(s) succeeded.
+#[cfg(windows)]
+pub(super) fn repair_windows_acl(path: &Path) -> bool {
+    // Step 1: restore inheritance.
+    match std::process::Command::new("icacls")
+        .arg(path)
+        .args(["/reset"])
+        .output()
+    {
+        Ok(o) if o.status.success() => {
+            log::info!(
+                "[security] icacls /reset succeeded for '{}'; ACL inheritance restored",
+                path.display()
+            );
+        }
+        Ok(o) => {
+            log::warn!(
+                "[security] icacls /reset exited {:?} for '{}'",
+                o.status.code(),
+                path.display()
+            );
+        }
+        Err(e) => {
+            log::warn!(
+                "[security] could not run icacls /reset for '{}': {e}",
+                path.display()
+            );
+        }
+    }
+
+    // Step 2: explicit grant for current user — handles CI environments
+    // where the temp/app dir's inheritable ACEs don't include the runner.
+    let username = std::env::var("USERNAME").unwrap_or_default();
+    let userdomain = std::env::var("USERDOMAIN").unwrap_or_default();
+    let computername = std::env::var("COMPUTERNAME").unwrap_or_default();
+    let qualified = qualify_windows_username(&username, &userdomain, &computername);
+    if let Some(grant_arg) = build_windows_icacls_grant_arg(&qualified) {
+        match std::process::Command::new("icacls")
+            .arg(path)
+            .args(["/grant:r"])
+            .arg(&grant_arg)
+            .output()
+        {
+            Ok(o) if o.status.success() => {
+                log::debug!(
+                    "[security] explicit grant '{grant_arg}' succeeded during repair of '{}'",
+                    path.display()
+                );
+            }
+            Ok(o) => {
+                log::warn!(
+                    "[security] explicit grant '{grant_arg}' exited {:?} during repair of '{}'",
+                    o.status.code(),
+                    path.display()
+                );
+            }
+            Err(e) => {
+                log::warn!(
+                    "[security] could not run icacls /grant during repair of '{}': {e}",
+                    path.display()
+                );
+            }
+        }
+    }
+
+    // Return whether the file is actually readable now — callers use this
+    // for logging/metrics; the retry in load_or_create_key is unconditional.
+    std::fs::read(path).is_ok()
+}
+
 /// XOR cipher with repeating key. Same function for encrypt and decrypt.
 fn xor_cipher(data: &[u8], key: &[u8]) -> Vec<u8> {
     if key.is_empty() {
@@ -381,6 +522,37 @@ fn build_windows_icacls_grant_arg(username: &str) -> Option<String> {
     Some(format!("{normalized}:F"))
 }
 
+/// Produce a domain-qualified Windows account name suitable for `icacls`.
+///
+/// On domain-joined machines `USERDOMAIN` is the domain name and differs from
+/// `COMPUTERNAME`.  On standalone machines they are equal, so we use the bare
+/// `username` in that case to avoid a redundant `DESKTOP-XYZ\alice` prefix.
+///
+/// Returns an empty string when both inputs are empty (caller must treat this
+/// as "cannot determine account name").
+#[cfg(windows)]
+fn qualify_windows_username(username: &str, userdomain: &str, computername: &str) -> String {
+    let username = username.trim();
+    let userdomain = userdomain.trim();
+    let computername = computername.trim();
+
+    if username.is_empty() {
+        return String::new();
+    }
+
+    // If USERDOMAIN is set and differs from COMPUTERNAME the machine is
+    // domain/AAD-joined; use the fully-qualified form so icacls resolves the
+    // account unambiguously.
+    if !userdomain.is_empty()
+        && !computername.is_empty()
+        && !userdomain.eq_ignore_ascii_case(computername)
+    {
+        format!("{userdomain}\\{username}")
+    } else {
+        username.to_string()
+    }
+}
+
 /// Hex-decode a hex string to bytes.
 #[allow(clippy::manual_is_multiple_of)]
 fn hex_decode(hex: &str) -> Result<Vec<u8>> {

src/openhuman/security/secrets_tests.rs

@@ -497,6 +497,253 @@ fn windows_icacls_grant_arg_preserves_valid_characters() {
     );
 }
 
+// ── qualify_windows_username ─────────────────────────────────
+
+#[cfg(windows)]
+#[test]
+fn qualify_windows_username_local_account() {
+    // USERDOMAIN == COMPUTERNAME → standalone machine → plain username
+    assert_eq!(
+        qualify_windows_username("alice", "DESKTOP-ABC", "DESKTOP-ABC"),
+        "alice"
+    );
+}
+
+#[cfg(windows)]
+#[test]
+fn qualify_windows_username_domain_joined() {
+    // USERDOMAIN != COMPUTERNAME → domain-joined → prefix with domain
+    assert_eq!(
+        qualify_windows_username("alice", "CORP", "DESKTOP-ABC"),
+        "CORP\\alice"
+    );
+}
+
+#[cfg(windows)]
+#[test]
+fn qualify_windows_username_case_insensitive_comparison() {
+    // Case-insensitive: "desktop-abc" == "DESKTOP-ABC" → local account
+    assert_eq!(
+        qualify_windows_username("bob", "desktop-abc", "DESKTOP-ABC"),
+        "bob"
+    );
+}
+
+#[cfg(windows)]
+#[test]
+fn qualify_windows_username_empty_computername() {
+    // COMPUTERNAME is unset — fall back to plain username to avoid prefixing
+    // with a potentially meaningless domain string
+    assert_eq!(qualify_windows_username("alice", "CORP", ""), "alice");
+}
+
+#[cfg(windows)]
+#[test]
+fn qualify_windows_username_empty_userdomain() {
+    // USERDOMAIN is unset — use plain username
+    assert_eq!(
+        qualify_windows_username("alice", "", "DESKTOP-ABC"),
+        "alice"
+    );
+}
+
+#[cfg(windows)]
+#[test]
+fn qualify_windows_username_empty_username_returns_empty() {
+    assert_eq!(qualify_windows_username("", "CORP", "DESKTOP-ABC"), "");
+}
+
+#[cfg(windows)]
+#[test]
+fn qualify_windows_username_whitespace_trimmed() {
+    assert_eq!(
+        qualify_windows_username("  alice  ", "  CORP  ", "  DESKTOP-XYZ  "),
+        "CORP\\alice"
+    );
+}
+
+// ── Windows self-repair path ─────────────────────────────────
+
+/// Simulate a locked key file on non-Windows: write the file, remove all
+/// read permissions, verify the store recovers after `chmod` restores them.
+/// On Windows the equivalent is tested by is_permission_error / repair_windows_acl.
+#[cfg(unix)]
+#[test]
+fn locked_key_file_fails_gracefully_on_unix() {
+    use std::os::unix::fs::PermissionsExt;
+
+    let tmp = TempDir::new().unwrap();
+    let store = SecretStore::new(tmp.path(), true);
+
+    // Trigger key creation so the file exists on disk.
+    let encrypted = store.encrypt("original-secret").unwrap();
+    assert!(store.key_path.exists());
+
+    // Lock the file before clearing the cache, so the next decrypt must read
+    // from disk and encounter the PermissionDenied error.
+    fs::set_permissions(&store.key_path, fs::Permissions::from_mode(0o000)).unwrap();
+
+    // Clear the cache so the decrypt path actually hits the disk.
+    super::clear_cached_key(&store.key_path);
+
+    // Linux CI containers commonly run as root, which bypasses file permission
+    // checks — chmod 0o000 has no effect and the file stays readable.  Only
+    // assert the graceful-failure behaviour when the lock actually took hold;
+    // otherwise the test would fail vacuously on root runners.
+    let file_is_locked = fs::read_to_string(&store.key_path).is_err();
+    if file_is_locked {
+        let result = store.decrypt(&encrypted);
+        assert!(
+            result.is_err(),
+            "decrypt must fail gracefully when key file is locked and cache is empty"
+        );
+    }
+
+    // Restore permissions so TempDir cleanup can remove the file.
+    fs::set_permissions(&store.key_path, fs::Permissions::from_mode(0o600)).unwrap();
+}
+
+/// End-to-end test for the Windows self-repair path.
+///
+/// Recreates the exact bad state that caused OPENHUMAN-TAURI-GN:
+///   1. Key file created, ACL corrupted with `icacls /inheritance:r` + no valid grant
+///      (simulated here with an explicit `Everyone:DENY` which is even stricter).
+///   2. In-memory cache cleared so the next call must actually read from disk.
+///   3. `decrypt` is called — the self-repair path must run `icacls /reset`,
+///      restore inherited ACLs, re-read the file, and return the correct plaintext.
+///
+/// The lock step may be a no-op when the test process runs as SYSTEM/Administrator
+/// (elevated tokens bypass DENY ACEs).  In that case the test skips the
+/// "verify locked" assertion and still validates that repair_windows_acl + decrypt
+/// complete without panicking or returning an unexpected error.
+///
+/// Run on Windows CI via the `rust-core-tests-windows` job in test-reusable.yml.
+#[cfg(windows)]
+#[test]
+fn self_repair_recovers_from_locked_key_file() {
+    let tmp = TempDir::new().unwrap();
+    let store = SecretStore::new(tmp.path(), true);
+
+    // Step 1: create the key file and produce a ciphertext to decrypt later.
+    let encrypted = store
+        .encrypt("secret-to-survive-acl-lockout")
+        .expect("initial encrypt must succeed");
+    assert!(
+        store.key_path.exists(),
+        "key file must exist after first encrypt"
+    );
+
+    // Step 2: clear the in-memory cache so the next decrypt reads from disk.
+    super::clear_cached_key(&store.key_path);
+
+    // Step 3: corrupt the ACL — strip inheritance AND add an explicit DENY for
+    // Everyone.  This is a strict superset of the production failure mode (where
+    // /inheritance:r ran but the /grant target was unresolvable, leaving no ACE).
+    let lock_status = std::process::Command::new("icacls")
+        .arg(&store.key_path)
+        .args(["/inheritance:r", "/deny"])
+        .arg("Everyone:F")
+        .status()
+        .expect("icacls must be available on Windows");
+    assert!(
+        lock_status.success(),
+        "icacls lock step must succeed — test setup invalid"
+    );
+
+    // Step 4: check whether the lock actually made the file unreadable.
+    // Elevated (SYSTEM/admin) tokens bypass DENY ACEs, so on those runners
+    // the file stays readable and we skip the self-repair assertion — but we
+    // still validate repair_windows_acl completes cleanly (no panic).
+    let file_is_locked = fs::read_to_string(&store.key_path).is_err();
+
+    if file_is_locked {
+        // Full E2E path: self-repair must restore access and return plaintext.
+        let decrypted = store
+            .decrypt(&encrypted)
+            .expect("self-repair must restore access and return correct plaintext");
+        assert_eq!(
+            decrypted, "secret-to-survive-acl-lockout",
+            "decrypted value must match original"
+        );
+        // Verify the repair is durable: clear the in-memory cache and decrypt a
+        // second time from disk.  If the ACL is truly fixed, this succeeds on the
+        // first read attempt without triggering the repair path again.  (A direct
+        // fs::read_to_string assertion here is flaky — Windows Defender / the
+        // Security Center can briefly re-acquire the file handle right after an
+        // icacls operation, causing intermittent PermissionDenied.  Going through
+        // load_or_create_key means the retry backoff in read_key_file_with_retry
+        // absorbs that transient window, which is exactly what production code does.)
+        super::clear_cached_key(&store.key_path);
+        let decrypted2 = store
+            .decrypt(&encrypted)
+            .expect("ACL fix must be durable: second from-disk decrypt must succeed");
+        assert_eq!(
+            decrypted2, "secret-to-survive-acl-lockout",
+            "second decrypt must return the same plaintext"
+        );
+    } else {
+        // Elevated runner: lock was bypassed.  Verify repair_windows_acl runs
+        // cleanly on an already-accessible file (icacls /reset is idempotent).
+        let repaired = super::repair_windows_acl(&store.key_path);
+        assert!(
+            repaired,
+            "repair_windows_acl must succeed on an accessible file"
+        );
+        let decrypted = store
+            .decrypt(&encrypted)
+            .expect("decrypt must succeed when file is accessible");
+        assert_eq!(decrypted, "secret-to-survive-acl-lockout");
+    }
+}
+
+/// Verify that the self-repair path does NOT trigger for non-permission errors
+/// (e.g. corrupt/truncated file) — we should get a clear error, not a silent
+/// retry that produces garbage.
+#[cfg(windows)]
+#[test]
+fn self_repair_does_not_trigger_for_corrupt_file() {
+    let tmp = TempDir::new().unwrap();
+    let store = SecretStore::new(tmp.path(), true);
+
+    // Write a corrupt (non-hex) key file directly — simulates on-disk corruption.
+    fs::create_dir_all(tmp.path()).unwrap();
+    fs::write(&store.key_path, "this-is-not-valid-hex!!!").unwrap();
+    super::clear_cached_key(&store.key_path);
+
+    let err = store.encrypt("anything").unwrap_err();
+    let msg = format!("{err:?}");
+    // Must surface a hex/corrupt error, not attempt a repair loop.
+    assert!(
+        msg.contains("corrupt") || msg.contains("hex") || msg.contains("Invalid"),
+        "corrupt file must surface a clear decode error, got: {msg}"
+    );
+}
+
+#[cfg(windows)]
+#[test]
+fn is_permission_error_matches_access_denied() {
+    use std::io::{Error, ErrorKind};
+    let perm_err = Error::from(ErrorKind::PermissionDenied);
+    assert!(is_permission_error(&perm_err));
+}
+
+#[cfg(windows)]
+#[test]
+fn is_permission_error_ignores_not_found() {
+    use std::io::{Error, ErrorKind};
+    let not_found = Error::from(ErrorKind::NotFound);
+    assert!(!is_permission_error(&not_found));
+}
+
+#[cfg(windows)]
+#[test]
+fn is_permission_error_matches_raw_os_error_5() {
+    use std::io::Error;
+    // raw OS error 5 = ERROR_ACCESS_DENIED
+    let err = Error::from_raw_os_error(5);
+    assert!(is_permission_error(&err));
+}
+
 #[test]
 fn generate_random_key_correct_length() {
     let key = generate_random_key();