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.)

Author: richvdh

src/Lifecycle.ts

@@ -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,

src/components/structures/MatrixChat.tsx

@@ -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;
         }

src/utils/tokens/tokens.ts

@@ -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,