diff --git a/client/src/components/Journey/JourneyMap.test.tsx b/client/src/components/Journey/JourneyMap.test.tsx index 61c27e5b7..2678fc038 100644 --- a/client/src/components/Journey/JourneyMap.test.tsx +++ b/client/src/components/Journey/JourneyMap.test.tsx @@ -550,4 +550,30 @@ describe('JourneyMap', () => { expect(colors).not.toContain('#ff0000'); expect(colors).not.toContain('#ffffff'); }); + + // A string handed to bindTooltip becomes innerHTML. A track name is a place + // name off a shared trip and an entry label is a journey title, so both are + // written by someone who is not necessarily the person reading the map — and + // this component is what the public journey page renders. + it('FE-COMP-JOURNEYMAP-044: escapes a track name before it becomes tooltip markup', () => { + vi.mocked(L.polyline).mockClear(); + render(' })]} />); + + const line = vi.mocked(L.polyline).mock.results + .map(r => r.value as any) + .find(v => v.bindTooltip.mock.calls.length > 0); + const [label] = line.bindTooltip.mock.calls[0]; + expect(label).not.toContain('', mood: null, entry_date: '2025-06-04' }]; + render(); + + const labels = vi.mocked(mockedMarker().bindTooltip).mock.calls.map(c => c[0]); + expect(labels.some(l => typeof l === 'string' && l.includes(' allCoords.push(c)) } @@ -243,7 +244,9 @@ function JourneyMap( }) const marker = L.marker(pos, { icon }).addTo(map) - marker.bindTooltip(item.label, { + // Escaped for the same reason as the track tooltip above: the label is an + // entry title, and this map is what the public journey page renders. + marker.bindTooltip(escapeHtml(item.label), { direction: 'top', offset: [0, -MARKER_H], className: 'map-tooltip', diff --git a/client/src/components/Map/MapViewGL.test.tsx b/client/src/components/Map/MapViewGL.test.tsx index 550e14a9b..eceae5a2f 100644 --- a/client/src/components/Map/MapViewGL.test.tsx +++ b/client/src/components/Map/MapViewGL.test.tsx @@ -851,6 +851,23 @@ describe('MapViewGL', () => { expect(glMarkers.created[1].lngLat).toEqual([2.12, 48.12]) }) + // The Leaflet twin of this is FE-COMP-MAPVIEW-074. Both renderers build their + // marker as an HTML string, so both need the escape pinned — otherwise it can + // be dropped from one of them without a single test going red. + it('FE-COMP-MAPVIEWGL-023b: an image_url that passes the prefix check is still escaped', async () => { + loadOnAttach() + render(} + {websiteUrl && ( + window.open(websiteUrl, '_blank', 'noopener,noreferrer')} variant="ghost" icon={} label={{t('inspector.website')}} /> )}
diff --git a/client/src/components/Planner/usePlacesSidebar.test.tsx b/client/src/components/Planner/usePlacesSidebar.test.tsx index 1d79a4da5..03918897b 100644 --- a/client/src/components/Planner/usePlacesSidebar.test.tsx +++ b/client/src/components/Planner/usePlacesSidebar.test.tsx @@ -643,7 +643,7 @@ describe('usePlacesSidebar context menu', () => { fireEvent.contextMenu(screen.getByTestId('row-3')); await user.click(screen.getByRole('button', { name: 'Open Website' })); - expect(open).toHaveBeenCalledWith('https://cafe.example', '_blank'); + expect(open).toHaveBeenCalledWith('https://cafe.example', '_blank', 'noopener,noreferrer'); fireEvent.contextMenu(screen.getByTestId('row-3')); await user.click(screen.getByRole('button', { name: 'Google Maps' })); diff --git a/client/src/components/Planner/usePlacesSidebar.ts b/client/src/components/Planner/usePlacesSidebar.ts index 40b34b4d2..aab094a62 100644 --- a/client/src/components/Planner/usePlacesSidebar.ts +++ b/client/src/components/Planner/usePlacesSidebar.ts @@ -14,6 +14,7 @@ import { useSaveToCollectionStore } from '../../store/saveToCollectionStore' import { placeToSaveTarget } from '../Collections/saveTarget' import type { Place, Category, Day, AssignmentsMap } from '../../types' import { getGoogleMapsUrlForPlace } from './placeGoogleMaps' +import { safeHttpUrl } from '../../utils/safeUrl' export interface PlacesSidebarProps { tripId: number @@ -276,7 +277,7 @@ export function usePlacesSidebar(props: PlacesSidebarProps) { ctxMenu.open(e, [ canEditPlaces && { label: t('common.edit'), icon: Pencil, onClick: () => props.onEditPlace(place) }, selDayId && { label: t('planner.addToDay'), icon: CalendarDays, onClick: () => props.onAssignToDay(place.id, selDayId) }, - place.website && { label: t('inspector.website'), icon: ExternalLink, onClick: () => window.open(place.website, '_blank') }, + safeHttpUrl(place.website) && { label: t('inspector.website'), icon: ExternalLink, onClick: () => window.open(safeHttpUrl(place.website)!, '_blank', 'noopener,noreferrer') }, googleMapsUrl && { label: t('inspector.google'), icon: Navigation, onClick: () => window.open(googleMapsUrl, '_blank') }, collectionsEnabled && { label: t('inspector.saveToCollection'), icon: Bookmark, onClick: () => useSaveToCollectionStore.getState().open(placeToSaveTarget(place)) }, { divider: true }, diff --git a/client/src/mobile/screens/trip/sheets/MPlaceSheet.tsx b/client/src/mobile/screens/trip/sheets/MPlaceSheet.tsx index 544a70b3b..d7e0360e5 100644 --- a/client/src/mobile/screens/trip/sheets/MPlaceSheet.tsx +++ b/client/src/mobile/screens/trip/sheets/MPlaceSheet.tsx @@ -17,6 +17,7 @@ import PlaceRating from '../../../../components/shared/StarRating' import TrackColorPicker from '../../../../components/shared/TrackColorPicker' import { resolveTrackColor, inheritedTrackColor } from '../../../../components/Map/trackColors' import { avatarSrc } from '../../../../utils/avatarSrc' +import { safeHttpUrl } from '../../../../utils/safeUrl' import { openFile } from '../../../../utils/fileDownload' import { getNavigationTargets, openNavigationTarget } from '../../../../components/Planner/placeNavigation' import { NavigationMenu } from '../../../../components/shared/NavigationMenu' @@ -527,9 +528,9 @@ export default function MPlaceSheet({ planner, shell }: MTripSheetsProps) { )} - {place.website && ( + {safeHttpUrl(place.website) && ( window.open(place.website!, '_blank', 'noopener,noreferrer')} + onClick={() => window.open(safeHttpUrl(place.website)!, '_blank', 'noopener,noreferrer')} label={t('inspector.website')} > diff --git a/client/src/pages/DashboardPage.test.tsx b/client/src/pages/DashboardPage.test.tsx index 88f93cc49..cb838d62a 100644 --- a/client/src/pages/DashboardPage.test.tsx +++ b/client/src/pages/DashboardPage.test.tsx @@ -1000,4 +1000,59 @@ describe('DashboardPage', () => { expect(within(grid()).queryByText('Paris Adventure')).not.toBeInTheDocument(); }); }); + + // Two people reported in a row that they could not find how to unarchive or + // rename a trip. The controls sat at opacity 0 until the card was hovered, and + // the archive button carried the same icon whether it would archive or restore. + describe('FE-PAGE-DASH-036: the cover controls are findable', () => { + const activeOnly = (trips: unknown[]) => + server.use( + http.get('/api/trips', ({ request }) => { + const url = new URL(request.url); + if (url.searchParams.get('archived')) return HttpResponse.json({ trips: [] }); + return HttpResponse.json({ trips }); + }), + ); + + it('names every action on the card, so none of them is icon-only guesswork', async () => { + activeOnly([buildTrip({ title: 'Lisbon 2025', start_date: '2025-05-01', end_date: '2025-05-08' })]); + const user = userEvent.setup(); + render(); + + await waitFor(() => expect(screen.getAllByText('Lisbon 2025').length).toBeGreaterThan(0)); + await user.click(screen.getByText('Completed')); + const card = (await screen.findAllByText('Lisbon 2025')) + .map(n => n.closest('.trip-card')) + .find(Boolean) as HTMLElement; + for (const label of ['Edit', 'Duplicate', 'Archive', 'Delete']) { + const button = card.querySelector(`[aria-label="${label}"]`) as HTMLElement; + expect(button).toBeTruthy(); + // A tooltip as well as the screen-reader label — the icons alone did not read. + expect(button.getAttribute('title')).toBe(label); + } + }); + + it('swaps the archive icon for a restore one once the trip is archived', async () => { + const archived = buildTrip({ title: 'Old Rome Trip', start_date: '2024-01-01', end_date: '2024-01-07', is_archived: 1 }); + server.use( + http.get('/api/trips', ({ request }) => { + const url = new URL(request.url); + return HttpResponse.json({ trips: url.searchParams.get('archived') ? [archived] : [] }); + }), + ); + const user = userEvent.setup(); + render(); + + await user.click(screen.getByText('Archived')); + await waitFor(() => expect(screen.getByText('Old Rome Trip')).toBeInTheDocument()); + + const card = screen.getByText('Old Rome Trip').closest('.trip-card') as HTMLElement; + const restore = card.querySelector('[aria-label="Restore"]') as HTMLElement; + expect(restore).toBeTruthy(); + // lucide stamps the icon name on the svg, which is how the archive and the + // restore variant tell themselves apart. + expect(restore.querySelector('svg')?.getAttribute('class')).toContain('archive-restore'); + expect(card.querySelector('[aria-label="Archive"]')).toBeNull(); + }); + }); }); diff --git a/client/src/pages/DashboardPage.tsx b/client/src/pages/DashboardPage.tsx index f1613eb9c..dad6a303c 100644 --- a/client/src/pages/DashboardPage.tsx +++ b/client/src/pages/DashboardPage.tsx @@ -15,7 +15,7 @@ import { MS_PER_DAY, daysUntil, getTripStatus, } from './dashboard/dashboardModel' import { - Plus, Edit2, Trash2, Archive, Copy, ArrowRight, MapPin, + Plus, Edit2, Trash2, Archive, ArchiveRestore, Copy, ArrowRight, MapPin, Plane, Hotel, Utensils, Clock, RefreshCw, ArrowRightLeft, Calendar, LayoutGrid, List, Ticket, X, CalendarPlus, ParkingSquare, } from 'lucide-react' @@ -304,6 +304,7 @@ function BoardingPassHero({ trip, bundle, locale, onOpen, onEdit, onCopy, onArch const heroPlugins = usePluginStore(s => s.plugins).filter(p => p.type === 'widget' && p.slot === 'hero') const stop = (e: React.MouseEvent, fn: () => void) => { e.stopPropagation(); fn() } const status = getTripStatus(trip) + const archiveLabel = trip.is_archived ? t('dashboard.restore') : t('dashboard.archive') const start = splitDate(trip.start_date, locale) const end = splitDate(trip.end_date, locale) @@ -404,10 +405,10 @@ function BoardingPassHero({ trip, bundle, locale, onOpen, onEdit, onCopy, onArch {badge}
- - - - + + + +
@@ -535,6 +536,7 @@ function TripCard({ trip, locale, badges, onOpen, onEdit, onCopy, onArchive, onD }): React.ReactElement { const { t } = useTranslation() const status = getTripStatus(trip) + const archiveLabel = trip.is_archived ? t('dashboard.restore') : t('dashboard.archive') const start = splitDate(trip.start_date, locale) const end = splitDate(trip.end_date, locale) const until = daysUntil(trip.start_date) @@ -557,10 +559,10 @@ function TripCard({ trip, locale, badges, onOpen, onEdit, onCopy, onArchive, onD :
}
{statusLabel}
- - - - + + + +

{trip.title}

diff --git a/client/src/pages/atlas/useAtlas.ts b/client/src/pages/atlas/useAtlas.ts index 293aa23c9..140f386a3 100644 --- a/client/src/pages/atlas/useAtlas.ts +++ b/client/src/pages/atlas/useAtlas.ts @@ -6,7 +6,7 @@ import apiClient, { mapsApi, pluginsApi, type PluginAtlasLayer } from '../../api import L from 'leaflet' import type { GeoJsonFeatureCollection } from '../../types' import { A2_TO_A3, countryStatus, findBucketDuplicate, isBucketDuplicateError, isCountryVisible, normalizeRegionName, withCountryMarkedVisited, wishlistA3Codes, countryColor, type AtlasData, type AtlasPlaceHit, type CountryDetail, type BucketItem } from './atlasModel' -import { continentForCountry, type VisitStatus } from '@trek/shared' +import { continentForCountry, escapeHtml, type VisitStatus } from '@trek/shared' import { useToast } from '../../components/shared/Toast' import { getApiErrorMessage } from '../../types' @@ -941,7 +941,7 @@ export function useAtlas() { iconAnchor: [14, 14], }) return L.marker([b.lat!, b.lng!], { icon }).bindTooltip( - `
${b.name}
${b.notes ? `
${b.notes}
` : ''}`, + `
${escapeHtml(b.name)}
${b.notes ? `
${escapeHtml(b.notes)}
` : ''}`, { className: 'atlas-tooltip', direction: 'top', offset: [0, -14] } ) }) diff --git a/client/src/styles/dashboard.css b/client/src/styles/dashboard.css index 9e509b44a..1e0dcea34 100644 --- a/client/src/styles/dashboard.css +++ b/client/src/styles/dashboard.css @@ -191,11 +191,12 @@ .trek-dash .hero-top { display: flex; justify-content: space-between; align-items: start; } .trek-dash .hero-badge { display: inline-flex; align-items: center; gap: 8px; - background: oklch(1 0 0 / .14); + background: oklch(0.22 0.01 260 / .5); backdrop-filter: blur(14px) saturate(1.4); -webkit-backdrop-filter: blur(14px) saturate(1.4); - border: 1px solid oklch(1 0 0 / .2); padding: 8px 14px 8px 12px; + border: 1px solid oklch(1 0 0 / .32); padding: 8px 14px 8px 12px; border-radius: 999px; font-size: 12px; font-weight: 500; letter-spacing: 0.06em; text-transform: uppercase; + box-shadow: 0 2px 8px oklch(0 0 0 / .26); } .trek-dash .hero-badge .pulse { width: 7px; height: 7px; border-radius: 50%; @@ -208,15 +209,18 @@ 100% { box-shadow: 0 0 0 0 oklch(0.84 0.18 85 / 0); } } .trek-dash .hero-tools { display: flex; gap: 8px; } +/* Same story as .trip-action-btn: a dark scrim instead of a white .14 fill, so + the tools read against a bright hero photo too. */ .trek-dash .hero-tool { - width: 38px; height: 38px; border-radius: 50%; - background: oklch(1 0 0 / .14); + width: 40px; height: 40px; border-radius: 50%; + background: oklch(0.22 0.01 260 / .5); backdrop-filter: blur(14px) saturate(1.4); -webkit-backdrop-filter: blur(14px) saturate(1.4); - border: 1px solid oklch(1 0 0 / .2); color: #fff; + border: 1px solid oklch(1 0 0 / .32); color: #fff; + box-shadow: 0 2px 8px oklch(0 0 0 / .26); display: grid; place-items: center; transition: background .15s, transform .12s; } -.trek-dash .hero-tool:hover { background: oklch(1 0 0 / .26); transform: scale(1.05); } -.trek-dash .hero-tool svg { width: 16px; height: 16px; } +.trek-dash .hero-tool:hover { background: oklch(0.22 0.01 260 / .78); transform: scale(1.05); } +.trek-dash .hero-tool svg { width: 17px; height: 17px; } .trek-dash .hero-title-block { align-self: end; padding-bottom: 24px; } .trek-dash .hero-eyebrow { display: inline-flex; align-items: center; gap: 10px; @@ -435,26 +439,36 @@ .trek-dash .trip-cover img { width: 100%; height: 100%; object-fit: cover; transition: transform .6s cubic-bezier(.2,.7,.2,1); } .trek-dash .trip-card:hover .trip-cover img { transform: scale(1.04); } .trek-dash .trip-cover::after { content: ""; position: absolute; inset: 0; background: linear-gradient(180deg, oklch(0 0 0 / 0) 40%, oklch(0 0 0 / .55) 100%); } +/* Shares the dark scrim of .trip-action-btn so both overlays on the cover read + as one set, and the label survives a bright photo behind it. */ .trek-dash .trip-status { position: absolute; top: 16px; left: 16px; z-index: 1; display: inline-flex; align-items: center; gap: 7px; padding: 6px 11px 6px 9px; border-radius: 999px; font-size: 11.5px; font-weight: 500; letter-spacing: 0.02em; - background: oklch(1 0 0 / .18); backdrop-filter: blur(14px) saturate(1.4); -webkit-backdrop-filter: blur(14px) saturate(1.4); - border: 1px solid oklch(1 0 0 / .22); color: #fff; text-transform: uppercase; + background: oklch(0.22 0.01 260 / .58); backdrop-filter: blur(14px) saturate(1.4); -webkit-backdrop-filter: blur(14px) saturate(1.4); + border: 1px solid oklch(1 0 0 / .34); color: #fff; text-transform: uppercase; + box-shadow: 0 2px 8px oklch(0 0 0 / .28); } .trek-dash .trip-status .indicator { width: 6px; height: 6px; border-radius: 50%; background: oklch(0.84 0.16 145); } .trek-dash .trip-status.completed .indicator { background: oklch(0.75 0.02 220); } .trek-dash .trip-status.upcoming .indicator { background: oklch(0.84 0.18 85); } .trek-dash .trip-status.idea .indicator { background: oklch(0.78 0.14 280); } -.trek-dash .trip-actions { position: absolute; top: 14px; right: 14px; z-index: 1; display: flex; gap: 6px; opacity: 0; transition: opacity .2s; } +/* The actions used to sit at opacity 0 until the card was hovered, over a white + .18 glass fill. Two people in a row reported they never found the archive and + rename buttons — invisible at rest, and on a light cover the white-on-white + fill hid them even while hovering. They stay on the card now, and the fill is + a dark scrim so the white icons carry against any photo. */ +.trek-dash .trip-actions { position: absolute; top: 14px; right: 14px; z-index: 1; display: flex; gap: 6px; opacity: .92; transition: opacity .2s; } .trek-dash .trip-card:hover .trip-actions { opacity: 1; } .trek-dash .trip-action-btn { - width: 36px; height: 36px; border-radius: 50%; - background: oklch(1 0 0 / .18); backdrop-filter: blur(14px); -webkit-backdrop-filter: blur(14px); - border: 1px solid oklch(1 0 0 / .22); color: #fff; display: grid; place-items: center; transition: background .15s; + width: 38px; height: 38px; border-radius: 50%; + background: oklch(0.22 0.01 260 / .58); backdrop-filter: blur(14px); -webkit-backdrop-filter: blur(14px); + border: 1px solid oklch(1 0 0 / .34); color: #fff; display: grid; place-items: center; + box-shadow: 0 2px 8px oklch(0 0 0 / .28); + transition: background .15s, transform .12s; } -.trek-dash .trip-action-btn:hover { background: oklch(1 0 0 / .3); } -.trek-dash .trip-action-btn svg { width: 16px; height: 16px; } +.trek-dash .trip-action-btn:hover { background: oklch(0.22 0.01 260 / .82); transform: scale(1.06); } +.trek-dash .trip-action-btn svg { width: 17px; height: 17px; } .trek-dash .trip-cover-content { position: absolute; left: 18px; right: 18px; bottom: 16px; z-index: 1; color: #fff; } .trek-dash .trip-name { font-size: 26px; font-weight: 600; letter-spacing: -0.025em; line-height: 1.05; margin: 0; text-shadow: 0 1px 7px oklch(0 0 0 / .3), 0 1px 2px oklch(0 0 0 / .38); } .trek-dash .trip-where { margin-top: 4px; font-size: 13px; opacity: .85; display: flex; align-items: center; gap: 6px; } @@ -686,8 +700,8 @@ .trek-dash .trips { grid-template-columns: 1fr; gap: 16px; margin-bottom: 28px; } .trek-dash .add-trip-card { min-height: 180px; } - /* Touch devices have no hover — keep the edit/copy/archive/delete actions - visible at all times instead of revealing them on hover. */ + /* Touch devices have no hover state to lift the actions to full strength, so + skip the resting dim entirely. */ .trek-dash .trip-actions { opacity: 1; } /* Compact list row on mobile — keeps the list view distinct from the grid. The diff --git a/client/src/utils/safeUrl.test.ts b/client/src/utils/safeUrl.test.ts new file mode 100644 index 000000000..f908a67b1 --- /dev/null +++ b/client/src/utils/safeUrl.test.ts @@ -0,0 +1,35 @@ +import { describe, it, expect } from 'vitest' +import { safeHttpUrl } from './safeUrl' + +describe('safeHttpUrl', () => { + it('FE-UTIL-SAFEURL-001: passes an ordinary https homepage through', () => { + expect(safeHttpUrl('https://louvre.fr/en/visit')).toBe('https://louvre.fr/en/visit') + }) + + it('FE-UTIL-SAFEURL-002: passes plain http through — plenty of small venues still run it', () => { + expect(safeHttpUrl('http://pension-alpenblick.at')).toBe('http://pension-alpenblick.at') + }) + + it('FE-UTIL-SAFEURL-003: rejects javascript:, which window.open would run in this origin', () => { + expect(safeHttpUrl('javascript:fetch("/api/trips")')).toBeNull() + expect(safeHttpUrl('JavaScript:alert(1)')).toBeNull() + expect(safeHttpUrl(' javascript:alert(1)')).toBeNull() + }) + + it('FE-UTIL-SAFEURL-004: rejects the other schemes that are not a web page', () => { + expect(safeHttpUrl('data:text/html,')).toBeNull() + expect(safeHttpUrl('vbscript:msgbox(1)')).toBeNull() + expect(safeHttpUrl('file:///etc/passwd')).toBeNull() + }) + + it('FE-UTIL-SAFEURL-005: rejects a bare host, since window.open would treat it as a relative path', () => { + expect(safeHttpUrl('louvre.fr')).toBeNull() + expect(safeHttpUrl('//evil.example')).toBeNull() + }) + + it('FE-UTIL-SAFEURL-006: treats absent and empty as nothing to open', () => { + expect(safeHttpUrl(null)).toBeNull() + expect(safeHttpUrl(undefined)).toBeNull() + expect(safeHttpUrl('')).toBeNull() + }) +}) diff --git a/client/src/utils/safeUrl.ts b/client/src/utils/safeUrl.ts new file mode 100644 index 000000000..a89390fa1 --- /dev/null +++ b/client/src/utils/safeUrl.ts @@ -0,0 +1,18 @@ +import { placeWebsiteSchema } from '@trek/shared' + +/** + * Allow-list a URL before handing it to window.open. + * + * A place's website is a plain string in the older contracts, and it reaches + * window.open from the inspector, both sidebar context menus and the mobile + * place sheet. `window.open('javascript:…')` does not open a page — it evaluates + * the script in a fresh document that inherits this origin, so a co-traveller + * with place_edit could plant one and wait for someone to click Website. + * + * Server-side validation closes the write path. This closes the read path, which + * is what covers rows written before that validation existed and instances that + * have not updated yet — the same split as safeHexColor. + */ +export function safeHttpUrl(value: string | null | undefined): string | null { + return typeof value === 'string' && placeWebsiteSchema.safeParse(value).success ? value : null +} diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index df95ecacb..0d6b93752 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -3891,6 +3891,24 @@ function runMigrations(db: Database.Database): void { db.exec('ALTER TABLE budget_items ADD COLUMN place_id INTEGER REFERENCES places(id) ON DELETE SET NULL DEFAULT NULL'); } }, + // A reservation_day_positions row only makes sense when its reservation and + // its day are on the same trip. The table carries no trip_id and its two + // foreign keys only ask that the ids exist, so pairs that never belonged + // together could accumulate; the writer refuses them now, and this clears + // whatever an older build let through. This slot keeps the index it shipped + // with on dev — the array is index-addressed against schema_version. + () => { + db.exec(` + DELETE FROM reservation_day_positions + WHERE rowid IN ( + SELECT rdp.rowid + FROM reservation_day_positions rdp + JOIN reservations r ON r.id = rdp.reservation_id + JOIN days d ON d.id = rdp.day_id + WHERE d.trip_id <> r.trip_id + ) + `); + }, // Storage slice 2 — collab note attachments historically stored 'files/' // in trip_files.filename while the file manager stored bare names in the same // column; the storage layer addresses objects as category + bare name, so diff --git a/server/src/nest/assignments/assignments.controller.ts b/server/src/nest/assignments/assignments.controller.ts index d2c0dbecd..7f274946c 100644 --- a/server/src/nest/assignments/assignments.controller.ts +++ b/server/src/nest/assignments/assignments.controller.ts @@ -113,9 +113,14 @@ export class DayAssignmentsController { /** * /api/trips/:tripId/assignments/:id/* — per-assignment ops (move, time, * participants), independent of the day path. Same parity rules as above. + * + * Same guard pair as the day controller, and for the same reason: TripAccessGuard + * is what resolves :tripId and what reads @RequirePermission. The per-handler + * getAssignmentForTrip calls answer a different question — whether the assignment + * sits on the trip in the URL, not whether the caller may be on that trip. */ @Controller('api/trips/:tripId/assignments') -@UseGuards(JwtAuthGuard) +@UseGuards(JwtAuthGuard, TripAccessGuard) export class AssignmentOpsController { constructor(private readonly assignments: AssignmentsService) {} @@ -142,6 +147,9 @@ export class AssignmentOpsController { @Get(':id/participants') participants(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string) { + if (!this.assignments.getAssignmentForTrip(id, tripId)) { + throw new HttpException({ error: 'Assignment not found' }, 404); + } return { participants: this.assignments.getParticipants(id) }; } @@ -191,7 +199,10 @@ export class AssignmentOpsController { @Body() body: AssignmentParticipantsDto, @Headers('x-socket-id') socketId?: string, ) { - const participants = this.assignments.setParticipants(id, body.user_ids); + if (!this.assignments.getAssignmentForTrip(id, tripId)) { + throw new HttpException({ error: 'Assignment not found' }, 404); + } + const participants = this.assignments.setParticipants(id, body.user_ids, tripId); this.assignments.broadcast(tripId, 'assignment:participants', { assignmentId: Number(id), participants }, socketId); return { participants }; } diff --git a/server/src/nest/assignments/assignments.mcp.ts b/server/src/nest/assignments/assignments.mcp.ts index 68b843a96..989a7a727 100644 --- a/server/src/nest/assignments/assignments.mcp.ts +++ b/server/src/nest/assignments/assignments.mcp.ts @@ -187,7 +187,7 @@ export class AssignmentsMcp { if (!this.assignments.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!this.guards.hasTripPermission('day_edit', tripId, ctx.userId)) return permissionDenied(); if (!this.assignments.getAssignmentForTrip(assignmentId, tripId)) return errorResult('Assignment not found.'); - const participants = this.assignments.setParticipants(assignmentId, userIds); + const participants = this.assignments.setParticipants(assignmentId, userIds, tripId); this.guards.safeBroadcast(tripId, 'assignment:participants', { assignmentId, participants }); return ok({ participants }); } diff --git a/server/src/nest/assignments/assignments.service.ts b/server/src/nest/assignments/assignments.service.ts index 379943811..b53b657e2 100644 --- a/server/src/nest/assignments/assignments.service.ts +++ b/server/src/nest/assignments/assignments.service.ts @@ -283,12 +283,21 @@ export class AssignmentsService { return this.getAssignmentWithPlace(Number(id)); } - setParticipants(assignmentId: string | number, userIds: number[]) { + /** + * Both callers have already proven the assignment sits on `tripId`; this + * settles the other half, that the ids in the body do too. Off-roster ids drop + * silently rather than 400, matching bag members and reservation travellers — + * the participants box sends the whole list back on every edit, so rejecting + * the request would strand a trip whose membership changed underneath it. + */ + setParticipants(assignmentId: string | number, userIds: number[], tripId: string | number) { + const roster = this.dbs.rosterUserIds(tripId); + const scoped = userIds.filter(id => roster.has(id)); this.dbs.transaction(() => { this.dbs.run('DELETE FROM assignment_participants WHERE assignment_id = ?', assignmentId); - if (userIds.length > 0) { + if (scoped.length > 0) { const insert = this.dbs.prepare('INSERT OR IGNORE INTO assignment_participants (assignment_id, user_id) VALUES (?, ?)'); - for (const userId of userIds) insert.run(assignmentId, userId); + for (const userId of scoped) insert.run(assignmentId, userId); } }); diff --git a/server/src/nest/budget/budget.controller.ts b/server/src/nest/budget/budget.controller.ts index ce9a84096..319c3ded6 100644 --- a/server/src/nest/budget/budget.controller.ts +++ b/server/src/nest/budget/budget.controller.ts @@ -94,6 +94,11 @@ export class BudgetController { { from_user_id: body.from_user_id, to_user_id: body.to_user_id, amount: body.amount, currency: body.currency }, user.id, ); + // A party who is not on this trip gets the same answer as a settlement that + // does not exist, so the endpoint cannot be used to probe for user ids. + if (!settlement) { + throw new HttpException({ error: 'Settlement not found' }, 404); + } this.budget.broadcast(tripId, 'budget:settlement-created', { settlement }, socketId); return { settlement }; } diff --git a/server/src/nest/budget/budget.mcp.ts b/server/src/nest/budget/budget.mcp.ts index 49f1602dd..5d2b74217 100644 --- a/server/src/nest/budget/budget.mcp.ts +++ b/server/src/nest/budget/budget.mcp.ts @@ -330,6 +330,7 @@ export class BudgetMcp { // Freeze-then-write composite, same as the REST path (no-op while the // schema has no currency input — see the #1445 note in the class doc). const settlement = await this.budget.createSettlement(tripId, { from_user_id, to_user_id, amount }, ctx.userId); + if (!settlement) return errorResult('Settlement not found.'); this.guards.safeBroadcast(tripId, 'budget:settlement-created', { settlement }); return ok({ settlement }); } diff --git a/server/src/nest/budget/budget.service.ts b/server/src/nest/budget/budget.service.ts index c5c3556ae..0239033ad 100644 --- a/server/src/nest/budget/budget.service.ts +++ b/server/src/nest/budget/budget.service.ts @@ -126,21 +126,25 @@ export class BudgetService { return rows.map(p => ({ ...p, avatar_url: avatarUrl(p) })); } - private knownUserIds(userIds: number[]): Set { - const unique = [...new Set(userIds)]; - if (unique.length === 0) return new Set(); - const rows = this.db.all<{ id: number }>( - `SELECT id FROM users WHERE id IN (${unique.map(() => '?').join(',')})`, - ...unique, - ); - return new Set(rows.map(r => r.id)); + /** + * The subset of `userIds` that is actually on this trip. Used to be a plain + * existence check against `users`, which let any id on the instance become a + * payer or a split member and come back out through the read-back join with a + * name and an avatar attached. Off-roster ids drop silently, the way the + * packing and reservations assignee paths handle them. + */ + private rosterMemberIds(tripId: string | number, userIds: number[]): Set { + const unique = new Set(userIds); + if (unique.size === 0) return new Set(); + const roster = this.db.rosterUserIds(tripId); + return new Set([...unique].filter(id => roster.has(id))); } /** Replace the payer rows of an item and keep total_price = sum of payer amounts. */ - private writeItemPayers(itemId: number | string, payers: { user_id: number; amount: number }[]) { + private writeItemPayers(itemId: number | string, tripId: string | number, payers: { user_id: number; amount: number }[]) { this.db.run('DELETE FROM budget_item_payers WHERE budget_item_id = ?', itemId); const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_payers (budget_item_id, user_id, amount) VALUES (?, ?, ?)'); - const known = this.knownUserIds(payers.map(p => p.user_id)); + const known = this.rosterMemberIds(tripId, payers.map(p => p.user_id)); let total = 0; for (const p of payers) { if (!(p.amount > 0) || !known.has(p.user_id)) continue; @@ -347,9 +351,9 @@ export class BudgetService { const payerTotal = (data.payers || []).reduce((a, p) => a + (p.amount > 0 ? p.amount : 0), 0); const total = data.payers && data.payers.length > 0 ? payerTotal : (data.total_price || 0); - const knownMembers = data.members ? this.knownUserIds(data.members.map(m => m.user_id)) : null; + const knownMembers = data.members ? this.rosterMemberIds(tripId, data.members.map(m => m.user_id)) : null; const members = data.members && knownMembers ? data.members.filter(m => knownMembers.has(m.user_id)) : undefined; - const knownIds = data.member_ids ? this.knownUserIds(data.member_ids) : null; + const knownIds = data.member_ids ? this.rosterMemberIds(tripId, data.member_ids) : null; const memberIds = data.member_ids && knownIds ? data.member_ids.filter(uid => knownIds.has(uid)) : undefined; const { note, ticket } = splitLegacyTicketNote(data.note, data.ticket_json); @@ -373,7 +377,7 @@ export class BudgetService { ); const itemId = result.lastInsertRowid as number; - if (data.payers && data.payers.length > 0) this.writeItemPayers(itemId, data.payers); + if (data.payers && data.payers.length > 0) this.writeItemPayers(itemId, tripId, data.payers); if (members && members.length > 0) { const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, ?)'); for (const m of members) insert.run(itemId, m.user_id, m.amount !== undefined && m.amount !== null ? m.amount : null); @@ -461,7 +465,7 @@ export class BudgetService { // Optional inline payer/member replacement (the edit modal saves all at once). if (data.payers !== undefined) { - this.writeItemPayers(id, data.payers); + this.writeItemPayers(id, tripId, data.payers); // writeItemPayers derives total_price from the payer sum (0 for no payers). // A "recorded total, nobody assigned" expense clears payers but still carries // an explicit total_price — re-apply it so it isn't clobbered to 0. @@ -470,14 +474,14 @@ export class BudgetService { } } if (data.members !== undefined) { - const known = this.knownUserIds(data.members.map(m => m.user_id)); + const known = this.rosterMemberIds(tripId, data.members.map(m => m.user_id)); const members = data.members.filter(m => known.has(m.user_id)); this.db.run('DELETE FROM budget_item_members WHERE budget_item_id = ?', id); const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, ?)'); for (const m of members) insert.run(id, m.user_id, m.amount !== undefined && m.amount !== null ? m.amount : null); this.db.run('UPDATE budget_items SET persons = ? WHERE id = ?', members.length || null, id); } else if (data.member_ids !== undefined) { - const known = this.knownUserIds(data.member_ids); + const known = this.rosterMemberIds(tripId, data.member_ids); const memberIds = data.member_ids.filter(uid => known.has(uid)); this.db.run('DELETE FROM budget_item_members WHERE budget_item_id = ?', id); const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, NULL)'); @@ -510,7 +514,7 @@ export class BudgetService { return this.db.transaction(() => { const item = this.db.get('SELECT id FROM budget_items WHERE id = ? AND trip_id = ?', id, tripId); if (!item) return null; - this.writeItemPayers(id, payers); + this.writeItemPayers(id, tripId, payers); const updated = this.db.get('SELECT * FROM budget_items WHERE id = ?', id)!; updated.members = this.loadItemMembers(id); updated.payers = this.loadItemPayers(id); @@ -540,7 +544,7 @@ export class BudgetService { this.db.run('DELETE FROM budget_item_members WHERE budget_item_id = ?', id); - const known = this.knownUserIds(userIds); + const known = this.rosterMemberIds(tripId, userIds); const memberIds = userIds.filter(uid => known.has(uid)); if (memberIds.length > 0) { const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid) VALUES (?, ?, ?)'); @@ -949,7 +953,21 @@ export class BudgetService { return this.setItemPayers(id, tripId, payers); } + /** + * Unlike a split member, a settlement cannot drop an off-roster id: it has + * exactly two named parties and both columns are NOT NULL, so a stranger in + * either slot makes the row meaningless rather than merely wider. Refuse the + * whole write. Returning null lands on the caller's existing "Settlement not + * found" 404, which is also what keeps the endpoint from confirming whether + * an id it rejected exists at all. + */ + private settlementPartiesOnTrip(tripId: string | number, data: { from_user_id: number; to_user_id: number }): boolean { + const roster = this.db.rosterUserIds(tripId); + return roster.has(data.from_user_id) && roster.has(data.to_user_id); + } + async createSettlement(tripId: string | number, data: { from_user_id: number; to_user_id: number; amount: number; currency?: string | null }, userId: number) { + if (!this.settlementPartiesOnTrip(tripId, data)) return null; // Freeze the FX rate for the display currency the amount was entered in so the // transfer keeps cancelling its expense when live rates drift (#1445). await this.freezeForeignRate(tripId, data); @@ -960,6 +978,7 @@ export class BudgetService { // Pass the settlement's stored currency so an edit that doesn't change it keeps // the already-frozen rate (#1445) — otherwise a live-rate drift would re-open a // settled position on an unrelated edit. + if (!this.settlementPartiesOnTrip(tripId, data)) return null; const existing = this.getSettlement(id, tripId); await this.freezeForeignRate(tripId, data, undefined, existing?.currency ?? null); return this.applySettlementUpdate(id, tripId, data); diff --git a/server/src/nest/collections/collections.service.ts b/server/src/nest/collections/collections.service.ts index 450e2ee99..51f3a0349 100644 --- a/server/src/nest/collections/collections.service.ts +++ b/server/src/nest/collections/collections.service.ts @@ -446,10 +446,23 @@ export class CollectionsService { // Saved places CRUD // ------------------------------------------------------------------------- + /** + * A tag has no list of its own, only a `user_id`, so "belongs here" resolves + * through the people on the list — the same shape places.service uses against + * a trip roster. Off-list tag ids drop silently rather than being stored and + * read straight back out: the tag read-back ships `tags.user_id`, so an + * unfiltered id answers who owns a tag the caller cannot otherwise see. + */ private attachTags(collectionPlaceId: number, tagIds: number[] | undefined): void { if (!tagIds || tagIds.length === 0) return; + const unique = [...new Set(tagIds)]; + const eligible = this.collectionMemberIds(this.collectionIdOfPlace(collectionPlaceId)); + const owned = this.db.all<{ id: number; user_id: number }>( + `SELECT id, user_id FROM tags WHERE id IN (${unique.map(() => '?').join(',')})`, + ...unique, + ); const stmt = this.db.prepare('INSERT OR IGNORE INTO collection_place_tags (collection_place_id, tag_id) VALUES (?, ?)'); - for (const tid of tagIds) stmt.run(collectionPlaceId, tid); + for (const t of owned) if (eligible.has(t.user_id)) stmt.run(collectionPlaceId, t.id); } /** Owner + accepted members — the users whose votes may live in this list. */ diff --git a/server/src/nest/database/database.service.ts b/server/src/nest/database/database.service.ts index 0d18ea549..103f4c8e1 100644 --- a/server/src/nest/database/database.service.ts +++ b/server/src/nest/database/database.service.ts @@ -56,6 +56,25 @@ export class DatabaseService { return isOwner(tripId, userId); } + /** + * The user ids a trip may refer to: its members plus the owner. Guests count — + * a guest is a credential-less users row joined into trip_members, and #1362 + * makes it assignable everywhere a real member is. + * + * This is canAccessTrip's question asked as a set rather than a predicate, so + * it lives beside it. Domains that accept user ids in a write body intersect + * against it: holding the permission says you may edit the trip, not that any + * id you name belongs to it. Runs on the connection rather than delegating to + * db/database, because it has no legacy export to stay parity with. + */ + rosterUserIds(tripId: number | string): Set { + const rows = this.all<{ user_id: number }>( + 'SELECT user_id FROM trip_members WHERE trip_id = ? UNION SELECT user_id FROM trips WHERE id = ?', + tripId, tripId, + ); + return new Set(rows.map(r => r.user_id)); + } + getPlaceWithTags(placeId: number | string): PlaceWithTags | null { return getPlaceWithTags(placeId); } diff --git a/server/src/nest/packing/packing.service.ts b/server/src/nest/packing/packing.service.ts index 1fef2f8a3..01c169ccb 100644 --- a/server/src/nest/packing/packing.service.ts +++ b/server/src/nest/packing/packing.service.ts @@ -231,7 +231,8 @@ export class PackingService { // "Shared with specific people" — record the recipients it covers. if (data.visibility === 'shared' && Array.isArray(data.recipient_ids)) { const ins = this.db.prepare('INSERT OR IGNORE INTO packing_item_recipients (item_id, user_id) VALUES (?, ?)'); - for (const uid of data.recipient_ids) if (uid !== ownerId) ins.run(id, uid); + const roster = this.tripRosterIds(tripId); + for (const uid of data.recipient_ids) if (uid !== ownerId && roster.has(uid)) ins.run(id, uid); } return id; }); @@ -350,7 +351,8 @@ export class PackingService { if (visibility === 'shared') { const ins = this.db.prepare('INSERT OR IGNORE INTO packing_item_recipients (item_id, user_id) VALUES (?, ?)'); const owner = item.owner_id ?? actingUserId; - for (const uid of recipientIds) if (uid !== owner) ins.run(id, uid); + const roster = this.tripRosterIds(tripId); + for (const uid of recipientIds) if (uid !== owner && roster.has(uid)) ins.run(id, uid); } // Leaving the Common tier drops any co-contributors (they only apply to Common). if (visibility !== 'common') this.db.run('DELETE FROM packing_item_contributors WHERE item_id = ?', id); @@ -479,10 +481,14 @@ export class PackingService { })); } - /** Owner + collaborators of a trip — the only user ids that may be assigned to a bag. */ + /** + * Owner + collaborators of a trip, guests included — the only user ids + * assignable anywhere on it, not just to a bag. The wording used to say + * "assigned to a bag", which is why the category and recipient writes below + * grew up without it. + */ private tripRosterIds(tripId: string | number): Set { - const rows = this.db.all<{ user_id: number }>('SELECT user_id FROM trip_members WHERE trip_id = ? UNION SELECT user_id FROM trips WHERE id = ?', tripId, tripId); - return new Set(rows.map(r => r.user_id)); + return this.db.rosterUserIds(tripId); } setBagMembers(tripId: string | number, bagId: string | number, userIds: number[]) { @@ -665,7 +671,9 @@ export class PackingService { if (Array.isArray(userIds) && userIds.length > 0) { const insert = this.db.prepare('INSERT OR IGNORE INTO packing_category_assignees (trip_id, category_name, user_id) VALUES (?, ?, ?)'); - for (const uid of userIds) insert.run(tripId, categoryName, uid); + // Same rule as setBagMembers: only people on this trip may be assigned. + const roster = this.tripRosterIds(tripId); + for (const uid of userIds) if (roster.has(uid)) insert.run(tripId, categoryName, uid); } }); diff --git a/server/src/nest/places/places.controller.ts b/server/src/nest/places/places.controller.ts index f1eac2207..eb5775ac6 100644 --- a/server/src/nest/places/places.controller.ts +++ b/server/src/nest/places/places.controller.ts @@ -20,7 +20,7 @@ import type { Response } from 'express'; import { isDemoWriteBlocked, DEMO_WRITE_ERROR } from '../common/demo-write'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; import { memoryStorage } from 'multer'; -import { hexColorSchema } from '@trek/shared'; +import { hexColorSchema, placeImageUrlSchema, placeWebsiteSchema } from '@trek/shared'; import type { User } from '../../types'; import { PlacesService } from './places.service'; import { isUpdateConflict } from '../common/conflictResult'; @@ -64,6 +64,25 @@ function validateRouteColor(body: Record): void { } } +// Same reason as route_color: these two leave the database for a renderer that +// treats them as a URL — the thumbnail into marker HTML, the homepage into +// window.open — and the open record means the pipe never sees them. +function validateUrlFields(body: Record): void { + const image = body.image_url; + if (image !== undefined && image !== null) { + if (typeof image !== 'string' || !placeImageUrlSchema.safeParse(image).success) { + throw new HttpException({ error: 'image_url must be an uploaded path, a photo-proxy path, an inline image or an https URL' }, 400); + } + } + const website = body.website; + // '' is how the UI clears the field; treat it like an absent value. + if (website !== undefined && website !== null && website !== '') { + if (typeof website !== 'string' || !placeWebsiteSchema.safeParse(website).success) { + throw new HttpException({ error: 'website must be an http or https URL' }, 400); + } + } +} + function parseBool(v: unknown, defaultVal: boolean): boolean { return v === undefined || v === null ? defaultVal : String(v) === 'true'; } @@ -133,6 +152,7 @@ export class PlacesController { const trip = this.requireTrip(tripId, user); validateLengths(body); validateRouteColor(body); + validateUrlFields(body); this.requireEdit(trip, user); const place = this.places.create(tripId, body as never); this.places.broadcast(tripId, 'place:created', { place }, socketId); @@ -433,6 +453,7 @@ export class PlacesController { const trip = this.requireTrip(tripId, user); validateLengths(body); validateRouteColor(body); + validateUrlFields(body); this.requireEdit(trip, user); const result = await this.places.update(tripId, id, body as never, ifMatch); if (!result) { diff --git a/server/src/nest/places/places.mcp.ts b/server/src/nest/places/places.mcp.ts index eb7d0a873..7399b4294 100644 --- a/server/src/nest/places/places.mcp.ts +++ b/server/src/nest/places/places.mcp.ts @@ -5,6 +5,7 @@ import { demoDenied, ok, } from '@trek/nest-mcp'; import { McpToolGuardsService } from '../mcp-shared/mcp-tool-guards.service'; +import { placeWebsiteSchema } from '@trek/shared'; import { z } from 'zod'; import { AuthService } from '../auth/auth.service'; import { AssignmentsService } from '../assignments/assignments.service'; @@ -63,7 +64,7 @@ export class PlacesMcp { google_ftid: z.string().optional().describe('Google Maps feature ID from search_place — enables direct Google Maps links'), osm_id: z.string().optional().describe('OpenStreetMap ID from search_place (e.g. "way:12345") — enables opening hours if no Google ID'), notes: z.string().max(2000).optional(), - website: z.string().max(500).optional(), + website: placeWebsiteSchema.optional(), phone: z.string().max(50).optional(), price: z.number().nonnegative().optional().describe('Cost of this place/activity (e.g. ticket price, entry fee)'), currency: z.string().length(3).optional().describe('ISO 4217 currency code (e.g. "EUR", "USD")'), @@ -103,7 +104,7 @@ export class PlacesMcp { google_ftid: z.string().optional().describe('Google Maps feature ID from search_place — enables direct Google Maps links'), osm_id: z.string().optional().describe('OpenStreetMap ID from search_place (e.g. "way:12345")'), place_notes: z.string().max(2000).optional().describe('Notes for the place'), - website: z.string().max(500).optional(), + website: placeWebsiteSchema.optional(), phone: z.string().max(50).optional(), assignment_notes: z.string().max(500).optional().describe('Notes for this day assignment'), price: z.number().nonnegative().optional().describe('Cost of this place/activity (e.g. ticket price, entry fee)'), @@ -158,7 +159,7 @@ export class PlacesMcp { end_time: z.string().max(50).optional().describe('End time (e.g. "11:00")'), duration_minutes: z.number().int().positive().optional(), notes: z.string().max(2000).optional(), - website: z.string().max(500).optional(), + website: placeWebsiteSchema.optional(), phone: z.string().max(50).optional(), transport_mode: z.enum(['walking', 'driving', 'cycling', 'transit', 'flight']).optional(), osm_id: z.string().optional().describe('OpenStreetMap ID (e.g. "way:12345")'), @@ -366,7 +367,7 @@ export class PlacesMcp { end_time: z.string().max(50).optional().describe('End time (e.g. "11:00")'), duration_minutes: z.number().int().positive().optional(), notes: z.string().max(2000).optional(), - website: z.string().max(500).optional(), + website: placeWebsiteSchema.optional(), phone: z.string().max(50).optional(), description: z.string().max(2000).optional(), }, diff --git a/server/src/nest/places/places.rpc.ts b/server/src/nest/places/places.rpc.ts index 3f61da812..91f20afa6 100644 --- a/server/src/nest/places/places.rpc.ts +++ b/server/src/nest/places/places.rpc.ts @@ -1,4 +1,4 @@ -import { placeCreateRequestSchema, placeUpdateRequestSchema } from '@trek/shared'; +import { placeCreateRequestSchema, placeImageUrlSchema, placeUpdateRequestSchema, placeWebsiteSchema } from '@trek/shared'; import { PluginController, PluginMethod } from '../plugins/host/rpc-kit/decorators'; import { PluginGuards } from '../plugins/host/plugin-guards.service'; import { BadParams, ForbiddenResource } from '../plugins/host/rpc-errors'; @@ -18,6 +18,23 @@ const PLACE_EDIT_ACTION = 'place_edit'; */ const PLACE_STR_LIMITS: Record = { name: 200, description: 2000, address: 500, notes: 2000 }; +/** + * The two URL fields the REST controller pins in validateUrlFields. Same reason + * they are checked there rather than in the contract: the write schemas are open + * records, so a plugin could otherwise store a thumbnail or a homepage the web + * app refuses — and both end up somewhere that treats them as a URL. + */ +function capUrls(input: Record): void { + const image = input.image_url; + if (image !== undefined && image !== null && !placeImageUrlSchema.safeParse(image).success) { + throw new BadParams('invalid place: image_url must be an uploaded path, a photo-proxy path, an inline image or an https URL'); + } + const website = input.website; + if (website !== undefined && website !== null && website !== '' && !placeWebsiteSchema.safeParse(website).success) { + throw new BadParams('invalid place: website must be an http or https URL'); + } +} + /** * The place surface a plugin may reach (#plugins). * @@ -41,6 +58,7 @@ export class PlacesRpc { const parsed = placeCreateRequestSchema.safeParse(params.input); if (!parsed.success) throw new BadParams(`invalid place: ${schemaMessage(parsed.error)}`); this.guards.capStrings(parsed.data as Record, PLACE_STR_LIMITS); + capUrls(parsed.data as Record); this.guards.requireTripEdit(tripId, actor, PLACE_EDIT_ACTION); const place = this.places.create(String(tripId), parsed.data as unknown as PlaceCreateInput); this.realtime.broadcast(tripId, 'place:created', { place }); @@ -56,6 +74,7 @@ export class PlacesRpc { const parsed = placeUpdateRequestSchema.safeParse(params.input); if (!parsed.success) throw new BadParams(`invalid place: ${schemaMessage(parsed.error)}`); this.guards.capStrings(parsed.data as Record, PLACE_STR_LIMITS); + capUrls(parsed.data as Record); this.guards.requireTripEdit(tripId, actor, PLACE_EDIT_ACTION); const place = this.places.update(String(tripId), String(placeId), parsed.data as PlaceUpdateInput); if (place === null) throw new ForbiddenResource(`no place ${placeId} on trip ${tripId}`); diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index 0c7ac1f03..e1aa65482 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -124,6 +124,29 @@ export class PlacesService { return this.permissions.checkPermission('place_edit', user.role, trip.user_id, user.id, trip.user_id !== user.id); } + /** + * The subset of `tagIds` that someone on this trip owns. A tag has no trip of + * its own, only a `user_id`, so "belongs to this trip" resolves through the + * roster — which keeps the shared case working: a member tags a place, another + * member re-saves it and sends the id back, and the tag survives because its + * owner is still on the trip. Scoping to the caller instead would quietly strip + * a co-traveller's tag on every foreign edit. + * + * Off-roster ids drop silently. The place body is an open record, so an id can + * arrive from an older client that had no business knowing it existed, and the + * read-back join hands `tags.user_id` straight to the caller. + */ + private tagsOnTrip(tripId: string | number, tagIds: number[]): number[] { + const unique = [...new Set(tagIds)]; + if (unique.length === 0) return []; + const roster = this.dbs.rosterUserIds(tripId); + const owned = this.dbs.all<{ id: number; user_id: number }>( + `SELECT id, user_id FROM tags WHERE id IN (${unique.map(() => '?').join(',')})`, + ...unique, + ); + return owned.filter(t => roster.has(t.user_id)).map(t => t.id); + } + broadcast(tripId: string, event: E, payload: TrekWsPayload, socketId: string | undefined): void { this.realtime.broadcast(tripId, event, payload, socketId); } @@ -225,7 +248,7 @@ export class PlacesService { if (tags && tags.length > 0) { const insertTag = this.dbs.prepare('INSERT OR IGNORE INTO place_tags (place_id, tag_id) VALUES (?, ?)'); - for (const tagId of tags) { + for (const tagId of this.tagsOnTrip(tripId, tags)) { insertTag.run(placeId, tagId); } } @@ -342,7 +365,7 @@ export class PlacesService { this.dbs.run('DELETE FROM place_tags WHERE place_id = ?', placeId); if (tags.length > 0) { const insertTag = this.dbs.prepare('INSERT OR IGNORE INTO place_tags (place_id, tag_id) VALUES (?, ?)'); - for (const tagId of tags) { + for (const tagId of this.tagsOnTrip(tripId, tags)) { insertTag.run(placeId, tagId); } } diff --git a/server/src/nest/reservations/reservations.mcp.ts b/server/src/nest/reservations/reservations.mcp.ts index 4e7114b0c..f8b3926ba 100644 --- a/server/src/nest/reservations/reservations.mcp.ts +++ b/server/src/nest/reservations/reservations.mcp.ts @@ -468,6 +468,13 @@ export class ReservationsMcp { if (this.auth.isDemoUser(ctx.userId)) return demoDenied(); if (!this.reservations.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!this.guards.hasTripPermission('reservation_edit', tripId, ctx.userId)) return permissionDenied(); + + // The service scopes the write to the trip on its own, so a foreign id is + // already harmless — say so rather than reporting a success that moved + // nothing, the way the sibling tools above do. + if (dayId && !this.days.getDay(dayId, tripId)) + return errorResult('dayId does not belong to this trip.'); + this.reservations.updatePositions(tripId, positions, dayId); this.guards.safeBroadcast(tripId, 'reservation:positions', { positions, dayId }); return ok({ success: true }); diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index 3f0a814db..15ebbaa2e 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -527,13 +527,26 @@ export class ReservationsService { return { reservation, accommodationCreated }; } - updatePositions(tripId: string | number, positions: { id: number; day_plan_position: number }[], dayId?: number | string | null) { + updatePositions(tripId: string | number, positions: { id: number; day_plan_position?: number }[], dayId?: number | string | null) { if (dayId) { - // Per-day positions for multi-day reservations - const stmt = this.db.prepare('INSERT OR REPLACE INTO reservation_day_positions (reservation_id, day_id, position) VALUES (?, ?, ?)'); + // Per-day positions for multi-day reservations, scoped the way the legacy + // branch below already scopes its update. The table carries no trip_id and + // its two foreign keys only ask that the ids exist, so the trip has to come + // from the join: the row materialises only when the reservation and the day + // agree on it. Doing that in the statement rather than as a pre-check also + // makes a stale id a quiet no-op instead of a foreign-key error surfacing + // as a 500. + const stmt = this.db.prepare(` + INSERT OR REPLACE INTO reservation_day_positions (reservation_id, day_id, position) + SELECT r.id, d.id, ? + FROM reservations r + JOIN days d ON d.trip_id = r.trip_id + WHERE r.id = ? AND d.id = ? AND r.trip_id = ? + `); this.db.transaction(() => { for (const item of positions) { - stmt.run(item.id, dayId, item.day_plan_position); + // position is NOT NULL while the wire contract leaves the value optional. + stmt.run(item.day_plan_position ?? 0, item.id, dayId, tripId); } }); } else { diff --git a/server/src/nest/todo/todo.service.ts b/server/src/nest/todo/todo.service.ts index 8d2025ba0..31bf80953 100644 --- a/server/src/nest/todo/todo.service.ts +++ b/server/src/nest/todo/todo.service.ts @@ -136,7 +136,12 @@ export class TodoService { if (Array.isArray(userIds) && userIds.length > 0) { const insert = this.db.prepare('INSERT OR IGNORE INTO todo_category_assignees (trip_id, category_name, user_id) VALUES (?, ?, ?)'); - for (const uid of userIds) insert.run(tripId, categoryName, uid); + // Only people on this trip may be assigned, the way packing filters bag + // members and reservations filter travellers. Dropped rather than + // rejected: a copied trip carries assignee ids across before its members + // exist, and a 400 would make the picker unusable there. + const roster = this.db.rosterUserIds(tripId); + for (const uid of userIds) if (roster.has(uid)) insert.run(tripId, categoryName, uid); } }); diff --git a/server/tests/e2e/assignments.e2e.test.ts b/server/tests/e2e/assignments.e2e.test.ts index ccde14883..e6b5c1ba2 100644 --- a/server/tests/e2e/assignments.e2e.test.ts +++ b/server/tests/e2e/assignments.e2e.test.ts @@ -194,4 +194,92 @@ describe('Assignments e2e (real auth guard + temp SQLite)', () => { expect(res.status).toBe(200); expect(res.body.assignment).toMatchObject({ id, day_id: 4, order_index: 0 }); }); + + // The per-assignment controller declared @RequirePermission but not the guard + // that reads it, so the decorators were inert metadata and :tripId was never + // checked against the caller at all. The handlers that ask + // getAssignmentForTrip(id, tripId) were happy as long as the assignment sat on + // the trip in the URL — which is true for the owner's trip too. + describe('a trip the caller cannot see', () => { + const FOREIGN_TRIP = 9; + + beforeEach(() => { + canAccessTrip.mockImplementation((tripId: unknown) => (Number(tripId) === 5 ? { id: 5, user_id: 1 } : undefined)); + db.prepare('INSERT OR IGNORE INTO days (id, trip_id) VALUES (30, ?), (31, ?)').run(FOREIGN_TRIP, FOREIGN_TRIP); + db.prepare('INSERT OR IGNORE INTO places (id, trip_id, name) VALUES (20, ?, ?)').run(FOREIGN_TRIP, 'Their hotel'); + }); + + const seedForeignAssignment = () => + Number(db.prepare('INSERT INTO day_assignments (day_id, place_id, order_index) VALUES (30, 20, 0)').run().lastInsertRowid); + + it('404s a move instead of reordering their itinerary', async () => { + const id = seedForeignAssignment(); + const res = await request(server) + .put(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/move`) + .set('Cookie', sessionCookie(1)) + .send({ new_day_id: 31, order_index: 0 }); + expect(res.status).toBe(404); + expect(db.prepare('SELECT day_id FROM day_assignments WHERE id = ?').get(id)).toEqual({ day_id: 30 }); + }); + + it('404s a time change instead of rewriting their schedule', async () => { + const id = seedForeignAssignment(); + const res = await request(server) + .put(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/time`) + .set('Cookie', sessionCookie(1)) + .send({ place_time: '23:00', end_time: null }); + expect(res.status).toBe(404); + expect(db.prepare('SELECT assignment_time FROM day_assignments WHERE id = ?').get(id)).toEqual({ assignment_time: null }); + }); + + it('404s a transport change', async () => { + const id = seedForeignAssignment(); + const res = await request(server) + .put(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/transport`) + .set('Cookie', sessionCookie(1)) + .send({ transport_mode: 'driving' }); + expect(res.status).toBe(404); + }); + + it('404s setting participants instead of writing to their assignment', async () => { + const id = seedForeignAssignment(); + const res = await request(server) + .put(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/participants`) + .set('Cookie', sessionCookie(1)) + .send({ user_ids: [1] }); + expect(res.status).toBe(404); + expect(db.prepare('SELECT COUNT(*) AS n FROM assignment_participants WHERE assignment_id = ?').get(id)).toEqual({ n: 0 }); + }); + + it('404s reading participants instead of disclosing who is on it', async () => { + const id = seedForeignAssignment(); + db.prepare('INSERT INTO assignment_participants (assignment_id, user_id) VALUES (?, 2)').run(id); + const res = await request(server) + .get(`/api/trips/${FOREIGN_TRIP}/assignments/${id}/participants`) + .set('Cookie', sessionCookie(1)); + expect(res.status).toBe(404); + }); + + // The guard has to reject an assignment borrowed from elsewhere even when the + // caller is legitimately on the trip named in the URL. + it('404s an assignment that belongs to another trip than the URL says', async () => { + const id = seedForeignAssignment(); + const res = await request(server) + .put(`/api/trips/5/assignments/${id}/participants`) + .set('Cookie', sessionCookie(1)) + .send({ user_ids: [1] }); + expect(res.status).toBe(404); + expect(db.prepare('SELECT COUNT(*) AS n FROM assignment_participants WHERE assignment_id = ?').get(id)).toEqual({ n: 0 }); + }); + + it('403s when the caller is on the trip but lacks day_edit', async () => { + const id = seedAssignment(); + checkPermission.mockReturnValue(false); + const res = await request(server) + .put(`/api/trips/5/assignments/${id}/time`) + .set('Cookie', sessionCookie(1)) + .send({ place_time: '09:00', end_time: null }); + expect(res.status).toBe(403); + }); + }); }); diff --git a/server/tests/e2e/budget.e2e.test.ts b/server/tests/e2e/budget.e2e.test.ts index 580e86222..0bb965214 100644 --- a/server/tests/e2e/budget.e2e.test.ts +++ b/server/tests/e2e/budget.e2e.test.ts @@ -73,6 +73,9 @@ describe('Budget e2e (real auth guard + temp SQLite, real budget SQL)', () => { "INSERT INTO users (id, username, email, password_hash, role, password_version) VALUES (2, 'e2e-peer', 'peer@example.test', 'x', 'user', 0)", ).run(); tripId = Number(db.prepare("INSERT INTO trips (user_id, title, currency) VALUES (1, 'E2E Trip', 'EUR')").run().lastInsertRowid); + // The peer settles up with the owner below, so they have to be on the trip: + // a settlement between people who do not share one is refused. + db.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (?, 2)').run(tripId); app = await build(); checkPermission = vi.spyOn(app.get(PermissionsService), 'checkPermission'); server = app.getHttpServer(); diff --git a/server/tests/e2e/reservations.e2e.test.ts b/server/tests/e2e/reservations.e2e.test.ts index bcac65060..7907ae5ca 100644 --- a/server/tests/e2e/reservations.e2e.test.ts +++ b/server/tests/e2e/reservations.e2e.test.ts @@ -185,4 +185,92 @@ describe('Reservations + accommodations e2e (real auth guard + temp SQLite, real expect(res.status).toBe(400); expect(res.body).toEqual({ error: 'place_id, start_day_id, and end_day_id are required' }); }); + + // The per-day branch of updatePositions used to bind the caller's ids straight + // into reservation_day_positions without ever looking at the tripId it was + // handed, so a request authorised on your own trip could rewrite the ordering + // of someone else's. The legacy branch three lines below it always scoped by + // trip_id — these pin that the two behave the same way now. + describe('positions are confined to the trip in the URL', () => { + let foreignTripId: number; + let foreignReservationId: number; + let foreignDayId: number; + let ownDayId: number; + // Days are unique per (trip_id, day_number) and this block seeds a fresh pair + // for every case, so the numbers have to keep climbing. + let dayNumber = 100; + + beforeEach(() => { + db.prepare( + "INSERT OR IGNORE INTO users (id, username, email, password_hash, role, password_version) VALUES (2, 'victim', 'victim@example.test', 'x', 'user', 0)", + ).run(); + foreignTripId = Number(db.prepare("INSERT INTO trips (user_id, title) VALUES (2, 'Someone else')").run().lastInsertRowid); + foreignReservationId = Number(db.prepare("INSERT INTO reservations (trip_id, title, type) VALUES (?, 'Secret', 'other')").run(foreignTripId).lastInsertRowid); + dayNumber += 1; + foreignDayId = Number(db.prepare("INSERT INTO days (trip_id, day_number, date) VALUES (?, ?, '2026-05-01')").run(foreignTripId, dayNumber).lastInsertRowid); + ownDayId = Number(db.prepare("INSERT INTO days (trip_id, day_number, date) VALUES (?, ?, '2026-05-01')").run(tripId, dayNumber).lastInsertRowid); + }); + + function positionRow(reservationId: number, dayId: number) { + return db.prepare('SELECT position FROM reservation_day_positions WHERE reservation_id = ? AND day_id = ?').get(reservationId, dayId); + } + + it('writes nothing when both ids belong to another trip', async () => { + const res = await request(server) + .put(`/api/trips/${tripId}/reservations/positions`) + .set('Cookie', sessionCookie(1)) + .send({ positions: [{ id: foreignReservationId, day_plan_position: 999 }], day_id: foreignDayId }); + expect(res.status).toBe(200); + expect(positionRow(foreignReservationId, foreignDayId)).toBeUndefined(); + }); + + it('writes nothing when only the reservation is foreign', async () => { + const res = await request(server) + .put(`/api/trips/${tripId}/reservations/positions`) + .set('Cookie', sessionCookie(1)) + .send({ positions: [{ id: foreignReservationId, day_plan_position: 5 }], day_id: ownDayId }); + expect(res.status).toBe(200); + expect(positionRow(foreignReservationId, ownDayId)).toBeUndefined(); + }); + + it('writes nothing when only the day is foreign', async () => { + const ownReservationId = Number(db.prepare("INSERT INTO reservations (trip_id, title, type) VALUES (?, 'Mine', 'other')").run(tripId).lastInsertRowid); + const res = await request(server) + .put(`/api/trips/${tripId}/reservations/positions`) + .set('Cookie', sessionCookie(1)) + .send({ positions: [{ id: ownReservationId, day_plan_position: 5 }], day_id: foreignDayId }); + expect(res.status).toBe(200); + expect(positionRow(ownReservationId, foreignDayId)).toBeUndefined(); + }); + + it('still stores a position when both ids are on the trip', async () => { + const ownReservationId = Number(db.prepare("INSERT INTO reservations (trip_id, title, type) VALUES (?, 'Mine', 'other')").run(tripId).lastInsertRowid); + const res = await request(server) + .put(`/api/trips/${tripId}/reservations/positions`) + .set('Cookie', sessionCookie(1)) + .send({ positions: [{ id: ownReservationId, day_plan_position: 3 }], day_id: ownDayId }); + expect(res.status).toBe(200); + expect(positionRow(ownReservationId, ownDayId)).toEqual({ position: 3 }); + }); + + // An id that exists nowhere used to raise a foreign-key error, which the + // exception filter turned into a 500 — telling the caller apart from the + // 200 a real id returns, for any id on the instance. + it('answers a nonexistent reservation the same way it answers a foreign one', async () => { + const res = await request(server) + .put(`/api/trips/${tripId}/reservations/positions`) + .set('Cookie', sessionCookie(1)) + .send({ positions: [{ id: 999999, day_plan_position: 1 }], day_id: ownDayId }); + expect(res.status).toBe(200); + }); + + it('does not fall over when day_plan_position is omitted', async () => { + const ownReservationId = Number(db.prepare("INSERT INTO reservations (trip_id, title, type) VALUES (?, 'Mine', 'other')").run(tripId).lastInsertRowid); + const res = await request(server) + .put(`/api/trips/${tripId}/reservations/positions`) + .set('Cookie', sessionCookie(1)) + .send({ positions: [{ id: ownReservationId }], day_id: ownDayId }); + expect(res.status).toBe(200); + }); + }); }); diff --git a/server/tests/integration/leg-mode-incoming.test.ts b/server/tests/integration/leg-mode-incoming.test.ts index 7279c7eff..4cae38bc2 100644 --- a/server/tests/integration/leg-mode-incoming.test.ts +++ b/server/tests/integration/leg-mode-incoming.test.ts @@ -35,31 +35,47 @@ describe('incoming_leg_transport_mode migration', () => { // The case above starts from an empty database and runs every migration, so it // passes wherever a migration sits in the array. - it('the trailing migration lands on a database that is one version behind', () => { + it('the trailing migrations land on a database that is behind', () => { // What actually has to hold: existing installs replay only the slots above // their schema_version, so a migration inserted mid-array is silently - // skipped on every one of them. Undoing what the LAST migration did and - // rewinding one version is the only way to prove it still runs. + // skipped on every one of them. Undoing what the trailing migrations did + // and rewinding is the only way to prove they still run. // // >>> Appending a migration? Re-point the undo below at whatever yours // >>> does. That is the whole maintenance cost of this guard. - // Trailing migration today: the storage slice-2 trip_files 'files/' prefix - // strip — undo it by re-inserting a prefixed row. + // Trailing migrations today (a dev/feat-storage-driver merge landed two + // in one release window, so the rewind spans both): + // version-2 — reservation_day_positions cross-trip cleanup: undone by + // re-inserting a mismatched reservation/day pair. + // version-1 — storage slice-2 trip_files 'files/' prefix strip: undone + // by re-inserting a prefixed row. const upgraded = new Database(':memory:'); upgraded.exec('PRAGMA foreign_keys = ON'); createTables(upgraded); runMigrations(upgraded); const { version } = upgraded.prepare('SELECT version FROM schema_version').get() as { version: number }; - upgraded.prepare("INSERT INTO users (id, username, email, password_hash) VALUES (1, 'u', 'u@example.test', 'x')").run(); - upgraded.prepare("INSERT INTO trips (id, user_id, title) VALUES (1, 1, 'T')").run(); - upgraded.prepare("INSERT INTO trip_files (trip_id, filename, original_name) VALUES (1, 'files/aaa.pdf', 'a.pdf')").run(); - upgraded.prepare('UPDATE schema_version SET version = ?').run(version - 1); + + upgraded.prepare("INSERT INTO users (id, username, email, password_hash) VALUES (1, 'a', 'a@test.local', 'x')").run(); + upgraded.prepare("INSERT INTO users (id, username, email, password_hash) VALUES (2, 'b', 'b@test.local', 'x')").run(); + const tripA = upgraded.prepare("INSERT INTO trips (user_id, title) VALUES (1, 'A')").run().lastInsertRowid; + const tripB = upgraded.prepare("INSERT INTO trips (user_id, title) VALUES (2, 'B')").run().lastInsertRowid; + const reservationOnA = upgraded.prepare("INSERT INTO reservations (trip_id, title, type) VALUES (?, 'r', 'other')").run(tripA).lastInsertRowid; + const dayOnA = upgraded.prepare("INSERT INTO days (trip_id, day_number, date) VALUES (?, 1, '2026-01-01')").run(tripA).lastInsertRowid; + const dayOnB = upgraded.prepare("INSERT INTO days (trip_id, day_number, date) VALUES (?, 1, '2026-01-01')").run(tripB).lastInsertRowid; + const insertPosition = upgraded.prepare('INSERT INTO reservation_day_positions (reservation_id, day_id, position) VALUES (?, ?, ?)'); + insertPosition.run(reservationOnA, dayOnB, 1); // the mismatched pair the migration clears + insertPosition.run(reservationOnA, dayOnA, 2); // a legitimate one it must leave alone + upgraded.prepare("INSERT INTO trip_files (trip_id, filename, original_name) VALUES (?, 'files/aaa.pdf', 'a.pdf')").run(tripA); + + upgraded.prepare('UPDATE schema_version SET version = ?').run(version - 2); runMigrations(upgraded); - const rows = upgraded.prepare('SELECT filename FROM trip_files').all() as { filename: string }[]; - expect(rows.map(r => r.filename)).toEqual(['aaa.pdf']); + const positions = upgraded.prepare('SELECT day_id FROM reservation_day_positions ORDER BY day_id').all() as { day_id: number }[]; + expect(positions).toEqual([{ day_id: Number(dayOnA) }]); + const files = upgraded.prepare('SELECT filename FROM trip_files').all() as { filename: string }[]; + expect(files.map(r => r.filename)).toEqual(['aaa.pdf']); expect(upgraded.prepare('SELECT version FROM schema_version').get()).toEqual({ version }); upgraded.close(); }); diff --git a/server/tests/unit/db/migration-hygiene.test.ts b/server/tests/unit/db/migration-hygiene.test.ts index 358318260..44b4ecd88 100644 --- a/server/tests/unit/db/migration-hygiene.test.ts +++ b/server/tests/unit/db/migration-hygiene.test.ts @@ -135,6 +135,8 @@ const ALLOWED_DESTRUCTIVE: Record = { 'Atlas enclave fix: DELETE ... WHERE place_id IN (places inside specific enclave boxes) — invalidate stale region cache; re-resolved on next request.', 'DELETE FROM visited_regions': 'Atlas geoBoundaries swap (#1119): DELETE ... WHERE id = ? — after UPDATE OR IGNORE re-codes a manually-marked region to its current code, drop only the single leftover row whose UNIQUE(user_id, region_code) collision caused the update to be skipped (a duplicate of a region the user already has).', + 'DELETE FROM reservation_day_positions': + 'DELETE ... WHERE the row joins a reservation and a day sitting on different trips. The table has no trip_id and its two foreign keys only require the ids to exist, so a pair that never belonged together was storable; the writer scopes to the trip now and this clears what earlier builds allowed. Bounded by the join — a row whose reservation and day agree on their trip is untouched.', }; describe('migration hygiene — destructive operation guard', () => { diff --git a/server/tests/unit/nest/assignments.controller.test.ts b/server/tests/unit/nest/assignments.controller.test.ts index e032dfa14..5580d5301 100644 --- a/server/tests/unit/nest/assignments.controller.test.ts +++ b/server/tests/unit/nest/assignments.controller.test.ts @@ -78,8 +78,10 @@ describe('AssignmentOpsController (parity with the per-assignment op routes)', ( expect(reconcile).toHaveBeenCalledWith('5', 'sock'); }); - it('GET /:id/participants returns participants (access-only)', () => { - const s = svc({ getParticipants: vi.fn().mockReturnValue([{ user_id: 2 }]) } as Partial); + it('GET /:id/participants 404 when the assignment is on another trip, else returns participants (access-only)', () => { + expect(thrown(() => new AssignmentOpsController(svc({ getAssignmentForTrip: vi.fn().mockReturnValue(undefined) } as Partial)).participants(user, '5', '9'))) + .toEqual({ status: 404, body: { error: 'Assignment not found' } }); + const s = svc({ getAssignmentForTrip: vi.fn().mockReturnValue({ id: 9 }), getParticipants: vi.fn().mockReturnValue([{ user_id: 2 }]) } as Partial); expect(new AssignmentOpsController(s).participants(user, '5', '9')).toEqual({ participants: [{ user_id: 2 }] }); }); @@ -92,10 +94,15 @@ describe('AssignmentOpsController (parity with the per-assignment op routes)', ( expect(reconcile).toHaveBeenCalledWith('5', 'sock'); }); - it('PUT /:id/participants sets + broadcasts (non-array bodies are the Zod pipe\'s 400, covered in e2e)', () => { + it('PUT /:id/participants 404 on a foreign assignment, else sets + broadcasts (non-array bodies are the Zod pipe\'s 400, covered in e2e)', () => { const setParticipants = vi.fn().mockReturnValue([{ user_id: 2 }]); const broadcast = vi.fn(); - expect(new AssignmentOpsController(svc({ setParticipants, broadcast } as Partial)).setParticipants(user, '5', '9', { user_ids: [2] }, 'sock')).toEqual({ participants: [{ user_id: 2 }] }); - expect(setParticipants).toHaveBeenCalledWith('9', [2]); + expect(thrown(() => new AssignmentOpsController(svc({ getAssignmentForTrip: vi.fn().mockReturnValue(undefined), setParticipants } as Partial)).setParticipants(user, '5', '9', { user_ids: [2] }))) + .toEqual({ status: 404, body: { error: 'Assignment not found' } }); + expect(setParticipants).not.toHaveBeenCalled(); + const s = svc({ getAssignmentForTrip: vi.fn().mockReturnValue({ id: 9 }), setParticipants, broadcast } as Partial); + expect(new AssignmentOpsController(s).setParticipants(user, '5', '9', { user_ids: [2] }, 'sock')).toEqual({ participants: [{ user_id: 2 }] }); + // The trip comes along so the service can confine the ids to its roster. + expect(setParticipants).toHaveBeenCalledWith('9', [2], '5'); expect(broadcast).toHaveBeenCalledWith('5', 'assignment:participants', { assignmentId: 9, participants: [{ user_id: 2 }] }, 'sock'); }); }); diff --git a/server/tests/unit/nest/assignments.service.test.ts b/server/tests/unit/nest/assignments.service.test.ts index bda5951c4..db6d1bf2d 100644 --- a/server/tests/unit/nest/assignments.service.test.ts +++ b/server/tests/unit/nest/assignments.service.test.ts @@ -205,7 +205,7 @@ describe('listDayAssignments', () => { testDb.prepare('INSERT INTO place_tags (place_id, tag_id) VALUES (?, ?)').run(place.id, tag.id); const a1 = createDayAssignment(testDb, day.id, place.id, { order_index: 1 }); const a2 = createDayAssignment(testDb, day.id, second.id, { order_index: 0 }); - svc.setParticipants(a1.id, [user.id]); + svc.setParticipants(a1.id, [user.id], trip.id); const list = svc.listDayAssignments(day.id); expect(list.map(a => a.id)).toEqual([a2.id, a1.id]); @@ -284,45 +284,64 @@ describe('moveAssignment', () => { describe('getParticipants / setParticipants', () => { it('ASG-SVC-017: setParticipants replaces the list and returns the joined rows', () => { - const { user, day, place } = fixture(); + const { user, trip, day, place } = fixture(); const { user: peer } = createUser(testDb); + addTripMember(testDb, trip.id, peer.id); const a = createDayAssignment(testDb, day.id, place.id); - svc.setParticipants(a.id, [user.id]); - const replaced = svc.setParticipants(a.id, [peer.id]); + svc.setParticipants(a.id, [user.id], trip.id); + const replaced = svc.setParticipants(a.id, [peer.id], trip.id); expect(replaced).toEqual([{ user_id: peer.id, username: peer.username, avatar: null }]); expect(svc.getParticipants(a.id)).toEqual(replaced); }); it('ASG-SVC-018: empty array clears all participants', () => { - const { user, day, place } = fixture(); + const { user, trip, day, place } = fixture(); const a = createDayAssignment(testDb, day.id, place.id); - svc.setParticipants(a.id, [user.id]); - expect(svc.setParticipants(a.id, [])).toEqual([]); + svc.setParticipants(a.id, [user.id], trip.id); + expect(svc.setParticipants(a.id, [], trip.id)).toEqual([]); expect(svc.getParticipants(a.id)).toEqual([]); }); it('ASG-SVC-019: username COALESCEs display_name over username', () => { - const { user, day, place } = fixture(); + const { user, trip, day, place } = fixture(); testDb.prepare('UPDATE users SET display_name = ? WHERE id = ?').run('Fancy Name', user.id); const a = createDayAssignment(testDb, day.id, place.id); - const rows = svc.setParticipants(a.id, [user.id]); + const rows = svc.setParticipants(a.id, [user.id], trip.id); expect(rows).toEqual([{ user_id: user.id, username: 'Fancy Name', avatar: null }]); }); it('ASG-SVC-020: duplicate user ids collapse via INSERT OR IGNORE', () => { - const { user, day, place } = fixture(); + const { user, trip, day, place } = fixture(); const a = createDayAssignment(testDb, day.id, place.id); - const rows = svc.setParticipants(a.id, [user.id, user.id]); + const rows = svc.setParticipants(a.id, [user.id, user.id], trip.id); expect(rows).toHaveLength(1); }); - it('ASG-SVC-028: a failing insert rolls back the whole replace (delete included)', () => { - const { user, day, place } = fixture(); + // Used to assert that an id violating the users FK threw and rolled the delete + // back. It cannot reach the insert any more: the roster filter drops an id that + // is not on the trip, so a list of nothing but strangers is simply an empty + // list, and the delete stands. The rollback itself is still covered — the + // transaction wraps delete and insert together, and ASG-SVC-018 pins that an + // explicit empty list clears. + it('ASG-SVC-028: a list of ids that are not on the trip clears rather than throwing', () => { + const { user, trip, day, place } = fixture(); const a = createDayAssignment(testDb, day.id, place.id); - svc.setParticipants(a.id, [user.id]); - // 999999 violates the users FK — the transaction must undo the delete too. - expect(() => svc.setParticipants(a.id, [999999])).toThrow(); - expect(svc.getParticipants(a.id)).toEqual([{ user_id: user.id, username: user.username, avatar: null }]); + svc.setParticipants(a.id, [user.id], trip.id); + expect(() => svc.setParticipants(a.id, [999999], trip.id)).not.toThrow(); + expect(svc.getParticipants(a.id)).toEqual([]); + }); + + it('ASG-SVC-029: keeps the trip members and drops the stranger in one call', () => { + const { user, trip, day, place } = fixture(); + const { user: member } = createUser(testDb); + const { user: stranger } = createUser(testDb); + addTripMember(testDb, trip.id, member.id); + const a = createDayAssignment(testDb, day.id, place.id); + + const rows = svc.setParticipants(a.id, [user.id, stranger.id, member.id], trip.id) as { user_id: number }[]; + + expect(rows.map(r => r.user_id).sort()).toEqual([user.id, member.id].sort()); + expect(JSON.stringify(rows)).not.toContain(stranger.username); }); }); diff --git a/server/tests/unit/nest/budget.service.db.test.ts b/server/tests/unit/nest/budget.service.db.test.ts index 2e8247258..6becc5019 100644 --- a/server/tests/unit/nest/budget.service.db.test.ts +++ b/server/tests/unit/nest/budget.service.db.test.ts @@ -55,7 +55,7 @@ vi.mock('../../../src/nest/budget/exchange-rates.service', () => ({ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; -import { createUser, createTrip } from '../../helpers/factories'; +import { createUser, createTrip, addTripMember } from '../../helpers/factories'; import { BudgetService } from '../../../src/nest/budget/budget.service'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; @@ -230,6 +230,7 @@ describe('calculateSettlement custom splits', () => { const { user: alice } = createUser(testDb, { username: 'alice' }); const { user: bob } = createUser(testDb, { username: 'bob' }); const trip = createTrip(testDb, alice.id, { title: 'Trip' }); + addTripMember(testDb, trip.id, bob.id); // 100 total, custom split: Alice owes 90, Bob owes 10. Alice paid the whole bill. budget.createBudgetItem(trip.id, { @@ -261,6 +262,8 @@ describe('calculateSettlement squares up to the cent (#1382)', () => { const { user: bob } = createUser(testDb, { username: 'bob' }); const { user: carol } = createUser(testDb, { username: 'carol' }); const trip = createTrip(testDb, alice.id, { title: 'Trip' }); + addTripMember(testDb, trip.id, bob.id); + addTripMember(testDb, trip.id, carol.id); const members = [{ user_id: alice.id }, { user_id: bob.id }, { user_id: carol.id }]; // Totals that never divide by three, so every expense leaves a remainder cent @@ -306,6 +309,8 @@ function seedIssue1543Trip(tripCurrency: string) { const { user: danil } = createUser(testDb, { username: 'danil' }); const { user: serega } = createUser(testDb, { username: 'serega' }); const trip = createTrip(testDb, me.id, { title: 'Trip' }); + addTripMember(testDb, trip.id, danil.id); + addTripMember(testDb, trip.id, serega.id); testDb.prepare('UPDATE trips SET currency = ? WHERE id = ?').run(tripCurrency, trip.id); const members = [{ user_id: me.id }, { user_id: danil.id }, { user_id: serega.id }]; @@ -384,6 +389,7 @@ describe('rebaseTripCurrency', () => { const { user: alice } = createUser(testDb, { username: 'alice' }); const { user: bob } = createUser(testDb, { username: 'bob' }); const trip = createTrip(testDb, alice.id, { title: 'Trip' }); + addTripMember(testDb, trip.id, bob.id); testDb.prepare("UPDATE trips SET currency = 'EUR' WHERE id = ?").run(trip.id); const members = [{ user_id: alice.id }, { user_id: bob.id }]; @@ -524,6 +530,57 @@ describe('composite service paths (ex budget.bridge delegation)', () => { }); }); +// A settlement names two parties and both columns are NOT NULL, so an id that is +// not on the trip cannot simply be dropped the way a split member can — the whole +// write is refused. Refusing also means the endpoint stops answering "does this +// user id exist", which it used to do by 500ing on the foreign key. +describe('settlement parties are confined to the trip', () => { + it('BUDGET-SVC-DB-028: refuses a payer who is not on the trip', async () => { + const { user: alice } = createUser(testDb, { username: 'alice' }); + const { user: outsider } = createUser(testDb, { username: 'outsider' }); + const trip = createTrip(testDb, alice.id); + + const created = await budget.createSettlement(trip.id, { from_user_id: outsider.id, to_user_id: alice.id, amount: 10 }, alice.id); + + expect(created).toBeNull(); + expect(budget.listSettlements(trip.id)).toEqual([]); + }); + + it('BUDGET-SVC-DB-029: answers a nonexistent user the same way, without throwing', async () => { + const { user: alice } = createUser(testDb, { username: 'alice' }); + const trip = createTrip(testDb, alice.id); + + await expect(budget.createSettlement(trip.id, { from_user_id: 999999, to_user_id: alice.id, amount: 10 }, alice.id)) + .resolves.toBeNull(); + }); + + it('BUDGET-SVC-DB-030: still records a settlement between the owner and a member', async () => { + const { user: alice } = createUser(testDb, { username: 'alice' }); + const { user: bob } = createUser(testDb, { username: 'bob' }); + const trip = createTrip(testDb, alice.id); + addTripMember(testDb, trip.id, bob.id); + + const created = await budget.createSettlement(trip.id, { from_user_id: bob.id, to_user_id: alice.id, amount: 10 }, alice.id); + + expect(created).toMatchObject({ from_user_id: bob.id, to_user_id: alice.id }); + }); + + it('BUDGET-SVC-DB-031: drops an off-trip payer from an item instead of refusing it', () => { + const { user: alice } = createUser(testDb, { username: 'alice' }); + const { user: outsider } = createUser(testDb, { username: 'outsider' }); + const trip = createTrip(testDb, alice.id); + + const item = budget.createBudgetItem(trip.id, { + name: 'Dinner', + payers: [{ user_id: alice.id, amount: 40 }, { user_id: outsider.id, amount: 60 }], + }) as { payers: { user_id: number }[]; total_price: number }; + + expect(item.payers.map(p => p.user_id)).toEqual([alice.id]); + // total_price is the sum of the payers that actually landed. + expect(item.total_price).toBe(40); + }); +}); + describe('post-fold quirk fixes', () => { it('BUDGET-SVC-DB-019: settlements prefer display_name over username (quirk fix)', () => { const { user: alice } = createUser(testDb, { username: 'alice' }); diff --git a/server/tests/unit/nest/budget.service.test.ts b/server/tests/unit/nest/budget.service.test.ts index 501598d79..f7317cc7b 100644 --- a/server/tests/unit/nest/budget.service.test.ts +++ b/server/tests/unit/nest/budget.service.test.ts @@ -139,8 +139,14 @@ describe('BudgetService', () => { }); describe('settlement ledger wrappers (#1445 freeze-then-write)', () => { + // Both wrappers refuse a party who is not on the trip before they freeze + // anything, so the roster lookup has to answer for these to reach the write + // at all. Users 1 and 2 are the two parties every case here settles between. + const rosterHas = (...userIds: number[]) => dbMock._stmt.all.mockReturnValue(userIds.map(user_id => ({ user_id }))); + it('createSettlement freezes the FX rate (await) before the raw insert', async () => { const s = svc(); + rosterHas(1, 2); const freezeSpy = vi.spyOn(s, 'freezeForeignRate').mockResolvedValue(); const insertSpy = vi.spyOn(s, 'insertSettlement').mockReturnValue({ id: 7 } as never); await s.createSettlement('5', { from_user_id: 1, to_user_id: 2, amount: 10 }, 3); @@ -150,6 +156,7 @@ describe('BudgetService', () => { it('updateSettlement threads the stored currency through the freeze', async () => { const s = svc(); + rosterHas(1, 2); const freezeSpy = vi.spyOn(s, 'freezeForeignRate').mockResolvedValue(); // Quirk fix: the stored-currency lookup is a targeted getSettlement, not // a full listSettlements scan. @@ -161,6 +168,18 @@ describe('BudgetService', () => { expect(freezeSpy).toHaveBeenCalledWith('5', { from_user_id: 1, to_user_id: 2, amount: 12, currency: 'USD' }, undefined, 'USD'); expect(applySpy).toHaveBeenCalledWith('7', '5', { from_user_id: 1, to_user_id: 2, amount: 12, currency: 'USD' }); }); + + it('refuses a party who is not on the trip, before freezing or writing', async () => { + const s = svc(); + rosterHas(1); // user 2 is not on this trip + const freezeSpy = vi.spyOn(s, 'freezeForeignRate').mockResolvedValue(); + const insertSpy = vi.spyOn(s, 'insertSettlement').mockReturnValue({ id: 7 } as never); + + await expect(s.createSettlement('5', { from_user_id: 1, to_user_id: 2, amount: 10 }, 3)).resolves.toBeNull(); + + expect(freezeSpy).not.toHaveBeenCalled(); + expect(insertSpy).not.toHaveBeenCalled(); + }); }); describe('syncReservationPrice', () => { diff --git a/server/tests/unit/nest/packing.service.test.ts b/server/tests/unit/nest/packing.service.test.ts index 44571e593..649db5838 100644 --- a/server/tests/unit/nest/packing.service.test.ts +++ b/server/tests/unit/nest/packing.service.test.ts @@ -515,6 +515,7 @@ describe('three-tier packing sharing (#858)', () => { const { user: friend } = createUser(testDb); const { user: stranger } = createUser(testDb); const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, friend.id); const item = svc.createItem(trip.id, { name: 'Power bank', visibility: 'shared', recipient_ids: [friend.id] }, owner.id) as any; expect(item.is_private).toBe(1); @@ -539,6 +540,7 @@ describe('three-tier packing sharing (#858)', () => { const { user: owner } = createUser(testDb); const { user: friend } = createUser(testDb); const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, friend.id); const item = svc.createItem(trip.id, { name: 'First aid', visibility: 'personal' }, owner.id) as any; // A non-owner who cannot see the item at all gets the missing-item answer, so @@ -1007,6 +1009,8 @@ describe('packing item object-level authorization', () => { const { user: intruder } = createUser(testDb); const { user: friend } = createUser(testDb); const trip = createTrip(testDb, owner.id); + // The recipient is a fellow traveller; the intruder stays off the trip on purpose. + addTripMember(testDb, trip.id, friend.id); const personal = svc.createItem(trip.id, { name: 'Diary', visibility: 'personal' }, owner.id) as any; const shared = svc.createItem(trip.id, { name: 'Power bank', visibility: 'shared', recipient_ids: [friend.id] }, owner.id) as any; const common = svc.createItem(trip.id, { name: 'Tent', visibility: 'common' }, owner.id) as any; diff --git a/server/tests/unit/nest/places.controller.test.ts b/server/tests/unit/nest/places.controller.test.ts index 51e91f730..5a86e25bc 100644 --- a/server/tests/unit/nest/places.controller.test.ts +++ b/server/tests/unit/nest/places.controller.test.ts @@ -304,6 +304,60 @@ describe('PlacesController (parity with the legacy /api/trips/:tripId/places rou }); }); + // image_url and website leave the database for something that treats them as a + // URL — the thumbnail into hand-built marker HTML, the homepage into + // window.open. The write body is an open record, so the Zod pipe never sees + // either one; they are checked here for the same reason route_color is. + // (update() is async since the storage slices; create() is not.) + describe('image_url and website', () => { + const imageErr = { status: 400, body: { error: 'image_url must be an uploaded path, a photo-proxy path, an inline image or an https URL' } }; + const siteErr = { status: 400, body: { error: 'website must be an http or https URL' } }; + const ctl = (over: Partial = {}) => new PlacesController(svc(over), new RuntimeEnvService(), storageStub); + + it('400s an image_url the marker builders were never meant to receive, before the permission check', async () => { + const canEdit = vi.fn().mockReturnValue(false); // would 403 if it got that far + for (const image_url of [ + 'javascript:alert(1)', + 'data:text/html,', + 'http://insecure.example/p.png', + '//evil.example/p.png', + 123, + ]) { + expect(await thrownAsync(() => ctl({ canEdit }).update(user, '5', '9', { image_url }))).toEqual(imageErr); + } + expect(thrown(() => ctl({ canEdit }).create(user, '5', { name: 'T', image_url: 'javascript:alert(1)' }))).toEqual(imageErr); + expect(canEdit).not.toHaveBeenCalled(); + }); + + it('keeps accepting every shape the app actually stores', async () => { + const update = vi.fn().mockReturnValue({ id: 9 }); + for (const image_url of [ + '/uploads/places/eiffel.jpg', + '/api/maps/place-photo/abc123', + 'data:image/jpeg;base64,/9j/4AAQSkZJRg==', + 'https://images.example/photo.jpg', + null, + ]) { + expect(await ctl({ update } as Partial).update(user, '5', '9', { image_url })).toEqual({ place: { id: 9 } }); + } + }); + + it('400s a website that window.open would not treat as a page', async () => { + const canEdit = vi.fn().mockReturnValue(false); + for (const website of ['javascript:fetch("/api/trips")', 'data:text/html,x', 'louvre.fr', 42]) { + expect(await thrownAsync(() => ctl({ canEdit }).update(user, '5', '9', { website }))).toEqual(siteErr); + } + expect(canEdit).not.toHaveBeenCalled(); + }); + + it('accepts http and https, and treats the empty string as clearing the field', async () => { + const update = vi.fn().mockReturnValue({ id: 9 }); + for (const website of ['https://louvre.fr', 'http://pension.at', '', null]) { + expect(await ctl({ update } as Partial).update(user, '5', '9', { website })).toEqual({ place: { id: 9 } }); + } + }); + }); + it('PUT /:id forwards the base-version token and 409s on a conflict (#1135)', async () => { const update = vi.fn().mockReturnValue({ conflict: true, server: { id: 9, name: 'Theirs' } }); const onUpdated = vi.fn(); const broadcast = vi.fn(); diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts index 69773134a..72a84aa36 100644 --- a/server/tests/unit/nest/places.service.test.ts +++ b/server/tests/unit/nest/places.service.test.ts @@ -64,7 +64,7 @@ const photoCacheStub = { removeIfUnreferenced: removeIfUnreferencedSpy } as unkn import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; -import { createUser, createTrip, createPlace, createCategory, createTag } from '../../helpers/factories'; +import { createUser, createTrip, createPlace, createCategory, createTag, addTripMember } from '../../helpers/factories'; import path from 'path'; import fs from 'fs'; import { DatabaseService } from '../../../src/nest/database/database.service'; @@ -204,6 +204,33 @@ describe('create', () => { expect(place.tags[0].id).toBe(tag.id); }); + // A tag belongs to a user, not a trip, and the place body is an open record, so + // an id from someone outside the trip could be attached and then read straight + // back — the tag projection carries the owner's user_id with it. + it('PLACE-SVC-008a — drops a tag owned by someone outside the trip', () => { + const { user } = createUser(testDb); + const { user: outsider } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const mine = createTag(testDb, user.id, { name: 'Mine' }) as any; + const theirs = createTag(testDb, outsider.id, { name: 'Theirs' }) as any; + + const place = svc.create(String(trip.id), { name: 'Tagged Place', tags: [mine.id, theirs.id] }) as any; + + expect(place.tags.map((t: any) => t.id)).toEqual([mine.id]); + }); + + it('PLACE-SVC-008b — keeps a co-traveller tag, so a shared place survives a foreign re-save', () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + const theirTag = createTag(testDb, member.id, { name: 'Theirs' }) as any; + + const place = svc.create(String(trip.id), { name: 'Shared Place', tags: [theirTag.id] }) as any; + + expect(place.tags.map((t: any) => t.id)).toEqual([theirTag.id]); + }); + it('PLACE-SVC-009 — place is associated with correct trip', () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); diff --git a/server/tests/unit/nest/todo.service.test.ts b/server/tests/unit/nest/todo.service.test.ts index b52e7931a..b3fcc351e 100644 --- a/server/tests/unit/nest/todo.service.test.ts +++ b/server/tests/unit/nest/todo.service.test.ts @@ -270,6 +270,35 @@ describe('getCategoryAssignees / updateCategoryAssignees', () => { expect(assignees['Logistics']).toHaveLength(1); }); + // Holding todo_edit says you may edit this trip's list, not that any user id + // you name belongs to it. The read-back joins users and hands the caller a + // display name and an avatar, so an unfiltered id turns the endpoint into a + // lookup for every account on the instance. + it('TODO-SVC-017a: drops a user who is not on the trip, and says nothing about them', () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const { user: stranger } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + + const rows = svc.updateCategoryAssignees(trip.id, 'Packing', [owner.id, stranger.id, member.id]) as { user_id: number }[]; + + expect(rows.map(r => r.user_id).sort()).toEqual([owner.id, member.id].sort()); + expect(JSON.stringify(rows)).not.toContain(stranger.username); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM todo_category_assignees WHERE user_id = ?').get(stranger.id)).toEqual({ n: 0 }); + }); + + it('TODO-SVC-017b: keeps a guest, who is a trip member like any other', () => { + const { user: owner } = createUser(testDb); + const { user: guest } = createUser(testDb); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(guest.id); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, guest.id); + + const rows = svc.updateCategoryAssignees(trip.id, 'Packing', [guest.id]) as { user_id: number }[]; + expect(rows.map(r => r.user_id)).toEqual([guest.id]); + }); + it('TODO-SVC-020: updateCategoryAssignees replaces existing assignees (not append)', () => { const { user: owner } = createUser(testDb); const { user: member } = createUser(testDb); diff --git a/shared/src/collection/collection.schema.ts b/shared/src/collection/collection.schema.ts index 894fef266..d5379d5fe 100644 --- a/shared/src/collection/collection.schema.ts +++ b/shared/src/collection/collection.schema.ts @@ -1,4 +1,4 @@ -import { placeCategorySchema, placeRatingVoteSchema } from '../place/place.schema'; +import { placeCategorySchema, placeImageUrlSchema, placeRatingVoteSchema, placeWebsiteSchema } from '../place/place.schema'; import { tagSchema } from '../tag/tag.schema'; import { z } from 'zod'; @@ -53,6 +53,9 @@ export const collectionPlaceSchema = z.object({ price: z.number().nullable().optional(), currency: z.string().nullable().optional(), notes: z.string().nullable().optional(), + // Deliberately open on the way out, pinned on the way in (see the save/update + // request schemas below): rows written before the request-side check existed + // still have to round-trip. image_url: z.string().nullable().optional(), google_place_id: z.string().nullable().optional(), google_ftid: z.string().nullable().optional(), @@ -150,11 +153,11 @@ export const collectionSavePlaceRequestSchema = z.object({ price: z.number().nullable().optional(), currency: z.string().nullable().optional(), notes: z.string().nullable().optional(), - image_url: z.string().nullable().optional(), + image_url: placeImageUrlSchema.nullable().optional(), google_place_id: z.string().nullable().optional(), google_ftid: z.string().nullable().optional(), osm_id: z.string().nullable().optional(), - website: z.string().nullable().optional(), + website: placeWebsiteSchema.nullable().optional(), phone: z.string().nullable().optional(), status: collectionStatusSchema.optional(), links: collectionLinksSchema.optional(), @@ -203,7 +206,7 @@ export const collectionPlaceUpdateRequestSchema = z.object({ // Replace the place's per-collection label assignments (omit to leave unchanged). label_ids: z.array(z.number()).optional(), // Custom thumbnail (#1136): null clears it (falls back to the auto-fetched photo). - image_url: z.string().nullable().optional(), + image_url: placeImageUrlSchema.nullable().optional(), }); export type CollectionPlaceUpdateRequest = z.infer; diff --git a/shared/src/place/place.schema.ts b/shared/src/place/place.schema.ts index 9c3fa5379..d0d63da28 100644 --- a/shared/src/place/place.schema.ts +++ b/shared/src/place/place.schema.ts @@ -19,6 +19,34 @@ const open = z.record(z.string(), z.unknown()); /** `#rgb` / `#rrggbb`, the form both map renderers and CSS accept. */ export const hexColorSchema = z.string().regex(/^#([0-9a-fA-F]{3}|[0-9a-fA-F]{6})$/); +/** + * The four shapes a place thumbnail is ever allowed to take: a file we stored, a + * photo-proxy path, an inline thumbnail, or a remote https image. + * + * The map markers build their HTML as a string, so what lands here is one input + * away from a renderer that forgets to escape. Pinning the scheme on the way in + * means a stored value cannot carry anything a future consumer has to defend + * against — the escaping in the marker builders stops being the only thing + * standing between the database and the DOM. + */ +export const placeImageUrlSchema = z.string().max(2048).refine( + v => + v.startsWith('/uploads/') + || v.startsWith('/api/maps/place-photo/') + || /^data:image\/(png|jpe?g|webp|gif|avif);base64,/i.test(v) + || /^https:\/\//i.test(v), + { message: 'must be an uploaded path, a photo-proxy path, an inline image or an https URL' }, +); + +/** + * A place's homepage. It reaches window.open() on the client, where a + * javascript: value would run in this origin rather than opening a page. + */ +export const placeWebsiteSchema = z.string().max(500).refine( + v => /^https?:\/\//i.test(v), + { message: 'must be an http or https URL' }, +); + /** * Embedded category as returned on a place — a trimmed projection of the * categories row (id/name/color/icon), built inline by placeService and