From cf62567c48f0f91bf12a7c4367279d49160e2ebf Mon Sep 17 00:00:00 2001 From: Maurice <61554723+mauriceboe@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:43:38 +0200 Subject: [PATCH] fix(map): stop untrusted values reaching the marker HTML (#1820) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(map): drop the casing layer before its source on teardown Tearing down the reservation overlay logged an error on every map unmount: Source "trek-reservations" cannot be removed while layer "trek-reservations-lines-transit-casing" is using it. The casing layer arrived with the transit paths in 3.2.0 but was never added to destroy(), which only removed the main line layer. The try/catch around it gave no protection either — the engine reports this by firing an error event rather than throwing — so the source was silently left behind on the style. The layer id is a named constant now, so the place that creates it and the place that removes it can no longer drift apart. The lifecycle test grew the assertion that would have caught it: the fake map now refuses to remove a source that a layer still references, the same way the real one does. Verified against the unfixed code — two tests fail without the change. * fix(map): stop untrusted values reaching the marker HTML Both map renderers build their marker HTML as a string and hand it to L.divIcon / innerHTML. Two of the interpolations carry values a user controls, and neither was escaped. place.image_url is the worse of the two, because it is not admin-only: any trip member with edit rights can set it. placeUpdateRequestSchema is a plain record of unknown values, and the length guard in places.controller covers name, description, address and notes but not image_url. The renderers do check the value, but only with startsWith('/uploads/') — a payload like /uploads/x" onerror="…" y=" satisfies that and then breaks out of the src="…" attribute. category.color is the same shape one line up, reachable by an admin. It also reaches SharedTripPage, which builds its own divIcon and answers without a guard, so that payload lands in front of people who have no account here. Colours are allow-listed rather than escaped. Escaping stops the attribute breakout but leaves a working CSS value behind, and `url(https://…)` in a background is a tracking pixel. Anything that is not #rgb/#rrggbb falls back. image_url is a URL, not an enum, so that one is escaped; the prefix check stays. Server side, the category contract now demands a hex colour, and the endpoints validate through DTO classes instead of @Body('color') — a property read carries no metatype, so the global pipe never saw it. That retires two entries from the body-contract allow list. It replaces the hand-rolled 400 for a missing name with the pipe's envelope, the same trade the places and trips migrations made. The response schema deliberately still accepts any string: the write path was open for a long time, so tightening it would make an instance fail on its own stored rows. The client allow-list is what covers those. Verified against the unpatched renderer — the three new cases fail without it. * perf(client): serialize the map icons without react-dom/server Six modules imported react-dom/server statically to turn a lucide icon into an SVG string. That put Fizz — roughly 190 kB raw, 57 kB gzip — into a chunk three lazy routes share, the Leaflet renderer among them, which is the default. For output that is always a single with a flat list of shape children. mapViewport chunk 189,532 -> 3,710 B Tooltip chunk 194,333 -> 1,970 + 4,860 B precache total 17,856 -> 17,675 KiB utils/iconMarkup.ts serializes React elements directly. It is generic over elements rather than over lucide's icon tables on purpose: iconNode is closed over inside createLucideIcon and never exported, so calling the forwardRef render function is what survives a lucide upgrade. Byte-compatibility is checked, not asserted. iconMarkup.parity.test.ts diffs the output against real Fizz for every exported icon in three prop shapes — the test file is the one place still allowed to import react-dom/server, and it never reaches a bundle. Two traps it covers: viewBox must not be kebab-cased into view-box, and Fizz emits rather than a self-closing tag. TripPDF loses its lazy renderer entirely — ensureRenderer, the module-level handle and the two "not ready yet" guards that returned an empty string. There is nothing left to load. An eslint rule keeps the import from creeping back. Without it the regression is invisible: the build stays green, the app keeps working, and the chunk quietly grows by 190 kB again. --- client/eslint.config.mjs | 21 ++++ client/src/components/Map/MapView.test.tsx | 36 ++++++ client/src/components/Map/MapView.tsx | 16 ++- client/src/components/Map/MapViewGL.tsx | 15 ++- .../src/components/Map/ReservationOverlay.tsx | 4 +- client/src/components/Map/placePopup.ts | 6 +- .../components/Map/reservationsMapbox.test.ts | 18 ++- .../src/components/Map/reservationsMapbox.ts | 17 ++- client/src/components/PDF/TripPDF.tsx | 18 +-- client/src/pages/SharedTripPage.tsx | 9 +- client/src/utils/iconMarkup.parity.test.ts | 69 +++++++++++ client/src/utils/iconMarkup.ts | 109 ++++++++++++++++++ client/src/utils/safeColor.ts | 19 +++ .../nest/categories/categories.controller.ts | 38 +++--- server/src/nest/categories/categories.dto.ts | 13 +++ .../nest/common/body-contract-allow-list.ts | 2 - server/tests/e2e/categories.e2e.test.ts | 28 ++++- .../unit/nest/categories.controller.test.ts | 14 +-- shared/src/category/category.schema.ts | 18 ++- 19 files changed, 396 insertions(+), 74 deletions(-) create mode 100644 client/src/utils/iconMarkup.parity.test.ts create mode 100644 client/src/utils/iconMarkup.ts create mode 100644 client/src/utils/safeColor.ts create mode 100644 server/src/nest/categories/categories.dto.ts 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;