From e125599a4770e0ace5217293c498939b4ee00c98 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 23 Jun 2021 17:38:52 +0200 Subject: [PATCH] prevent decryption result getting lost after reaction updates entry --- src/matrix/room/BaseRoom.js | 2 +- src/matrix/room/Room.js | 2 +- src/matrix/room/timeline/Timeline.js | 67 ++++++++++++++----- src/matrix/room/timeline/entries/BaseEntry.js | 2 + .../room/timeline/entries/EventEntry.js | 13 +++- src/observable/list/SortedArray.js | 12 +++- 6 files changed, 77 insertions(+), 21 deletions(-) diff --git a/src/matrix/room/BaseRoom.js b/src/matrix/room/BaseRoom.js index 8df281fd..a1bf7b03 100644 --- a/src/matrix/room/BaseRoom.js +++ b/src/matrix/room/BaseRoom.js @@ -299,7 +299,7 @@ export class BaseRoom extends EventEmitter { if (this._timeline) { // these should not be added if not already there this._timeline.replaceEntries(gapResult.updatedEntries); - this._timeline.addOrReplaceEntries(gapResult.entries); + this._timeline.addEntries(gapResult.entries); } }); } diff --git a/src/matrix/room/Room.js b/src/matrix/room/Room.js index 62b5c3ff..482d167f 100644 --- a/src/matrix/room/Room.js +++ b/src/matrix/room/Room.js @@ -239,7 +239,7 @@ export class Room extends BaseRoom { if (this._timeline) { // these should not be added if not already there this._timeline.replaceEntries(updatedEntries); - this._timeline.addOrReplaceEntries(newEntries); + this._timeline.addEntries(newEntries); } if (this._observedEvents) { this._observedEvents.updateEvents(updatedEntries); diff --git a/src/matrix/room/timeline/Timeline.js b/src/matrix/room/timeline/Timeline.js index 257d2c3b..423643cf 100644 --- a/src/matrix/room/timeline/Timeline.js +++ b/src/matrix/room/timeline/Timeline.js @@ -182,17 +182,11 @@ export class Timeline { return null; } + /** @package */ updateOwnMember(member) { this._ownMember = member; } - replaceEntries(entries) { - this._addLocalRelationsToNewRemoteEntries(entries); - for (const entry of entries) { - this._remoteEntries.update(entry); - } - } - _addLocalRelationsToNewRemoteEntries(entries) { // because it is not safe to iterate a derived observable collection // before it has any subscriptions, we bail out if this isn't @@ -223,8 +217,22 @@ export class Timeline { } } + // used in replaceEntries + static _entryUpdater(existingEntry, entry) { + entry.updateFrom(existingEntry); + return entry; + } + /** @package */ - addOrReplaceEntries(newEntries) { + replaceEntries(entries) { + this._addLocalRelationsToNewRemoteEntries(entries); + for (const entry of entries) { + this._remoteEntries.getAndUpdate(entry, Timeline._entryUpdater); + } + } + + /** @package */ + addEntries(newEntries) { this._addLocalRelationsToNewRemoteEntries(newEntries); this._remoteEntries.setManySorted(newEntries); } @@ -251,7 +259,7 @@ export class Timeline { )); try { const entries = await readerRequest.complete(); - this.addOrReplaceEntries(entries); + this.addEntries(entries); return entries.length < amount; } finally { this._disposables.disposeTracked(readerRequest); @@ -366,13 +374,13 @@ export function tests() { closeCallback: () => {}, fragmentIdComparer, pendingEvents, clock: new MockClock()}); // 1. load timeline await timeline.load(new User(alice), "join", new NullLogItem()); - // 2. test replaceEntries and addOrReplaceEntries don't fail + // 2. test replaceEntries and addEntries don't fail const event1 = withTextBody("hi!", withSender(bob, createEvent("m.room.message", "!abc"))); const entry1 = new EventEntry({event: event1, fragmentId: 1, eventIndex: 1}, fragmentIdComparer); timeline.replaceEntries([entry1]); const event2 = withTextBody("hi bob!", withSender(alice, createEvent("m.room.message", "!def"))); const entry2 = new EventEntry({event: event2, fragmentId: 1, eventIndex: 2}, fragmentIdComparer); - timeline.addOrReplaceEntries([entry2]); + timeline.addEntries([entry2]); // 3. add local relation (redaction) pendingEvents.append(new PendingEvent({data: { roomId, @@ -396,7 +404,7 @@ export function tests() { await timeline.load(new User(bob), "join", new NullLogItem()); timeline.entries.subscribe(new ListObserver()); const event = withTextBody("hi bob!", withSender(alice, createEvent("m.room.message", "!abc"))); - timeline.addOrReplaceEntries([new EventEntry({event, fragmentId: 1, eventIndex: 2}, fragmentIdComparer)]); + timeline.addEntries([new EventEntry({event, fragmentId: 1, eventIndex: 2}, fragmentIdComparer)]); let entry = getIndexFromIterable(timeline.entries, 0); // 2. add local reaction pendingEvents.append(new PendingEvent({data: { @@ -467,7 +475,7 @@ export function tests() { await timeline.load(new User(bob), "join", new NullLogItem()); timeline.entries.subscribe(new ListObserver()); // 3. add message to timeline - timeline.addOrReplaceEntries([messageEntry]); + timeline.addEntries([messageEntry]); const entry = getIndexFromIterable(timeline.entries, 0); assert.equal(entry, messageEntry); assert.equal(entry.annotations["👋"].count, 1); @@ -488,7 +496,7 @@ export function tests() { event: withContent(createAnnotation(messageEntry.id, "👋"), createEvent("m.reaction", "!def", bob)), fragmentId: 1, eventIndex: 3, roomId }, fragmentIdComparer); - timeline.addOrReplaceEntries([messageEntry, reactionEntry]); + timeline.addEntries([messageEntry, reactionEntry]); // 3. redact reaction pendingEvents.append(new PendingEvent({data: { roomId, @@ -521,7 +529,7 @@ export function tests() { }})); await poll(() => timeline.entries.length === 1); // 3. add remote reaction target - timeline.addOrReplaceEntries([messageEntry]); + timeline.addEntries([messageEntry]); await poll(() => timeline.entries.length === 2); const entry = getIndexFromIterable(timeline.entries, 0); assert.equal(entry, messageEntry); @@ -555,7 +563,7 @@ export function tests() { }})); await poll(() => timeline.entries.length === 1); // 4. add reaction target - timeline.addOrReplaceEntries([new EventEntry({ + timeline.addEntries([new EventEntry({ event: withTextBody("hi bob!", createEvent("m.room.message", messageId, alice)), fragmentId: 1, eventIndex: 2}, fragmentIdComparer) ]); @@ -564,5 +572,32 @@ export function tests() { const entry = getIndexFromIterable(timeline.entries, 0); assert.equal(entry.pendingAnnotations.get("👋"), -1); }, + "decrypted entry preserves content when receiving other update without decryption": async assert => { + // 1. create encrypted and decrypted entry + const encryptedEntry = new EventEntry({ + event: withContent({ciphertext: "abc"}, createEvent("m.room.encrypted", "!abc", alice)), + fragmentId: 1, eventIndex: 1, roomId + }, fragmentIdComparer); + const decryptedEntry = encryptedEntry.clone(); + decryptedEntry.setDecryptionResult({ + event: withTextBody("hi bob!", createEvent("m.room.message", encryptedEntry.id, encryptedEntry.sender)) + }); + // 2. setup the timeline + const timeline = new Timeline({roomId, storage: await createMockStorage(), closeCallback: () => {}, + fragmentIdComparer, pendingEvents: new ObservableArray(), clock: new MockClock()}); + await timeline.load(new User(alice), "join", new NullLogItem()); + timeline.addEntries([decryptedEntry]); + const observer = new ListObserver(); + timeline.entries.subscribe(observer); + // 3. replace the entry with one that is not decrypted + // (as would happen when receiving a reaction, + // as it does not rerun the decryption) + // and check that the decrypted content is preserved + timeline.replaceEntries([encryptedEntry]); + const {value, type} = await observer.next(); + assert.equal(type, "update"); + assert.equal(value.eventType, "m.room.message"); + assert.equal(value.content.body, "hi bob!"); + } }; } diff --git a/src/matrix/room/timeline/entries/BaseEntry.js b/src/matrix/room/timeline/entries/BaseEntry.js index 67ba158d..ae3ddf05 100644 --- a/src/matrix/room/timeline/entries/BaseEntry.js +++ b/src/matrix/room/timeline/entries/BaseEntry.js @@ -47,4 +47,6 @@ export class BaseEntry { asEventKey() { return new EventKey(this.fragmentId, this.entryIndex); } + + updateFrom() {} } diff --git a/src/matrix/room/timeline/entries/EventEntry.js b/src/matrix/room/timeline/entries/EventEntry.js index 3a6889f7..a0c3799d 100644 --- a/src/matrix/room/timeline/entries/EventEntry.js +++ b/src/matrix/room/timeline/entries/EventEntry.js @@ -28,11 +28,20 @@ export class EventEntry extends BaseEventEntry { clone() { const clone = new EventEntry(this._eventEntry, this._fragmentIdComparer); - clone._decryptionResult = this._decryptionResult; - clone._decryptionError = this._decryptionError; + clone.updateFrom(this); return clone; } + updateFrom(other) { + super.updateFrom(other); + if (other._decryptionResult && !this._decryptionResult) { + this._decryptionResult = other._decryptionResult; + } + if (other._decryptionError && !this._decryptionError) { + this._decryptionError = other._decryptionError; + } + } + get event() { return this._eventEntry.event; } diff --git a/src/observable/list/SortedArray.js b/src/observable/list/SortedArray.js index 39cfcde5..323cf776 100644 --- a/src/observable/list/SortedArray.js +++ b/src/observable/list/SortedArray.js @@ -46,6 +46,16 @@ export class SortedArray extends BaseObservableList { return findAndUpdateInArray(predicate, this._items, this, updater); } + getAndUpdate(item, updater, updateParams = null) { + const idx = this.indexOf(item); + if (idx !== -1) { + const existingItem = this._items[idx]; + const newItem = updater(existingItem, item); + this._items[idx] = newItem; + this.emitUpdate(idx, newItem, updateParams); + } + } + update(item, updateParams = null) { const idx = this.indexOf(item); if (idx !== -1) { @@ -169,4 +179,4 @@ export function tests() { assert.equal(it.next().done, true); } } -} \ No newline at end of file +}