diff --git a/client/eslint.config.mjs b/client/eslint.config.mjs index 6e48d7e4f..f007649a9 100644 --- a/client/eslint.config.mjs +++ b/client/eslint.config.mjs @@ -75,4 +75,25 @@ export default tseslint.config( 'preserve-caught-error': 'warn', }, }, + { + // react-dom/server was worth ~190 KB raw / 57 KB gzip in a chunk three lazy + // routes share — including the Leaflet renderer, which is the default — for + // output that is always a single . utils/iconMarkup.ts does that job + // without Fizz, and this keeps the import from creeping back: nothing else + // would notice, the build stays green and the app keeps working. + files: ['src/**/*.{ts,tsx}'], + ignores: ['src/**/*.test.{ts,tsx}'], + rules: { + 'no-restricted-imports': ['error', { + paths: [{ + name: 'react-dom/server', + message: 'Use renderIconMarkup from utils/iconMarkup — Fizz is ~190 KB for one .', + }], + }], + 'no-restricted-syntax': ['error', { + selector: "ImportExpression > Literal[value=/^react-dom\\u002F(server|static)/]", + message: 'Use renderIconMarkup from utils/iconMarkup — Fizz is ~190 KB for one .', + }], + }, + }, ); diff --git a/client/src/components/Map/MapView.test.tsx b/client/src/components/Map/MapView.test.tsx index 6c9fc6c1b..9bcf0c5d7 100644 --- a/client/src/components/Map/MapView.test.tsx +++ b/client/src/components/Map/MapView.test.tsx @@ -1017,3 +1017,39 @@ describe('MapView photo thumbnails', () => { expect(vi.mocked(photoService.fetchPhoto)).not.toHaveBeenCalled() }) }) + +// The marker HTML is a hand-built string handed to L.divIcon, i.e. innerHTML. +// Two of its interpolations carry values a user controls. +describe('MapView — untrusted values in the marker HTML', () => { + const iconHtml = () => screen.getAllByTestId('marker').map(el => el.getAttribute('data-icon-html') || '').join('') + + it('FE-COMP-MAPVIEW-072: a category colour that is not a hex value is dropped, not escaped', () => { + // Escaping alone would stop the attribute breakout but still leave a working + // CSS value, so the colour is allow-listed and anything else falls back. + render( { + render() + expect(iconHtml()).toContain('#00ff00') + }) +}) diff --git a/client/src/components/Map/MapView.tsx b/client/src/components/Map/MapView.tsx index 0389dbd89..ab1a1dc37 100644 --- a/client/src/components/Map/MapView.tsx +++ b/client/src/components/Map/MapView.tsx @@ -1,6 +1,6 @@ import { useEffect, useRef, useState, useMemo, useCallback, createElement, memo } from 'react' import DOM from 'react-dom' -import { renderToStaticMarkup } from 'react-dom/server' +import { renderIconMarkup } from '../../utils/iconMarkup' import { MapContainer, TileLayer, Marker, Polyline, CircleMarker, Circle, useMap, Tooltip } from 'react-leaflet' import MarkerClusterGroup from 'react-leaflet-cluster' import L from 'leaflet' @@ -13,6 +13,8 @@ import { PluginMapMarkers } from './MapPluginMarkers' import { PluginMapLayers } from './MapPluginLayers' import { useTransportRoutes } from '../../hooks/useTransportRoutes' import { visibleRouteReservations } from '../../utils/reservationRoutes' +import { safeHexColor } from '../../utils/safeColor' +import { escapeHtml } from '@trek/shared' import type { Reservation, RouteVia } from '../../types' import { POI_CATEGORY_BY_KEY, type Poi } from './poiCategories' import { resolveTrackColor, hasManualTrackColor } from './trackColors' @@ -24,7 +26,7 @@ import { computeMapViewport, TILE_SIZE_RASTER, type ViewportPadding } from '../. function categoryIconSvg(iconName: string | null | undefined, size: number): string { const IconComponent = (iconName && CATEGORY_ICON_MAP[iconName]) || CATEGORY_ICON_MAP['MapPin'] try { - return renderToStaticMarkup(createElement(IconComponent, { size, color: 'white', strokeWidth: 2.5 })) + return renderIconMarkup(createElement(IconComponent, { size, color: 'white', strokeWidth: 2.5 })) } catch { return '' } } import type { Place } from '../../types' @@ -75,12 +77,14 @@ function createPlaceIcon(place, orderNumbers, isSelected) { const cached = iconCache.get(cacheKey) if (cached) return cached const size = isSelected ? 44 : 36 - const borderColor = isSelected ? '#111827' : (place.category_color || 'white') + // Allow-listed, not escaped: the value lands in style="…" of a divIcon, where + // escaping stops the attribute breakout but still permits a CSS url(). + const borderColor = isSelected ? '#111827' : safeHexColor(place.category_color, 'white') const borderWidth = isSelected ? 3 : 2.5 const shadow = isSelected ? '0 0 0 3px rgba(17,24,39,0.25), 0 4px 14px rgba(0,0,0,0.3)' : '0 2px 8px rgba(0,0,0,0.22)' - const bgColor = place.category_color || '#6b7280' + const bgColor = safeHexColor(place.category_color, '#6b7280') // Number badges (bottom-right) let badgeHtml = '' @@ -115,7 +119,7 @@ function createPlaceIcon(place, orderNumbers, isSelected) { box-shadow:${shadow}; overflow:hidden;background:${bgColor}; "> - + ${badgeHtml} `, @@ -157,7 +161,7 @@ function createPoiIcon(category: string) { if (cached) return cached const cat = POI_CATEGORY_BY_KEY[category] const color = cat?.color || '#6b7280' - const svg = cat ? renderToStaticMarkup(createElement(cat.Icon, { size: 13, color: 'white', strokeWidth: 2.5 })) : '' + const svg = cat ? renderIconMarkup(createElement(cat.Icon, { size: 13, color: 'white', strokeWidth: 2.5 })) : '' const icon = L.divIcon({ className: '', html: `
${svg}
`, diff --git a/client/src/components/Map/MapViewGL.tsx b/client/src/components/Map/MapViewGL.tsx index 9482f9dab..c5c2bd42d 100644 --- a/client/src/components/Map/MapViewGL.tsx +++ b/client/src/components/Map/MapViewGL.tsx @@ -1,5 +1,5 @@ import { useEffect, useRef, useMemo, useState, createElement } from 'react' -import { renderToStaticMarkup } from 'react-dom/server' +import { renderIconMarkup } from '../../utils/iconMarkup' import type mapboxgl from 'mapbox-gl' import { useSettingsStore } from '../../store/settingsStore' import { useAuthStore } from '../../store/authStore' @@ -11,6 +11,8 @@ import { attachLocationMarker, type LocationMarkerHandle } from './locationMarke import { ReservationMapboxOverlay } from './reservationsMapbox' import { useTransportRoutes } from '../../hooks/useTransportRoutes' import { visibleRouteReservations } from '../../utils/reservationRoutes' +import { safeHexColor } from '../../utils/safeColor' +import { escapeHtml } from '@trek/shared' import { MAPBOX_DEFAULT_STYLE, styleForActiveProvider, basemapLanguage, type GlMapProvider } from './glProviders' import LocationButton from './LocationButton' import { useGeolocation } from '../../hooks/useGeolocation' @@ -25,7 +27,7 @@ import { computeMapViewport, TILE_SIZE_GL } from '../../utils/mapViewport' function categoryIconSvg(iconName: string | null | undefined, size: number): string { const IconComponent = (iconName && CATEGORY_ICON_MAP[iconName]) || CATEGORY_ICON_MAP['MapPin'] try { - return renderToStaticMarkup(createElement(IconComponent, { size, color: 'white', strokeWidth: 2.5 })) + return renderIconMarkup(createElement(IconComponent, { size, color: 'white', strokeWidth: 2.5 })) } catch { return '' } } @@ -130,12 +132,13 @@ interface Props { function createMarkerElement(place: Place & { category_color?: string; category_icon?: string }, photoUrl: string | null, orderNumbers: number[] | null, selected: boolean): HTMLDivElement { const size = selected ? 44 : 36 - const borderColor = selected ? '#111827' : (place.category_color || 'white') + // See MapView: allow-listed rather than escaped, because this is a CSS context. + const borderColor = selected ? '#111827' : safeHexColor(place.category_color, 'white') const borderWidth = selected ? 3 : 2.5 const shadow = selected ? '0 0 0 3px rgba(17,24,39,0.25), 0 4px 14px rgba(0,0,0,0.3)' : '0 2px 8px rgba(0,0,0,0.22)' - const bgColor = place.category_color || '#6b7280' + const bgColor = safeHexColor(place.category_color, '#6b7280') // The visual circle is `size` + 2*border on each side. To make the // mapbox `anchor: 'center'` land on the real visual middle of the marker @@ -182,7 +185,7 @@ function createMarkerElement(place: Place & { category_color?: string; category_ overflow:hidden;background:${bgColor}; box-sizing:content-box; "> - + ${badgeHtml} ` @@ -325,7 +328,7 @@ function buildPluginMarkerPopup(mk: PluginMapMarker): HTMLDivElement { function createPoiMarkerElement(category: string): HTMLDivElement { const cat = POI_CATEGORY_BY_KEY[category] const color = cat?.color || '#6b7280' - const svg = cat ? renderToStaticMarkup(createElement(cat.Icon, { size: 13, color: 'white', strokeWidth: 2.5 })) : '' + const svg = cat ? renderIconMarkup(createElement(cat.Icon, { size: 13, color: 'white', strokeWidth: 2.5 })) : '' const el = document.createElement('div') el.style.cssText = 'width:26px;height:26px;cursor:pointer;' el.innerHTML = `
${svg}
` diff --git a/client/src/components/Map/ReservationOverlay.tsx b/client/src/components/Map/ReservationOverlay.tsx index ca616ef0e..ac9daeccf 100644 --- a/client/src/components/Map/ReservationOverlay.tsx +++ b/client/src/components/Map/ReservationOverlay.tsx @@ -1,5 +1,5 @@ import { Fragment, createElement, useMemo, useState } from 'react' -import { renderToStaticMarkup } from 'react-dom/server' +import { renderIconMarkup } from '../../utils/iconMarkup' import { Marker, Polyline, Tooltip, useMap, useMapEvents } from 'react-leaflet' import L from 'leaflet' import { Plane, Train, Ship, Car, Bus, Sailboat, Bike, CarTaxiFront, Route, TramFront } from 'lucide-react' @@ -43,7 +43,7 @@ function useEndpointPane() { function endpointIcon(type: TransportType, label: string | null): L.DivIcon { const { icon: IconCmp, color } = TYPE_META[type] - const svg = renderToStaticMarkup(createElement(IconCmp, { size: 13, color: 'white', strokeWidth: 2.5 })) + const svg = renderIconMarkup(createElement(IconCmp, { size: 13, color: 'white', strokeWidth: 2.5 })) const labelHtml = label ? `${escapeHtml(label)}` : '' const estWidth = label ? Math.max(40, label.length * 6 + 28) : 26 return L.divIcon({ diff --git a/client/src/components/Map/placePopup.ts b/client/src/components/Map/placePopup.ts index ae79577e7..930f8466d 100644 --- a/client/src/components/Map/placePopup.ts +++ b/client/src/components/Map/placePopup.ts @@ -1,5 +1,5 @@ import { createElement } from 'react' -import { renderToStaticMarkup } from 'react-dom/server' +import { renderIconMarkup } from '../../utils/iconMarkup' import { CATEGORY_ICON_MAP } from '../shared/categoryIcons' import { POI_CATEGORY_BY_KEY, type Poi } from './poiCategories' import type { Place } from '../../types' @@ -28,7 +28,7 @@ function esc(s: string | null | undefined): string { function iconSvg(iconName: string | null | undefined, size: number, color: string): string { const Icon = (iconName && CATEGORY_ICON_MAP[iconName]) || CATEGORY_ICON_MAP['MapPin'] try { - return renderToStaticMarkup(createElement(Icon, { size, color, strokeWidth: 2 })) + return renderIconMarkup(createElement(Icon, { size, color, strokeWidth: 2 })) } catch { return '' } @@ -61,7 +61,7 @@ export function buildPlacePopupHtml(place: PlaceWithCategory, photoUrl: string | export function buildPoiPopupHtml(poi: Poi): string { const cat = POI_CATEGORY_BY_KEY[poi.category] const color = cat?.color || '#6b7280' - const icon = cat ? renderToStaticMarkup(createElement(cat.Icon, { size: 12, color, strokeWidth: 2 })) : '' + const icon = cat ? renderIconMarkup(createElement(cat.Icon, { size: 12, color, strokeWidth: 2 })) : '' const head = `
${icon}${esc(poi.name)}
` const address = poi.address ? `
${esc(poi.address)}
` : '' return `${CARD_OPEN}${head}${address}` diff --git a/client/src/components/Map/reservationsMapbox.test.ts b/client/src/components/Map/reservationsMapbox.test.ts index 2a7a9f39f..15ae03367 100644 --- a/client/src/components/Map/reservationsMapbox.test.ts +++ b/client/src/components/Map/reservationsMapbox.test.ts @@ -3,6 +3,7 @@ import { ReservationMapboxOverlay, RESERVATION_SOURCE_ID, RESERVATION_LINE_LAYER_ID, + TRANSIT_CASING_LAYER_ID, type ReservationOverlayOptions, } from './reservationsMapbox' import type { Reservation, ReservationEndpoint } from '../../types' @@ -165,7 +166,17 @@ function freshMap(projection: Projection = spread) { addLayer: vi.fn((layer: { id: string }) => { layers.set(layer.id, layer) }), getLayer: (id: string) => layers.get(id), removeLayer: vi.fn((id: string) => { layers.delete(id) }), - removeSource: vi.fn((id: string) => { sources.delete(id) }), + // The real engine refuses to drop a source that a layer still points at, and + // reports it by firing an error rather than throwing. Modelled as a throw here + // so a missed layer fails the test instead of only dirtying a console. + removeSource: vi.fn((id: string) => { + for (const [layerId, layer] of layers) { + if ((layer as { source?: string }).source === id) { + throw new Error(`Source "${id}" cannot be removed while layer "${layerId}" is using it.`) + } + } + sources.delete(id) + }), on: vi.fn(function (this: void, event: string, fn: Handler) { (handlers[event] ||= []).push(fn) }), off: vi.fn((event: string, fn: Handler) => { handlers[event] = (handlers[event] || []).filter(h => h !== fn) }), getZoom: () => 12, @@ -265,7 +276,12 @@ describe('ReservationMapboxOverlay lifecycle', () => { expect(map.listeners('zoomend')).toBe(0) expect(map.listeners('moveend')).toBe(0) expect(map.removeLayer).toHaveBeenCalledWith(RESERVATION_LINE_LAYER_ID) + expect(map.removeLayer).toHaveBeenCalledWith(TRANSIT_CASING_LAYER_ID) expect(map.removeSource).toHaveBeenCalledWith(RESERVATION_SOURCE_ID) + // Nothing left behind: the casing layer used to survive teardown, which kept + // the source alive with it and made every map unmount log an error. + expect(map._layers.size).toBe(0) + expect(map._sources.size).toBe(0) }) it('FE-COMP-RESGL-004: destroy survives a map whose style is already gone', () => { diff --git a/client/src/components/Map/reservationsMapbox.ts b/client/src/components/Map/reservationsMapbox.ts index 91b73e960..63af95c59 100644 --- a/client/src/components/Map/reservationsMapbox.ts +++ b/client/src/components/Map/reservationsMapbox.ts @@ -7,7 +7,7 @@ // renderers produce the same visual result on the globe or a flat projection. import { createElement } from 'react' -import { renderToStaticMarkup } from 'react-dom/server' +import { renderIconMarkup } from '../../utils/iconMarkup' import type mapboxgl from 'mapbox-gl' import { Plane, Train, Ship, Car, Bus, Sailboat, Bike, CarTaxiFront, Route, TramFront } from 'lucide-react' import { getTransitMapSegments } from './transitGeometry' @@ -17,6 +17,8 @@ import type { Reservation, ReservationEndpoint } from '../../types' export const RESERVATION_SOURCE_ID = 'trek-reservations' export const RESERVATION_LINE_LAYER_ID = 'trek-reservations-lines' +/** Sits under the coloured transit lines; named here so teardown can find it. */ +export const TRANSIT_CASING_LAYER_ID = `${RESERVATION_LINE_LAYER_ID}-transit-casing` type TransportType = 'flight' | 'train' | 'cruise' | 'car' | 'bus' | 'taxi' | 'bicycle' | 'ferry' | 'transit' | 'transport_other' const TRANSPORT_TYPES: TransportType[] = ['flight', 'train', 'cruise', 'car', 'bus', 'taxi', 'bicycle', 'ferry', 'transit', 'transport_other'] @@ -153,7 +155,7 @@ function buildItems(reservations: Reservation[]): TransportItem[] { // ── DOM helpers for HTML markers ────────────────────────────────────────── function endpointMarkerHtml(type: TransportType, label: string | null): string { const { icon: IconCmp } = TYPE_META[type] - const svg = renderToStaticMarkup(createElement(IconCmp, { size: 13, color: 'white', strokeWidth: 2.5 })) + const svg = renderIconMarkup(createElement(IconCmp, { size: 13, color: 'white', strokeWidth: 2.5 })) const labelHtml = label ? `${escapeHtml(label)}` : '' return `
[0] + +const ICONS = Object.entries(lucide).filter( + ([name, value]) => + /^[A-Z]/.test(name) && + typeof value === 'object' && + value !== null && + (value as { $$typeof?: symbol }).$$typeof === Symbol.for('react.forward_ref') +) as [string, IconComponent][] + +// The three prop shapes the call sites actually use. +const PROP_SHAPES: Record[] = [ + {}, + { size: 13, strokeWidth: 2.5, color: 'white' }, + { size: 24, strokeWidth: 1.8, color: 'rgba(255,255,255,0.92)', className: 'x' }, +] + +describe('renderIconMarkup parity with react-dom/server', () => { + it('FE-UTIL-ICONMARKUP-001: the icon set is actually found', () => { + // Guards against a lucide restructure silently turning this suite into a no-op. + expect(ICONS.length).toBeGreaterThan(1000) + }) + + it('FE-UTIL-ICONMARKUP-002: every exported icon serializes byte-identically', () => { + const mismatches: string[] = [] + for (const [name, Icon] of ICONS) { + for (const props of PROP_SHAPES) { + const expected = renderToStaticMarkup(createElement(Icon, props)) + const actual = renderIconMarkup(createElement(Icon, props)) + if (actual !== expected) { + mismatches.push(`${name} ${JSON.stringify(props)}\n fizz: ${expected}\n ours: ${actual}`) + } + } + } + expect(mismatches.slice(0, 5).join('\n')).toBe('') + }) + + it('FE-UTIL-ICONMARKUP-003: viewBox keeps its camel case', () => { + // A naive prop-name-to-kebab rule turns this into view-box, and the icon + // renders as an invisible zero-size box. + expect(renderIconMarkup(createElement(lucide.Utensils))).toContain('viewBox="0 0 24 24"') + }) + + it('FE-UTIL-ICONMARKUP-004: attribute values are escaped like React escapes them', () => { + const hostile = 'red" onload="alert(1)' + const expected = renderToStaticMarkup(createElement(lucide.Utensils, { color: hostile })) + expect(renderIconMarkup(createElement(lucide.Utensils, { color: hostile }))).toBe(expected) + expect(renderIconMarkup(createElement(lucide.Utensils, { color: hostile }))).not.toContain('onload="alert(1)"') + }) + + it('FE-UTIL-ICONMARKUP-005: a throwing component yields an empty string, not an exception', () => { + const Boom = () => { throw new Error('boom') } + expect(renderIconMarkup(createElement(Boom))).toBe('') + }) +}) diff --git a/client/src/utils/iconMarkup.ts b/client/src/utils/iconMarkup.ts new file mode 100644 index 000000000..105ddbb5e --- /dev/null +++ b/client/src/utils/iconMarkup.ts @@ -0,0 +1,109 @@ +import type { ReactElement, ReactNode } from 'react' + +/** + * Static-markup serializer for the lucide icons the map and PDF code turn into + * SVG strings. + * + * It replaces renderToStaticMarkup, whose Fizz runtime is ~190 kB raw / ~57 kB + * gzip of a chunk that three lazy route chunks share — including the Leaflet + * renderer, which is the default. For output that is always a single with + * a flat list of shape children, that is a poor trade. + * + * Deliberately generic over React elements rather than over lucide's icon + * tables: `iconNode` is closed over inside createLucideIcon and not exported, so + * calling the forwardRef render function is what keeps this working across + * lucide upgrades. iconMarkup.parity.test.ts diffs the output against real Fizz + * for every exported icon, so drift fails the suite instead of shipping. + * + * Byte-compatibility notes, all verified against react-dom 19.2.6: + * - React escapes & " ' < > the same way in attribute values and in text, so + * one escape function covers both. + * - Only the props in ATTRIBUTE_NAMES are renamed. Everything else keeps its + * literal name, which is what preserves the camelCase SVG attributes — + * viewBox has to stay viewBox and must NOT become view-box. + * - Fizz emits , never a self-closing tag. So do we. + */ + +const FORWARD_REF = Symbol.for('react.forward_ref') + +/** React's DOM property -> attribute renames, limited to what an SVG carries. */ +const ATTRIBUTE_NAMES: Record = { + className: 'class', + htmlFor: 'for', + strokeWidth: 'stroke-width', + strokeLinecap: 'stroke-linecap', + strokeLinejoin: 'stroke-linejoin', + strokeDasharray: 'stroke-dasharray', + strokeDashoffset: 'stroke-dashoffset', + strokeOpacity: 'stroke-opacity', + strokeMiterlimit: 'stroke-miterlimit', + fillOpacity: 'fill-opacity', + fillRule: 'fill-rule', + clipRule: 'clip-rule', + clipPath: 'clip-path', + vectorEffect: 'vector-effect', + paintOrder: 'paint-order', + shapeRendering: 'shape-rendering', + stopColor: 'stop-color', + stopOpacity: 'stop-opacity', +} + +const SKIP_PROPS = new Set(['children', 'key', 'ref', 'dangerouslySetInnerHTML']) + +type ElementProps = Record & { children?: ReactNode } +type ForwardRefComponent = { $$typeof: symbol; render: (props: ElementProps, ref: null) => ReactNode } + +function escape(value: string | number): string { + return String(value) + .replace(/&/g, '&') + .replace(/"/g, '"') + .replace(/'/g, ''') + .replace(//g, '>') +} + +function serializeProps(props: ElementProps): string { + let out = '' + for (const name of Object.keys(props)) { + if (SKIP_PROPS.has(name)) continue + const value = props[name] + if (value === undefined || value === null || value === false) continue + if (typeof value === 'function' || typeof value === 'symbol' || typeof value === 'object') continue + out += ` ${ATTRIBUTE_NAMES[name] ?? name}="${escape(value === true ? '' : (value as string | number))}"` + } + return out +} + +function serialize(node: unknown): string { + if (node === null || node === undefined || typeof node === 'boolean') return '' + if (typeof node === 'string' || typeof node === 'number') return escape(node) + if (Array.isArray(node)) return node.map(serialize).join('') + + const element = node as ReactElement + const type = element.type as unknown + const props = (element.props ?? {}) as ElementProps + + if (typeof type === 'string') { + return `<${type}${serializeProps(props)}>${serialize(props.children)}` + } + if (typeof type === 'object' && type !== null && (type as ForwardRefComponent).$$typeof === FORWARD_REF) { + return serialize((type as ForwardRefComponent).render(props, null)) + } + if (typeof type === 'function') { + return serialize((type as (p: ElementProps) => ReactNode)(props)) + } + throw new Error(`renderIconMarkup: unsupported element type ${String(type)}`) +} + +/** + * Empty string when the element cannot be rendered: a throwing icon component + * should cost its own glyph, not the marker it sits in. That matches how the + * call sites already guarded renderToStaticMarkup. + */ +export function renderIconMarkup(element: ReactElement): string { + try { + return serialize(element) + } catch { + return '' + } +} diff --git a/client/src/utils/safeColor.ts b/client/src/utils/safeColor.ts new file mode 100644 index 000000000..1299f6dd2 --- /dev/null +++ b/client/src/utils/safeColor.ts @@ -0,0 +1,19 @@ +import { hexColorSchema } from '@trek/shared' + +/** + * Allow-list a colour before it goes into a hand-built HTML string. + * + * Category and tag colours are plain strings in the older contracts, and both map + * renderers paste them straight into a style="…" attribute of markup that ends up + * in L.divIcon / innerHTML / setHTML. Escaping alone would stop the attribute + * breakout but still let a CSS value through — `url(https://…)` in a background + * is a working tracking pixel — so a colour is allow-listed instead: anything + * that is not #rgb or #rrggbb falls back. + * + * Server-side validation closes the write path. This closes the read path, which + * is what covers rows written before that validation existed, instances that have + * not updated, and anything a compromised admin account left behind. + */ +export function safeHexColor(value: string | null | undefined, fallback: string): string { + return typeof value === 'string' && hexColorSchema.safeParse(value).success ? value : fallback +} diff --git a/server/src/nest/categories/categories.controller.ts b/server/src/nest/categories/categories.controller.ts index cbd9339bc..a0f9da370 100644 --- a/server/src/nest/categories/categories.controller.ts +++ b/server/src/nest/categories/categories.controller.ts @@ -2,6 +2,7 @@ import { Body, Controller, Delete, Get, HttpException, Param, Post, Put, UseGuar import type { Category, CategoryListResponse } from '@trek/shared'; import type { User } from '../../types'; import { CategoriesService } from './categories.service'; +import { CategoryCreateDto, CategoryUpdateDto } from './categories.dto'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { AdminGuard } from '../auth/admin.guard'; import { CurrentUser } from '../auth/current-user.decorator'; @@ -9,11 +10,17 @@ import { CurrentUser } from '../auth/current-user.decorator'; /** * /api/categories — place-category palette CRUD. * - * Byte-identical to the legacy Express route (server/src/routes/categories.ts): - * listing is open to any authenticated user; create/update/delete require admin - * (JwtAuthGuard + AdminGuard). Status codes match the Nest defaults the legacy - * route also used (201 on create, 200 elsewhere), and the bespoke 400/404 bodies - * are reproduced exactly. + * Listing is open to any authenticated user; create/update/delete require admin + * (JwtAuthGuard + AdminGuard). Status codes match the legacy Express route + * (201 on create, 200 elsewhere) and the bespoke 404 body is reproduced exactly. + * + * Bodies now validate against the @trek/shared schemas through the DTO classes + * and the global ZodValidationPipe. That replaced the hand-rolled 400 ("Category + * name is required") with the pipe's envelope — the same trade the places, todo + * and trips migrations made. It matters here because `color` was reaching a + * style="…" attribute of hand-built marker HTML on both map renderers and on the + * share page, which answers without a guard; `@Body('color')` carries no + * metatype, so nothing validated it. */ @Controller('api/categories') export class CategoriesController { @@ -27,30 +34,17 @@ export class CategoriesController { @Post() @UseGuards(JwtAuthGuard, AdminGuard) - create( - @CurrentUser() user: User, - @Body('name') name?: string, - @Body('color') color?: string, - @Body('icon') icon?: string, - ): { category: Category } { - if (!name) { - throw new HttpException({ error: 'Category name is required' }, 400); - } - return { category: this.categories.create(user.id, name, color, icon) }; + create(@CurrentUser() user: User, @Body() body: CategoryCreateDto): { category: Category } { + return { category: this.categories.create(user.id, body.name, body.color, body.icon) }; } @Put(':id') @UseGuards(JwtAuthGuard, AdminGuard) - update( - @Param('id') id: string, - @Body('name') name?: string, - @Body('color') color?: string, - @Body('icon') icon?: string, - ): { category: Category } { + update(@Param('id') id: string, @Body() body: CategoryUpdateDto): { category: Category } { if (!this.categories.getById(id)) { throw new HttpException({ error: 'Category not found' }, 404); } - return { category: this.categories.update(id, name, color, icon) }; + return { category: this.categories.update(id, body.name, body.color, body.icon) }; } @Delete(':id') diff --git a/server/src/nest/categories/categories.dto.ts b/server/src/nest/categories/categories.dto.ts new file mode 100644 index 000000000..94b453606 --- /dev/null +++ b/server/src/nest/categories/categories.dto.ts @@ -0,0 +1,13 @@ +import { createZodDto } from 'nestjs-zod'; +import { createCategoryRequestSchema, updateCategoryRequestSchema } from '@trek/shared'; + +/** + * createZodDto wrappers over the @trek/shared category contracts. + * + * Typing a controller parameter with one of these is what lets the global + * ZodValidationPipe validate it by metatype — `@Body('color')` reads a property + * and carries no metatype, so it was never validated at all. The schemas in + * shared/ stay the single source of truth. + */ +export class CategoryCreateDto extends createZodDto(createCategoryRequestSchema) {} +export class CategoryUpdateDto extends createZodDto(updateCategoryRequestSchema) {} diff --git a/server/src/nest/common/body-contract-allow-list.ts b/server/src/nest/common/body-contract-allow-list.ts index be1393433..a1d70afd1 100644 --- a/server/src/nest/common/body-contract-allow-list.ts +++ b/server/src/nest/common/body-contract-allow-list.ts @@ -12,8 +12,6 @@ export const BODY_CONTRACT_ALLOW_LIST: string[] = [ 'BookingImportController.confirm', 'BookingImportController.preview', 'BookingImportController.previewAsync', - 'CategoriesController.create', - 'CategoriesController.update', 'ImmichMemoriesController.putSettings', 'ImmichMemoriesController.search', 'ImmichMemoriesController.test', diff --git a/server/tests/e2e/categories.e2e.test.ts b/server/tests/e2e/categories.e2e.test.ts index 22916e14b..098af48ec 100644 --- a/server/tests/e2e/categories.e2e.test.ts +++ b/server/tests/e2e/categories.e2e.test.ts @@ -35,6 +35,7 @@ vi.mock('../../src/db/database', () => ({ db, closeDb: () => {}, reinitialize: ( import { CategoriesModule } from '../../src/nest/categories/categories.module'; import { DatabaseModule } from '../../src/nest/database/database.module'; import { TrekExceptionFilter } from '../../src/nest/common/trek-exception.filter'; +import { ZodValidationPipe } from '../../src/nest/common/zod-validation.pipe'; function insertCategory(name: string, color = '#6366f1', icon = '📍', userId = 1): number { const res = db @@ -52,6 +53,11 @@ describe('Categories e2e (real JwtAuthGuard + AdminGuard + temp SQLite)', () => const nest = moduleRef.createNestApplication(); nest.use(cookieParser()); nest.useGlobalFilters(new TrekExceptionFilter()); + // The harness builds a container around the one domain module, so the APP_PIPE + // from app.module.ts is not in it. Registering it here is what the other e2e + // suites do (see places.e2e.test.ts) and it is required for the DTO bodies to + // be validated at all. + nest.useGlobalPipes(new ZodValidationPipe()); await nest.init(); return nest; } @@ -107,7 +113,27 @@ describe('Categories e2e (real JwtAuthGuard + AdminGuard + temp SQLite)', () => it('400 when an admin creates without a name', async () => { const res = await request(server).post('/api/categories').set('Cookie', sessionCookie(1)).send({}); expect(res.status).toBe(400); - expect(res.body).toEqual({ error: 'Category name is required' }); + }); + + // The colour is pasted into a style="…" attribute of hand-built marker HTML on + // both map renderers and on the share page, which answers without a guard. It + // was reaching the database unvalidated, because @Body('color') reads a property + // and carries no metatype for the pipe to work with. + it('400 when an admin sends a colour that is not a hex value', async () => { + const res = await request(server) + .post('/api/categories') + .set('Cookie', sessionCookie(1)) + .send({ name: 'Food', color: 'red" onmouseover="alert(1)' }); + expect(res.status).toBe(400); + }); + + it('400 when an admin updates a category to a non-hex colour', async () => { + const id = insertCategory('Food', '#fff', '🍔'); + const res = await request(server) + .put(`/api/categories/${id}`) + .set('Cookie', sessionCookie(1)) + .send({ color: 'url(https://evil.example/px)' }); + expect(res.status).toBe(400); }); it('200 when an admin updates, COALESCE preserving omitted fields', async () => { diff --git a/server/tests/unit/nest/categories.controller.test.ts b/server/tests/unit/nest/categories.controller.test.ts index 6b7b95635..ec0fe9b62 100644 --- a/server/tests/unit/nest/categories.controller.test.ts +++ b/server/tests/unit/nest/categories.controller.test.ts @@ -31,17 +31,13 @@ describe('CategoriesController (parity with the legacy /api/categories route)', }); describe('POST /', () => { - it('400 when name is missing', () => { - const create = vi.fn(); - expect(thrown(() => makeController({ create }).create(admin, undefined))).toEqual({ - status: 400, body: { error: 'Category name is required' }, - }); - expect(create).not.toHaveBeenCalled(); - }); + // A body without a name no longer reaches the handler: createCategoryRequestSchema + // requires it, and the global ZodValidationPipe rejects it by metatype. The + // contract itself is covered by shared/src/category/category.schema.spec.ts. it('creates and returns { category }', () => { const create = vi.fn().mockReturnValue(cat); - expect(makeController({ create }).create(admin, 'Food', '#fff', '🍔')).toEqual({ category: cat }); + expect(makeController({ create }).create(admin, { name: 'Food', color: '#fff', icon: '🍔' })).toEqual({ category: cat }); expect(create).toHaveBeenCalledWith(1, 'Food', '#fff', '🍔'); }); }); @@ -59,7 +55,7 @@ describe('CategoriesController (parity with the legacy /api/categories route)', it('updates and returns { category }', () => { const getById = vi.fn().mockReturnValue(cat); const update = vi.fn().mockReturnValue({ ...cat, name: 'Drinks' }); - expect(makeController({ getById, update }).update('1', 'Drinks')).toEqual({ category: { ...cat, name: 'Drinks' } }); + expect(makeController({ getById, update }).update('1', { name: 'Drinks' })).toEqual({ category: { ...cat, name: 'Drinks' } }); expect(update).toHaveBeenCalledWith('1', 'Drinks', undefined, undefined); }); }); diff --git a/shared/src/category/category.schema.ts b/shared/src/category/category.schema.ts index 0ad42d5b7..6d761b7e7 100644 --- a/shared/src/category/category.schema.ts +++ b/shared/src/category/category.schema.ts @@ -1,4 +1,5 @@ import { z } from 'zod'; +import { hexColorSchema } from '../place/place.schema'; /** * Category API contract — single source of truth for the /api/categories endpoints. @@ -12,6 +13,13 @@ import { z } from 'zod'; * messages are reproduced in the controller so the bodies stay byte-identical. */ +/** + * Note the asymmetry below: the response keeps `color: z.string()` while the + * requests demand a hex value. Tightening the response would make an instance + * fail on its own stored rows — the write path was unvalidated for a long time, + * so anything could be in there. The client allow-lists on read for the same + * reason (client/src/utils/safeColor.ts). + */ export const categorySchema = z.object({ id: z.number(), name: z.string(), @@ -24,7 +32,10 @@ export type Category = z.infer; export const createCategoryRequestSchema = z.object({ name: z.string().min(1), - color: z.string().optional(), + // Hex only: the value is interpolated into a style="…" attribute of the + // hand-built marker HTML on both map renderers, and on the share page that + // answers without a guard. + color: hexColorSchema.optional(), icon: z.string().optional(), }); export type CreateCategoryRequest = z.infer; @@ -32,7 +43,10 @@ export type CreateCategoryRequest = z.infer; /** All fields optional — the service COALESCEs each against the stored value. */ export const updateCategoryRequestSchema = z.object({ name: z.string().optional(), - color: z.string().optional(), + // Hex only: the value is interpolated into a style="…" attribute of the + // hand-built marker HTML on both map renderers, and on the share page that + // answers without a guard. + color: hexColorSchema.optional(), icon: z.string().optional(), }); export type UpdateCategoryRequest = z.infer;