Process `m.room.encryption` events before emitting `RoomMember` events #2914

richvdh · 2022-11-28T14:04:49Z

The root cause of element-hq/element-web#23819 is as follows:

In matrix-react-sdk, ensureDMExists tries to create an encrypted DM room, and assumes it is ready for use (including sending encrypted events) as soon as it receives a RoomStateEvent.NewMember notification indicating that the other user has been invited or joined.
However, in sync.ts, we process the membership events in a /sync response (including emitting RoomStateEvent.NewMember notifications) here, which is long before we process any m.room.encryption event (here).
The upshot is that we can end up trying to send an encrypted event in the new room before processing the m.room.encryption event, which causes the crypto layer to blow up with the error in Verification fails to start (intermittently) when there is no existing DM with the target user element-hq/element-web#23819.

Strictly speaking, ensureDMExists probably ought to be listening for ClientEvent.Room as well as RoomStateEvent.NewMember; but that doesn't help us, because ClientEvent.Room is also emitted before we process the crypto event.

So, we need to process the crypto event before we start emitting these other events; but a corollary of that is that we need to do so before we store the new room in the client's store. That makes things tricky, because currently the crypto layer expects the room to have been stored in the client first.

So... we have to rearrange everything to pass the newly-created Room object into the crypto layer, rather than just the room id, so that it doesn't need to rely on getting the Room from the client's store.

I've tried to structure the commits in this PR in a way to make them easy to review.

(This is another go at #2910).

🚨 BREAKING CHANGES

Process m.room.encryption events before emitting RoomMember events (#2914). Fixes Verification fails to start (intermittently) when there is no existing DM with the target user element-hq/element-web#23819.

Passing in a `Room` means that we can set up encryption before the room is added to the client

we already have the room ehre, so may as well call `trackRoomDevicesImpl` directly.

This fixes the problematic race condition.

richvdh · 2022-11-29T18:59:33Z

src/crypto/index.ts

     */
-    public async onCryptoEvent(event: MatrixEvent): Promise<void> {
-        const roomId = event.getRoomId()!;
+    public async onCryptoEvent(room: Room, event: MatrixEvent): Promise<void> {


strictly speaking, this is a breaking change, because this method is accessible outside the js-sdk. But it's really not intended to be called anywhere except internally.

If we really want I could add a new onCryptoEventWithARealRoom method, use that from sync.ts etc, and deprecate the existing method. But it feels to me like that would be adding a bunch of dead code for very little gain.

richvdh · 2022-11-29T19:04:25Z

spec/integ/megolm-integ.spec.ts

+        // it probably won't be decrypted yet, because it takes a while to process the olm keys
+        const decryptedEvent = await testUtils.awaitDecryption(event, { waitOnDecryptionFailure: true });


this is needed due to timing differences in processing /sync responses: there are fewer awaits, which means that we finish processing the /sync while there are still to-device messages in the queue for processing.

richvdh · 2022-11-29T19:05:29Z

spec/unit/crypto.spec.ts

@@ -1140,4 +1141,48 @@ describe("Crypto", function() {
            await client!.client.crypto!.start();
        });
    });
+
+    describe("setRoomEncryption", () => {


this is really just to keep the coverage metric high. It's testing the now-essentially-unused Crypto.setRoomEncryption.

weeman1337

Thanks for all the comments. Helped me to review this one.

Looks good to me. 👍 Also tested a bit using the dummy PR deployment. Everything seems to be working. Did some verifications (without having a DM). All succeeded.

weeman1337 · 2022-11-30T09:48:29Z

src/crypto/index.ts

@@ -2844,7 +2883,7 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
            delete content['io.element.performance_metrics'];
        }

-        const encryptedContent = await alg.encryptMessage(room, event.getType(), content);
+        const encryptedContent = await alg.encryptMessage(room, event.getType(), content) as IContent;


Not sure about this cast. encryptMessage returns an object. That is all we know. Wouldn't it be better to update the encryptMessage definition instead?

yes, but honestly I've not touched this code and I'm sick to the back teeth of trying to land this fix so I just wanted to get something that passed the typechecker.

@GoodGuyMarco

* Process `m.room.encryption` events before emitting `RoomMember` events ([\matrix-org#2914](matrix-org#2914)). Fixes element-hq/element-web#23819. * Don't expose `calls` on `GroupCall` ([\matrix-org#2941](matrix-org#2941)). * Support MSC3391: Account data deletion ([\matrix-org#2967](matrix-org#2967)). * Add a message ID on each to-device message ([\matrix-org#2938](matrix-org#2938)). * Enable multiple users' power levels to be set at once ([\matrix-org#2892](matrix-org#2892)). Contributed by @GoodGuyMarco. * Include pending events in thread summary and count again ([\matrix-org#2922](matrix-org#2922)). Fixes element-hq/element-web#23642. * Make GroupCall work better with widgets ([\matrix-org#2935](matrix-org#2935)). * Add method to get outgoing room key requests for a given event ([\matrix-org#2930](matrix-org#2930)). * Fix messages loaded during initial fetch ending up out of order ([\matrix-org#2971](matrix-org#2971)). Fixes element-hq/element-web#23972. * Fix #23919: Root message for new thread loaded from network ([\matrix-org#2965](matrix-org#2965)). Fixes element-hq/element-web#23919. * Fix #23916: Prevent edits of the last message in a thread getting lost ([\matrix-org#2951](matrix-org#2951)). Fixes element-hq/element-web#23916 and element-hq/element-web#23942. * Fix infinite loop when restoring cached read receipts ([\matrix-org#2963](matrix-org#2963)). Fixes element-hq/element-web#23951. * Don't swallow errors coming from the shareSession call ([\matrix-org#2962](matrix-org#2962)). Fixes element-hq/element-web#23792. * Make sure that MegolmEncryption.setupPromise always resolves ([\matrix-org#2960](matrix-org#2960)). * Do not calculate highlight notifs for threads unknown to the room ([\matrix-org#2957](matrix-org#2957)). * Cache read receipts for unknown threads ([\matrix-org#2953](matrix-org#2953)). * bugfix: sliding sync initial room timelines shouldn't notify ([\matrix-org#2933](matrix-org#2933)). * Redo key sharing after own device verification ([\matrix-org#2921](matrix-org#2921)). Fixes element-hq/element-web#23333. * Move updated threads to the end of the thread list ([\matrix-org#2923](matrix-org#2923)). Fixes element-hq/element-web#23876. * Fix highlight notifications increasing when total notification is zero ([\matrix-org#2937](matrix-org#2937)). Fixes element-hq/element-web#23885. * Fix synthesizeReceipt ([\matrix-org#2916](matrix-org#2916)). Fixes element-hq/element-web#23827 element-hq/element-web#23754 and element-hq/element-web#23847.

github-actions bot deployed to PR Documentation Preview November 28, 2022 14:12 View deployment

richvdh added the T-Defect label Nov 28, 2022

richvdh force-pushed the rav/fix_e2e_fail_in_new_room branch 6 times, most recently from a3a0c40 to 1bb5c67 Compare November 29, 2022 16:47

github-actions bot deployed to PR Documentation Preview November 29, 2022 16:50 View deployment

github-actions bot deployed to PR Documentation Preview November 29, 2022 18:09 View deployment

richvdh added 8 commits November 29, 2022 18:41

Pass a room parameter into Crypto.onCryptoEvent

e2aefa2

Factor out setRoomEncryptionImpl which takes a Room

dc0d751

Passing in a `Room` means that we can set up encryption before the room is added to the client

Factor out trackRoomDevicesImpl which takes a Room

248d244

Inline a call to trackRoomDevices

5656b22

we already have the room ehre, so may as well call `trackRoomDevicesImpl` directly.

Call onCryptoEvent before processing state events

4928d33

This fixes the problematic race condition.

Add a new test

7c41145

lint

9d20db1

Add a test for setRoomEncryption

f660a6f

richvdh force-pushed the rav/fix_e2e_fail_in_new_room branch from c5e68fe to f660a6f Compare November 29, 2022 18:41

github-actions bot deployed to PR Documentation Preview November 29, 2022 18:44 View deployment

richvdh mentioned this pull request Nov 29, 2022

Dummy PR to test https://github.com/matrix-org/matrix-js-sdk/pull/2914 matrix-org/matrix-react-sdk#9650

Closed

richvdh commented Nov 29, 2022

View reviewed changes

richvdh marked this pull request as ready for review November 29, 2022 19:13

richvdh requested a review from a team as a code owner November 29, 2022 19:13

richvdh requested review from kerryarchibald and weeman1337 November 29, 2022 19:13

weeman1337 added the X-Breaking-Change label Nov 30, 2022

weeman1337 approved these changes Nov 30, 2022

View reviewed changes

richvdh merged commit 1606274 into develop Nov 30, 2022

richvdh deleted the rav/fix_e2e_fail_in_new_room branch November 30, 2022 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process `m.room.encryption` events before emitting `RoomMember` events #2914

Process `m.room.encryption` events before emitting `RoomMember` events #2914

richvdh commented Nov 28, 2022 •

edited by github-actions bot

Loading

richvdh Nov 29, 2022

richvdh Nov 29, 2022

richvdh Nov 29, 2022

weeman1337 left a comment

weeman1337 Nov 30, 2022

richvdh Nov 30, 2022

		// it probably won't be decrypted yet, because it takes a while to process the olm keys
		const decryptedEvent = await testUtils.awaitDecryption(event, { waitOnDecryptionFailure: true });

Process m.room.encryption events before emitting RoomMember events #2914

Process m.room.encryption events before emitting RoomMember events #2914

Conversation

richvdh commented Nov 28, 2022 • edited by github-actions bot Loading

🚨 BREAKING CHANGES

richvdh Nov 29, 2022

Choose a reason for hiding this comment

richvdh Nov 29, 2022

Choose a reason for hiding this comment

richvdh Nov 29, 2022

Choose a reason for hiding this comment

weeman1337 left a comment

Choose a reason for hiding this comment

weeman1337 Nov 30, 2022

Choose a reason for hiding this comment

richvdh Nov 30, 2022

Choose a reason for hiding this comment

Process `m.room.encryption` events before emitting `RoomMember` events #2914

Process `m.room.encryption` events before emitting `RoomMember` events #2914

richvdh commented Nov 28, 2022 •

edited by github-actions bot

Loading