From 4dfbd3f3cd4253962fbd690b5aeca5b2e82e81da Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 21 Sep 2020 17:53:29 +0200 Subject: [PATCH 1/4] don't run afterSyncCompleted and next sync request in parallel as the otk count the next sync request reports will be outdated if afterSyncCompleted uploaded OTKs, and the next afterSyncCompleted , having the wrong server OTK count, will again upload OTKs. This will overwrite existing OTK keys which will throw BAD_MESSAGE_KEY_ID when creating new sessions with those OTKs --- src/matrix/Sync.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/matrix/Sync.js b/src/matrix/Sync.js index c520003b..cffa8682 100644 --- a/src/matrix/Sync.js +++ b/src/matrix/Sync.js @@ -93,14 +93,13 @@ export class Sync { } async _syncLoop(syncToken) { - let afterSyncCompletedPromise = Promise.resolve(); // if syncToken is falsy, it will first do an initial sync ... while(this._status.get() !== SyncStatus.Stopped) { let roomStates; try { console.log(`starting sync request with since ${syncToken} ...`); const timeout = syncToken ? INCREMENTAL_TIMEOUT : undefined; - const syncResult = await this._syncRequest(syncToken, timeout, afterSyncCompletedPromise); + const syncResult = await this._syncRequest(syncToken, timeout); syncToken = syncResult.syncToken; roomStates = syncResult.roomStates; this._status.set(SyncStatus.Syncing); @@ -113,7 +112,7 @@ export class Sync { } } if (this._status.get() !== SyncStatus.Stopped) { - afterSyncCompletedPromise = this._runAfterSyncCompleted(roomStates); + await this._runAfterSyncCompleted(roomStates); } } } @@ -144,7 +143,7 @@ export class Sync { await Promise.all(roomsPromises.concat(sessionPromise)); } - async _syncRequest(syncToken, timeout, prevAfterSyncCompletedPromise) { + async _syncRequest(syncToken, timeout) { let {syncFilterId} = this._session; if (typeof syncFilterId !== "string") { this._currentRequest = this._hsApi.createFilter(this._session.user.id, {room: {state: {lazy_load_members: true}}}); @@ -153,9 +152,6 @@ export class Sync { const totalRequestTimeout = timeout + (80 * 1000); // same as riot-web, don't get stuck on wedged long requests this._currentRequest = this._hsApi.sync(syncToken, syncFilterId, timeout, {timeout: totalRequestTimeout}); const response = await this._currentRequest.response(); - // wait here for the afterSyncCompleted step of the previous sync to complete - // before we continue processing this sync response - await prevAfterSyncCompletedPromise; const isInitialSync = !syncToken; syncToken = response.next_batch; From 015c6b1c70e14795a6531958ff87366465f4c105 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 21 Sep 2020 17:56:23 +0200 Subject: [PATCH 2/4] interpret unreported signed_curve25519 as 0 OTKs --- src/matrix/Session.js | 2 +- src/matrix/e2ee/Account.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/matrix/Session.js b/src/matrix/Session.js index 36a9b866..efdc5904 100644 --- a/src/matrix/Session.js +++ b/src/matrix/Session.js @@ -384,7 +384,7 @@ export class Session { // sync transaction succeeded, modify object state now this._syncInfo = syncInfo; } - if (this._e2eeAccount && e2eeAccountChanges) { + if (this._e2eeAccount) { this._e2eeAccount.afterSync(e2eeAccountChanges); } } diff --git a/src/matrix/e2ee/Account.js b/src/matrix/e2ee/Account.js index 37fab7d2..c0cfd8de 100644 --- a/src/matrix/e2ee/Account.js +++ b/src/matrix/e2ee/Account.js @@ -171,7 +171,7 @@ export class Account { writeSync(deviceOneTimeKeysCount, txn) { // we only upload signed_curve25519 otks - const otkCount = deviceOneTimeKeysCount.signed_curve25519; + const otkCount = deviceOneTimeKeysCount.signed_curve25519 || 0; if (Number.isSafeInteger(otkCount) && otkCount !== this._serverOTKCount) { txn.session.set(SERVER_OTK_COUNT_SESSION_KEY, otkCount); return otkCount; From c9ee5a5db27b513cf2c8d0e846107bf5cc4ac2a4 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 21 Sep 2020 17:57:01 +0200 Subject: [PATCH 3/4] stay in catchup mode as long as there are device messages this implements https://github.com/vector-im/element-web/issues/2782 it also implements 0 timeout for catchup, getting rid of the catching up with your convo banner for 30s upon reconnection. --- src/matrix/Session.js | 14 ++++++++++---- src/matrix/Sync.js | 30 ++++++++++++++++++++++++++---- 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/src/matrix/Session.js b/src/matrix/Session.js index efdc5904..2bd93136 100644 --- a/src/matrix/Session.js +++ b/src/matrix/Session.js @@ -389,11 +389,17 @@ export class Session { } } - async afterSyncCompleted() { - const needsToUploadOTKs = await this._e2eeAccount.generateOTKsIfNeeded(this._storage); + async afterSyncCompleted(isCatchupSync) { const promises = [this._deviceMessageHandler.decryptPending(this.rooms)]; - if (needsToUploadOTKs) { - promises.push(this._e2eeAccount.uploadKeys(this._storage)); + // we don't start uploading one-time keys until we've caught up with + // to-device messages, to help us avoid throwing away one-time-keys that we + // are about to receive messages for + // (https://github.com/vector-im/riot-web/issues/2782). + if (!isCatchupSync) { + const needsToUploadOTKs = await this._e2eeAccount.generateOTKsIfNeeded(this._storage); + if (needsToUploadOTKs) { + promises.push(this._e2eeAccount.uploadKeys(this._storage)); + } } // run key upload and decryption in parallel await Promise.all(promises); diff --git a/src/matrix/Sync.js b/src/matrix/Sync.js index cffa8682..dc169deb 100644 --- a/src/matrix/Sync.js +++ b/src/matrix/Sync.js @@ -98,11 +98,27 @@ export class Sync { let roomStates; try { console.log(`starting sync request with since ${syncToken} ...`); - const timeout = syncToken ? INCREMENTAL_TIMEOUT : undefined; + // unless we are happily syncing already, we want the server to return + // as quickly as possible, even if there are no events queued. This + // serves two purposes: + // + // * When the connection dies, we want to know asap when it comes back, + // so that we can hide the error from the user. (We don't want to + // have to wait for an event or a timeout). + // + // * We want to know if the server has any to_device messages queued up + // for us. We do that by calling it with a zero timeout until it + // doesn't give us any more to_device messages. + const timeout = this._status.get() === SyncStatus.Syncing ? INCREMENTAL_TIMEOUT : 0; const syncResult = await this._syncRequest(syncToken, timeout); syncToken = syncResult.syncToken; roomStates = syncResult.roomStates; - this._status.set(SyncStatus.Syncing); + // initial sync or catchup sync + if (this._status.get() !== SyncStatus.Syncing && syncResult.hadToDeviceMessages) { + this._status.set(SyncStatus.CatchupSync); + } else { + this._status.set(SyncStatus.Syncing); + } } catch (err) { if (!(err instanceof AbortError)) { console.warn("stopping sync because of error"); @@ -118,9 +134,10 @@ export class Sync { } async _runAfterSyncCompleted(roomStates) { + const isCatchupSync = this._status.get() === SyncStatus.CatchupSync; const sessionPromise = (async () => { try { - await this._session.afterSyncCompleted(); + await this._session.afterSyncCompleted(isCatchupSync); } catch (err) { console.error("error during session afterSyncCompleted, continuing", err.stack); } @@ -186,7 +203,12 @@ export class Sync { rs.room.afterSync(rs.changes); } - return {syncToken, roomStates}; + const toDeviceEvents = response.to_device?.events; + return { + syncToken, + roomStates, + hadToDeviceMessages: Array.isArray(toDeviceEvents) && toDeviceEvents.length > 0, + }; } async _openPrepareSyncTxn() { From e6a46874c40e0828b7882fd4d90f87e006e1d8d0 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 21 Sep 2020 17:58:13 +0200 Subject: [PATCH 4/4] wrap olm error for creating session in DecryptionError so we can relate it back to the event that caused it --- src/matrix/e2ee/olm/Decryption.js | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/matrix/e2ee/olm/Decryption.js b/src/matrix/e2ee/olm/Decryption.js index fc16852a..a193e016 100644 --- a/src/matrix/e2ee/olm/Decryption.js +++ b/src/matrix/e2ee/olm/Decryption.js @@ -115,7 +115,12 @@ export class Decryption { } // could not decrypt with any existing session if (typeof plaintext !== "string" && isPreKeyMessage(message)) { - const createResult = this._createSessionAndDecrypt(senderKey, message, timestamp); + let createResult; + try { + createResult = this._createSessionAndDecrypt(senderKey, message, timestamp); + } catch (error) { + throw new DecryptionError(`Could not create inbound olm session: ${error.message}`, event, {senderKey, error}); + } senderKeyDecryption.addNewSession(createResult.session); plaintext = createResult.plaintext; } @@ -123,8 +128,8 @@ export class Decryption { let payload; try { payload = JSON.parse(plaintext); - } catch (err) { - throw new DecryptionError("PLAINTEXT_NOT_JSON", event, {plaintext, err}); + } catch (error) { + throw new DecryptionError("PLAINTEXT_NOT_JSON", event, {plaintext, error}); } this._validatePayload(payload, event); return new DecryptionResult(payload, senderKey, payload.keys);