mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-27 21:08:00 +00:00
fix(keyring): cache availability probe with OnceLock to prevent repeated keychain dialogs (#2651)
This commit is contained in:
@@ -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');
|
||||
|
||||
@@ -4,7 +4,11 @@ import { getCoreStateSnapshot } from '../../../lib/coreState/store';
|
||||
import { bootCheckTransport } from '../../../services/bootCheckService';
|
||||
import { testCoreRpcConnection } from '../../../services/coreRpcClient';
|
||||
import { isTauri } from '../../../services/webviewAccountService';
|
||||
import { getStoredCoreMode, getStoredCoreToken } from '../../../utils/configPersistence';
|
||||
import {
|
||||
getStoredCoreMode,
|
||||
getStoredCoreToken,
|
||||
storeCoreMode,
|
||||
} from '../../../utils/configPersistence';
|
||||
import {
|
||||
oauthAuthReadinessUserMessage,
|
||||
prepareOAuthLoginLaunch,
|
||||
@@ -25,6 +29,7 @@ vi.mock('../../../services/bootCheckService', () => ({
|
||||
vi.mock('../../../utils/configPersistence', () => ({
|
||||
getStoredCoreMode: vi.fn(),
|
||||
getStoredCoreToken: vi.fn().mockReturnValue(null),
|
||||
storeCoreMode: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock('../../../services/webviewAccountService', () => ({
|
||||
@@ -57,8 +62,21 @@ describe('oauthAuthReadiness', () => {
|
||||
vi.mocked(isTauri).mockReturnValue(true);
|
||||
});
|
||||
|
||||
it('defaults to local mode in Tauri when no core mode is stored', async () => {
|
||||
vi.mocked(getStoredCoreMode).mockReturnValue(null);
|
||||
// isTauri is true from beforeEach — should auto-set 'local' and proceed
|
||||
|
||||
const result = await waitForOAuthAuthReadiness(2_000);
|
||||
|
||||
expect(vi.mocked(storeCoreMode)).toHaveBeenCalledWith('local');
|
||||
expect(result).toEqual({ ready: true });
|
||||
});
|
||||
|
||||
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);
|
||||
|
||||
|
||||
@@ -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');
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
|
||||
@@ -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());
|
||||
|
||||
@@ -50,3 +50,6 @@ pub use store::init_workspace;
|
||||
#[cfg(test)]
|
||||
#[allow(unused_imports)]
|
||||
pub(crate) use ops::force_backend_for_test;
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests;
|
||||
|
||||
@@ -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.
|
||||
@@ -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__";
|
||||
@@ -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)?;
|
||||
|
||||
@@ -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"
|
||||
);
|
||||
// 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]
|
||||
|
||||
Reference in New Issue
Block a user