From ada401f4c010bc0227add921e2647e751430e624 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Damir=20Jeli=C4=87?= <poljar@termina.org.uk>
Date: Fri, 9 Dec 2022 15:46:33 +0100
Subject: [PATCH] Make sure that MegolmEncryption.setupPromise always resolves 
 (#2960)

ensureOutboundSession uses and modifies the setupPromise of the
MegolmEncryption class. Some comments suggest that setupPromise will
always resolve, in other words it should never contain a promise that
will get rejected.

Other comments also seem to suggest that the return value of
ensureOutboundSession, a promise as well, may fail.

The critical error here is that the promise that gets set as
the next setupPromise, as well as the promise that ensureOutboundSession
returns, is the same promise.

It seems that the intention was for setupPromise to contain a promise
that will always resolve to either `null` or `OutboundSessionInfo`.

We can see that a couple of lines before we set setupPromise to its new
value we construct a promise that logs and discards errors using the
`Promise.catch()` method.

The `Promise.catch()` method does not mutate the promise, instead it
returns a new promise. The intention of the original author might have
been to set the next setupPromise to the promise which `Promise.catch()`
produces.

This patch modifies the updating of setupPromise in the
ensureOutboundSession so that setupPromise discards errors correctly.

Using `>>=` to represent the promise chaining operation, setupPromise is
now updated using the following logic:

    setupPromise = previousSetupPromise >>= setup >>= discardErrors
---
 spec/unit/crypto/algorithms/megolm.spec.ts | 20 +++++++++++++
 src/crypto/algorithms/megolm.ts            | 33 +++++++++++++++++-----
 2 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/spec/unit/crypto/algorithms/megolm.spec.ts b/spec/unit/crypto/algorithms/megolm.spec.ts
index 5b00466d8..2dca6765d 100644
--- a/spec/unit/crypto/algorithms/megolm.spec.ts
+++ b/spec/unit/crypto/algorithms/megolm.spec.ts
@@ -490,6 +490,26 @@ describe("MegolmDecryption", function () {
 
                 expect(mockBaseApis.queueToDevice).not.toHaveBeenCalled();
             });
+
+            it("shouldn't wedge the setup promise if sharing a room key fails", async () => {
+                // @ts-ignore - private field access
+                const initialSetupPromise = await megolmEncryption.setupPromise;
+                expect(initialSetupPromise).toBe(null);
+
+                // @ts-ignore - private field access
+                megolmEncryption.prepareSession = () => {
+                    throw new Error("Can't prepare session");
+                };
+
+                await expect(() =>
+                    // @ts-ignore - private field access
+                    megolmEncryption.ensureOutboundSession(mockRoom, {}, {}, true),
+                ).rejects.toThrow();
+
+                // @ts-ignore - private field access
+                const finalSetupPromise = await megolmEncryption.setupPromise;
+                expect(finalSetupPromise).toBe(null);
+            });
         });
     });
 
diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts
index aed0ab28f..e002ffa3d 100644
--- a/src/crypto/algorithms/megolm.ts
+++ b/src/crypto/algorithms/megolm.ts
@@ -248,6 +248,23 @@ export class MegolmEncryption extends EncryptionAlgorithm {
      * @param singleOlmCreationPhase - Only perform one round of olm
      *     session creation
      *
+     * This method updates the setupPromise field of the class by chaining a new
+     * call on top of the existing promise, and then catching and discarding any
+     * errors that might happen while setting up the outbound group session. This
+     * is done to ensure that `setupPromise` always resolves to `null` or the
+     * `OutboundSessionInfo`.
+     *
+     * Using `>>=` to represent the promise chaining operation, it does the
+     * following:
+     *
+     * ```
+     * setupPromise = previousSetupPromise >>= setup >>= discardErrors
+     * ```
+     *
+     * The initial value for the `setupPromise` is a promise that resolves to
+     * `null`. The forceDiscardSession() resets setupPromise to this initial
+     * promise.
+     *
      * @returns Promise which resolves to the
      *    OutboundSessionInfo when setup is complete.
      */
@@ -278,18 +295,20 @@ export class MegolmEncryption extends EncryptionAlgorithm {
         };
 
         // first wait for the previous share to complete
-        const prom = this.setupPromise.then(setup);
+        const fallible = this.setupPromise.then(setup);
 
-        // Ensure any failures are logged for debugging
-        prom.catch((e) => {
+        // Ensure any failures are logged for debugging and make sure that the
+        // promise chain remains unbroken
+        //
+        // setupPromise resolves to `null` or the `OutboundSessionInfo` whether
+        // or not the share succeeds
+        this.setupPromise = fallible.catch((e) => {
             logger.error(`Failed to setup outbound session in ${this.roomId}`, e);
+            return null;
         });
 
-        // setupPromise resolves to `session` whether or not the share succeeds
-        this.setupPromise = prom;
-
         // but we return a promise which only resolves if the share was successful.
-        return prom;
+        return fallible;
     }
 
     private async prepareSession(