From 9bdf4ff1fd156247054221f055908d5e08a8a3c3 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Sat, 12 Sep 2026 19:46:05 -0400 Subject: [PATCH] fix(security): bookmarks/reminders stop storing decrypted text server-side For encrypted rooms, bookmarks persist only {roomId, eventId, savedAt} and reminders only their non-text fields; the preview, room name and sender resolve locally at render/fire time from the timeline (with a "Message unavailable" fallback). A one-time, loop-guarded cleanup strips text from existing entries in currently-encrypted rooms. Unencrypted rooms are unchanged. Unit-tested. Fixes #10 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PPmy3tPq869XDW4njjVaKA --- src/app/features/bookmarks/BookmarksPanel.tsx | 33 ++++-- src/app/features/room/message/Message.tsx | 3 + src/app/hooks/useBookmarks.test.ts | 81 ++++++++++++++ src/app/hooks/useBookmarks.ts | 104 ++++++++++++++++-- src/app/hooks/useReminders.ts | 72 +++++++++++- 5 files changed, 272 insertions(+), 21 deletions(-) create mode 100644 src/app/hooks/useBookmarks.test.ts diff --git a/src/app/features/bookmarks/BookmarksPanel.tsx b/src/app/features/bookmarks/BookmarksPanel.tsx index ff41c3825..8b815b65e 100644 --- a/src/app/features/bookmarks/BookmarksPanel.tsx +++ b/src/app/features/bookmarks/BookmarksPanel.tsx @@ -103,7 +103,8 @@ function BookmarkItem({ bookmark, onJump, onRemove, preview, senderName }: Bookm const mx = useMatrixClient(); const useAuthentication = useMediaAuthentication(); const room = mx.getRoom(bookmark.roomId) ?? undefined; - const displayRoomName = room?.name ?? bookmark.roomName; + // E2EE-room bookmarks store no roomName; fall back past the '' placeholder. + const displayRoomName = room?.name || bookmark.roomName || 'Unknown room'; const avatarUrl = room ? (getRoomAvatarUrl(mx, room, 96, useAuthentication) ?? undefined) : undefined; @@ -162,7 +163,7 @@ function BookmarkItem({ bookmark, onJump, onRemove, preview, senderName }: Bookm style={{ justifyContent: 'flex-start', height: 'unset', padding: config.space.S200 }} > - {preview ?? (bookmark.previewText || '(no preview)')} + {preview ?? (bookmark.previewText || 'Message unavailable')} @@ -173,13 +174,16 @@ type LiveBookmarkItemProps = BookmarkItemProps & { room: Room }; // Renders the same layout as BookmarkItem, but resolves the message body live so // edits (m.replace, applied by useRoomEvent) and redactions are reflected. The -// stored snapshot (previewText) remains the fallback for loading/failed/empty states. +// stored snapshot (previewText) remains the fallback for loading/failed/empty +// states; bookmarks from E2EE rooms have no snapshot at all (account data is +// server-readable), so the live event is their only source of text. function LiveBookmarkItem({ room, bookmark, onJump, onRemove }: LiveBookmarkItemProps) { const liveEvent = useRoomEvent(room, bookmark.eventId, () => room.findEventById(bookmark.eventId), ); - const snapshot = bookmark.previewText || '(no preview)'; + const snapshot = + bookmark.previewText || (liveEvent === undefined ? 'Loading…' : 'Message unavailable'); let preview: ReactNode = snapshot; // undefined (loading) and null (fetch failed / not found) both keep the snapshot. @@ -234,7 +238,7 @@ function RoomGroupHeader({ const mx = useMatrixClient(); const useAuthentication = useMediaAuthentication(); const room = mx.getRoom(roomId) ?? undefined; - const displayRoomName = room?.name ?? roomName; + const displayRoomName = room?.name || roomName || 'Unknown room'; const avatarUrl = room ? (getRoomAvatarUrl(mx, room, 96, useAuthentication) ?? undefined) : undefined; @@ -327,13 +331,20 @@ export function BookmarksPanel({ onClose }: BookmarksPanelProps) { () => query.length === 0 ? bookmarks - : bookmarks.filter( - (bk) => - bk.previewText.toLowerCase().includes(query) || + : bookmarks.filter((bk) => { + // E2EE-room bookmarks have no stored text: match against the locally + // cached event body / live room name instead (nothing is fetched here). + const room = mx.getRoom(bk.roomId); + const localBody = room?.findEventById(bk.eventId)?.getContent()?.body; + return ( + (bk.previewText?.toLowerCase().includes(query) ?? false) || + (typeof localBody === 'string' && localBody.toLowerCase().includes(query)) || bk.roomName.toLowerCase().includes(query) || - (bk.senderName?.toLowerCase().includes(query) ?? false), - ), - [bookmarks, query], + (room?.name.toLowerCase().includes(query) ?? false) || + (bk.senderName?.toLowerCase().includes(query) ?? false) + ); + }), + [mx, bookmarks, query], ); // Prune collapsed roomIds that no longer have any bookmark, so a room re-saved diff --git a/src/app/features/room/message/Message.tsx b/src/app/features/room/message/Message.tsx index 7bfe2d761..3b7bc646f 100644 --- a/src/app/features/room/message/Message.tsx +++ b/src/app/features/room/message/Message.tsx @@ -1273,6 +1273,9 @@ export const Message = React.memo( const content = mEvent.getContent(); const body: string = (content?.body as string | undefined) ?? ''; + // For E2EE rooms useBookmarks strips the text + // fields before persisting (account data is + // server-readable); the panel resolves them live. addBookmark({ roomId: room.roomId, eventId, diff --git a/src/app/hooks/useBookmarks.test.ts b/src/app/hooks/useBookmarks.test.ts new file mode 100644 index 000000000..87f6fcea0 --- /dev/null +++ b/src/app/hooks/useBookmarks.test.ts @@ -0,0 +1,81 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { + Bookmark, + cleanupEncryptedBookmarks, + hasBookmarkText, + stripBookmarkText, + toStorableBookmark, +} from './useBookmarks'; +import { cleanupEncryptedReminders, Reminder, toStorableReminder } from './useReminders'; + +// E2EE policy for bookmarks/reminders (Gitea #10): account data is stored +// unencrypted on the homeserver, so entries for encrypted rooms must carry no +// message text or names. `isEncryptedRoom` is injected to keep these pure. +const isEncrypted = (roomId: string) => roomId === '!enc'; + +const full = (roomId: string): Bookmark => ({ + roomId, + eventId: `$ev-${roomId}`, + savedAt: 100, + previewText: 'secret text', + roomName: 'Room', + senderName: 'Alice', +}); + +test('stripBookmarkText keeps only roomId/eventId/savedAt', () => { + const stripped = stripBookmarkText(full('!enc')); + assert.deepEqual(stripped, { roomId: '!enc', eventId: '$ev-!enc', savedAt: 100, roomName: '' }); + assert.equal(hasBookmarkText(stripped), false); + assert.equal(hasBookmarkText(full('!enc')), true); +}); + +test('toStorableBookmark strips text for encrypted rooms only', () => { + const plain = full('!plain'); + assert.equal(toStorableBookmark(plain, isEncrypted), plain); + const enc = toStorableBookmark(full('!enc'), isEncrypted); + assert.equal(enc.previewText, undefined); + assert.equal(enc.senderName, undefined); + assert.equal(enc.roomName, ''); +}); + +test('cleanupEncryptedBookmarks returns undefined when nothing to strip', () => { + const already = stripBookmarkText(full('!enc')); + assert.equal(cleanupEncryptedBookmarks([full('!plain'), already], isEncrypted), undefined); + assert.equal(cleanupEncryptedBookmarks([], isEncrypted), undefined); +}); + +test('cleanupEncryptedBookmarks strips legacy encrypted entries and keeps the rest', () => { + const plain = full('!plain'); + const out = cleanupEncryptedBookmarks([plain, full('!enc')], isEncrypted); + assert.ok(out); + assert.equal(out[0], plain); + assert.deepEqual(out[1], stripBookmarkText(full('!enc'))); + // Running it again on the result is a no-op (no write loop). + assert.equal(cleanupEncryptedBookmarks(out, isEncrypted), undefined); +}); + +const reminder = (roomId: string): Reminder => ({ + roomId, + eventId: `$ev-${roomId}`, + timestamp: 200, + message: 'secret text', +}); + +test('toStorableReminder drops message for encrypted rooms only', () => { + const plain = reminder('!plain'); + assert.equal(toStorableReminder(plain, isEncrypted), plain); + assert.deepEqual(toStorableReminder(reminder('!enc'), isEncrypted), { + roomId: '!enc', + eventId: '$ev-!enc', + timestamp: 200, + }); +}); + +test('cleanupEncryptedReminders strips legacy entries once', () => { + const out = cleanupEncryptedReminders([reminder('!plain'), reminder('!enc')], isEncrypted); + assert.ok(out); + assert.equal(out[0].message, 'secret text'); + assert.equal(out[1].message, undefined); + assert.equal(cleanupEncryptedReminders(out, isEncrypted), undefined); +}); diff --git a/src/app/hooks/useBookmarks.ts b/src/app/hooks/useBookmarks.ts index 0e3f319b8..0b3c0fbfa 100644 --- a/src/app/hooks/useBookmarks.ts +++ b/src/app/hooks/useBookmarks.ts @@ -1,4 +1,5 @@ -import { useCallback } from 'react'; +import { useCallback, useEffect } from 'react'; +import { MatrixClient } from 'matrix-js-sdk'; import { useMatrixClient } from './useMatrixClient'; import { createAccountDataListStore } from './createAccountDataListStore'; @@ -6,7 +7,14 @@ export type Bookmark = { roomId: string; eventId: string; savedAt: number; - previewText: string; + // Snapshot fields. Account data is stored UNENCRYPTED on the homeserver, so + // for E2EE rooms none of these are persisted (see stripBookmarkText) — the + // panel resolves the preview / room / author live from the local timeline + // instead. They remain for unencrypted rooms (already server-visible) and for + // entries saved before this policy, so older stored bookmarks still render. + previewText?: string; + // Kept as a (possibly empty) string in memory because the grouping helpers in + // utils/bookmarks read it directly; the store omits it from the wire when empty. roomName: string; // Author display name, snapshotted at save time. Optional for backward // compatibility with bookmarks stored before attribution was added; the panel @@ -16,22 +24,83 @@ export type Bookmark = { senderName?: string; }; +// Wire shape: identical to Bookmark except that `roomName` may be absent. +type StoredBookmark = Omit & { roomName?: string }; + const BOOKMARKS_KEY = 'io.lotus.bookmarks'; const MAX_BOOKMARKS = 500; type BookmarksContent = { - bookmarks: Bookmark[]; + bookmarks: StoredBookmark[]; }; +/** True when the entry carries any of the text fields we refuse to upload for E2EE rooms. */ +export const hasBookmarkText = (b: Bookmark): boolean => + b.previewText !== undefined || b.roomName !== '' || b.senderName !== undefined; + +/** + * Reduce a bookmark to its non-text fields ({roomId, eventId, savedAt}). Used for + * messages in encrypted rooms so no decrypted plaintext (or who said it) ever + * reaches the server's account-data store. + */ +export const stripBookmarkText = (b: Bookmark): Bookmark => ({ + roomId: b.roomId, + eventId: b.eventId, + savedAt: b.savedAt, + roomName: '', +}); + +/** + * Apply the E2EE storage policy to a bookmark about to be persisted. + * `isEncryptedRoom` is injected so the policy is testable without a client. + */ +export const toStorableBookmark = ( + b: Bookmark, + isEncryptedRoom: (roomId: string) => boolean, +): Bookmark => (isEncryptedRoom(b.roomId) ? stripBookmarkText(b) : b); + +/** + * One-time cleanup for entries persisted before the E2EE policy: returns the list + * with text fields stripped for every bookmark whose room is currently encrypted, + * or `undefined` when nothing needs to change (so callers can skip the write). + */ +export const cleanupEncryptedBookmarks = ( + bookmarks: Bookmark[], + isEncryptedRoom: (roomId: string) => boolean, +): Bookmark[] | undefined => { + let changed = false; + const next = bookmarks.map((b) => { + if (hasBookmarkText(b) && isEncryptedRoom(b.roomId)) { + changed = true; + return stripBookmarkText(b); + } + return b; + }); + return changed ? next : undefined; +}; + +const isEncryptedRoomFor = + (mx: MatrixClient) => + (roomId: string): boolean => + mx.getRoom(roomId)?.hasEncryptionStateEvent() ?? false; + // Shared, concurrency-safe store. See createAccountDataListStore for why the // snapshot + write queue must be module-scoped (writes are serialized to avoid // lost updates, since setAccountData replaces the whole content with no merge). const bookmarksStore = createAccountDataListStore({ eventType: BOOKMARKS_KEY, - read: (content) => content?.bookmarks ?? [], - write: (bookmarks) => ({ bookmarks }), + read: (content) => (content?.bookmarks ?? []).map((b) => ({ ...b, roomName: b.roomName ?? '' })), + write: (bookmarks) => ({ + // Drop the in-memory '' placeholder so stripped entries stay {roomId, eventId, savedAt}. + bookmarks: bookmarks.map(({ roomName, ...rest }) => (roomName ? { ...rest, roomName } : rest)), + }), }); +// Guard so the load-time cleanup below runs once per client, not on every mount +// (useBookmarks is mounted by every message row) and not again after its own +// write echoes back. +let cleanedUpFor: MatrixClient | null = null; + export function useBookmarks(): { bookmarks: Bookmark[]; addBookmark: (b: Bookmark) => Promise; @@ -41,12 +110,33 @@ export function useBookmarks(): { const mx = useMatrixClient(); const bookmarks = bookmarksStore.useValue(mx); + // Strip text from entries saved (by older builds) for rooms that are encrypted, + // writing the store back at most once per client. + useEffect(() => { + if (cleanedUpFor === mx) return; + cleanedUpFor = mx; + const cleaned = cleanupEncryptedBookmarks(bookmarksStore.getLatest(mx), isEncryptedRoomFor(mx)); + if (cleaned) { + bookmarksStore + .enqueueWrite( + mx, + (current) => cleanupEncryptedBookmarks(current, isEncryptedRoomFor(mx)) ?? current, + ) + .catch(() => { + // Retry on the next client (re)mount. + if (cleanedUpFor === mx) cleanedUpFor = null; + }); + } + }, [mx]); + const addBookmark = useCallback( (b: Bookmark) => bookmarksStore.enqueueWrite(mx, (current) => { + // Never upload plaintext for E2EE rooms — account data is server-readable. + const stored = toStorableBookmark(b, isEncryptedRoomFor(mx)); // Avoid duplicates - const filtered = current.filter((bk) => bk.eventId !== b.eventId); - let next = [b, ...filtered]; + const filtered = current.filter((bk) => bk.eventId !== stored.eventId); + let next = [stored, ...filtered]; if (next.length > MAX_BOOKMARKS) { next = next.slice(0, MAX_BOOKMARKS); } diff --git a/src/app/hooks/useReminders.ts b/src/app/hooks/useReminders.ts index 9002e4f61..a15ec2ae1 100644 --- a/src/app/hooks/useReminders.ts +++ b/src/app/hooks/useReminders.ts @@ -1,4 +1,5 @@ -import { useCallback } from 'react'; +import { useCallback, useEffect } from 'react'; +import { MatrixClient } from 'matrix-js-sdk'; import { useMatrixClient } from './useMatrixClient'; import { createAccountDataListStore } from './createAccountDataListStore'; @@ -6,7 +7,10 @@ export type Reminder = { roomId: string; eventId: string; timestamp: number; - message: string; + // Message preview. Account data is stored UNENCRYPTED on the homeserver, so + // this is omitted for E2EE rooms (see stripReminderText) and the toast resolves + // the text locally at fire time. Optional for entries stored before that policy. + message?: string; }; const REMINDERS_KEY = 'io.lotus.reminders'; @@ -15,6 +19,43 @@ type RemindersContent = { reminders: Reminder[]; }; +/** Reduce a reminder to its non-text fields ({roomId, eventId, timestamp}). */ +export const stripReminderText = (r: Reminder): Reminder => ({ + roomId: r.roomId, + eventId: r.eventId, + timestamp: r.timestamp, +}); + +/** Apply the E2EE storage policy to a reminder about to be persisted. */ +export const toStorableReminder = ( + r: Reminder, + isEncryptedRoom: (roomId: string) => boolean, +): Reminder => (isEncryptedRoom(r.roomId) ? stripReminderText(r) : r); + +/** + * One-time cleanup for entries persisted before the E2EE policy; `undefined` + * when nothing needs to change (so callers can skip the write). + */ +export const cleanupEncryptedReminders = ( + reminders: Reminder[], + isEncryptedRoom: (roomId: string) => boolean, +): Reminder[] | undefined => { + let changed = false; + const next = reminders.map((r) => { + if (r.message !== undefined && isEncryptedRoom(r.roomId)) { + changed = true; + return stripReminderText(r); + } + return r; + }); + return changed ? next : undefined; +}; + +const isEncryptedRoomFor = + (mx: MatrixClient) => + (roomId: string): boolean => + mx.getRoom(roomId)?.hasEncryptionStateEvent() ?? false; + // Shared, concurrency-safe store. See createAccountDataListStore for why the // snapshot + write queue must be module-scoped (writes are serialized to avoid // lost updates, since setAccountData replaces the whole content with no merge). @@ -24,6 +65,9 @@ const remindersStore = createAccountDataListStore( write: (reminders) => ({ reminders }), }); +// Once-per-client guard for the load-time cleanup (mirrors useBookmarks). +let cleanedUpFor: MatrixClient | null = null; + export function useReminders(): { reminders: Reminder[]; addReminder: (r: Reminder) => Promise; @@ -33,8 +77,30 @@ export function useReminders(): { const mx = useMatrixClient(); const reminders = remindersStore.useValue(mx); + // Strip text from entries saved (by older builds) for rooms that are encrypted, + // writing the store back at most once per client. + useEffect(() => { + if (cleanedUpFor === mx) return; + cleanedUpFor = mx; + if (cleanupEncryptedReminders(remindersStore.getLatest(mx), isEncryptedRoomFor(mx))) { + remindersStore + .enqueueWrite( + mx, + (current) => cleanupEncryptedReminders(current, isEncryptedRoomFor(mx)) ?? current, + ) + .catch(() => { + if (cleanedUpFor === mx) cleanedUpFor = null; + }); + } + }, [mx]); + const addReminder = useCallback( - (r: Reminder) => remindersStore.enqueueWrite(mx, (current) => [...current, r]), + (r: Reminder) => + remindersStore.enqueueWrite(mx, (current) => [ + ...current, + // Never upload plaintext for E2EE rooms — account data is server-readable. + toStorableReminder(r, isEncryptedRoomFor(mx)), + ]), [mx], );