Fix an instance of failed to decrypt error when an in flight /keys/query fails. (#3486)

* Fix an instance of failed to decrypt error

Specifically, when checking the event sender matches who sent us the
session keys we skip waiting for pending device list updates if we
already know who owns the session key.

* Apply suggestions from code review

Co-authored-by: Richard van der Hoff <[email protected]>

* Update src/crypto/algorithms/olm.ts

Co-authored-by: Richard van der Hoff <[email protected]>

* Fix line wrapping

* Update src/crypto/algorithms/olm.ts

* Fix null check

---------

Co-authored-by: Richard van der Hoff <[email protected]>

Author: erikjohnston

src/crypto/algorithms/olm.ts

@@ -194,24 +194,37 @@ class OlmDecryption extends DecryptionAlgorithm {
 
         // check that the device that encrypted the event belongs to the user that the event claims it's from.
         //
-        // To do this, we need to make sure that our device list is up-to-date. If the device is unknown, we can only
-        // assume that the device logged out and accept it anyway. Some event handlers, such as secret sharing, may be
-        // more strict and reject events that come from unknown devices.
+        // If the device is unknown then we check that we don't have any pending key-query requests for the sender. If
+        // after that the device is still unknown, then we can only assume that the device logged out and accept it
+        // anyway. Some event handlers, such as secret sharing, may be more strict and reject events that come from
+        // unknown devices.
         //
         // This is a defence against the following scenario:
         //
         //   * Alice has verified Bob and Mallory.
         //   * Mallory gets control of Alice's server, and sends a megolm session to Alice using her (Mallory's)
         //     senderkey, but claiming to be from Bob.
         //   * Mallory sends more events using that session, claiming to be from Bob.
-        //   * Alice sees that the senderkey is verified (since she verified Mallory) so marks events those
-        //     events as verified even though the sender is forged.
+        //   * Alice sees that the senderkey is verified (since she verified Mallory) so marks events those events as
+        //     verified even though the sender is forged.
         //
         // In practice, it's not clear that the js-sdk would behave that way, so this may be only a defence in depth.
 
-        await this.crypto.deviceList.downloadKeys([event.getSender()!], false);
-        const senderKeyUser = this.crypto.deviceList.getUserByIdentityKey(olmlib.OLM_ALGORITHM, deviceKey);
-        if (senderKeyUser !== event.getSender() && senderKeyUser != undefined) {
+        let senderKeyUser = this.crypto.deviceList.getUserByIdentityKey(olmlib.OLM_ALGORITHM, deviceKey);
+        if (senderKeyUser === undefined || senderKeyUser === null) {
+            // Wait for any pending key query fetches for the user to complete before trying the lookup again.
+            try {
+                await this.crypto.deviceList.downloadKeys([event.getSender()!], false);
+            } catch (e) {
+                throw new DecryptionError("OLM_BAD_SENDER_CHECK_FAILED", "Could not verify sender identity", {
+                    sender: deviceKey,
+                    err: e as Error,
+                });
+            }
+
+            senderKeyUser = this.crypto.deviceList.getUserByIdentityKey(olmlib.OLM_ALGORITHM, deviceKey);
+        }
+        if (senderKeyUser !== event.getSender() && senderKeyUser !== undefined && senderKeyUser !== null) {
             throw new DecryptionError("OLM_BAD_SENDER", "Message claimed to be from " + event.getSender(), {
                 real_sender: senderKeyUser,
             });