From 3aaba5e30cf1b8113553c2dc7ead2d31acd160ff Mon Sep 17 00:00:00 2001 From: Jarl <857085930@qq.com> Date: Mon, 18 May 2026 06:20:10 +0800 Subject: [PATCH] fix: harden token handling and key rotation logs (#1999) --- app/src/providers/CoreStateProvider.tsx | 14 +---- .../__tests__/CoreStateProvider.test.tsx | 28 +++++++++ .../store/__tests__/userScopedStorage.test.ts | 34 ++++++++++ app/src/store/userScopedStorage.ts | 62 ++++++++++++++++++- src/openhuman/inference/provider/reliable.rs | 45 ++++++++++---- .../inference/provider/reliable_tests.rs | 9 +++ 6 files changed, 166 insertions(+), 26 deletions(-) diff --git a/app/src/providers/CoreStateProvider.tsx b/app/src/providers/CoreStateProvider.tsx index 09e678ddf..37dde8035 100644 --- a/app/src/providers/CoreStateProvider.tsx +++ b/app/src/providers/CoreStateProvider.tsx @@ -458,25 +458,13 @@ export default function CoreStateProvider({ children }: { children: ReactNode }) const onSessionTokenUpdated = (event: Event) => { const customEvent = event as CustomEvent<{ sessionToken?: string | null }>; const token = customEvent.detail?.sessionToken; - if (!token) { + if (typeof token !== 'string' || token.length === 0) { return; } snapshotRequestIdRef.current += 1; logoutGuardUntilRef.current = 0; - memoryTokenRef.current = token; - commitState(previous => ({ - ...previous, - isBootstrapping: false, - isReady: true, - snapshot: { - ...previous.snapshot, - auth: { ...previous.snapshot.auth, isAuthenticated: true }, - sessionToken: token, - }, - })); - void refresh().catch(err => { log('refresh failed after deep-link session update: %O', sanitizeError(err)); }); diff --git a/app/src/providers/__tests__/CoreStateProvider.test.tsx b/app/src/providers/__tests__/CoreStateProvider.test.tsx index 24e22f036..515f156db 100644 --- a/app/src/providers/__tests__/CoreStateProvider.test.tsx +++ b/app/src/providers/__tests__/CoreStateProvider.test.tsx @@ -371,6 +371,34 @@ describe('CoreStateProvider — identity-change cache clearing', () => { expect(vi.mocked(tauriCommands.logout)).toHaveBeenCalledTimes(1); }); + it('ignores forged session-token-updated events that do not match the core snapshot (#1937)', async () => { + fetchSnapshot.mockResolvedValue(makeSnapshot({ userId: 'u1', sessionToken: 'tok1' })); + listTeams.mockResolvedValue([]); + + render( + + + + ); + + await waitFor(() => expect(screen.getByTestId('token').textContent).toBe('tok1')); + + // Keep the follow-up refresh pending so this assertion observes the + // event handler itself. A forged event must not be able to replace the + // in-memory auth token before refreshCore re-pulls authoritative state. + fetchSnapshot.mockImplementation(() => new Promise(() => {}) as never); + + await act(async () => { + window.dispatchEvent( + new CustomEvent('core-state:session-token-updated', { + detail: { sessionToken: 'attacker-controlled-token' }, + }) + ); + }); + + expect(screen.getByTestId('token').textContent).toBe('tok1'); + }); + it('setMeetAutoOrchestratorHandoff swallows refresh errors after the RPC succeeds (#1299)', async () => { fetchSnapshot.mockResolvedValueOnce(makeSnapshot({ userId: 'u1', sessionToken: 'tok1' })); listTeams.mockResolvedValue([]); diff --git a/app/src/store/__tests__/userScopedStorage.test.ts b/app/src/store/__tests__/userScopedStorage.test.ts index cbdb5df3b..9eb52e613 100644 --- a/app/src/store/__tests__/userScopedStorage.test.ts +++ b/app/src/store/__tests__/userScopedStorage.test.ts @@ -90,4 +90,38 @@ describe('userScopedStorage', () => { setItemSpy.mockRestore(); } }); + + test('userScopedStorage redacts auth/session tokens before persisting redux blobs (#1938)', async () => { + const mod = await importModule(); + mod.primeActiveUserId('user-123'); + + const persistedReduxBlob = JSON.stringify({ + sessionToken: 'top-level-token', + auth: JSON.stringify({ + isAuthenticated: true, + sessionToken: 'nested-token', + accessToken: 'nested-access-token', + }), + nestedObject: { + token: 'object-token', + child: { refreshToken: 'object-refresh-token', safe: 'keep-me' }, + }, + nestedArray: [{ accessToken: 'array-access-token', safe: 'array-safe' }], + harmless: JSON.stringify({ theme: 'dark' }), + }); + + await mod.userScopedStorage.setItem('persist:coreState', persistedReduxBlob); + + const stored = localStorage.getItem('user-123:persist:coreState'); + expect(stored).not.toBeNull(); + expect(stored).not.toContain('top-level-token'); + expect(stored).not.toContain('nested-token'); + expect(stored).not.toContain('nested-access-token'); + expect(stored).not.toContain('object-token'); + expect(stored).not.toContain('object-refresh-token'); + expect(stored).not.toContain('array-access-token'); + expect(stored).toContain('keep-me'); + expect(stored).toContain('array-safe'); + expect(stored).toContain('dark'); + }); }); diff --git a/app/src/store/userScopedStorage.ts b/app/src/store/userScopedStorage.ts index fa8341c33..15d0ab716 100644 --- a/app/src/store/userScopedStorage.ts +++ b/app/src/store/userScopedStorage.ts @@ -179,6 +179,66 @@ function namespacedKey(key: string): string | null { return `${activeUserId}:${key}`; } +const SENSITIVE_PERSIST_KEYS = new Set(['sessionToken', 'token', 'accessToken', 'refreshToken']); + +function redactSensitivePersistValue(value: unknown): unknown { + if (Array.isArray(value)) { + return value.map(redactSensitivePersistValue); + } + if (!value || typeof value !== 'object') { + return value; + } + + const next: Record = {}; + for (const [key, child] of Object.entries(value as Record)) { + if (SENSITIVE_PERSIST_KEYS.has(key)) { + continue; + } + next[key] = redactSensitivePersistValue(child); + } + return next; +} + +function sanitizePersistBlob(value: string): string { + try { + const parsed = JSON.parse(value) as Record; + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { + return value; + } + + let changed = false; + const sanitized: Record = {}; + for (const [key, child] of Object.entries(parsed)) { + if (SENSITIVE_PERSIST_KEYS.has(key)) { + changed = true; + continue; + } + + if (typeof child === 'string') { + try { + const nested = JSON.parse(child); + const redacted = redactSensitivePersistValue(nested); + const nextChild = JSON.stringify(redacted); + sanitized[key] = nextChild; + changed ||= nextChild !== child; + continue; + } catch { + // redux-persist stores many slice fields as JSON strings, but plain + // strings are valid too; leave non-JSON strings untouched. + } + } + + const redacted = redactSensitivePersistValue(child); + sanitized[key] = redacted; + changed ||= redacted !== child; + } + + return changed ? JSON.stringify(sanitized) : value; + } catch { + return value; + } +} + /** * `Storage`-shaped object compatible with redux-persist's storage contract. * Methods return promises because redux-persist treats storage as async. @@ -199,7 +259,7 @@ export const userScopedStorage = { const ns = namespacedKey(key); if (!ns) return; try { - localStorage.setItem(ns, value); + localStorage.setItem(ns, sanitizePersistBlob(value)); } catch { // ignore quota / unavailable } diff --git a/src/openhuman/inference/provider/reliable.rs b/src/openhuman/inference/provider/reliable.rs index e311b0e47..6c2c93d5c 100644 --- a/src/openhuman/inference/provider/reliable.rs +++ b/src/openhuman/inference/provider/reliable.rs @@ -278,6 +278,15 @@ fn push_failure( )); } +fn rotated_key_log_detail(after_rotate_index: usize, total: usize) -> String { + let slot = if total == 0 { + 0 + } else { + after_rotate_index.saturating_sub(1) % total + 1 + }; + format!("slot={slot}/{total}") +} + /// Format the final bail message produced when every provider+model in the /// chain has failed. /// @@ -447,12 +456,15 @@ impl Provider for ReliableProvider { // On rate-limit, try rotating API key if rate_limited && !non_retryable_rate_limit { - if let Some(new_key) = self.rotate_key() { + if self.rotate_key().is_some() { tracing::info!( provider = provider_name, error = %error_detail, - "Rate limited, rotated API key (key ending ...{})", - &new_key[new_key.len().saturating_sub(4)..] + key_slot = %rotated_key_log_detail( + self.key_index.load(Ordering::Relaxed), + self.api_keys.len() + ), + "Rate limited, rotated API key" ); } } @@ -579,12 +591,15 @@ impl Provider for ReliableProvider { ); if rate_limited && !non_retryable_rate_limit { - if let Some(new_key) = self.rotate_key() { + if self.rotate_key().is_some() { tracing::info!( provider = provider_name, error = %error_detail, - "Rate limited, rotated API key (key ending ...{})", - &new_key[new_key.len().saturating_sub(4)..] + key_slot = %rotated_key_log_detail( + self.key_index.load(Ordering::Relaxed), + self.api_keys.len() + ), + "Rate limited, rotated API key" ); } } @@ -739,12 +754,15 @@ impl Provider for ReliableProvider { ); if rate_limited && !non_retryable_rate_limit { - if let Some(new_key) = self.rotate_key() { + if self.rotate_key().is_some() { tracing::info!( provider = provider_name, error = %error_detail, - "Rate limited, rotated API key (key ending ...{})", - &new_key[new_key.len().saturating_sub(4)..] + key_slot = %rotated_key_log_detail( + self.key_index.load(Ordering::Relaxed), + self.api_keys.len() + ), + "Rate limited, rotated API key" ); } } @@ -864,12 +882,15 @@ impl Provider for ReliableProvider { ); if rate_limited && !non_retryable_rate_limit { - if let Some(new_key) = self.rotate_key() { + if self.rotate_key().is_some() { tracing::info!( provider = provider_name, error = %error_detail, - "Rate limited, rotated API key (key ending ...{})", - &new_key[new_key.len().saturating_sub(4)..] + key_slot = %rotated_key_log_detail( + self.key_index.load(Ordering::Relaxed), + self.api_keys.len() + ), + "Rate limited, rotated API key" ); } } diff --git a/src/openhuman/inference/provider/reliable_tests.rs b/src/openhuman/inference/provider/reliable_tests.rs index 00ba1763c..c683f0750 100644 --- a/src/openhuman/inference/provider/reliable_tests.rs +++ b/src/openhuman/inference/provider/reliable_tests.rs @@ -857,6 +857,15 @@ fn failure_reason_upstream_unhealthy_wins_over_all_others() { // configured a chain yet, so the next step is obvious without // re-reading the docs. +#[test] +fn rotated_key_log_detail_does_not_expose_key_suffix() { + let detail = rotated_key_log_detail(2, 4); + + assert_eq!(detail, "slot=2/4"); + assert!(!detail.contains("sk-")); + assert!(!detail.contains("...")); +} + #[test] fn format_failure_aggregate_prepends_user_hint_when_no_fallbacks_configured() { let failures = vec![