Api, Cli, Desktop: Resolves #16054: Add fail-closed guardrails for locked notes - #16098
Api, Cli, Desktop: Resolves #16054: Add fail-closed guardrails for locked notes#16098keshav0479 wants to merge 3 commits into
Conversation
…rd CLI lock changes
|
All feedback addressed and description now covers the new behaviour with ss and plugin video. |
|
That looks good to me. @mrjo118, please confirm it's ok with you as well, and feel free to merge if it is |
| } | ||
| }, | ||
| enabledCondition: 'selectionIncludesHtmlNotes && (multipleNotesSelected || !noteIsReadOnly)', | ||
| enabledCondition: 'selectionIncludesHtmlNotes && (multipleNotesSelected || !noteIsReadOnly) && !noteLockContentUnavailable', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes hide it there too please
| 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(); |
There was a problem hiding this comment.
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
| } | ||
| // A locked note's revisions are not all flagged themselves, so the note state is checked too. | ||
| const whereQuery = isNoteLockEnabled() ? { sql: 'is_locked = 0 AND item_id NOT IN (SELECT id FROM notes WHERE is_locked = 1)' } : null; | ||
| const whereQuery = isNoteLockEnabled() ? { sql: 'is_locked = 0' } : null; |
There was a problem hiding this comment.
Does the whereQuery apply to DELETE? If it does, we don't locked notes to be excluded for that
Summary
Resolves #16054
Adds fail-closed checks for the paths that reach a note without going through the editor, behind the existing feature flag.
Data API:
CLI:
Convert to Markdown:
With the flag off, nothing changes, everything runs through the old code paths.
Testing
yarn tscyarn workspace @joplin/lib test(routes/notes, routes/revisions, convertNoteToMarkdown, Note, NoteLockService, NoteLockSession, RevisionService)yarn workspace joplin test(command-cat, command-set, command-edit)Screenshots and video
Secure Notes plugin on a locked note, session unlocked:
video_20260802_134207.mp4
AI Assistance Disclosure
I used AI tools while working on this PR for code suggestions and review, checking scope and tests, and drafting parts of this description, including the disclosure. I reviewed the final changes and reran the tests listed above myself.