mirror of
https://github.com/element-hq/element-web
synced 2024-11-22 01:05:42 +03:00
Log clearer errors when picklekey goes missing (#27)
* tokens.ts: improve documentation Improve variable naming and documentation on the methods in `tokens.ts`. * rename restoreFromLocalStorage Since the session data isn't actually stored in localstorage, this feels like a misleading name. * 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.)
This commit is contained in:
Richard van der Hoff
GitHub
parent
d337fba76e
commit
433c14e5a9
4 changed files with 119 additions and 77 deletions
|
|
@ -203,7 +203,7 @@ export async function loadSession(opts: ILoadSessionOpts = {}): Promise<boolean>
|
|||
false,
|
||||
).then(() => true);
|
||||
}
|
||||
const success = await restoreFromLocalStorage({
|
||||
const success = await restoreSessionFromStorage({
|
||||
ignoreGuest: Boolean(opts.ignoreGuest),
|
||||
});
|
||||
if (success) {
|
||||
|
|
@ -548,17 +548,19 @@ async function abortLogin(): Promise<void> {
|
|||
}
|
||||
}
|
||||
|
||||
// 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<boolean> {
|
||||
/** 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<boolean> {
|
||||
const ignoreGuest = opts?.ignoreGuest;
|
||||
|
||||
if (!localStorage) {
|
||||
|
|
@ -582,10 +584,11 @@ export async function restoreFromLocalStorage(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");
|
||||
|
|
@ -595,7 +598,7 @@ export async function restoreFromLocalStorage(opts?: { ignoreGuest?: boolean }):
|
|||
{
|
||||
userId: userId,
|
||||
deviceId: deviceId,
|
||||
accessToken: decryptedAccessToken!,
|
||||
accessToken: decryptedAccessToken,
|
||||
refreshToken: decryptedRefreshToken,
|
||||
homeserverUrl: hsUrl,
|
||||
identityServerUrl: isUrl,
|
||||
|
|
|
|||
|
|
@ -367,7 +367,7 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
|
|||
// 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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -16,13 +16,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 restoreSessionFromStorage.
|
||||
*/
|
||||
export const ACCESS_TOKEN_IV = "access_token";
|
||||
export const REFRESH_TOKEN_IV = "refresh_token";
|
||||
|
|
@ -60,50 +60,63 @@ async function pickleKeyToAesKey(pickleKey: string): Promise<Uint8Array> {
|
|||
);
|
||||
}
|
||||
|
||||
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
|
||||
* @returns the decrypted token, or the plain text token. Returns undefined when token cannot be decrypted
|
||||
*
|
||||
* 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.
|
||||
*/
|
||||
export async function tryDecryptToken(
|
||||
pickleKey: string | undefined,
|
||||
token: IEncryptedPayload | string | undefined,
|
||||
tokenIv: string,
|
||||
): Promise<string | undefined> {
|
||||
if (pickleKey && isEncryptedPayload(token)) {
|
||||
const encrKey = await pickleKeyToAesKey(pickleKey);
|
||||
const decryptedToken = await decryptAES(token, encrKey, tokenIv);
|
||||
encrKey.fill(0);
|
||||
return decryptedToken;
|
||||
}
|
||||
// if the token wasn't encrypted (plain string) just return it back
|
||||
token: IEncryptedPayload | string,
|
||||
tokenName: string,
|
||||
): Promise<string> {
|
||||
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;
|
||||
}
|
||||
|
||||
/**
|
||||
* 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,
|
||||
|
|
@ -122,12 +135,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 {
|
||||
|
|
@ -159,9 +172,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,
|
||||
|
|
@ -177,9 +192,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,
|
||||
|
|
|
|||
|
|
@ -15,7 +15,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";
|
||||
|
|
@ -137,7 +137,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";
|
||||
|
|
@ -172,7 +177,7 @@ describe("Lifecycle", () => {
|
|||
mac: expect.any(String),
|
||||
};
|
||||
|
||||
describe("restoreFromLocalStorage()", () => {
|
||||
describe("restoreSessionFromStorage()", () => {
|
||||
beforeEach(() => {
|
||||
initLocalStorageMock();
|
||||
initSessionStorageMock();
|
||||
|
|
@ -196,18 +201,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();
|
||||
});
|
||||
|
|
@ -220,12 +225,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({
|
||||
|
|
@ -245,7 +250,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");
|
||||
|
|
@ -259,7 +264,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
|
||||
|
|
@ -267,7 +272,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(
|
||||
{
|
||||
|
|
@ -287,13 +292,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();
|
||||
});
|
||||
|
|
@ -308,7 +313,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");
|
||||
|
|
@ -316,7 +321,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(
|
||||
{
|
||||
|
|
@ -354,7 +359,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");
|
||||
|
||||
|
|
@ -376,7 +381,7 @@ describe("Lifecycle", () => {
|
|||
},
|
||||
);
|
||||
|
||||
expect(await restoreFromLocalStorage()).toEqual(true);
|
||||
expect(await restoreSessionFromStorage()).toEqual(true);
|
||||
|
||||
expect(StorageAccess.idbSave).toHaveBeenCalledWith(
|
||||
"account",
|
||||
|
|
@ -395,7 +400,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(
|
||||
|
|
@ -422,7 +427,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");
|
||||
|
|
@ -434,7 +439,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(
|
||||
{
|
||||
|
|
@ -484,7 +489,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(
|
||||
|
|
@ -511,7 +516,24 @@ 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);
|
||||
});
|
||||
|
||||
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);
|
||||
|
||||
await expect(restoreSessionFromStorage()).rejects.toThrow(
|
||||
"Error decrypting secret access_token: no pickle key found.",
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue