Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring and simplification in decryption error handling #4138

Merged
merged 4 commits into from
Apr 2, 2024
Merged

Refactoring and simplification in decryption error handling #4138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
31ba2aa
Clean up decryption failure integ tests
richvdh Mar 26, 2024
6723135
Bump timeouts on some tests
richvdh Mar 26, 2024
7fb5481
Factor out `onDecryptionKeyMissingError` call
richvdh Mar 25, 2024
a3a9a5f
Factor out `onMegolmDecryptionError`
richvdh Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 28 additions & 24 deletions spec/integ/crypto/cross-signing.spec.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading