Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions app/src/components/BootCheckGate/BootCheckGate.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -620,15 +620,8 @@ export default function BootCheckGate({ children }: BootCheckGateProps) {
try {
const checkResult = await runBootCheck(mode, transport);
log('[boot-check] gate — check result=%s', checkResult.kind);

if (checkResult.kind === 'match') {
// Gate resolves — render children.
setPhase('result');
setResult(checkResult);
} else {
setPhase('result');
setResult(checkResult);
}
setPhase('result');
setResult(checkResult);
} catch (err) {
logError('[boot-check] gate — unexpected error: %o', err);
setPhase('result');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ vi.mock('../../../services/bootCheckService', () => ({
vi.mock('../../../utils/configPersistence', () => ({
getStoredCoreMode: vi.fn(),
getStoredCoreToken: vi.fn().mockReturnValue(null),
storeCoreMode: vi.fn(),
}));

vi.mock('../../../services/webviewAccountService', () => ({
Expand Down Expand Up @@ -59,6 +60,9 @@ describe('oauthAuthReadiness', () => {

it('returns core_mode_unset when BootCheckGate has not committed a mode', async () => {
vi.mocked(getStoredCoreMode).mockReturnValue(null);
// Must be web (non-Tauri) — in Tauri the code defaults to 'local' and never
// returns core_mode_unset.
vi.mocked(isTauri).mockReturnValue(false);

const result = await waitForOAuthAuthReadiness(500);

Expand Down
15 changes: 14 additions & 1 deletion app/src/components/oauth/oauthAuthReadiness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ import { getCoreStateSnapshot } from '../../lib/coreState/store';
import { bootCheckTransport } from '../../services/bootCheckService';
import { getCoreRpcUrl, testCoreRpcConnection } from '../../services/coreRpcClient';
import { isTauri } from '../../services/webviewAccountService';
import { getStoredCoreMode, getStoredCoreToken } from '../../utils/configPersistence';
import {
getStoredCoreMode,
getStoredCoreToken,
storeCoreMode,
} from '../../utils/configPersistence';

const logPrefix = '[oauth-auth-readiness]';
const log = debug('oauth:auth-readiness');
Expand Down Expand Up @@ -83,6 +87,15 @@ export async function waitForOAuthAuthReadiness(
sawCoreMode = true;
break;
}
// In the Tauri desktop app the core is always embedded locally. If the
// picker hasn't run yet (e.g. first launch before BootCheckGate finishes,
// or core mode was just cleared), default to 'local' so OAuth can proceed
// without forcing the user to navigate back through the runtime picker.
if (isTauri()) {
storeCoreMode('local');
sawCoreMode = true;
break;
}
await delay(POLL_MS);
}

Expand Down
12 changes: 11 additions & 1 deletion src/openhuman/app_state/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,17 @@ pub async fn snapshot() -> Result<RpcOutcome<AppStateSnapshot>, String> {
// snapshot. On Windows this doubled the surface area for the
// "Timed out waiting for auth profile lock" failure reported in
// Sentry against `openhuman.app_state_snapshot`.
let session_profile = load_app_session_profile(&config)?;
//
// `load_app_session_profile` calls `acquire_lock()`, which busy-waits
// with `thread::sleep` for up to ~35s when the lock is contended. Calling
// it directly on a tokio worker thread blocks that thread for the entire
// wait, exhausting the thread pool under concurrent snapshot calls and
// triggering `ERR_CONNECTION_TIMED_OUT` on all RPC connections.
let config_for_profile = config.clone();
let session_profile =
tokio::task::spawn_blocking(move || load_app_session_profile(&config_for_profile))
.await
.unwrap_or_else(|e| Err(format!("[app_state] auth profile load task panicked: {e}")))?;
let mut auth = session_state_from_profile(session_profile.as_ref());
let session_token = session_token_from_profile(session_profile.as_ref());
let stored_user = sanitize_snapshot_user(auth.user.clone());
Expand Down
3 changes: 3 additions & 0 deletions src/openhuman/keyring/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,6 @@ pub use store::init_workspace;
#[cfg(test)]
#[allow(unused_imports)]
pub(crate) use ops::force_backend_for_test;

#[cfg(test)]
mod tests;
23 changes: 23 additions & 0 deletions src/openhuman/keyring/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,21 @@
//! All public functions delegate to the active backend selected by [`crate::openhuman::keyring::store`].

use std::path::Path;
use std::sync::OnceLock;

use chacha20poly1305::aead::{rand_core::RngCore, OsRng};

use crate::openhuman::keyring::error::KeyringError;
use crate::openhuman::keyring::store::backend;

// Cached result of the one-time keychain probe. Running the probe on every
// `is_available()` call means 4 OS-keychain round-trips (delete / set / get /
// delete) per call, which triggers repeated macOS access-permission dialogs
// and starves callers that poll frequently (e.g. snapshot, wallet guards).
// The backend selection is already frozen in a OnceLock, so the probe result
// is stable for the lifetime of the process — caching it here is safe.
static AVAILABILITY_CACHE: OnceLock<bool> = OnceLock::new();

// ── Outcome type ─────────────────────────────────────────────────────────────

/// Outcome of a file-to-keychain migration attempt.
Expand Down Expand Up @@ -82,7 +91,16 @@ pub fn delete(user_id: &str, key: &str) -> Result<(), KeyringError> {
/// For the `file` and `mock` backends this always returns `true`. For the
/// `os` backend on Linux headless systems (no Secret Service daemon) this
/// returns `false`; callers should fall back to file-based storage.
///
/// The result is cached after the first call — the probe runs exactly once
/// per process lifetime. This prevents repeated OS-keychain round-trips (and
/// the macOS access-permission dialogs they trigger) when polled by
/// wallet guards or snapshot loops.
pub fn is_available() -> bool {
*AVAILABILITY_CACHE.get_or_init(probe_availability)
}

fn probe_availability() -> bool {
const PROBE_USER: &str = "__probe__";
const PROBE_KEY: &str = "__openhuman_keyring_probe__";
const PROBE_VALUE: &str = "__probe_value__";
Expand All @@ -100,6 +118,11 @@ pub fn is_available() -> bool {
}

let result = (|| -> Result<bool, KeyringError> {
// Delete any leftover probe key from a previous launch before writing.
// Without this, `set` fails with "item already exists" (-25299) on
// every launch after the first, causing `is_available` to incorrectly
// return false even when the keychain is fully functional.
let _ = delete(PROBE_USER, PROBE_KEY);
set(PROBE_USER, PROBE_KEY, PROBE_VALUE)?;
let readback = get(PROBE_USER, PROBE_KEY)?;
delete(PROBE_USER, PROBE_KEY)?;
Expand Down
159 changes: 159 additions & 0 deletions src/openhuman/keyring/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,165 @@ fn get_or_create_random_idempotent_file_backend() {
assert_eq!(first, second, "idempotent read-back");
}

// ── is_available probe: idempotency regression tests ─────────────────────────
//
// The probe in `is_available()` must return `true` on every launch, not just the
// first one. Before the fix, `set()` was called blindly: on macOS the Keychain
// returns `-25299 "item already exists"` when the probe key survives from a
// prior launch, flipping `is_available` to `false` even though the keychain is
// fully functional. The fix deletes the probe key first so the write always
// lands clean.
//
// We verify this with a `StrictSetBackend` — a `FileBackend` wrapper that
// rejects `set()` on an already-existing key, exactly matching the Keychain
// semantics that exposed the bug. This lets the regression run in CI without
// the `OPENHUMAN_TEST_OS_KEYCHAIN=1` gate.

/// Wraps `FileBackend` but fails `set` with `KeyringError::Backend` when the
/// key already exists, mirroring macOS Keychain's `-25299` behaviour.
struct StrictSetBackend {
inner: FileBackend,
}

impl StrictSetBackend {
fn new(dir: &std::path::Path) -> Self {
Self {
inner: FileBackend::new(dir),
}
}
}

impl KeyringBackend for StrictSetBackend {
fn get(&self, key: &str) -> Result<Option<String>, KeyringError> {
self.inner.get(key)
}
fn set(&self, key: &str, value: &str) -> Result<(), KeyringError> {
if self.inner.get(key)?.is_some() {
return Err(KeyringError::Backend(
"Platform secure storage failure: The specified item already exists in the keychain. (OSStatus -25299)".into(),
));
}
self.inner.set(key, value)
}
fn delete(&self, key: &str) -> Result<(), KeyringError> {
self.inner.delete(key)
}
fn name(&self) -> &'static str {
"strict-set-mock"
}
}

/// Run the OLD (broken) probe logic against any backend.
/// Returns `true` only if the round-trip succeeded.
fn old_probe<B: KeyringBackend>(b: &B) -> bool {
const KEY: &str = "__probe__:__openhuman_keyring_probe__";
const VAL: &str = "__probe_value__";
if b.set(KEY, VAL).is_err() {
return false;
}
let ok = b.get(KEY).ok().flatten().as_deref() == Some(VAL);
let _ = b.delete(KEY);
ok
}

/// Run the NEW (fixed) probe logic against any backend.
/// Deletes any residue before writing, making the probe idempotent.
fn new_probe<B: KeyringBackend>(b: &B) -> bool {
const KEY: &str = "__probe__:__openhuman_keyring_probe__";
const VAL: &str = "__probe_value__";
let _ = b.delete(KEY);
if b.set(KEY, VAL).is_err() {
return false;
}
let ok = b.get(KEY).ok().flatten().as_deref() == Some(VAL);
let _ = b.delete(KEY);
ok
}

#[test]
fn probe_old_logic_fails_when_key_preexists() {
// Demonstrates the bug: old probe returns false the second time because
// `set` on an already-existing key fails with "item already exists".
let dir = TempDir::new().expect("tempdir");
let b = StrictSetBackend::new(dir.path());

// First probe: key absent → succeeds.
assert!(old_probe(&b), "first probe should succeed");

// Key was deleted by the probe — re-seed it to simulate a leftover from a
// previous launch (e.g. the app was force-quit after writing but before
// the delete could run, or a previous `is_available` call left it behind).
b.inner
.set("__probe__:__openhuman_keyring_probe__", "stale")
.unwrap();

// Second probe with residue: `set` hits "already exists" → returns false.
assert!(
!old_probe(&b),
"old probe must fail when probe key pre-exists (regression target)"
);
}

#[test]
fn probe_new_logic_succeeds_even_when_key_preexists() {
// Verifies the fix: new probe deletes any residue first, so it always
// succeeds regardless of leftover state.
let dir = TempDir::new().expect("tempdir");
let b = StrictSetBackend::new(dir.path());

assert!(new_probe(&b), "first probe should succeed");

// Re-seed to simulate a leftover key (same scenario as the bug test).
b.inner
.set("__probe__:__openhuman_keyring_probe__", "stale")
.unwrap();

assert!(
new_probe(&b),
"new probe must succeed even when probe key pre-exists"
);
}

#[test]
fn probe_new_logic_is_idempotent_across_multiple_runs() {
// New probe must return true on every consecutive call.
let dir = TempDir::new().expect("tempdir");
let b = StrictSetBackend::new(dir.path());

for i in 0..5 {
assert!(new_probe(&b), "probe run {i} should succeed");
}
}

#[test]
fn is_available_returns_true_on_repeated_calls_os_backend() {
// End-to-end: the real `is_available()` must return true on consecutive
// calls even when the OS backend retains the probe key between runs.
// Guarded by OPENHUMAN_TEST_OS_KEYCHAIN=1 to avoid blocking CI on a
// Keychain permission dialog.
if !os_keychain_available() {
eprintln!("skip: set OPENHUMAN_TEST_OS_KEYCHAIN=1 to run OS keychain tests");
return;
}
// Use the OsBackend directly to simulate the cross-launch residue.
let b = backend::OsBackend;
let probe_key = "__probe__:__openhuman_keyring_probe__";
// Pre-seed to mimic a leftover from a previous app launch.
let _ = b.set(probe_key, "__probe_value__");
// `is_available()` must still return true.
assert!(
super::ops::is_available(),
"is_available must return true even with pre-existing probe key in OS keychain"
);
Comment thread
M3gA-Mind marked this conversation as resolved.
// Repeated call should also stay true (exercises the OnceLock cached path).
assert!(
super::ops::is_available(),
"is_available must remain true on repeated calls (cached result)"
);
// Cleanup.
let _ = b.delete(probe_key);
}

// ── migrate_from_file (via module functions, requires OS keychain or file backend) ──

#[test]
Expand Down
Loading