Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
Only report undecryptable events once (#12501)
Browse files Browse the repository at this point in the history 
* persist previously-reported event IDs as a bloom filter

* Pin to older `@types/seedrandom`

... to work around Callidon/bloom-filters#72

* Inline `DecryptionFailureTracker.addDecryptionFailure`

* Remove redundant TRACK_INTERVAL

There really doesn't seem to be much point to this batching up of decryption
failure reports. We still call the analytics callback the same number of times.

* Rename `trackedEvents` to `reportedEvents`

* Fix incorrect documentation on `visibleEvents`

This *does* overlap with `failures`.

* Combine `addFailure` and `reportFailure`

* Calculate client properties before starting reporting

* Clear localstorage after each test

... otherwise they interfere

* Remove redundant comment

* Ensure that reports are cleared on a logout/login cycle

* make private const private and const

---------

Co-authored-by: Richard van der Hoff <[email protected]>
  • Loading branch information
@uhoreg @richvdh
uhoreg and richvdh authored May 20, 2024
1 parent 3e10394 commit 1bb70c5
Show file tree
Hide file tree
Showing 5 changed files with 197 additions and 73 deletions.
2 changes: 2 additions & 0 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
"@zxcvbn-ts/language-common": "^3.0.4",
"@zxcvbn-ts/language-en": "^3.0.2",
"await-lock": "^2.1.0",
"bloom-filters": "^3.0.1",
"blurhash": "^2.0.3",
"classnames": "^2.2.6",
"commonmark": "^0.31.0",
Expand Down Expand Up @@ -182,6 +183,7 @@
"@types/react-transition-group": "^4.4.0",
"@types/sanitize-html": "2.11.0",
"@types/sdp-transform": "^2.4.6",
"@types/seedrandom": "<3.0.5",
"@types/tar-js": "^0.3.2",
"@types/ua-parser-js": "^0.7.36",
"@types/uuid": "^9.0.2",
Expand Down
30 changes: 19 additions & 11 deletions src/DecryptionFailureTracker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,16 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { ScalableBloomFilter } from "bloom-filters";
import { CryptoEvent, HttpApiEvent, MatrixClient, MatrixEventEvent, MatrixEvent } from "matrix-js-sdk/src/matrix";
import { Error as ErrorEvent } from "@matrix-org/analytics-events/types/typescript/Error";
import { DecryptionFailureCode } from "matrix-js-sdk/src/crypto-api";

import { PosthogAnalytics } from "./PosthogAnalytics";

/** The key that we use to store the `reportedEvents` bloom filter in localstorage */
const DECRYPTION_FAILURE_STORAGE_KEY = "mx_decryption_failure_event_ids";

export class DecryptionFailure {
/**
* The time between our initial failure to decrypt and our successful
Expand Down Expand Up @@ -104,8 +108,8 @@ export class DecryptionFailureTracker {
*/
public visibleEvents: Set<string> = new Set();

/** Event IDs of failures that were reported previously */
private reportedEvents: Set<string> = new Set();
/** Bloom filter tracking event IDs of failures that were reported previously */
private reportedEvents: ScalableBloomFilter = new ScalableBloomFilter();

/** Set to an interval ID when `start` is called */
public checkInterval: number | null = null;
Expand Down Expand Up @@ -172,13 +176,18 @@ export class DecryptionFailureTracker {
return DecryptionFailureTracker.internalInstance;
}

// loadReportedEvents() {
// this.reportedEvents = new Set(JSON.parse(localStorage.getItem('mx-decryption-failure-event-ids')) || []);
// }
private loadReportedEvents(): void {
const storedFailures = localStorage.getItem(DECRYPTION_FAILURE_STORAGE_KEY);
if (storedFailures) {
this.reportedEvents = ScalableBloomFilter.fromJSON(JSON.parse(storedFailures));
} else {
this.reportedEvents = new ScalableBloomFilter();
}
}

// saveReportedEvents() {
// localStorage.setItem('mx-decryption-failure-event-ids', JSON.stringify([...this.reportedEvents]));
// }
private saveReportedEvents(): void {
localStorage.setItem(DECRYPTION_FAILURE_STORAGE_KEY, JSON.stringify(this.reportedEvents.saveAsJSON()));
}

/** Callback for when an event is decrypted.
*
Expand Down Expand Up @@ -290,6 +299,7 @@ export class DecryptionFailureTracker {
* Start checking for and tracking failures.
*/
public async start(client: MatrixClient): Promise<void> {
this.loadReportedEvents();
await this.calculateClientProperties(client);
this.registerHandlers(client);
this.checkInterval = window.setInterval(
Expand Down Expand Up @@ -385,9 +395,7 @@ export class DecryptionFailureTracker {
}
this.failures = failuresNotReady;

// Commented out for now for expediency, we need to consider unbound nature of storing
// this in localStorage
// this.saveReportedEvents();
this.saveReportedEvents();
}

/**
Expand Down
14 changes: 5 additions & 9 deletions src/components/structures/MatrixChat.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import {
MatrixClient,
MatrixEvent,
RoomType,
SyncStateData,
SyncState,
SyncStateData,
TimelineEvents,
} from "matrix-js-sdk/src/matrix";
import { defer, IDeferred, QueryDict } from "matrix-js-sdk/src/utils";
Expand Down Expand Up @@ -128,7 +128,7 @@ import { TimelineRenderingType } from "../../contexts/RoomContext";
import { UseCaseSelection } from "../views/elements/UseCaseSelection";
import { ValidatedServerConfig } from "../../utils/ValidatedServerConfig";
import { isLocalRoom } from "../../utils/localRoom/isLocalRoom";
import { SdkContextClass, SDKContext } from "../../contexts/SDKContext";
import { SDKContext, SdkContextClass } from "../../contexts/SDKContext";
import { viewUserDeviceSettings } from "../../actions/handlers/viewUserDeviceSettings";
import { cleanUpBroadcasts, VoiceBroadcastResumer } from "../../voice-broadcast";
import GenericToast from "../views/toasts/GenericToast";
Expand Down Expand Up @@ -1585,13 +1585,9 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
);
});

const dft = DecryptionFailureTracker.instance;

// Shelved for later date when we have time to think about persisting history of
// tracked events across sessions.
// dft.loadTrackedEventHashMap();

dft.start(cli).catch((e) => logger.error("Unable to start DecryptionFailureTracker", e));
DecryptionFailureTracker.instance
.start(cli)
.catch((e) => logger.error("Unable to start DecryptionFailureTracker", e));

cli.on(ClientEvent.Room, (room) => {
if (cli.isCryptoEnabled()) {
Expand Down
124 changes: 100 additions & 24 deletions test/DecryptionFailureTracker-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,15 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { mocked, Mocked } from "jest-mock";
import { CryptoEvent, HttpApiEvent, MatrixEvent, MatrixEventEvent } from "matrix-js-sdk/src/matrix";
import { mocked, Mocked, MockedObject } from "jest-mock";
import { CryptoEvent, HttpApiEvent, MatrixClient, MatrixEvent, MatrixEventEvent } from "matrix-js-sdk/src/matrix";
import { decryptExistingEvent, mkDecryptionFailureMatrixEvent } from "matrix-js-sdk/src/testing";
import { CryptoApi, DecryptionFailureCode, UserVerificationStatus } from "matrix-js-sdk/src/crypto-api";
import { sleep } from "matrix-js-sdk/src/utils";

import { DecryptionFailureTracker, ErrorProperties } from "../src/DecryptionFailureTracker";
import { stubClient } from "./test-utils";
import * as Lifecycle from "../src/Lifecycle";

async function createFailedDecryptionEvent(opts: { sender?: string; code?: DecryptionFailureCode } = {}) {
return await mkDecryptionFailureMatrixEvent({
Expand All @@ -39,6 +40,10 @@ function eventDecrypted(tracker: DecryptionFailureTracker, e: MatrixEvent, nowTs
}

describe("DecryptionFailureTracker", function () {
afterEach(() => {
localStorage.clear();
});

it("tracks a failed decryption for a visible event", async function () {
const failedDecryptionEvent = await createFailedDecryptionEvent();

Expand Down Expand Up @@ -247,6 +252,7 @@ describe("DecryptionFailureTracker", function () {
() => count++,
() => "UnknownError",
);
await tracker.start(mockClient());

tracker.addVisibleEvent(decryptedEvent);

Expand All @@ -264,10 +270,7 @@ describe("DecryptionFailureTracker", function () {
expect(count).toBe(1);
});

it.skip("should not track a failure for an event that was tracked in a previous session", async () => {
// This test uses localStorage, clear it beforehand
localStorage.clear();

it("should not report a failure for an event that was reported in a previous session", async () => {
const decryptedEvent = await createFailedDecryptionEvent();

let count = 0;
Expand All @@ -276,6 +279,7 @@ describe("DecryptionFailureTracker", function () {
() => count++,
() => "UnknownError",
);
await tracker.start(mockClient());

tracker.addVisibleEvent(decryptedEvent);

Expand All @@ -289,21 +293,84 @@ describe("DecryptionFailureTracker", function () {
// Simulate the browser refreshing by destroying tracker and creating a new tracker
// @ts-ignore access to private constructor
const secondTracker = new DecryptionFailureTracker(
(total: number) => (count += total),
() => count++,
() => "UnknownError",
);
await secondTracker.start(mockClient());

secondTracker.addVisibleEvent(decryptedEvent);

//secondTracker.loadTrackedEvents();

eventDecrypted(secondTracker, decryptedEvent, Date.now());
secondTracker.checkFailures(Infinity);

// should only track a single failure per event
expect(count).toBe(1);
});

it("should report a failure for an event that was tracked but not reported in a previous session", async () => {
const decryptedEvent = await createFailedDecryptionEvent();

let count = 0;

// @ts-ignore access to private constructor
const tracker = new DecryptionFailureTracker(
() => count++,
() => "UnknownError",
);
await tracker.start(mockClient());

tracker.addVisibleEvent(decryptedEvent);

// Indicate decryption
eventDecrypted(tracker, decryptedEvent, Date.now());

// we do *not* call `checkFailures` here
expect(count).toBe(0);

// Simulate the browser refreshing by destroying tracker and creating a new tracker
// @ts-ignore access to private constructor
const secondTracker = new DecryptionFailureTracker(
() => count++,
() => "UnknownError",
);
await secondTracker.start(mockClient());

secondTracker.addVisibleEvent(decryptedEvent);

eventDecrypted(secondTracker, decryptedEvent, Date.now());
secondTracker.checkFailures(Infinity);
expect(count).toBe(1);
});

it("should report a failure for an event that was reported before a logout/login cycle", async () => {
const decryptedEvent = await createFailedDecryptionEvent();

let count = 0;

// @ts-ignore access to private constructor
const tracker = new DecryptionFailureTracker(
() => count++,
() => "UnknownError",
);
await tracker.start(mockClient());

tracker.addVisibleEvent(decryptedEvent);

// Indicate decryption
eventDecrypted(tracker, decryptedEvent, Date.now());
tracker.checkFailures(Infinity);
expect(count).toBe(1);

// Simulate a logout/login cycle
await Lifecycle.onLoggedOut();
await tracker.start(mockClient());

tracker.addVisibleEvent(decryptedEvent);
eventDecrypted(tracker, decryptedEvent, Date.now());
tracker.checkFailures(Infinity);
expect(count).toBe(2);
});

it("should count different error codes separately for multiple failures with different error codes", async () => {
const counts: Record<string, number> = {};

Expand Down Expand Up @@ -521,12 +588,7 @@ describe("DecryptionFailureTracker", function () {
it("listens for client events", async () => {
// Test that the decryption failure tracker registers the right event
// handlers on start, and unregisters them when the client logs out.
const client = mocked(stubClient());
const mockCrypto = {
getVersion: jest.fn().mockReturnValue("Rust SDK 0.7.0 (61b175b), Vodozemac 0.5.1"),
getUserVerificationStatus: jest.fn().mockResolvedValue(new UserVerificationStatus(false, false, false)),
} as unknown as Mocked<CryptoApi>;
client.getCrypto.mockReturnValue(mockCrypto);
const client = mockClient();

let errorCount: number = 0;
// @ts-ignore access to private constructor
Expand Down Expand Up @@ -568,13 +630,7 @@ describe("DecryptionFailureTracker", function () {
});

it("tracks client information", async () => {
const client = mocked(stubClient());
const mockCrypto = {
getVersion: jest.fn().mockReturnValue("Rust SDK 0.7.0 (61b175b), Vodozemac 0.5.1"),
getUserVerificationStatus: jest.fn().mockResolvedValue(new UserVerificationStatus(false, false, false)),
} as unknown as Mocked<CryptoApi>;
client.getCrypto.mockReturnValue(mockCrypto);

const client = mockClient();
const propertiesByErrorCode: Record<string, ErrorProperties> = {};
// @ts-ignore access to private constructor
const tracker = new DecryptionFailureTracker(
Expand Down Expand Up @@ -610,7 +666,9 @@ describe("DecryptionFailureTracker", function () {
const now = Date.now();
eventDecrypted(tracker, federatedDecryption, now);

mockCrypto.getUserVerificationStatus.mockResolvedValue(new UserVerificationStatus(true, true, false));
mocked(client.getCrypto()!.getUserVerificationStatus).mockResolvedValue(
new UserVerificationStatus(true, true, false),
);
client.emit(CryptoEvent.KeysChanged, {});
await sleep(100);
eventDecrypted(tracker, localDecryption, now);
Expand All @@ -628,7 +686,7 @@ describe("DecryptionFailureTracker", function () {

// change client params, and make sure the reports the right values
client.getDomain.mockReturnValue("example.com");
mockCrypto.getVersion.mockReturnValue("Olm 0.0.0");
mocked(client.getCrypto()!.getVersion).mockReturnValue("Olm 0.0.0");
// @ts-ignore access to private method
await tracker.calculateClientProperties(client);

Expand Down Expand Up @@ -673,3 +731,21 @@ describe("DecryptionFailureTracker", function () {
expect(failure?.timeToDecryptMillis).toEqual(50000);
});
});

function mockClient(): MockedObject<MatrixClient> {
const client = mocked(stubClient());
const mockCrypto = {
getVersion: jest.fn().mockReturnValue("Rust SDK 0.7.0 (61b175b), Vodozemac 0.5.1"),
getUserVerificationStatus: jest.fn().mockResolvedValue(new UserVerificationStatus(false, false, false)),
} as unknown as Mocked<CryptoApi>;
client.getCrypto.mockReturnValue(mockCrypto);

// @ts-ignore
client.stopClient = jest.fn(() => {});
// @ts-ignore
client.removeAllListeners = jest.fn(() => {});

client.store = { destroy: jest.fn(() => {}) } as any;

return client;
}
Loading

0 comments on commit 1bb70c5

Please sign in to comment.