diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 46c76f682..a19cbbc3b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -11,6 +11,13 @@ on: - 'client/**' - 'shared/**' - 'plugin-sdk/**' + # npm workspaces hoist every dependency into the single root lockfile, so + # a dependency bump touches none of the four directories above — and a + # native binding like better-sqlite3 is exactly what needs the suite. + - 'package.json' + - 'package-lock.json' + - 'Dockerfile' + - 'docker-compose.yml' - '.github/workflows/test.yml' jobs: diff --git a/server/src/nest/day-notes/day-notes.mcp.ts b/server/src/nest/day-notes/day-notes.mcp.ts index 71a2782ef..a0ba802f7 100644 --- a/server/src/nest/day-notes/day-notes.mcp.ts +++ b/server/src/nest/day-notes/day-notes.mcp.ts @@ -40,7 +40,7 @@ export class DayNotesMcp { dayId: z.number().int().positive(), text: z.string().min(1).max(500), time: z.string().max(250).optional().describe('Time label (e.g. "09:00" or "Morning")'), - icon: z.string().optional().describe('Emoji icon for the note'), + icon: z.string().max(64).optional().describe('Emoji icon for the note'), }, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, access: { group: 'trips', mode: 'write' }, @@ -67,7 +67,7 @@ export class DayNotesMcp { noteId: z.number().int().positive(), text: z.string().min(1).max(500).optional(), time: z.string().max(250).nullable().optional().describe('Time label (e.g. "09:00" or "Morning"), or null to clear'), - icon: z.string().optional().describe('Emoji icon for the note'), + icon: z.string().max(64).optional().describe('Emoji icon for the note'), }, annotations: TOOL_ANNOTATIONS_WRITE, access: { group: 'trips', mode: 'write' }, diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index f0d6285a9..1d27a5f4a 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -119,8 +119,9 @@ export function createMcpTestRegistry(): McpRegistry { const journeyDomain = new JourneyDomainService(dbService, realtimeService, new TrekPhotosRepository(dbService)); // The last three were previously omitted, which left them `undefined` at // runtime — silently fine while nothing called them, a TypeError the moment - // the journey skeleton hooks landed on the place write paths. tsconfig only - // includes `src`, so nothing here is typechecked; pass them for real. + // the journey skeleton hooks landed on the place write paths. tsconfig.tests.json + // covers `tests` now and CI runs it (npm run typecheck:tests), so a missed + // dependency fails the build — pass them for real regardless of the gate. const placesService = new PlacesService( dbService, permissionsService, realtimeService, mapsService, queryHelpersService, new UnsplashService(dbService, new RuntimeEnvService(), generalStorage), diff --git a/server/tests/helpers/test-db.ts b/server/tests/helpers/test-db.ts index cabce9375..cd283ed10 100644 --- a/server/tests/helpers/test-db.ts +++ b/server/tests/helpers/test-db.ts @@ -217,6 +217,20 @@ export function resetTestDb(db: Database.Database): void { seedDefaults(db); } +/** + * Byte-for-byte the statement in src/db/database.ts. + * + * Exported because the same query is copied into ~90 test files, and every one + * of those copies had dropped `t.currency` — so the budget domain, which reads + * exactly that column off the access row, was only ever exercising its 'EUR' + * fallback. Import this instead of retyping it. + */ +export const CAN_ACCESS_TRIP_SQL = ` + SELECT t.id, t.user_id, t.currency FROM trips t + LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = ? + WHERE t.id = ? AND (t.user_id = ? OR m.user_id IS NOT NULL) + `; + /** * Returns the mock factory for vi.mock('../../src/db/database', ...). * The returned object mirrors the shape of database.ts exports. @@ -266,11 +280,7 @@ export function buildDbMock(testDb: Database.Database) { }; }, canAccessTrip: (tripId: number | string, userId: number) => { - return testDb.prepare(` - SELECT t.id, t.user_id FROM trips t - LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = ? - WHERE t.id = ? AND (t.user_id = ? OR m.user_id IS NOT NULL) - `).get(userId, tripId, userId); + return testDb.prepare(CAN_ACCESS_TRIP_SQL).get(userId, tripId, userId); }, isOwner: (tripId: number | string, userId: number) => { return !!testDb.prepare('SELECT id FROM trips WHERE id = ? AND user_id = ?').get(tripId, userId); diff --git a/server/tests/integration/security.test.ts b/server/tests/integration/security.test.ts index ceac29db1..9e721c1c6 100644 --- a/server/tests/integration/security.test.ts +++ b/server/tests/integration/security.test.ts @@ -221,7 +221,10 @@ describe('File download path traversal', () => { const res = await request(app) .get(`/api/trips/${trip.id}/files/${fileId}/download`) .set(authHeader(user.id)); - // path.basename() strips traversal in the download controller; normalized file does not exist in uploads - expect(res.status).not.toBe(200); + // path.basename() strips traversal in the download controller; the normalized + // name does not exist in uploads, so the answer is the same 404 a missing file + // gets. Pinned exactly: a 500 from a thrown guard would also be "not 200". + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'File not found' }); }); }); diff --git a/server/tests/unit/nest/booking-import/kitinerary-extractor.service.test.ts b/server/tests/unit/nest/booking-import/kitinerary-extractor.service.test.ts index 4780f1f08..1ebf54290 100644 --- a/server/tests/unit/nest/booking-import/kitinerary-extractor.service.test.ts +++ b/server/tests/unit/nest/booking-import/kitinerary-extractor.service.test.ts @@ -8,10 +8,13 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; * at all, and every branch that resolves it is a filesystem lookup that behaves * differently on the three platforms TREK ships to. */ -const { existsSync, readdirSync, readEnv } = vi.hoisted(() => ({ +const { existsSync, readdirSync, readEnv, execSync, execFile } = vi.hoisted(() => ({ existsSync: vi.fn(), readdirSync: vi.fn(), readEnv: vi.fn(), + execSync: vi.fn(), + // A plain function, because the service promisifies it at module load. + execFile: vi.fn(), })); vi.mock('node:fs', () => ({ @@ -20,6 +23,10 @@ vi.mock('node:fs', () => ({ writeFileSync: vi.fn(), unlinkSync: vi.fn(), })); +// The last branch of the probe shells out. Unmocked, the suite spawns a real +// process on every machine and comes back green-or-red depending on whether the +// developer happens to have KItinerary installed. +vi.mock('node:child_process', () => ({ execSync, execFile })); vi.mock('../../../../src/app-config', () => ({ readEnv })); import { KitineraryExtractorService } from '../../../../src/nest/booking-import/kitinerary-extractor.service'; @@ -35,6 +42,7 @@ beforeEach(() => { vi.clearAllMocks(); existsSync.mockReturnValue(false); readdirSync.mockReturnValue([]); + execSync.mockImplementation(() => { throw new Error('command not found'); }); }); describe('KitineraryExtractorService binary probe', () => { @@ -70,4 +78,10 @@ describe('KitineraryExtractorService binary probe', () => { it('KIT-EXT-006: extracting without a binary fails loudly', async () => { await expect(boot().extract(Buffer.from(''), 'x.pdf')).rejects.toThrow('not available'); }); + + it('KIT-EXT-007: falls back to the binary on PATH when nothing is on disk', () => { + execSync.mockReturnValue(Buffer.from('')); + expect(boot().isAvailable()).toBe(true); + expect(execSync).toHaveBeenCalledWith('kitinerary-extractor --version', expect.objectContaining({ timeout: 3000 })); + }); }); diff --git a/shared/src/book/book.schema.spec.ts b/shared/src/book/book.schema.spec.ts index 1f29fc7b7..936b48aa8 100644 --- a/shared/src/book/book.schema.spec.ts +++ b/shared/src/book/book.schema.spec.ts @@ -3,6 +3,7 @@ import { bookBadgeElementSchema, bookIconElementSchema, bookSpreadSchema, + bookStatsElementSchema, bookTextElementSchema, normalizeBookDocument, } from './book.schema'; @@ -136,6 +137,22 @@ describe('a badge code', () => { }); }); +describe('the figures a stats element carries', () => { + const stats = (over: Record = {}) => ({ id: 's1', kind: 'stats', frame, ...over }); + + it('keeps the metrics it is drawn from', () => { + const parsed = bookStatsElementSchema.parse(stats({ values: { distance: 1420000, days: 12 } })); + expect(parsed.values).toEqual({ distance: 1420000, days: 12 }); + }); + + it('drops a key no reader could ever index by, without failing the element', () => { + // Dropped rather than rejected: a rejected stats element is stripped as + // unreadable, which loses the page the figures were on. + const parsed = bookStatsElementSchema.parse(stats({ values: { distance: 12, sneaky: 1, ['x'.repeat(5000)]: 2 } })); + expect(parsed.values).toEqual({ distance: 12 }); + }); +}); + describe('normalizeBookDocument', () => { it('keeps the nine elements it can read when the tenth is from another version', () => { const nine = Array.from({ length: 9 }, (_, i) => text({ id: `t${i + 1}`, text: `line ${i + 1}` })); @@ -172,6 +189,34 @@ describe('normalizeBookDocument', () => { expect(out.page.pageWidth).toBe(210); }); + it('falls back to the preset for a page dimension nothing could be printed at', () => { + // A page block that fails takes the whole document with it, and the empty + // book that comes back is what the server then writes down. So each of + // these degrades to its default and the spread survives. + for (const bad of [0, -5, 1e12]) { + const out = normalizeBookDocument({ + ...doc([text({ id: 'a' })]), + page: { preset: 'square-210', pageWidth: bad, pageHeight: 210, bleed: 3, safe: 5 }, + }); + expect(out.page.pageWidth).toBe(210); + expect(out.spreads[0]!.elements.map((el) => el.id)).toEqual(['a']); + } + + const negativeBleed = normalizeBookDocument({ + ...doc([text({ id: 'a' })]), + page: { preset: 'square-210', pageWidth: 210, pageHeight: 210, bleed: -3, safe: 5 }, + }); + expect(negativeBleed.page.bleed).toBe(3); + expect(negativeBleed.spreads[0]!.elements).toHaveLength(1); + }); + + it('drops an element parked a kilometre off the spread rather than the book', () => { + const out = normalizeBookDocument(doc([text({ id: 'a' }), text({ id: 'far', frame: { x: 1e9, y: 0, w: 60, h: 40 } })])); + + expect(out.spreads[0]!.elements.map((el) => el.id)).toEqual(['a']); + expect(out.title).toBe('Iceland, end to end'); + }); + it('hands back an empty book for something that is not a document at all', () => { for (const junk of [undefined, null, 'a book, honestly', 42, [], { spreads: 'nope' }]) { const out = normalizeBookDocument(junk); diff --git a/shared/src/book/book.schema.ts b/shared/src/book/book.schema.ts index f1c603c24..3a2e23610 100644 --- a/shared/src/book/book.schema.ts +++ b/shared/src/book/book.schema.ts @@ -25,8 +25,16 @@ import { z } from 'zod'; * edits the text in Studio, which sets `overridden` and stops the sync. */ -/** Two decimals of a millimetre. Anything finer is below print resolution. */ -const mm = z.number().finite().transform(v => Math.round(v * 100) / 100); +/** + * Two decimals of a millimetre. Anything finer is below print resolution. + * + * Bounded at ten metres in either direction, which is four orders of magnitude + * past the largest book anyone binds and still leaves room for an element + * parked well off the spread. Without a bound a stored `1e9` is a legal + * millimetre, and the editor and the PDF renderer both try to draw a sheet a + * thousand kilometres wide. + */ +const mm = z.number().finite().min(-10000).max(10000).transform(v => Math.round(v * 100) / 100); const hex = z.string().regex(/^#[0-9a-fA-F]{6}$/, 'expected #rrggbb'); @@ -491,8 +499,21 @@ export const bookStatsElementSchema = z.object({ layout: z.enum(['grid', 'row', 'column']).default('grid'), showIcons: z.boolean().default(true), units: bookUnitsSchema.default('metric'), - /** Metric to value. Distance is metres, everything else a plain count. */ - values: z.record(z.string(), z.number().finite()).default({}), + /** + * Metric to value. Distance is metres, everything else a plain count. + * + * Anything that is not a metric is dropped rather than rejected: both readers + * only ever index by a `BOOK_METRICS` member, so a stray key is payload that + * gets persisted and re-served to every viewer without ever being drawn. + * Filtered instead of typed as `z.record(z.enum(BOOK_METRICS), …)`, because on + * zod 4 an enum key makes the record exhaustive — every metric would become + * required and every stats element written so far would fail, then be thrown + * away as unreadable. + */ + values: z.record(z.string(), z.number().finite()).default({}) + .transform(v => Object.fromEntries( + Object.entries(v).filter(([k]) => (BOOK_METRICS as readonly string[]).includes(k)), + )), }); export const bookCountriesElementSchema = z.object({ @@ -729,11 +750,19 @@ export type BookPageNumbers = z.infer; export const bookPageSetupSchema = z.object({ preset: z.enum(['square-210', 'square-300', 'a4-landscape', 'a4-portrait', 'a5-landscape', 'custom']).default('square-210'), - /** A single page. A double spread is drawn twice this wide. */ - pageWidth: mm.default(210), - pageHeight: mm.default(210), - bleed: mm.default(3), - safe: mm.default(5), + /** + * A single page. A double spread is drawn twice this wide. + * + * These four fall back rather than fail, like `version` below. The salvage + * pass only strips unreadable *elements*, so a page block the contract + * cannot read takes the whole book down to an empty one — and that empty + * book is what the server writes back. A nonsense dimension degrading to the + * preset is the smaller loss by a long way. + */ + pageWidth: mm.refine(v => v > 0, 'page width must be positive').catch(210).default(210), + pageHeight: mm.refine(v => v > 0, 'page height must be positive').catch(210).default(210), + bleed: mm.refine(v => v >= 0, 'bleed cannot be negative').catch(3).default(3), + safe: mm.refine(v => v >= 0, 'safe margin cannot be negative').catch(5).default(5), pageNumbers: bookPageNumbersSchema.default(() => bookPageNumbersSchema.parse({})), }); export type BookPageSetup = z.infer; diff --git a/shared/src/day/day.schema.spec.ts b/shared/src/day/day.schema.spec.ts index f5b9c9568..1aad1b307 100644 --- a/shared/src/day/day.schema.spec.ts +++ b/shared/src/day/day.schema.spec.ts @@ -35,6 +35,15 @@ describe('dayNoteCreateRequestSchema', () => { it('accepts null time/icon (moveDayNote re-sends the nullable entity fields)', () => { expect(dayNoteCreateRequestSchema.safeParse({ text: 'ok', time: null, icon: null }).success).toBe(true); }); + + it('takes an icon as a label and not as a payload', () => { + // Both shapes the field is written in: a lucide export name from NOTE_ICONS + // and a plain emoji, plus the longest ZWJ sequence anyone types. + expect(dayNoteCreateRequestSchema.safeParse({ text: 'ok', icon: 'ParkingSquare' }).success).toBe(true); + expect(dayNoteCreateRequestSchema.safeParse({ text: 'ok', icon: '👩‍👩‍👧‍👦' }).success).toBe(true); + expect(dayNoteCreateRequestSchema.safeParse({ text: 'ok', icon: 'x'.repeat(64) }).success).toBe(true); + expect(dayNoteCreateRequestSchema.safeParse({ text: 'ok', icon: 'x'.repeat(65) }).success).toBe(false); + }); }); describe('dayNoteUpdateRequestSchema', () => { @@ -42,6 +51,7 @@ describe('dayNoteUpdateRequestSchema', () => { expect(dayNoteUpdateRequestSchema.safeParse({}).success).toBe(true); expect(dayNoteUpdateRequestSchema.safeParse({ icon: '🍽️' }).success).toBe(true); expect(dayNoteUpdateRequestSchema.safeParse({ text: 'x'.repeat(501) }).success).toBe(false); + expect(dayNoteUpdateRequestSchema.safeParse({ icon: 'x'.repeat(65) }).success).toBe(false); }); it('accepts an explicit null time (clears the label)', () => { diff --git a/shared/src/day/day.schema.ts b/shared/src/day/day.schema.ts index 5ebdb6f51..1836fb0b8 100644 --- a/shared/src/day/day.schema.ts +++ b/shared/src/day/day.schema.ts @@ -9,8 +9,10 @@ import { z } from 'zod'; * Trip-scoped, both gated by the 'day_edit' permission. Served by the Nest * DaysController / DayNotesController (server/src/nest/days/). Day rows (with * their assignments) are wide and DB-derived, so list responses stay open. Day - * notes cap text at 500 and time at 150 chars (the legacy - * validateStringLengths middleware) — reproduced in the controller. + * note writes are capped field by field: text 500, time 2000, icon 64, color 9. + * Time is wider than the legacy validateStringLengths middleware's 150 because + * the raw-body route it replaced already accepted more, and narrowing it now + * would reject payloads today's client is allowed to send. */ /** @@ -86,7 +88,9 @@ export type DayTransportRequest = z.infer; export const dayNoteCreateRequestSchema = z.object({ text: z.string().min(1).max(500), time: z.string().max(2000).nullable().optional(), - icon: z.string().nullable().optional(), + // A lucide name from NOTE_ICONS or a single emoji; 64 leaves room for a ZWJ + // sequence with skin-tone modifiers and still keeps the column a label. + icon: z.string().max(64).nullable().optional(), /** One of NOTE_COLORS, or null for the neutral card (#1629). */ color: z.string().max(9).nullable().optional(), sort_order: z.number().optional(), @@ -96,7 +100,7 @@ export type DayNoteCreateRequest = z.infer; export const dayNoteUpdateRequestSchema = z.object({ text: z.string().max(500).optional(), time: z.string().max(2000).nullable().optional(), - icon: z.string().nullable().optional(), + icon: z.string().max(64).nullable().optional(), color: z.string().max(9).nullable().optional(), sort_order: z.number().optional(), }); diff --git a/shared/src/realtime/events.schema.ts b/shared/src/realtime/events.schema.ts index e89aa8371..84903d12b 100644 --- a/shared/src/realtime/events.schema.ts +++ b/shared/src/realtime/events.schema.ts @@ -352,7 +352,7 @@ export const TREK_WS_EVENTS = { }, } as const satisfies Record; -/** Every registered event name (65 trip-scoped + 29 user-scoped). */ +/** Every registered event name. The registry above is the count; this derives from it. */ export type TrekWsEventName = keyof typeof TREK_WS_EVENTS; /** Names broadcast to a trip room (`{ type, tripId, ...payload }` envelope). */