From bafcd456c5d74c01d38727a773be0a20c106ec3b Mon Sep 17 00:00:00 2001 From: Maurice Date: Fri, 4 Sep 2026 12:15:38 +0200 Subject: [PATCH] fix(collab): notes show and keep their website on the phone The note website round-trips correctly through the API and the desktop grid renders it, but the mobile notes tab never carried the field: the form had no input, the payload had no key, and neither the card nor the read-only sheet showed a stored link. Since an editor only ever opens the form and a viewer only ever opens the sheet, no role on a phone could see or reach a website saved from a desktop. Editing a note on mobile did not wipe an existing link. The key was omitted rather than nulled, and the server's CASE WHEN guard kept the stored value, so this was a visibility gap rather than a destructive one. The form now always sends the field, null included, because the server only clears on an explicit non-undefined value. Every render path goes through safeExternalHref: the REST contract accepts any string, so an unguarded href would execute a stored javascript: value in this origin. The desktop expanded view had the same omission and is fixed alongside it, so a member without collab_edit no longer loses the link there either. Closes #2222 --- .../components/Collab/CollabNotes.test.tsx | 30 +++++- client/src/components/Collab/CollabNotes.tsx | 23 +++- .../mobile/screens/trip/tabs/MCollabNotes.tsx | 55 +++++++++- .../unit/mobile/trip/MCollabNotes.test.tsx | 102 +++++++++++++++++- 4 files changed, 201 insertions(+), 9 deletions(-) diff --git a/client/src/components/Collab/CollabNotes.test.tsx b/client/src/components/Collab/CollabNotes.test.tsx index 34f01347f..576029c1a 100644 --- a/client/src/components/Collab/CollabNotes.test.tsx +++ b/client/src/components/Collab/CollabNotes.test.tsx @@ -1275,7 +1275,7 @@ describe('CollabNotes', () => { }); }); -// FE-W5CNT-001 to FE-W5CNT-029 +// FE-W5CNT-001 to FE-W5CNT-036 // Fills in the load/error/attachment/category branches of useCollabNotes and the // view modal that the smoke tests above do not reach. @@ -1947,4 +1947,32 @@ describe('CollabNotes details', () => { expect(full.querySelector(`[id="${href.slice(1)}"]`)).not.toBeNull(); expect(ref).not.toHaveAttribute('target'); }); + + it('FE-W5CNT-035: the expanded note links its website out to a new tab', async () => { + const user = userEvent.setup(); + serveNotes({ notes: [buildNote({ id: 9, title: 'Hotel', website: 'www.hotel.test' })] }); + server.use(http.get('/api/trips/1/collab/link-preview', () => HttpResponse.json({ title: 'Hotel', image: null }))); + render(); + await screen.findByText('Hotel'); + await user.click(screen.getByTitle('collab.notes.expand')); + const full = await openedNoteBody(); + + // A member without collab_edit never opens the edit modal, so this is the + // only place the link is readable for them (#2222). + const link = within(full).getByRole('link'); + expect(link).toHaveAttribute('href', 'https://www.hotel.test'); + expect(link).toHaveAttribute('rel', 'noopener noreferrer'); + expect(link).toHaveTextContent('hotel.test'); + }); + + it('FE-W5CNT-036: a javascript: website stays unlinked in the expanded note', async () => { + const user = userEvent.setup(); + serveNotes({ notes: [buildNote({ id: 10, title: 'Hotel', website: 'javascript:alert(1)' })] }); + render(); + await screen.findByText('Hotel'); + await user.click(screen.getByTitle('collab.notes.expand')); + const full = await openedNoteBody(); + + expect(within(full).queryAllByRole('link')).toHaveLength(0); + }); }); diff --git a/client/src/components/Collab/CollabNotes.tsx b/client/src/components/Collab/CollabNotes.tsx index 434373379..22803e705 100644 --- a/client/src/components/Collab/CollabNotes.tsx +++ b/client/src/components/Collab/CollabNotes.tsx @@ -4,7 +4,7 @@ import remarkGfm from 'remark-gfm' import remarkBreaks from 'remark-breaks' import { sanitizedMarkdownComponents, sanitizedMarkdownPlugins } from '../shared/markdownSanitize' import { createPortal } from 'react-dom' -import { Plus, Pencil, X, StickyNote, Settings } from 'lucide-react' +import { Plus, Pencil, X, StickyNote, Settings, ExternalLink } from 'lucide-react' import { collabApi } from '../../api/client' import { useCanDo } from '../../store/permissionsStore' import { useTripStore } from '../../store/tripStore' @@ -21,6 +21,7 @@ import { CategorySettingsModal } from './CollabNotesCategorySettingsModal' import { NoteCard } from './CollabNotesCard' import { FilePreviewPortal } from './CollabNotesFilePreviewPortal' import { AuthedImg } from './CollabNotesAuthedImg' +import { safeExternalHref } from '../../utils/safeUrl' // ── Main Component ────────────────────────────────────────────────────────── interface CollabNotesProps { @@ -393,9 +394,18 @@ function CollabNotesGrid(S: NotesState) { ) } +/** Label for the link chip: the host is what people recognise, "www." is noise. */ +function linkHost(url: string): string { + try { return new URL(url).hostname.replace(/^www\./, '') } catch { return url } +} + function ViewNoteModal(S: NotesState) { const { viewingNote, setViewingNote, canEdit, setEditingNote, getCategoryColor, t, setPreviewFile } = S if (!viewingNote) return null + // A member without collab_edit only ever gets this modal, so the link has to + // be here too (#2222). Allow-listed like the card tile: the field takes any + // string, and a javascript: one would run in this origin. + const websiteHref = safeExternalHref(viewingNote.website) return createPortal(
{viewingNote.content || ''} + {websiteHref && ( + + {linkHost(websiteHref)} + + )} {(viewingNote.attachments || []).length > 0 && (
{t('files.title')}
diff --git a/client/src/mobile/screens/trip/tabs/MCollabNotes.tsx b/client/src/mobile/screens/trip/tabs/MCollabNotes.tsx index 5a47312ed..1a9d4c911 100644 --- a/client/src/mobile/screens/trip/tabs/MCollabNotes.tsx +++ b/client/src/mobile/screens/trip/tabs/MCollabNotes.tsx @@ -3,11 +3,12 @@ import Markdown from 'react-markdown' import remarkGfm from 'remark-gfm' import remarkBreaks from 'remark-breaks' import { sanitizedMarkdownPlugins, sanitizedMarkdownComponents } from '../../../../components/shared/markdownSanitize' -import { Check, FileText, Paperclip, Pin, PinOff, Plus, StickyNote, Trash2, X } from 'lucide-react' +import { Check, ExternalLink, FileText, Paperclip, Pin, PinOff, Plus, StickyNote, Trash2, X } from 'lucide-react' import MDancingTrek from '../../../components/MDancingTrek' import { collabApi } from '../../../../api/client' import { addListener, removeListener } from '../../../../api/websocket' import { openFile } from '../../../../utils/fileDownload' +import { safeExternalHref } from '../../../../utils/safeUrl' import MSheet from '../../../components/MSheet' import { Eyebrow, FIELD_AREA_CLS, FIELD_CLS, FormSheetFooter, FormSheetHeader } from '../sheets/PlSheetChrome' import MConfirmSheet from '../../settings/MConfirmSheet' @@ -36,9 +37,15 @@ interface NoteFormSubmitData { content?: string category?: string color: string + website: string | null pendingFiles: File[] } +/** Label for a link chip: the host is what people recognise, "www." is noise. */ +function linkHost(url: string): string { + try { return new URL(url).hostname.replace(/^www\./, '') } catch { return url } +} + /** * Trip-tab Collab / Notes. Same architecture as MCollabChat: own state, own * `collabApi` calls, own WebSocket listener — no `tripStore`/`tripActions` @@ -98,6 +105,7 @@ export default function MCollabNotes({ planner }: MCollabNotesProps) { try { const res = (await collabApi.createNote(tripId, { title: data.title, content: data.content, category: data.category, color: data.color, + website: data.website, })) as NoteResponse created = res.note } catch { @@ -119,7 +127,7 @@ export default function MCollabNotes({ planner }: MCollabNotesProps) { const handleUpdate = useCallback(async ( noteId: number, - data: { title?: string; content?: string; category?: string; color?: string; pinned?: boolean }, + data: { title?: string; content?: string; category?: string; color?: string; pinned?: boolean; website?: string | null }, pendingFiles: File[] = [], ) => { let updated: CollabNoteData | undefined @@ -256,6 +264,7 @@ export default function MCollabNotes({ planner }: MCollabNotesProps) { if (formTarget && formTarget !== 'new') { await handleUpdate(formTarget.id, { title: data.title, content: data.content, category: data.category, color: data.color, + website: data.website, }, data.pendingFiles) } else { await handleCreate(data) @@ -296,6 +305,10 @@ function NoteCardRow({ note, color, canEdit, onTap, onTogglePin, onDelete, t }: t: TripPlanner['t'] }) { const initial = (note.username || '?')[0]?.toUpperCase() || '?' + // An editor is routed to the form sheet, never to the viewer, so the card is + // the only surface where both roles can reach the link (#2222). The field + // takes any string, so the href is allow-listed the way the desktop tile is. + const websiteHref = safeExternalHref(note.website) return (
@@ -312,6 +325,17 @@ function NoteCardRow({ note, color, canEdit, onTap, onTogglePin, onDelete, t }: )} + {websiteHref && ( + + + + )} {canEdit && (
+ {t('collab.notes.website')} + setWebsite(e.target.value)} + placeholder={t('collab.notes.websitePlaceholder')} + className={FIELD_CLS} + /> + {canUploadFiles && ( <> {t('collab.notes.attachFiles')} @@ -588,6 +627,7 @@ function NoteViewSheet({ open, note, onClose, t }: { useEffect(() => { if (open) setSnapshot(note) }, [open, note]) + const websiteHref = safeExternalHref(snapshot?.website) return ( @@ -604,6 +644,17 @@ function NoteViewSheet({ open, note, onClose, t }: { {snapshot.content}
)} + {websiteHref && ( + + + {linkHost(websiteHref)} + + )} {(snapshot?.attachments.length ?? 0) > 0 && (
{snapshot?.attachments.map(f => ( diff --git a/client/tests/unit/mobile/trip/MCollabNotes.test.tsx b/client/tests/unit/mobile/trip/MCollabNotes.test.tsx index a4f5c6cdb..7069bb8d5 100644 --- a/client/tests/unit/mobile/trip/MCollabNotes.test.tsx +++ b/client/tests/unit/mobile/trip/MCollabNotes.test.tsx @@ -10,7 +10,7 @@ import { resetAllStores } from '../../../helpers/store' import { server } from '../../../helpers/msw/server' import { act, fireEvent, render, screen, waitFor, within } from '../../../helpers/render' -// FE-MOB-CNOTE-001 to FE-MOB-CNOTE-026 +// FE-MOB-CNOTE-001 to FE-MOB-CNOTE-033 // react-markdown ships as ESM-only chunks that jsdom chokes on; the note cards // only care that the raw text reaches the renderer. @@ -251,7 +251,7 @@ describe('MCollabNotes', () => { fireEvent.click(screen.getByRole('button', { name: 'collab.notes.create' })) await waitFor(() => expect(createdBodies).toEqual([ - { title: 'Insurance', content: 'call the hotline', category: 'Docs', color: '#ef4444' }, + { title: 'Insurance', content: 'call the hotline', category: 'Docs', color: '#ef4444', website: null }, ])) expect(await screen.findByText('Insurance')).toBeInTheDocument() await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()) @@ -270,7 +270,7 @@ describe('MCollabNotes', () => { fireEvent.click(screen.getByRole('button', { name: 'collab.notes.create' })) await waitFor(() => expect(createdBodies).toEqual([ - { title: 'Ferry', category: 'Transport', color: '#6366f1' }, + { title: 'Ferry', category: 'Transport', color: '#6366f1', website: null }, ])) }) @@ -292,7 +292,7 @@ describe('MCollabNotes', () => { fireEvent.click(screen.getByRole('button', { name: 'collab.notes.create' })) await waitFor(() => expect(createdBodies).toEqual([ - { title: 'Ferry', category: 'Food', color: '#6366f1' }, + { title: 'Ferry', category: 'Food', color: '#6366f1', website: null }, ])) }) @@ -324,7 +324,7 @@ describe('MCollabNotes', () => { fireEvent.click(screen.getByRole('button', { name: 'collab.notes.save' })) await waitFor(() => expect(updatedBodies).toEqual([ - { id: '1', body: { title: 'Visa (done)', content: 'apply early', category: 'Docs', color: '#ef4444' } }, + { id: '1', body: { title: 'Visa (done)', content: 'apply early', category: 'Docs', color: '#ef4444', website: null } }, ])) expect(await screen.findByText('Visa (done)')).toBeInTheDocument() }) @@ -515,4 +515,96 @@ describe('MCollabNotes', () => { unmount() expect(removeListener).toHaveBeenCalledWith(handler) }) + + it('FE-MOB-CNOTE-027: creates a note with the trimmed website', async () => { + serveNotes([]) + serveWrites() + await renderNotes() + + fireEvent.click(screen.getByRole('button', { name: /collab\.notes\.new/ })) + fireEvent.change(screen.getByPlaceholderText('collab.notes.titlePlaceholder'), { target: { value: 'Ferry' } }) + fireEvent.change(screen.getByPlaceholderText('collab.notes.websitePlaceholder'), { target: { value: ' https://ferry.test/booking ' } }) + fireEvent.click(screen.getByRole('button', { name: 'collab.notes.create' })) + + await waitFor(() => expect(createdBodies).toEqual([ + { title: 'Ferry', color: '#6366f1', website: 'https://ferry.test/booking' }, + ])) + }) + + it('FE-MOB-CNOTE-028: a blank website is posted as null instead of an empty string', async () => { + serveNotes([]) + serveWrites() + await renderNotes() + + fireEvent.click(screen.getByRole('button', { name: /collab\.notes\.new/ })) + fireEvent.change(screen.getByPlaceholderText('collab.notes.titlePlaceholder'), { target: { value: 'Ferry' } }) + fireEvent.change(screen.getByPlaceholderText('collab.notes.websitePlaceholder'), { target: { value: ' ' } }) + fireEvent.click(screen.getByRole('button', { name: 'collab.notes.create' })) + + await waitFor(() => expect(createdBodies).toEqual([ + { title: 'Ferry', color: '#6366f1', website: null }, + ])) + }) + + it('FE-MOB-CNOTE-029: editing prefills the stored website and saves it back unchanged', async () => { + serveNotes([note(1, { title: 'Visa', website: 'https://visa.test' })]) + serveWrites() + await renderNotes() + + fireEvent.click(screen.getByText('Visa')) + expect(screen.getByPlaceholderText('collab.notes.websitePlaceholder')).toHaveValue('https://visa.test') + fireEvent.click(screen.getByRole('button', { name: 'collab.notes.save' })) + + await waitFor(() => expect(updatedBodies).toEqual([ + { id: '1', body: { title: 'Visa', color: '#6366f1', website: 'https://visa.test' } }, + ])) + }) + + it('FE-MOB-CNOTE-030: clearing the website sends an explicit null so the server drops it', async () => { + serveNotes([note(1, { title: 'Visa', website: 'https://visa.test' })]) + serveWrites() + await renderNotes() + + fireEvent.click(screen.getByText('Visa')) + fireEvent.change(screen.getByPlaceholderText('collab.notes.websitePlaceholder'), { target: { value: '' } }) + fireEvent.click(screen.getByRole('button', { name: 'collab.notes.save' })) + + await waitFor(() => expect(updatedBodies).toEqual([ + { id: '1', body: { title: 'Visa', color: '#6366f1', website: null } }, + ])) + }) + + it('FE-MOB-CNOTE-031: the viewer sheet links a bare host out to a new tab', async () => { + serveNotes([note(1, { title: 'Visa', website: 'www.visa.test' })]) + await renderNotes(buildPlanner({ can: (() => false) as TripPlanner['can'] })) + + fireEvent.click(screen.getByText('Visa')) + const link = within(screen.getByRole('dialog', { name: 'Visa' })).getByRole('link') + expect(link).toHaveAttribute('href', 'https://www.visa.test') + expect(link).toHaveAttribute('target', '_blank') + expect(link).toHaveAttribute('rel', 'noopener noreferrer') + expect(link).toHaveTextContent('visa.test') + }) + + it('FE-MOB-CNOTE-032: a javascript: website is never turned into a link', async () => { + serveNotes([note(1, { title: 'Visa', website: 'javascript:alert(1)' })]) + await renderNotes(buildPlanner({ can: (() => false) as TripPlanner['can'] })) + + expect(screen.queryAllByRole('link')).toHaveLength(0) + fireEvent.click(screen.getByText('Visa')) + expect(screen.getByRole('dialog', { name: 'Visa' })).toBeInTheDocument() + expect(screen.queryAllByRole('link')).toHaveLength(0) + }) + + it('FE-MOB-CNOTE-033: only a card with a website carries the link affordance', async () => { + serveNotes([ + note(1, { title: 'Visa', website: 'https://visa.test' }), + note(2, { title: 'Tent' }), + ]) + await renderNotes() + + const links = screen.getAllByRole('link', { name: 'collab.notes.website' }) + expect(links).toHaveLength(1) + expect(links[0]).toHaveAttribute('href', 'https://visa.test') + }) })