From d6dcd36c251c61a32a91c05c197cba0f517eae31 Mon Sep 17 00:00:00 2001 From: mrjo118 Date: Tue, 18 Aug 2026 15:15:09 +0100 Subject: [PATCH] Desktop, Mobile, Cli: Fix note screen is not properly refreshed when updated by the sync, when E2EE is enabled (#16179) --- .gitignore | 1 + .ignore.eslint | 1 + .../shared/note-screen-shared.test.ts | 73 +++++++++++++++++++ .../components/shared/note-screen-shared.ts | 14 +++- .../lib/services/DecryptionWorker.test.ts | 24 ++++++ packages/lib/services/DecryptionWorker.ts | 26 +++++-- 6 files changed, 131 insertions(+), 8 deletions(-) create mode 100644 packages/lib/components/shared/note-screen-shared.test.ts diff --git a/.gitignore b/.gitignore index 89030ff91c..05cbae003c 100644 --- a/.gitignore +++ b/.gitignore @@ -1403,6 +1403,7 @@ packages/lib/components/shared/config/plugins/useOnInstallHandler.js packages/lib/components/shared/config/shouldShowMissingPasswordWarning.test.js packages/lib/components/shared/config/shouldShowMissingPasswordWarning.js packages/lib/components/shared/dropbox-login-shared.js +packages/lib/components/shared/note-screen-shared.test.js packages/lib/components/shared/note-screen-shared.js packages/lib/components/shared/reduxSharedMiddleware.js packages/lib/components/shared/side-menu-shared.test.js diff --git a/.ignore.eslint b/.ignore.eslint index 28ab0ed10e..49606c7be0 100644 --- a/.ignore.eslint +++ b/.ignore.eslint @@ -1428,6 +1428,7 @@ packages/lib/components/shared/config/plugins/useOnInstallHandler.js packages/lib/components/shared/config/shouldShowMissingPasswordWarning.test.js packages/lib/components/shared/config/shouldShowMissingPasswordWarning.js packages/lib/components/shared/dropbox-login-shared.js +packages/lib/components/shared/note-screen-shared.test.js packages/lib/components/shared/note-screen-shared.js packages/lib/components/shared/reduxSharedMiddleware.js packages/lib/components/shared/side-menu-shared.test.js diff --git a/packages/lib/components/shared/note-screen-shared.test.ts b/packages/lib/components/shared/note-screen-shared.test.ts new file mode 100644 index 0000000000..a4e75f990a --- /dev/null +++ b/packages/lib/components/shared/note-screen-shared.test.ts @@ -0,0 +1,73 @@ +import Note from '../../models/Note'; +import { setupDatabaseAndSynchronizer, switchClient } from '../../testing/test-utils'; +import shared, { BaseNoteScreenComponent } from './note-screen-shared'; + +const deferred = () => { + let resolve: (value: T)=> void; + const promise = new Promise(resolvePromise => { + resolve = resolvePromise; + }); + return { promise, resolve: resolve! }; +}; + +const newComponent = () => ({ + props: { + provisionalNoteIds: [], + noteId: 'note-id', + folders: [], + sharedData: undefined, + noteVisiblePanes: ['editor'], + }, + state: { mode: 'edit' }, + setState: jest.fn(), + scheduleFocusUpdate: jest.fn(), +}) as unknown as BaseNoteScreenComponent; + +describe('note-screen-shared', () => { + beforeEach(async () => { + await setupDatabaseAndSynchronizer(1); + await switchClient(1); + jest.spyOn(shared, 'attachedResources').mockResolvedValue({}); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('should reload an encrypted note after decrypting it', async () => { + const encryptedNote = { id: 'note-id', encryption_cipher_text: 'cipher text', deleted_time: 0 }; + const decryptedNote = { ...encryptedNote, encryption_cipher_text: '', title: 'Title', body: 'Body' }; + const decryptStarted = deferred(); + const decryption = deferred(); + jest.spyOn(Note, 'load').mockResolvedValue(encryptedNote as never); + jest.spyOn(Note, 'decrypt').mockImplementation(() => { + decryptStarted.resolve(); + return decryption.promise as never; + }); + const component = newComponent(); + + const reloadPromise = shared.reloadNote(component); + await decryptStarted.promise; + expect(component.setState).not.toHaveBeenCalled(); + + decryption.resolve(decryptedNote); + await reloadPromise; + + expect(component.setState).toHaveBeenCalledWith(expect.objectContaining({ note: decryptedNote })); + }); + + it.each([ + ['the master key is not loaded', Object.assign(new Error('Master key is not loaded'), { code: 'masterKeyNotLoaded' })], + ['decryption otherwise fails', new Error('Invalid ciphertext')], + ])('should use the empty-note branch when %s', async (_description, error) => { + const encryptedNote = { id: 'note-id', encryption_cipher_text: 'cipher text' }; + jest.spyOn(Note, 'load').mockResolvedValue(encryptedNote as never); + jest.spyOn(Note, 'decrypt').mockRejectedValue(error); + const component = newComponent(); + + const result = await shared.reloadNote(component); + + expect(result).toBeNull(); + expect(component.setState).toHaveBeenCalledWith(expect.objectContaining({ note: {}, isLoading: true })); + }); +}); diff --git a/packages/lib/components/shared/note-screen-shared.ts b/packages/lib/components/shared/note-screen-shared.ts index 5eef7d11bf..f7f9fda045 100644 --- a/packages/lib/components/shared/note-screen-shared.ts +++ b/packages/lib/components/shared/note-screen-shared.ts @@ -311,7 +311,19 @@ shared.isModified = function(comp: BaseNoteScreenComponent) { shared.reloadNote = async (comp: BaseNoteScreenComponent, useDefaultEditorState = false) => { const isProvisionalNote = comp.props.provisionalNoteIds.includes(comp.props.noteId); - const note = await Note.load(comp.props.noteId); + let note = await Note.load(comp.props.noteId); + if (note?.encryption_cipher_text) { + try { + note = await Note.decrypt(note); + } catch (error) { + reg.logger().info(`Could not decrypt note ${note.id}, note could not be refreshed:`, error.message); + // All decryption errors, including masterKeyNotLoaded, intentionally use the non-existent note branch below. + // A forced reload must not retain the previously loaded plaintext, as it presents a risk of data loss if the + // user is typing during the reload. Aside from certain edge cases, a user cannot directly open a note which + // is still encrypted, so normally would not see this. + note = null; + } + } let mode = comp.state.mode; if (useDefaultEditorState) { diff --git a/packages/lib/services/DecryptionWorker.test.ts b/packages/lib/services/DecryptionWorker.test.ts index 5fb6f0b035..7a79ecf437 100644 --- a/packages/lib/services/DecryptionWorker.test.ts +++ b/packages/lib/services/DecryptionWorker.test.ts @@ -1,4 +1,6 @@ import { setupDatabaseAndSynchronizer, switchClient, decryptionWorker } from '../testing/test-utils'; +import BaseItem from '../models/BaseItem'; +import Note from '../models/Note'; describe('services/DecryptionWorker', () => { @@ -7,6 +9,10 @@ describe('services/DecryptionWorker', () => { await switchClient(1); }); + afterEach(() => { + jest.restoreAllMocks(); + }); + it('should not return null when a call to .start is cancelled', async () => { const worker = decryptionWorker(); @@ -23,4 +29,22 @@ describe('services/DecryptionWorker', () => { expect(result).toHaveProperty('error'); } }); + + it('should skip a note that was decrypted on demand after the worker loaded it', async () => { + const note = await Note.save({ title: 'Test note', body: 'Test body' }); + const encryptedSnapshot = { ...note, encryption_applied: 1, encryption_cipher_text: 'cipher text' }; + const worker = decryptionWorker(); + + jest.spyOn(worker.encryptionService(), 'loadedMasterKeysCount').mockReturnValue(1); + jest.spyOn(BaseItem, 'itemsThatNeedDecryption').mockResolvedValue({ + items: [encryptedSnapshot], + hasMore: false, + }); + const decryptSpy = jest.spyOn(Note, 'decrypt'); + + const result = await worker.start(); + + expect(decryptSpy).not.toHaveBeenCalled(); + expect(result.decryptedItemCount).toBe(1); + }); }); diff --git a/packages/lib/services/DecryptionWorker.ts b/packages/lib/services/DecryptionWorker.ts index 76606d02d0..2b886f5c15 100644 --- a/packages/lib/services/DecryptionWorker.ts +++ b/packages/lib/services/DecryptionWorker.ts @@ -1,5 +1,5 @@ import BaseItem, { ItemsThatNeedDecryptionResult } from '../models/BaseItem'; -import BaseModel from '../BaseModel'; +import BaseModel, { ModelType } from '../BaseModel'; import MasterKey from '../models/MasterKey'; import Resource from '../models/Resource'; import ResourceService from './ResourceService'; @@ -219,6 +219,12 @@ export default class DecryptionWorker { await this.kvStore().deleteValue(errorKey); }; + const markSuccessfulDecryption = async (decryptedItemType: number) => { + await clearDecryptionCounter(); + if (!decryptedItemCounts[decryptedItemType]) decryptedItemCounts[decryptedItemType] = 0; + decryptedItemCounts[decryptedItemType]++; + }; + // Don't log in production as it results in many messages when importing many items // this.logger().debug('DecryptionWorker: decrypting: ' + item.id + ' (' + ItemClass.tableName() + ')'); try { @@ -231,13 +237,19 @@ export default class DecryptionWorker { continue; } + if (item.type_ === ModelType.Note) { + // Validate if still eligible to decrypt using the latest encryption_applied value, as notes may be decrypted on demand while the decryption worker is running. + // If it has been decrypted already, avoid decrypting it again to avoid potentially overwriting it with an outdated version + const encryptionApplied = (await ItemClass.load(item.id, { fields: ['encryption_applied'] }))?.encryption_applied; + if (!encryptionApplied) { + this.logger().info(`DecryptionWorker: Skipping decryption for note ${item.id} as it was already decrypted on demand`); + await markSuccessfulDecryption(item.type_); + continue; + } + } + const decryptedItem = await ItemClass.decrypt(item); - - await clearDecryptionCounter(); - - if (!decryptedItemCounts[decryptedItem.type_]) decryptedItemCounts[decryptedItem.type_] = 0; - - decryptedItemCounts[decryptedItem.type_]++; + await markSuccessfulDecryption(decryptedItem.type_); if (decryptedItem.type_ === Resource.modelType() && !!decryptedItem.encryption_blob_encrypted) { // itemsThatNeedDecryption() will return the resource again if the blob has not been decrypted,