From 3f66769e4f26649dd1de3a3a2216962e9ebf0787 Mon Sep 17 00:00:00 2001 From: Emil Lundberg Date: Tue, 3 Dec 2024 16:04:33 +0100 Subject: [PATCH 1/4] Fix corruption data race in cached users The cause of this data corruption is: after successfully logging in and unlocking the keystore, the `useEffect` in `LocalStorageKeystore` calls `setCachedUsers` to update the cached users (to copy the `prfKeys` from the logged-in user's `privateData` to the matching cached user). But when there are two tabs open simultaneously, that code runs simultaneously in both tabs, and the `globalUserHandleB64u` is different between the two tabs. The user1 tab has `globalUserHandleB64u` set to the user handle of user1, and the user2 tab has `globalUserHandleB64u` set to the user handle of user2. So the result is that both cached users get updated with user2's `prfKeys`, which then causes the user to be logged into user2 even if they press the cache button labeled "Log in as user1". --- src/services/LocalStorageKeystore.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/services/LocalStorageKeystore.ts b/src/services/LocalStorageKeystore.ts index 6033a557d..11865682f 100644 --- a/src/services/LocalStorageKeystore.ts +++ b/src/services/LocalStorageKeystore.ts @@ -181,9 +181,6 @@ export function useLocalStorageKeystore(): LocalStorageKeystore { { exportedMainKey, privateData }: UnlockSuccess, user: CachedUser | UserData | null, ): Promise => { - setMainKey(exportedMainKey); - setPrivateData(privateData); - if (user) { const userHandleB64u = ("prfKeys" in user ? user.userHandleB64u @@ -199,13 +196,21 @@ export function useLocalStorageKeystore(): LocalStorageKeystore { ); setUserHandleB64u(userHandleB64u); + + // This must happen before setPrivateData in order to prevent the + // useEffect updating cachedUsers from corrupting cache entries for other + // users logged in in other tabs. setGlobalUserHandleB64u(userHandleB64u); + setCachedUsers((cachedUsers) => { // Move most recently used user to front of list const otherUsers = (cachedUsers || []).filter((cu) => cu.userHandleB64u !== newUser.userHandleB64u); return [newUser, ...otherUsers]; }); } + + setMainKey(exportedMainKey); + setPrivateData(privateData); }; const init = async ( From e122daf2f168abbe129d433a15de00d9f8ab8287 Mon Sep 17 00:00:00 2001 From: Emil Lundberg Date: Tue, 3 Dec 2024 12:35:31 +0100 Subject: [PATCH 2/4] Extract named logout function --- src/context/SessionContext.tsx | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/context/SessionContext.tsx b/src/context/SessionContext.tsx index a2eed4e22..0ec5c08a4 100644 --- a/src/context/SessionContext.tsx +++ b/src/context/SessionContext.tsx @@ -25,18 +25,18 @@ export const SessionContextProvider = ({ children }) => { const api = useApi(isOnline); const keystore = useLocalStorageKeystore(); + const logout = async () => { + // Clear URL parameters + sessionStorage.setItem('freshLogin', 'true'); + api.clearSession(); + await keystore.close(); + }; + const value: SessionContextValue = { api, isLoggedIn: api.isLoggedIn() && keystore.isOpen(), keystore, - logout: async () => { - - // Clear URL parameters - sessionStorage.setItem('freshLogin', 'true'); - api.clearSession(); - await keystore.close(); - - }, + logout, }; return ( From 9cc6dac5f7bae516dd640fa87f15d9558823b05f Mon Sep 17 00:00:00 2001 From: Emil Lundberg Date: Tue, 3 Dec 2024 18:12:43 +0100 Subject: [PATCH 3/4] Send keystore.close event to log out api module too --- src/context/SessionContext.tsx | 7 +++++-- src/services/LocalStorageKeystore.ts | 11 ++++++++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/context/SessionContext.tsx b/src/context/SessionContext.tsx index 0ec5c08a4..225002967 100644 --- a/src/context/SessionContext.tsx +++ b/src/context/SessionContext.tsx @@ -2,7 +2,7 @@ import React, { createContext, useContext } from 'react'; import StatusContext from './StatusContext'; import { BackendApi, useApi } from '../api'; -import { useLocalStorageKeystore } from '../services/LocalStorageKeystore'; +import { KeystoreEvent, useLocalStorageKeystore } from '../services/LocalStorageKeystore'; import type { LocalStorageKeystore } from '../services/LocalStorageKeystore'; @@ -23,7 +23,8 @@ const SessionContext: React.Context = createContext({ export const SessionContextProvider = ({ children }) => { const { isOnline } = useContext(StatusContext); const api = useApi(isOnline); - const keystore = useLocalStorageKeystore(); + const keystoreEvents = new EventTarget(); + const keystore = useLocalStorageKeystore(keystoreEvents); const logout = async () => { // Clear URL parameters @@ -32,6 +33,8 @@ export const SessionContextProvider = ({ children }) => { await keystore.close(); }; + keystoreEvents.addEventListener(KeystoreEvent.Close, logout, { once: true }); + const value: SessionContextValue = { api, isLoggedIn: api.isLoggedIn() && keystore.isOpen(), diff --git a/src/services/LocalStorageKeystore.ts b/src/services/LocalStorageKeystore.ts index 11865682f..46715f83f 100644 --- a/src/services/LocalStorageKeystore.ts +++ b/src/services/LocalStorageKeystore.ts @@ -27,6 +27,13 @@ export type CachedUser = { prfKeys: WebauthnPrfSaltInfo[]; } +export enum KeystoreEvent { + /** The keystore has been closed. This event should be propagated to all tabs. */ + Close = 'keystore.close', + /** The keystore has been closed. This event should not be propagated to other tabs. */ + CloseTabLocal = 'keystore.closeTabLocal', +} + export type CommitCallback = () => Promise; export interface LocalStorageKeystore { isOpen(): boolean, @@ -75,7 +82,7 @@ export interface LocalStorageKeystore { } /** A stateful wrapper around the keystore module, storing state in the browser's localStorage and sessionStorage. */ -export function useLocalStorageKeystore(): LocalStorageKeystore { +export function useLocalStorageKeystore(eventTarget: EventTarget): LocalStorageKeystore { const [cachedUsers, setCachedUsers,] = useLocalStorage("cachedUsers", []); const [privateData, setPrivateData, clearPrivateData] = useLocalStorage("privateData", null); const [globalUserHandleB64u, setGlobalUserHandleB64u, clearGlobalUserHandleB64u] = useLocalStorage("userHandle", null); @@ -97,6 +104,7 @@ export function useLocalStorageKeystore(): LocalStorageKeystore { const closeTabLocal = useCallback( () => { clearSessionStorage(); + eventTarget.dispatchEvent(new CustomEvent(KeystoreEvent.CloseTabLocal)); }, [clearSessionStorage], ); @@ -107,6 +115,7 @@ export function useLocalStorageKeystore(): LocalStorageKeystore { clearPrivateData(); clearGlobalUserHandleB64u(); closeTabLocal(); + eventTarget.dispatchEvent(new CustomEvent(KeystoreEvent.Close)); }, [closeTabLocal, idb, clearGlobalUserHandleB64u, clearPrivateData], ); From 95b8bedfdbeac8b8594b46dc576121fc5db96d0c Mon Sep 17 00:00:00 2001 From: Emil Lundberg Date: Fri, 6 Dec 2024 13:36:42 +0100 Subject: [PATCH 4/4] Log out of api module on KeystoreEvent.CloseTabLocal --- src/context/SessionContext.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/context/SessionContext.tsx b/src/context/SessionContext.tsx index 225002967..7fece4101 100644 --- a/src/context/SessionContext.tsx +++ b/src/context/SessionContext.tsx @@ -34,6 +34,7 @@ export const SessionContextProvider = ({ children }) => { }; keystoreEvents.addEventListener(KeystoreEvent.Close, logout, { once: true }); + keystoreEvents.addEventListener(KeystoreEvent.CloseTabLocal, api.clearSession, { once: true }); const value: SessionContextValue = { api,