-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Api, Cli, Desktop: Resolves #16054: Add fail-closed guardrails for locked notes #16098
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.
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
base: dev
Are you sure you want to change the base?
Changes from all commits
d2ccc48
f182b58
9566213
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import Note from '@joplin/lib/models/Note'; | ||
| import Setting from '@joplin/lib/models/Setting'; | ||
| import { setupDatabaseAndSynchronizer, switchClient } from '@joplin/lib/testing/test-utils'; | ||
| import { setupCommandForTesting, setupApplication } from './utils/testUtils'; | ||
| const Command = require('./command-cat'); | ||
|
|
||
| describe('command-cat', () => { | ||
|
|
||
| beforeEach(async () => { | ||
| await setupDatabaseAndSynchronizer(1); | ||
| await switchClient(1); | ||
| await setupApplication(); | ||
| }); | ||
|
|
||
| it('should refuse to display a locked note', async () => { | ||
| Setting.setValue('featureFlag.noteLock', true); | ||
| const note = await Note.save({ title: 'hello', body: 'ciphertext', is_locked: 1, parent_id: '' }); | ||
|
|
||
| let output = ''; | ||
| const command = setupCommandForTesting(Command, (text: string) => { output += text; }); | ||
|
|
||
| await expect(command.action({ note: note.id, options: {} })).rejects.toThrow('locked note'); | ||
| expect(output).toBe(''); | ||
| }); | ||
|
|
||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| import * as fs from 'fs-extra'; | ||
| import Note from '@joplin/lib/models/Note'; | ||
| import Setting from '@joplin/lib/models/Setting'; | ||
| import { setupDatabaseAndSynchronizer, switchClient } from '@joplin/lib/testing/test-utils'; | ||
| import { setupCommandForTesting, setupApplication } from './utils/testUtils'; | ||
| const Command = require('./command-edit'); | ||
|
|
||
| describe('command-edit', () => { | ||
|
|
||
| beforeEach(async () => { | ||
| await setupDatabaseAndSynchronizer(1); | ||
| await switchClient(1); | ||
| await setupApplication(); | ||
| }); | ||
|
|
||
| it('should refuse to edit a locked note without writing a temp file', async () => { | ||
| Setting.setValue('featureFlag.noteLock', true); | ||
| const note = await Note.save({ title: 'hello', body: 'ciphertext', is_locked: 1, parent_id: '' }); | ||
| const tempDir = Setting.value('tempDir'); | ||
| await fs.ensureDir(tempDir); | ||
| const filesBefore = await fs.readdir(tempDir); | ||
|
|
||
| const command = setupCommandForTesting(Command); | ||
|
|
||
| await expect(command.action({ note: note.id })).rejects.toThrow('locked note'); | ||
| expect(await fs.readdir(tempDir)).toEqual(filesBefore); | ||
| }); | ||
|
|
||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import Note from '@joplin/lib/models/Note'; | ||
| import Setting from '@joplin/lib/models/Setting'; | ||
| import { setupDatabaseAndSynchronizer, switchClient } from '@joplin/lib/testing/test-utils'; | ||
| import { setupCommandForTesting, setupApplication } from './utils/testUtils'; | ||
| const Command = require('./command-set'); | ||
|
|
||
| describe('command-set', () => { | ||
|
|
||
| beforeEach(async () => { | ||
| await setupDatabaseAndSynchronizer(1); | ||
| await switchClient(1); | ||
| await setupApplication(); | ||
| }); | ||
|
|
||
| // This guard lives in BaseCommand.encryptionCheck, so it also covers attach, done, edit, ren and todo. | ||
| it.each([ | ||
| { label: 'refuse to change a locked note', flagEnabled: true, expectedError: 'Cannot change a locked note', expectedTitle: 'hello' }, | ||
| { label: 'change a locked note when note lock is disabled', flagEnabled: false, expectedError: null, expectedTitle: 'renamed' }, | ||
| ])('should $label', async ({ flagEnabled, expectedError, expectedTitle }) => { | ||
| Setting.setValue('featureFlag.noteLock', flagEnabled); | ||
| const note = await Note.save({ title: 'hello', body: 'ciphertext', is_locked: 1, parent_id: '' }); | ||
|
|
||
| const command = setupCommandForTesting(Command); | ||
| let error: string|null = null; | ||
| try { | ||
| await command.action({ note: note.id, name: 'title', value: 'renamed' }); | ||
| } catch (e) { | ||
| error = (e as Error).message; | ||
| } | ||
|
|
||
| expect(error).toBe(expectedError); | ||
| expect((await Note.load(note.id)).title).toBe(expectedTitle); | ||
| }); | ||
|
|
||
| it('should block changing the lock state of a plain note', async () => { | ||
| Setting.setValue('featureFlag.noteLock', true); | ||
| const note = await Note.save({ title: 'hello', body: 'plain', parent_id: '' }); | ||
|
|
||
| const command = setupCommandForTesting(Command); | ||
| await expect(command.action({ note: note.id, name: 'is_locked', value: '1' })).rejects.toThrow('The note lock state cannot be changed from the command line'); | ||
| expect((await Note.load(note.id)).is_locked).toBe(0); | ||
| }); | ||
|
|
||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,10 @@ import { itemIsReadOnly } from '../models/utils/readOnly'; | |
| import { ModelType } from '../BaseModel'; | ||
| import ItemChange from '../models/ItemChange'; | ||
| import Setting from '../models/Setting'; | ||
| import isNoteLockEnabled from '../services/noteLock/isNoteLockEnabled'; | ||
| import NoteLockNote from '../services/noteLock/NoteLockNote'; | ||
| import NoteLockSession from '../services/noteLock/NoteLockSession'; | ||
| import type { DecryptedNoteLockKey } from '../services/noteLock/NoteLockKey'; | ||
| import Logger from '@joplin/utils/Logger'; | ||
|
|
||
| const logger = Logger.create('convertNoteToMarkdown'); | ||
|
|
@@ -33,6 +37,7 @@ export const runtime = (): CommandRuntime => { | |
| try { | ||
| let isFirst = true; | ||
| let processedCount = 0; | ||
| let noteLockKey: DecryptedNoteLockKey = null; | ||
| for (const note of notes) { | ||
| if (note.markup_language === MarkupLanguage.Markdown) { | ||
| logger.warn('Skipping item: Already Markdown.'); | ||
|
|
@@ -41,8 +46,20 @@ export const runtime = (): CommandRuntime => { | |
| if (await itemIsReadOnly(Note, ModelType.Note, ItemChange.SOURCE_UNSPECIFIED, note.id, Setting.value('sync.userId'), context.state.shareService)) { | ||
| throw new Error(_('Cannot convert read-only item: "%s"', note.title)); | ||
| } | ||
| const noteIsLocked = isNoteLockEnabled() && NoteLockNote.isLocked(note); | ||
| if (noteIsLocked && !noteLockKey) { | ||
| if (!NoteLockSession.instance().isUnlocked()) { | ||
| throw new Error(_('Cannot convert locked note: "%s"', note.title)); | ||
| } | ||
| // Captured once so a session lock mid-run cannot fail the remaining conversions. | ||
| noteLockKey = NoteLockSession.instance().decryptedKey(); | ||
| } | ||
|
|
||
| // A locked note converts through a full gated load and save, so the body is | ||
| // decrypted for the conversion and the converted copy is encrypted again. | ||
| const sourceNote = noteIsLocked ? await Note.load(note.id, { useNoteLock: true, noteLockKey }) : note; | ||
|
|
||
| const markdownBody = await convertHtmlToMarkdown().execute(context, note.body); | ||
| const markdownBody = await convertHtmlToMarkdown().execute(context, sourceNote.body); | ||
| const newNote = await Note.duplicate(note.id); | ||
|
|
||
| newNote.body = markdownBody; | ||
|
|
@@ -51,7 +68,8 @@ export const runtime = (): CommandRuntime => { | |
| newNote.user_updated_time = note.user_updated_time; | ||
| newNote.updated_time = Date.now(); | ||
|
|
||
| await Note.save(newNote, { autoTimestamp: false }); | ||
| const toSave = noteIsLocked ? { ...newNote, isDecrypted: true } : newNote; | ||
| await Note.save(toSave, { autoTimestamp: false, useNoteLock: noteIsLocked, noteLockKey }); | ||
| await Note.delete(note.id, { toTrash: true }); | ||
| processedCount ++; | ||
|
|
||
|
|
@@ -73,6 +91,6 @@ export const runtime = (): CommandRuntime => { | |
| await shim.showErrorDialog(_('Could not convert notes to Markdown: %s', error.message)); | ||
| } | ||
| }, | ||
| enabledCondition: 'selectionIncludesHtmlNotes && (multipleNotesSelected || !noteIsReadOnly)', | ||
| enabledCondition: 'selectionIncludesHtmlNotes && (multipleNotesSelected || !noteIsReadOnly) && !noteLockContentUnavailable', | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When the enabledCondition resolves false, does the banner not show (please check both desktop and mobile routes), or does the availability of the banner still need addressing?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checked both. The unlock and cannot-decrypt panels return before renderConvertHtmlToMarkdown is reached, so the banner can't show when content is unavailable, and mobile doesn't have the convert banner or route at all. One edge, a locked note with unsaved changes keeps the editor mounted when the session locks, so the banner stays and Convert it hits the command's error dialog. Can hide it there too if you want.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes hide it there too please |
||
| }; | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to first check if any of the notes are locked before the loop, then grab the decrypted key before iterating the loop, so that the key remains available for bulk conversion.
Also, the validation above if the session is locked should apply when the noteLockKey is populated, not just when it is not