Skip to content

Commit

Permalink
Refactoring and simplification in decryption error handling (#4138)
Browse files Browse the repository at this point in the history 
* Clean up decryption failure integ tests

* Fix the names
* Stop waiting as soon as the event is decrypted, even if code is wrong (so
  tests fail rather than time out if the code is wrong)

* Bump timeouts on some tests

These tend to fail due to slow init of wasm artifacts

* Factor out `onDecryptionKeyMissingError` call

* Factor out `onMegolmDecryptionError`
  • Loading branch information
@richvdh
richvdh authored Apr 2, 2024
1 parent cfcd191 commit dbab185
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 110 deletions.
52 changes: 28 additions & 24 deletions spec/integ/crypto/cross-signing.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,33 +81,37 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("cross-signing (%s)", (backend: s
};
}

beforeEach(async () => {
// anything that we don't have a specific matcher for silently returns a 404
fetchMock.catch(404);
fetchMock.config.warnOnFallback = false;

const homeserverUrl = "https://alice-server.com";
aliceClient = createClient({
baseUrl: homeserverUrl,
userId: TEST_USER_ID,
accessToken: "akjgkrgjs",
deviceId: TEST_DEVICE_ID,
cryptoCallbacks: createCryptoCallbacks(),
});
beforeEach(
async () => {
// anything that we don't have a specific matcher for silently returns a 404
fetchMock.catch(404);
fetchMock.config.warnOnFallback = false;

const homeserverUrl = "https://alice-server.com";
aliceClient = createClient({
baseUrl: homeserverUrl,
userId: TEST_USER_ID,
accessToken: "akjgkrgjs",
deviceId: TEST_DEVICE_ID,
cryptoCallbacks: createCryptoCallbacks(),
});

syncResponder = new SyncResponder(homeserverUrl);
e2eKeyResponder = new E2EKeyResponder(homeserverUrl);
/** an object which intercepts `/keys/upload` requests on the test homeserver */
new E2EKeyReceiver(homeserverUrl);
syncResponder = new SyncResponder(homeserverUrl);
e2eKeyResponder = new E2EKeyResponder(homeserverUrl);
/** an object which intercepts `/keys/upload` requests on the test homeserver */
new E2EKeyReceiver(homeserverUrl);

// Silence warnings from the backup manager
fetchMock.getOnce(new URL("/_matrix/client/v3/room_keys/version", homeserverUrl).toString(), {
status: 404,
body: { errcode: "M_NOT_FOUND" },
});
// Silence warnings from the backup manager
fetchMock.getOnce(new URL("/_matrix/client/v3/room_keys/version", homeserverUrl).toString(), {
status: 404,
body: { errcode: "M_NOT_FOUND" },
});

await initCrypto(aliceClient);
});
await initCrypto(aliceClient);
},
/* it can take a while to initialise the crypto library on the first pass, so bump up the timeout. */
10000,
);

afterEach(async () => {
await aliceClient.stopClient();
Expand Down
33 changes: 12 additions & 21 deletions spec/integ/crypto/crypto.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import * as testUtils from "../../test-utils/test-utils";
import {
advanceTimersUntil,
CRYPTO_BACKENDS,
emitPromise,
getSyncResponse,
InitCrypto,
mkEventCustom,
Expand Down Expand Up @@ -466,19 +467,13 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
});

describe("Unable to decrypt error codes", function () {
it("Encryption fails with expected UISI error", async () => {
it("Decryption fails with UISI error", async () => {
expectAliceKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} });
await startClientAndAwaitFirstSync();

const awaitUISI = new Promise<void>((resolve) => {
aliceClient.on(MatrixEventEvent.Decrypted, (ev) => {
if (ev.decryptionFailureReason === DecryptionFailureCode.MEGOLM_UNKNOWN_INBOUND_SESSION_ID) {
resolve();
}
});
});
// A promise which resolves, with the MatrixEvent which wraps the event, once the decryption fails.
const awaitDecryption = emitPromise(aliceClient, MatrixEventEvent.Decrypted);

// Alice gets both the events in a single sync
const syncResponse = {
next_batch: 1,
rooms: {
Expand All @@ -490,21 +485,16 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,

syncResponder.sendOrQueueSyncResponse(syncResponse);
await syncPromise(aliceClient);

await awaitUISI;
const ev = await awaitDecryption;
expect(ev.decryptionFailureReason).toEqual(DecryptionFailureCode.MEGOLM_UNKNOWN_INBOUND_SESSION_ID);
});

it("Encryption fails with expected Unknown Index error", async () => {
it("Decryption fails with Unknown Index error", async () => {
expectAliceKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} });
await startClientAndAwaitFirstSync();

const awaitUnknownIndex = new Promise<void>((resolve) => {
aliceClient.on(MatrixEventEvent.Decrypted, (ev) => {
if (ev.decryptionFailureReason === DecryptionFailureCode.OLM_UNKNOWN_MESSAGE_INDEX) {
resolve();
}
});
});
// A promise which resolves, with the MatrixEvent which wraps the event, once the decryption fails.
const awaitDecryption = emitPromise(aliceClient, MatrixEventEvent.Decrypted);

await aliceClient.getCrypto()!.importRoomKeys([testData.RATCHTED_MEGOLM_SESSION_DATA]);

Expand All @@ -521,10 +511,11 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
syncResponder.sendOrQueueSyncResponse(syncResponse);
await syncPromise(aliceClient);

await awaitUnknownIndex;
const ev = await awaitDecryption;
expect(ev.decryptionFailureReason).toEqual(DecryptionFailureCode.OLM_UNKNOWN_MESSAGE_INDEX);
});

it("Encryption fails with Unable to decrypt for other errors", async () => {
it("Decryption fails with Unable to decrypt for other errors", async () => {
expectAliceKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} });
await startClientAndAwaitFirstSync();

Expand Down
45 changes: 24 additions & 21 deletions spec/integ/crypto/megolm-backup.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,28 +192,31 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("megolm-keys backup (%s)", (backe
}
}

beforeEach(async () => {
fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA);

// ignore requests to send room key requests
fetchMock.put("express:/_matrix/client/v3/sendToDevice/m.room_key_request/:request_id", {});

aliceClient = await initTestClient();
const aliceCrypto = aliceClient.getCrypto()!;
await aliceCrypto.storeSessionBackupPrivateKey(
Buffer.from(testData.BACKUP_DECRYPTION_KEY_BASE64, "base64"),
testData.SIGNED_BACKUP_DATA.version!,
);

// start after saving the private key
await aliceClient.startClient();
beforeEach(
async () => {
fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA);

// ignore requests to send room key requests
fetchMock.put("express:/_matrix/client/v3/sendToDevice/m.room_key_request/:request_id", {});

aliceClient = await initTestClient();
const aliceCrypto = aliceClient.getCrypto()!;
await aliceCrypto.storeSessionBackupPrivateKey(
Buffer.from(testData.BACKUP_DECRYPTION_KEY_BASE64, "base64"),
testData.SIGNED_BACKUP_DATA.version!,
);

// tell Alice to trust the dummy device that signed the backup, and re-check the backup.
// XXX: should we automatically re-check after a device becomes verified?
await waitForDeviceList();
await aliceClient.getCrypto()!.setDeviceVerified(testData.TEST_USER_ID, testData.TEST_DEVICE_ID);
await aliceClient.getCrypto()!.checkKeyBackupAndEnable();
});
// start after saving the private key
await aliceClient.startClient();

// tell Alice to trust the dummy device that signed the backup, and re-check the backup.
// XXX: should we automatically re-check after a device becomes verified?
await waitForDeviceList();
await aliceClient.getCrypto()!.setDeviceVerified(testData.TEST_USER_ID, testData.TEST_DEVICE_ID);
await aliceClient.getCrypto()!.checkKeyBackupAndEnable();
} /* it can take a while to initialise the crypto library on the first pass, so bump up the timeout. */,
10000,
);

it("Alice checks key backups when receiving a message she can't decrypt", async () => {
fetchMock.get("express:/_matrix/client/v3/room_keys/keys/:room_id/:session_id", (url, request) => {
Expand Down
86 changes: 42 additions & 44 deletions src/rust-crypto/rust-crypto.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1683,52 +1683,50 @@ class EventDecryptor {
forwardingCurve25519KeyChain: res.forwardingCurve25519KeyChain,
};
} catch (err) {
// We need to map back to regular decryption errors (used for analytics for example)
// The DecryptionErrors are used by react-sdk so is implicitly part of API, but poorly typed
if (err instanceof RustSdkCryptoJs.MegolmDecryptionError) {
const content = event.getWireContent();
let jsError;
switch (err.code) {
case RustSdkCryptoJs.DecryptionErrorCode.MissingRoomKey: {
jsError = new DecryptionError(
DecryptionFailureCode.MEGOLM_UNKNOWN_INBOUND_SESSION_ID,
"The sender's device has not sent us the keys for this message.",
{
session: content.sender_key + "|" + content.session_id,
},
);
this.perSessionBackupDownloader.onDecryptionKeyMissingError(
event.getRoomId()!,
event.getWireContent().session_id!,
);
break;
}
case RustSdkCryptoJs.DecryptionErrorCode.UnknownMessageIndex: {
jsError = new DecryptionError(
DecryptionFailureCode.OLM_UNKNOWN_MESSAGE_INDEX,
"The sender's device has not sent us the keys for this message at this index.",
{
session: content.sender_key + "|" + content.session_id,
},
);
this.perSessionBackupDownloader.onDecryptionKeyMissingError(
event.getRoomId()!,
event.getWireContent().session_id!,
);
break;
}
// We don't map MismatchedIdentityKeys for now, as there is no equivalent in legacy.
// Just put it on the `UNKNOWN_ERROR` bucket.
default: {
jsError = new DecryptionError(DecryptionFailureCode.UNKNOWN_ERROR, err.description, {
session: content.sender_key + "|" + content.session_id,
});
break;
}
}
throw jsError;
this.onMegolmDecryptionError(event, err);
} else {
throw new DecryptionError(DecryptionFailureCode.UNKNOWN_ERROR, "Unknown error");
}
throw new DecryptionError(DecryptionFailureCode.UNKNOWN_ERROR, "Unknown error");
}
}

/**
* Handle a `MegolmDecryptionError` returned by the rust SDK.
*
* Fires off a request to the `perSessionBackupDownloader`, if appropriate, and then throws a `DecryptionError`.
*/
private onMegolmDecryptionError(event: MatrixEvent, err: RustSdkCryptoJs.MegolmDecryptionError): never {
const content = event.getWireContent();

// If the error looks like it might be recoverable from backup, queue up a request to try that.
if (
err.code === RustSdkCryptoJs.DecryptionErrorCode.MissingRoomKey ||
err.code === RustSdkCryptoJs.DecryptionErrorCode.UnknownMessageIndex
) {
this.perSessionBackupDownloader.onDecryptionKeyMissingError(event.getRoomId()!, content.session_id!);
}

const errorDetails = { session: content.sender_key + "|" + content.session_id };
switch (err.code) {
case RustSdkCryptoJs.DecryptionErrorCode.MissingRoomKey:
throw new DecryptionError(
DecryptionFailureCode.MEGOLM_UNKNOWN_INBOUND_SESSION_ID,
"The sender's device has not sent us the keys for this message.",
errorDetails,
);

case RustSdkCryptoJs.DecryptionErrorCode.UnknownMessageIndex:
throw new DecryptionError(
DecryptionFailureCode.OLM_UNKNOWN_MESSAGE_INDEX,
"The sender's device has not sent us the keys for this message at this index.",
errorDetails,
);

// We don't map MismatchedIdentityKeys for now, as there is no equivalent in legacy.
// Just put it on the `UNKNOWN_ERROR` bucket.
default:
throw new DecryptionError(DecryptionFailureCode.UNKNOWN_ERROR, err.description, errorDetails);
}
}

Expand Down

0 comments on commit dbab185

Please sign in to comment.