From 642058d936de488d1e086c24e0644f6651760e26 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 11 Sep 2024 13:03:54 +0100 Subject: [PATCH 1/3] tokens.ts: improve documentation Improve variable naming and documentation on the methods in `tokens.ts`. --- src/utils/tokens/tokens.ts | 70 ++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 26 deletions(-) diff --git a/src/utils/tokens/tokens.ts b/src/utils/tokens/tokens.ts index f526775e63e..b5cb8d95cba 100644 --- a/src/utils/tokens/tokens.ts +++ b/src/utils/tokens/tokens.ts @@ -24,13 +24,13 @@ import * as StorageAccess from "../StorageAccess"; */ /* - * Keys used when storing the tokens in indexeddb or localstorage + * Names used when storing the tokens in indexeddb or localstorage */ export const ACCESS_TOKEN_STORAGE_KEY = "mx_access_token"; export const REFRESH_TOKEN_STORAGE_KEY = "mx_refresh_token"; /* - * Used as initialization vector during encryption in persistTokenInStorage - * And decryption in restoreFromLocalStorage + * Names of the tokens. Used as part of the calculation to derive AES keys during encryption in persistTokenInStorage, + * and decryption in restoreFromLocalStorage. */ export const ACCESS_TOKEN_IV = "access_token"; export const REFRESH_TOKEN_IV = "refresh_token"; @@ -71,23 +71,31 @@ async function pickleKeyToAesKey(pickleKey: string): Promise { const isEncryptedPayload = (token?: IEncryptedPayload | string | undefined): token is IEncryptedPayload => { return !!token && typeof token !== "string"; }; + /** * Try to decrypt a token retrieved from storage - * Where token is not encrypted (plain text) returns the plain text token - * Where token is encrypted, attempts decryption. Returns successfully decrypted token, else undefined. - * @param pickleKey pickle key used during encryption of token, or undefined - * @param token - * @param tokenIv initialization vector used during encryption of token eg ACCESS_TOKEN_IV + * + * Where token is not encrypted (plain text) returns the plain text token. + * + * Where token is encrypted, attempts decryption. Returns successfully decrypted token, or throws if + * decryption failed. + * + * @param pickleKey Pickle key: used to derive the encryption key, or undefined if the token is not encrypted. + * Must be the same as provided to {@link persistTokenInStorage}. + * @param token token to be decrypted. + * @param tokenName Name of the token. Used in logging, but also used as an input when generating the actual AES key, + * so the same value must be provided to {@link persistTokenInStorage}. + * * @returns the decrypted token, or the plain text token. Returns undefined when token cannot be decrypted */ export async function tryDecryptToken( pickleKey: string | undefined, token: IEncryptedPayload | string | undefined, - tokenIv: string, + tokenName: string, ): Promise { if (pickleKey && isEncryptedPayload(token)) { const encrKey = await pickleKeyToAesKey(pickleKey); - const decryptedToken = await decryptAES(token, encrKey, tokenIv); + const decryptedToken = await decryptAES(token, encrKey, tokenName); encrKey.fill(0); return decryptedToken; } @@ -100,18 +108,24 @@ export async function tryDecryptToken( /** * Persist a token in storage - * When pickle key is present, will attempt to encrypt the token - * Stores in idb, falling back to localStorage * - * @param storageKey key used to store the token - * @param initializationVector Initialization vector for encrypting the token. Only used when `pickleKey` is present - * @param token the token to store, when undefined any existing token at the storageKey is removed from storage - * @param pickleKey optional pickle key used to encrypt token - * @param hasTokenStorageKey Localstorage key for an item which stores whether we expect to have a token in indexeddb, eg "mx_has_access_token". + * When pickle key is present, will attempt to encrypt the token. If encryption fails (typically because + * WebCrypto is unavailable), the key will be stored unencrypted. + * + * Stores in IndexedDB, falling back to localStorage. + * + * @param storageKey key used to store the token. Note: not an encryption key; rather a localstorage or indexeddb key. + * @param tokenName Name of the token. Used in logging, but also used as an input when generating the actual AES key, + * so the same value must be provided to {@link tryDecryptToken} when decrypting. + * @param token the token to store. When undefined, any existing token at the `storageKey` is removed from storage. + * @param pickleKey Pickle key: used to derive the key used to encrypt token. If `undefined`, the token will be stored + * unencrypted. + * @param hasTokenStorageKey Localstorage key for an item which stores whether we expect to have a token in indexeddb, + * eg "mx_has_access_token". */ export async function persistTokenInStorage( storageKey: string, - initializationVector: string, + tokenName: string, token: string | undefined, pickleKey: string | undefined, hasTokenStorageKey: string, @@ -130,12 +144,12 @@ export async function persistTokenInStorage( try { // try to encrypt the access token using the pickle key const encrKey = await pickleKeyToAesKey(pickleKey); - encryptedToken = await encryptAES(token, encrKey, initializationVector); + encryptedToken = await encryptAES(token, encrKey, tokenName); encrKey.fill(0); } catch (e) { // This is likely due to the browser not having WebCrypto or somesuch. // Warn about it, but fall back to storing the unencrypted token. - logger.warn(`Could not encrypt token for ${storageKey}`, e); + logger.warn(`Could not encrypt token for ${tokenName}`, e); } } try { @@ -167,9 +181,11 @@ export async function persistTokenInStorage( } /** - * Wraps persistTokenInStorage with accessToken storage keys - * @param token the token to store, when undefined any existing accessToken is removed from storage - * @param pickleKey optional pickle key used to encrypt token + * Wraps {@link persistTokenInStorage} with accessToken storage keys + * + * @param token - The token to store. When undefined, any existing accessToken is removed from storage. + * @param pickleKey - Pickle key: used to derive the key used to encrypt token. If `undefined`, the token will be stored + * unencrypted. */ export async function persistAccessTokenInStorage( token: string | undefined, @@ -185,9 +201,11 @@ export async function persistAccessTokenInStorage( } /** - * Wraps persistTokenInStorage with refreshToken storage keys - * @param token the token to store, when undefined any existing refreshToken is removed from storage - * @param pickleKey optional pickle key used to encrypt token + * Wraps {@link persistTokenInStorage} with refreshToken storage keys. + * + * @param token - The token to store. When undefined, any existing refreshToken is removed from storage. + * @param pickleKey - Pickle key: used to derive the key used to encrypt token. If `undefined`, the token will be stored + * unencrypted. */ export async function persistRefreshTokenInStorage( token: string | undefined, From b13655b5c31beeb7c1d1ffc916f600bfd5de7985 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 11 Sep 2024 13:42:22 +0100 Subject: [PATCH 2/3] rename restoreFromLocalStorage Since the session data isn't actually stored in localstorage, this feels like a misleading name. --- src/Lifecycle.ts | 26 ++++++++------- src/components/structures/MatrixChat.tsx | 2 +- src/utils/tokens/tokens.ts | 2 +- test/Lifecycle-test.ts | 42 ++++++++++++------------ 4 files changed, 37 insertions(+), 35 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index 38852c68fba..0575d2163f8 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -211,7 +211,7 @@ export async function loadSession(opts: ILoadSessionOpts = {}): Promise false, ).then(() => true); } - const success = await restoreFromLocalStorage({ + const success = await restoreSessionFromStorage({ ignoreGuest: Boolean(opts.ignoreGuest), }); if (success) { @@ -556,17 +556,19 @@ async function abortLogin(): Promise { } } -// returns a promise which resolves to true if a session is found in -// localstorage -// -// N.B. Lifecycle.js should not maintain any further localStorage state, we -// are moving towards using SessionStore to keep track of state related -// to the current session (which is typically backed by localStorage). -// -// The plan is to gradually move the localStorage access done here into -// SessionStore to avoid bugs where the view becomes out-of-sync with -// localStorage (e.g. isGuest etc.) -export async function restoreFromLocalStorage(opts?: { ignoreGuest?: boolean }): Promise { +/** Attempt to restore the session from localStorage or indexeddb. + * + * @returns true if a session was found; false if no existing session was found. + * + * N.B. Lifecycle.js should not maintain any further localStorage state, we + * are moving towards using SessionStore to keep track of state related + * to the current session (which is typically backed by localStorage). + * + * The plan is to gradually move the localStorage access done here into + * SessionStore to avoid bugs where the view becomes out-of-sync with + * localStorage (e.g. isGuest etc.) + */ +export async function restoreSessionFromStorage(opts?: { ignoreGuest?: boolean }): Promise { const ignoreGuest = opts?.ignoreGuest; if (!localStorage) { diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index 53514ab817b..53d4c96acaf 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -375,7 +375,7 @@ export default class MatrixChat extends React.PureComponent { // Create and start the client // accesses the new credentials just set in storage during attemptDelegatedAuthLogin // and sets logged in state - await Lifecycle.restoreFromLocalStorage({ ignoreGuest: true }); + await Lifecycle.restoreSessionFromStorage({ ignoreGuest: true }); await this.postLoginSetup(); return; } diff --git a/src/utils/tokens/tokens.ts b/src/utils/tokens/tokens.ts index b5cb8d95cba..b6634171e07 100644 --- a/src/utils/tokens/tokens.ts +++ b/src/utils/tokens/tokens.ts @@ -30,7 +30,7 @@ export const ACCESS_TOKEN_STORAGE_KEY = "mx_access_token"; export const REFRESH_TOKEN_STORAGE_KEY = "mx_refresh_token"; /* * Names of the tokens. Used as part of the calculation to derive AES keys during encryption in persistTokenInStorage, - * and decryption in restoreFromLocalStorage. + * and decryption in restoreSessionFromStorage. */ export const ACCESS_TOKEN_IV = "access_token"; export const REFRESH_TOKEN_IV = "refresh_token"; diff --git a/test/Lifecycle-test.ts b/test/Lifecycle-test.ts index f9146630f9b..b0d044c10e0 100644 --- a/test/Lifecycle-test.ts +++ b/test/Lifecycle-test.ts @@ -23,7 +23,7 @@ import { mocked, MockedObject } from "jest-mock"; import fetchMock from "fetch-mock-jest"; import StorageEvictedDialog from "../src/components/views/dialogs/StorageEvictedDialog"; -import { logout, restoreFromLocalStorage, setLoggedIn } from "../src/Lifecycle"; +import { logout, restoreSessionFromStorage, setLoggedIn } from "../src/Lifecycle"; import { MatrixClientPeg } from "../src/MatrixClientPeg"; import Modal from "../src/Modal"; import * as StorageAccess from "../src/utils/StorageAccess"; @@ -180,7 +180,7 @@ describe("Lifecycle", () => { mac: expect.any(String), }; - describe("restoreFromLocalStorage()", () => { + describe("restoreSessionFromStorage()", () => { beforeEach(() => { initLocalStorageMock(); initSessionStorageMock(); @@ -204,18 +204,18 @@ describe("Lifecycle", () => { // @ts-ignore dirty mocking global.localStorage = undefined; - expect(await restoreFromLocalStorage()).toEqual(false); + expect(await restoreSessionFromStorage()).toEqual(false); }); it("should return false when no session data is found in local storage", async () => { - expect(await restoreFromLocalStorage()).toEqual(false); + expect(await restoreSessionFromStorage()).toEqual(false); expect(logger.log).toHaveBeenCalledWith("No previous session found."); }); it("should abort login when we expect to find an access token but don't", async () => { initLocalStorageMock({ mx_has_access_token: "true" }); - await expect(() => restoreFromLocalStorage()).rejects.toThrow(); + await expect(() => restoreSessionFromStorage()).rejects.toThrow(); expect(Modal.createDialog).toHaveBeenCalledWith(StorageEvictedDialog); expect(mockClient.clearStores).toHaveBeenCalled(); }); @@ -228,12 +228,12 @@ describe("Lifecycle", () => { }); it("should ignore guest accounts when ignoreGuest is true", async () => { - expect(await restoreFromLocalStorage({ ignoreGuest: true })).toEqual(false); + expect(await restoreSessionFromStorage({ ignoreGuest: true })).toEqual(false); expect(logger.log).toHaveBeenCalledWith(`Ignoring stored guest account: ${userId}`); }); it("should restore guest accounts when ignoreGuest is false", async () => { - expect(await restoreFromLocalStorage({ ignoreGuest: false })).toEqual(true); + expect(await restoreSessionFromStorage({ ignoreGuest: false })).toEqual(true); expect(MatrixClientPeg.replaceUsingCreds).toHaveBeenCalledWith( expect.objectContaining({ @@ -253,7 +253,7 @@ describe("Lifecycle", () => { }); it("should persist credentials", async () => { - expect(await restoreFromLocalStorage()).toEqual(true); + expect(await restoreSessionFromStorage()).toEqual(true); expect(localStorage.setItem).toHaveBeenCalledWith("mx_user_id", userId); expect(localStorage.setItem).toHaveBeenCalledWith("mx_has_access_token", "true"); @@ -267,7 +267,7 @@ describe("Lifecycle", () => { it("should persist access token when idb is not available", async () => { jest.spyOn(StorageAccess, "idbSave").mockRejectedValue("oups"); - expect(await restoreFromLocalStorage()).toEqual(true); + expect(await restoreSessionFromStorage()).toEqual(true); expect(StorageAccess.idbSave).toHaveBeenCalledWith("account", "mx_access_token", accessToken); // put accessToken in localstorage as fallback @@ -275,7 +275,7 @@ describe("Lifecycle", () => { }); it("should create and start new matrix client with credentials", async () => { - expect(await restoreFromLocalStorage()).toEqual(true); + expect(await restoreSessionFromStorage()).toEqual(true); expect(MatrixClientPeg.replaceUsingCreds).toHaveBeenCalledWith( { @@ -295,13 +295,13 @@ describe("Lifecycle", () => { }); it("should remove fresh login flag from session storage", async () => { - expect(await restoreFromLocalStorage()).toEqual(true); + expect(await restoreSessionFromStorage()).toEqual(true); expect(sessionStorage.removeItem).toHaveBeenCalledWith("mx_fresh_login"); }); it("should start matrix client", async () => { - expect(await restoreFromLocalStorage()).toEqual(true); + expect(await restoreSessionFromStorage()).toEqual(true); expect(MatrixClientPeg.start).toHaveBeenCalled(); }); @@ -316,7 +316,7 @@ describe("Lifecycle", () => { }); it("should persist credentials", async () => { - expect(await restoreFromLocalStorage()).toEqual(true); + expect(await restoreSessionFromStorage()).toEqual(true); // refresh token from storage is re-persisted expect(localStorage.setItem).toHaveBeenCalledWith("mx_has_refresh_token", "true"); @@ -324,7 +324,7 @@ describe("Lifecycle", () => { }); it("should create new matrix client with credentials", async () => { - expect(await restoreFromLocalStorage()).toEqual(true); + expect(await restoreSessionFromStorage()).toEqual(true); expect(MatrixClientPeg.replaceUsingCreds).toHaveBeenCalledWith( { @@ -362,7 +362,7 @@ describe("Lifecycle", () => { }); it("should persist credentials", async () => { - expect(await restoreFromLocalStorage()).toEqual(true); + expect(await restoreSessionFromStorage()).toEqual(true); expect(localStorage.setItem).toHaveBeenCalledWith("mx_has_access_token", "true"); @@ -384,7 +384,7 @@ describe("Lifecycle", () => { }, ); - expect(await restoreFromLocalStorage()).toEqual(true); + expect(await restoreSessionFromStorage()).toEqual(true); expect(StorageAccess.idbSave).toHaveBeenCalledWith( "account", @@ -403,7 +403,7 @@ describe("Lifecycle", () => { }); // Perform the restore - expect(await restoreFromLocalStorage()).toEqual(true); + expect(await restoreSessionFromStorage()).toEqual(true); // Ensure that the expected calls were made expect(MatrixClientPeg.replaceUsingCreds).toHaveBeenCalledWith( @@ -430,7 +430,7 @@ describe("Lifecycle", () => { }); it("should persist credentials", async () => { - expect(await restoreFromLocalStorage()).toEqual(true); + expect(await restoreSessionFromStorage()).toEqual(true); // refresh token from storage is re-persisted expect(localStorage.setItem).toHaveBeenCalledWith("mx_has_refresh_token", "true"); @@ -442,7 +442,7 @@ describe("Lifecycle", () => { }); it("should create new matrix client with credentials", async () => { - expect(await restoreFromLocalStorage()).toEqual(true); + expect(await restoreSessionFromStorage()).toEqual(true); expect(MatrixClientPeg.replaceUsingCreds).toHaveBeenCalledWith( { @@ -492,7 +492,7 @@ describe("Lifecycle", () => { it("should create and start new matrix client with credentials", async () => { // Perform the restore - expect(await restoreFromLocalStorage()).toEqual(true); + expect(await restoreSessionFromStorage()).toEqual(true); // Ensure that the expected calls were made expect(MatrixClientPeg.replaceUsingCreds).toHaveBeenCalledWith( @@ -519,7 +519,7 @@ describe("Lifecycle", () => { initIdbMock(idbStorageSession); mockClient.isVersionSupported.mockRejectedValue(new Error("Oh, noes, the server is down!")); - expect(await restoreFromLocalStorage()).toEqual(true); + expect(await restoreSessionFromStorage()).toEqual(true); }); }); }); From ab50038df8e3eb0014709ec420a764230c1962c9 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 11 Sep 2024 13:36:28 +0100 Subject: [PATCH 3/3] Lifecycle: bail out if picklekey is missing Currently, if we have an accesstoken which is encrypted with a picklekey, but the picklekey has gone missing, we carry on with no access token at all. This is sure to blow up in some way or other later on, but in a rather cryptic way. Instead, let's bail out early. (This will produce a "can't restore session" error, but we normally see one of those anyway because we can't initialise the crypto store.) --- src/Lifecycle.ts | 7 ++++--- src/utils/tokens/tokens.ts | 29 ++++++++++++++--------------- test/Lifecycle-test.ts | 23 ++++++++++++++++++++++- 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index 0575d2163f8..47e44c43dc1 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -592,10 +592,11 @@ export async function restoreSessionFromStorage(opts?: { ignoreGuest?: boolean } if (pickleKey) { logger.log(`Got pickle key for ${userId}|${deviceId}`); } else { - logger.log("No pickle key available"); + logger.log(`No pickle key available for ${userId}|${deviceId}`); } const decryptedAccessToken = await tryDecryptToken(pickleKey, accessToken, ACCESS_TOKEN_IV); - const decryptedRefreshToken = await tryDecryptToken(pickleKey, refreshToken, REFRESH_TOKEN_IV); + const decryptedRefreshToken = + refreshToken && (await tryDecryptToken(pickleKey, refreshToken, REFRESH_TOKEN_IV)); const freshLogin = sessionStorage.getItem("mx_fresh_login") === "true"; sessionStorage.removeItem("mx_fresh_login"); @@ -605,7 +606,7 @@ export async function restoreSessionFromStorage(opts?: { ignoreGuest?: boolean } { userId: userId, deviceId: deviceId, - accessToken: decryptedAccessToken!, + accessToken: decryptedAccessToken, refreshToken: decryptedRefreshToken, homeserverUrl: hsUrl, identityServerUrl: isUrl, diff --git a/src/utils/tokens/tokens.ts b/src/utils/tokens/tokens.ts index b6634171e07..a82259242aa 100644 --- a/src/utils/tokens/tokens.ts +++ b/src/utils/tokens/tokens.ts @@ -68,10 +68,6 @@ async function pickleKeyToAesKey(pickleKey: string): Promise { ); } -const isEncryptedPayload = (token?: IEncryptedPayload | string | undefined): token is IEncryptedPayload => { - return !!token && typeof token !== "string"; -}; - /** * Try to decrypt a token retrieved from storage * @@ -86,24 +82,27 @@ const isEncryptedPayload = (token?: IEncryptedPayload | string | undefined): tok * @param tokenName Name of the token. Used in logging, but also used as an input when generating the actual AES key, * so the same value must be provided to {@link persistTokenInStorage}. * - * @returns the decrypted token, or the plain text token. Returns undefined when token cannot be decrypted + * @returns the decrypted token, or the plain text token. */ export async function tryDecryptToken( pickleKey: string | undefined, - token: IEncryptedPayload | string | undefined, + token: IEncryptedPayload | string, tokenName: string, -): Promise { - if (pickleKey && isEncryptedPayload(token)) { - const encrKey = await pickleKeyToAesKey(pickleKey); - const decryptedToken = await decryptAES(token, encrKey, tokenName); - encrKey.fill(0); - return decryptedToken; - } - // if the token wasn't encrypted (plain string) just return it back +): Promise { if (typeof token === "string") { + // Looks like an unencrypted token return token; } - // otherwise return undefined + + // Otherwise, it must be an encrypted token. + if (!pickleKey) { + throw new Error(`Error decrypting secret ${tokenName}: no pickle key found.`); + } + + const encrKey = await pickleKeyToAesKey(pickleKey); + const decryptedToken = await decryptAES(token, encrKey, tokenName); + encrKey.fill(0); + return decryptedToken; } /** diff --git a/test/Lifecycle-test.ts b/test/Lifecycle-test.ts index b0d044c10e0..6ee6cb11540 100644 --- a/test/Lifecycle-test.ts +++ b/test/Lifecycle-test.ts @@ -145,7 +145,12 @@ describe("Lifecycle", () => { mockStore[tableKey] = table; }, ); - jest.spyOn(StorageAccess, "idbDelete").mockClear().mockResolvedValue(undefined); + jest.spyOn(StorageAccess, "idbDelete") + .mockClear() + .mockImplementation(async (tableKey: string, key: string | string[]) => { + const table = mockStore[tableKey]; + delete table?.[key as string]; + }); }; const homeserverUrl = "https://server.org"; @@ -521,6 +526,22 @@ describe("Lifecycle", () => { expect(await restoreSessionFromStorage()).toEqual(true); }); + + it("should throw if the token was persisted with a pickle key but there is no pickle key available now", async () => { + initLocalStorageMock(localStorageSession); + initIdbMock({}); + + // Create a pickle key, and store it, encrypted, in IDB. + const pickleKey = (await PlatformPeg.get()!.createPickleKey(credentials.userId, credentials.deviceId))!; + localStorage.setItem("mx_has_pickle_key", "true"); + await persistAccessTokenInStorage(credentials.accessToken, pickleKey); + + // Now destroy the pickle key + await PlatformPeg.get()!.destroyPickleKey(credentials.userId, credentials.deviceId); + + console.log("10"); + expect(await restoreSessionFromStorage()).toEqual(true); + }); }); });