diff --git a/server/src/config.ts b/server/src/config.ts index ea963ab0e..03ffc4de7 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -37,16 +37,22 @@ if (_encryptionKey) { try { _encryptionKey = fs.readFileSync(encKeyFile, 'utf8').trim(); } catch (readErr: unknown) { - if ((readErr as NodeJS.ErrnoException)?.code !== 'ENOENT') { + const code = (readErr as NodeJS.ErrnoException)?.code; + if (code !== 'ENOENT') { // The file is there, we just cannot read it (permissions, I/O error). // Continuing would resolve a different key and overwrite this one below, // which permanently orphans every stored secret. Refuse to start instead. + // The errno goes in the line because this exit crash-loops the container, + // and the code is the only part that says which of these it is. console.error( - 'FATAL: could not read', - encKeyFile, - '-', + `FATAL: could not read ${encKeyFile} (${code ?? 'no errno'}) -`, readErr instanceof Error ? readErr.message : readErr, ); + if (code === 'EISDIR') { + // Compose creating a missing bind-mount source as a directory is by far + // the most common way this file stops being a file. + console.error('EISDIR: the path is a directory. A bind mount pointed at data/.encryption_key created it as one — remove it and mount the data directory instead.'); + } console.error('Fix the file permissions or set ENCRYPTION_KEY explicitly.'); process.exit(1); } diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index 3d382a65b..55383d398 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -4089,6 +4089,40 @@ function runMigrations(db: Database.Database): void { () => { db.exec("UPDATE trip_files SET filename = substr(filename, 7) WHERE filename LIKE 'files/%'"); }, + // Give back the notes the TICKETJSON step chopped up (#1658). + // + // That step matched with LIKE, which is case-insensitive in SQLite, so a + // note somebody had typed starting "ticketjson:" was read as a receipt: the + // first eleven characters were dropped, the rest was moved into ticket_json + // and the note was set to NULL. The step now matches with GLOB, but that + // only helps a fresh install — the array is index-addressed against + // schema_version, so a repaired step never runs again on a database that + // already applied it. + // + // A receipt comes out of JSON.stringify and always parses; typed text does + // not. So a row moves back only when its ticket_json fails to parse AND the + // note is still empty — anything that parses, and any row someone has + // written a note on since, is left exactly as it is. The eleven marker + // characters themselves are gone for good: their case was the only thing + // separating a receipt from a note, and writing "TICKETJSON:" back would + // hand the row straight to the reader that reads that prefix as a receipt. + // Appended LAST — the array is index-addressed against schema_version. + () => { + const rows = db + .prepare( + `SELECT id, ticket_json FROM budget_items + WHERE ticket_json IS NOT NULL AND ticket_json != '' AND COALESCE(note, '') = ''` + ) + .all() as Array<{ id: number; ticket_json: string }>; + const restore = db.prepare('UPDATE budget_items SET note = ?, ticket_json = NULL WHERE id = ?'); + for (const row of rows) { + try { + JSON.parse(row.ticket_json); + } catch { + restore.run(row.ticket_json, row.id); + } + } + }, ]; if (currentVersion < migrations.length) { diff --git a/server/src/middleware/globalMiddleware.ts b/server/src/middleware/globalMiddleware.ts index aaca45557..ec0a4b083 100644 --- a/server/src/middleware/globalMiddleware.ts +++ b/server/src/middleware/globalMiddleware.ts @@ -165,7 +165,15 @@ export function applyGlobalMiddleware( contentSecurityPolicy: { directives: { defaultSrc: ["'self'"], - scriptSrc: ["'self'", "'wasm-unsafe-eval'"], + // 'unsafe-eval' is load-bearing, not leftover: heic-to's libheif build + // initialises embind through new Function(), and that is what converts + // an iPhone .heic the moment somebody picks one. 'wasm-unsafe-eval' + // alone was tried first and was not enough (93b51a0b). The package + // ships a CSP-safe entry point at heic-to/csp; dropping this directive + // means switching client/src/utils/convertHeic.ts over to it and + // verifying a real .heic upload in a browser, not just deleting the + // string here. + scriptSrc: ["'self'", "'wasm-unsafe-eval'", "'unsafe-eval'"], styleSrc: ["'self'", "'unsafe-inline'", "https://fonts.googleapis.com", "https://unpkg.com"], imgSrc: ["'self'", "data:", "blob:", "https:"], connectSrc: [ diff --git a/server/src/nest/auth/auth.controller.ts b/server/src/nest/auth/auth.controller.ts index 4b57cfa92..b543de0a2 100644 --- a/server/src/nest/auth/auth.controller.ts +++ b/server/src/nest/auth/auth.controller.ts @@ -66,7 +66,8 @@ export const AVATAR_FILE_FILTER: Options['fileFilter'] = (_req, file, cb) => { * the demo-mode block), settings, key validation, MFA setup/enable/disable, MCP * tokens and the short-lived ws/resource tokens. The per-IP rate limits reuse * the shared buckets (the inline rateLimiter(5) shares the 'login' bucket, as in - * the legacy code). create-token answers 201; everything else 200. + * the legacy code); the two token-minting routes are keyed per account instead, + * see limitUser. create-token answers 201; everything else 200. */ @Controller('api/auth') @UseGuards(JwtAuthGuard) @@ -79,6 +80,21 @@ export class AuthController { } } + /** + * Same limiter, keyed on the account instead of the address. What these + * buckets bound is one account's share of the process-wide ephemeral token + * store, and an address is the wrong unit for that: an office, a school or a + * mobile carrier arrives as one IP, and the first user to reconnect a lot + * would take the ceiling away from everybody behind it. The pre-auth buckets + * (login, register, forgot-password) keep the IP key — there is no account + * to charge yet. + */ + private limitUser(bucket: string, userId: number, max: number): void { + if (!this.rl.check(bucket, String(userId), max, WINDOW, Date.now())) { + throw new HttpException({ error: 'Too many attempts. Please try again later.' }, 429); + } + } + @Get('me') @MfaExempt('the client needs to know who it is to render the setup screen') me(@CurrentUser() user: User) { @@ -307,13 +323,13 @@ export class AuthController { @Post('ws-token') @HttpCode(200) - wsToken(@CurrentUser() user: User, @Req() req: Request) { + wsToken(@CurrentUser() user: User) { // Own bucket, not 'login': a client that reconnects its socket in a loop // must not be able to lock itself out of signing in. The ceiling is far // above any real client, which mints one token per socket connect, but it // stops a single account from filling the process-wide ephemeral store and // 503-ing every other user's ws and download tokens. - this.limit('ws_token', req, 120); + this.limitUser('ws_token', user.id, 120); const result = this.tokens.createWsToken(user.id); if (result.error) { throw new HttpException({ error: result.error }, result.status!); @@ -323,8 +339,8 @@ export class AuthController { @Post('resource-token') @HttpCode(200) - resourceToken(@CurrentUser() user: User, @Body() body: ResourceTokenDto, @Req() req: Request) { - this.limit('resource_token', req, 120); + resourceToken(@CurrentUser() user: User, @Body() body: ResourceTokenDto) { + this.limitUser('resource_token', user.id, 120); const token = this.tokens.createResourceToken(user.id, body.purpose); if (!token) { throw new HttpException({ error: 'Service unavailable' }, 503); diff --git a/server/src/nest/collab/collab.service.ts b/server/src/nest/collab/collab.service.ts index 173b5cdab..3208a7aff 100644 --- a/server/src/nest/collab/collab.service.ts +++ b/server/src/nest/collab/collab.service.ts @@ -422,7 +422,11 @@ export class CollabService { createMessage(tripId: string | number, userId: number, text: string, replyTo?: number | null): { error?: string; message?: ReturnType } { if (replyTo) { - const replyMsg = this.db.get('SELECT id FROM collab_messages WHERE id = ? AND trip_id = ?', replyTo, tripId); + // A soft-deleted message is gone as far as anyone replying is concerned: + // its row survives only so the placeholder can be drawn where it was. + const replyMsg = this.db.get( + 'SELECT id FROM collab_messages WHERE id = ? AND trip_id = ? AND deleted = 0', replyTo, tripId, + ); if (!replyMsg) return { error: 'reply_not_found' }; } @@ -432,7 +436,8 @@ export class CollabService { const message = this.db.get(` SELECT m.*, u.username, u.avatar, - rm.text AS reply_text, ru.username AS reply_username + CASE WHEN rm.deleted = 1 THEN '' ELSE rm.text END AS reply_text, + ru.username AS reply_username FROM collab_messages m JOIN users u ON m.user_id = u.id LEFT JOIN collab_messages rm ON m.reply_to = rm.id diff --git a/server/src/nest/common/idempotency.interceptor.ts b/server/src/nest/common/idempotency.interceptor.ts index 77a9ec4ed..f630bc4cb 100644 --- a/server/src/nest/common/idempotency.interceptor.ts +++ b/server/src/nest/common/idempotency.interceptor.ts @@ -118,6 +118,19 @@ export class IdempotencyInterceptor implements NestInterceptor { ): Observable { const originalJson = res.json.bind(res); const database = this.database; + + let done!: () => void; + inFlight.set(signature, new Promise((resolve) => { done = resolve; })); + let released = false; + // Idempotent: whichever of the two paths below gets there first releases the + // waiter, and the other one is a no-op. + const release = () => { + if (released) return; + released = true; + inFlight.delete(signature); + done(); + }; + res.json = function (body: unknown): Response { if (res.statusCode >= 200 && res.statusCode < 300) { try { @@ -133,16 +146,24 @@ export class IdempotencyInterceptor implements NestInterceptor { // Non-fatal: if storage fails, the request still succeeds. } } + // Release here, not in finalize: this is the point the row exists, and a + // waiter woken any earlier looks the key up, misses, and runs the handler + // a second time - the duplicate write the key is meant to prevent. + // Release here, not in finalize: this is the point the row exists, and a + // waiter woken any earlier looks the key up, misses, and runs the handler + // a second time - the duplicate write the key is meant to prevent. + release(); return originalJson(body); }; - let done!: () => void; - inFlight.set(signature, new Promise((resolve) => { done = resolve; })); - return next.handle().pipe( finalize(() => { - inFlight.delete(signature); - done(); + // Backstop for a handler that never reaches res.json: it threw, or it + // answered through @Res() with send/end. Deferred by a full tick because + // finalize runs when the handler's observable completes and Nest writes + // the response several microtasks after that - firing straight away + // would beat the wrapper above to it on the ordinary path. + setImmediate(release); }), ); } diff --git a/server/src/nest/journey/journey-public.controller.ts b/server/src/nest/journey/journey-public.controller.ts index a52fe23af..7ac6293e4 100644 --- a/server/src/nest/journey/journey-public.controller.ts +++ b/server/src/nest/journey/journey-public.controller.ts @@ -55,7 +55,6 @@ export class JourneyPublicController { @Param('token') token: string, @Param('provider') provider: string, @Param('assetId') assetId: string, - @Param('ownerId') ownerId: string, @Param('kind') kind: string, @Res() res: Response, ): Promise { @@ -82,8 +81,9 @@ export class JourneyPublicController { return; } - // :ownerId stays in the path so old share links keep routing, but it is not - // read any more: the owner comes from the row the token resolved to. + // :ownerId stays in the route pattern so old share links keep routing, but + // it is not bound at all any more: the owner comes from the row the token + // resolved to. const effectiveOwnerId = valid.ownerId; const streaming = this.journey.streamProviderAsset( res, diff --git a/server/src/nest/journey/journey-share.service.ts b/server/src/nest/journey/journey-share.service.ts index 53ad8ccf0..e4a406717 100644 --- a/server/src/nest/journey/journey-share.service.ts +++ b/server/src/nest/journey/journey-share.service.ts @@ -73,6 +73,24 @@ export class JourneyShareService { return { token, created: true }; } + /** + * Read the link on behalf of a user. Owner only, the same check create and + * delete take: the token is the whole credential and it keeps working after a + * contributor is removed, so handing it out is handing out the journey. + * + * The refusal is its own answer rather than a null link, because those two + * mean opposite things to the caller — a published journey that reads as + * unpublished puts a "create link" button in front of somebody it will then + * refuse. Both the REST route and the MCP tool read the link through here. + */ + readJourneyShareLink( + journeyId: number, + userId: number, + ): { allowed: false } | { allowed: true; link: JourneyShareTokenInfo | null } { + if (!this.journey.isOwner(journeyId, userId)) return { allowed: false }; + return { allowed: true, link: this.getJourneyShareLink(journeyId) }; + } + getJourneyShareLink(journeyId: number): JourneyShareTokenInfo | null { const row = this.db.prepare('SELECT * FROM journey_share_tokens WHERE journey_id = ?').get(journeyId) as any; if (!row) return null; diff --git a/server/src/nest/journey/journey.controller.ts b/server/src/nest/journey/journey.controller.ts index a1a9f7c54..dce099593 100644 --- a/server/src/nest/journey/journey.controller.ts +++ b/server/src/nest/journey/journey.controller.ts @@ -599,7 +599,7 @@ export class JourneyController { baseVersion: body.baseVersion, }); if (result === null) { - throw new HttpException({ error: 'Journey not found' }, 404); + throw this.bookRefusal(Number(id), user.id); } if ('conflict' in result) { /* @@ -624,10 +624,24 @@ export class JourneyController { deleteBook(@CurrentUser() user: User, @Param('id') id: string) { const removed = this.books.deleteBook(Number(id), user.id); if (removed === null) { - throw new HttpException({ error: 'Journey not found' }, 404); + throw this.bookRefusal(Number(id), user.id); } } + /** + * Why a book write was refused, in a way the editor can act on. + * + * Both refusals come back as null, and answering 404 for both would tell a + * viewer who has just spent an hour laying out pages that their journey does + * not exist. Someone who cannot see the journey at all still gets the 404 — + * the status is not an existence oracle for them. + */ + private bookRefusal(journeyId: number, userId: number): HttpException { + return this.books.canOpen(journeyId, userId) + ? new HttpException({ error: 'Not allowed' }, 403) + : new HttpException({ error: 'Journey not found' }, 404); + } + @Post(':id/entries') createEntry(@CurrentUser() user: User, @Param('id') id: string, @Body() body: JourneyEntryCreateDto, @Headers('x-socket-id') socketId?: string) { if (!body.entry_date) { @@ -693,7 +707,15 @@ export class JourneyController { // ── Share Link ────────────────────────────────────────────────────────── @Get(':id/share-link') getShareLink(@CurrentUser() user: User, @Param('id') id: string) { - return { link: this.journey.getJourneyShareLink(Number(id), user.id) }; + // Reading the token is owner-only. It refuses like its siblings rather than + // answering with a null link: "not published" and "not yours to see" are + // different answers, and a client told the first one offers a create button + // that the POST below then refuses. + const result = this.journey.getJourneyShareLink(Number(id), user.id); + if (!result.allowed) { + throw new HttpException({ error: 'Not allowed' }, 403); + } + return { link: result.link }; } @Post(':id/share-link') diff --git a/server/src/nest/journey/journey.mcp.ts b/server/src/nest/journey/journey.mcp.ts index fcb7d3470..98dba95d4 100644 --- a/server/src/nest/journey/journey.mcp.ts +++ b/server/src/nest/journey/journey.mcp.ts @@ -415,10 +415,11 @@ export class JourneyMcp { access: { group: 'journey', mode: 'share' }, }) getJourneyShareLink({ journeyId }: { journeyId: number }, ctx: McpContext) { - // Owner only, same as the REST route: handing out the token is handing out - // the journey. - if (!this.journey.isOwner(journeyId, ctx.userId)) return notFound('Journey not found or access denied.'); - return ok({ shareLink: this.share.getJourneyShareLink(journeyId) }); + // Same read the REST route uses, so the owner check cannot drift apart + // between the two surfaces: handing out the token is handing out the journey. + const result = this.share.readJourneyShareLink(journeyId, ctx.userId); + if (!result.allowed) return notFound('Journey not found or access denied.'); + return ok({ shareLink: result.link }); } @Tool({ diff --git a/server/src/nest/journey/journey.service.ts b/server/src/nest/journey/journey.service.ts index ff13a8a8a..12d45e069 100644 --- a/server/src/nest/journey/journey.service.ts +++ b/server/src/nest/journey/journey.service.ts @@ -78,13 +78,10 @@ export class JourneyService { removeContributor(id: number, userId: number, targetUserId: number) { return this.journey.removeContributor(id, userId, targetUserId); } // Share links - // Authorization: the token is the whole credential, so reading it needs the - // same owner check create/delete use. A viewer who reads it keeps a working - // anonymous link after being removed as a contributor. - getJourneyShareLink(id: number, userId: number) { - if (!this.journey.isOwner(id, userId)) return null; - return this.share.getJourneyShareLink(id); - } + // Authorization lives in JourneyShareService.readJourneyShareLink, which the + // MCP tool reads through as well; { allowed: false } is a refusal, not "no + // link exists". + getJourneyShareLink(id: number, userId: number) { return this.share.readJourneyShareLink(id, userId); } createOrUpdateJourneyShareLink(id: number, userId: number, data: Parameters[2]) { return this.share.createOrUpdateJourneyShareLink(id, userId, data); } deleteJourneyShareLink(id: number, userId: number) { return this.share.deleteJourneyShareLink(id, userId); } diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 36f656020..f8447f5b0 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -9,7 +9,7 @@ import type { } from '@trek/shared'; import { readEnv, getAppUrl } from '../../app-config'; import { safeFetchFollow, SsrfBlockedError } from '../../utils/ssrfGuard'; -import { exceedsDeclaredLength, readCappedText } from '../../utils/cappedFetch'; +import { discardBody, exceedsDeclaredLength, readCappedText } from '../../utils/cappedFetch'; import { resolveApiKey, type ApiKeySource } from '../settings/instance-api-keys'; // ── Photo cache (disk-backed) ──────────────────────────────────────────────── import { PlacePhotoCacheService } from '../place-photos/place-photo-cache.service'; @@ -342,6 +342,20 @@ interface GooglePlaceDetails extends GooglePlaceResult { // unbounded body out of memory. const MAX_MAPS_PAGE_BYTES = 2_000_000; +const GOOGLE_SHORT_HOSTS = ['goo.gl', 'maps.app.goo.gl']; + +/** + * Google Maps lives on every country domain — google.de, maps.google.co.uk, + * google.com.au — so the host is matched by shape. A fixed list of .com hosts + * would quietly stop resolving the ccTLD links people actually paste. The TLD + * labels stay short (2-3 letters, optionally two of them) so that + * `google.evil.com` is not a Google host. + */ +function isGoogleMapsHost(hostname: string): boolean { + return GOOGLE_SHORT_HOSTS.includes(hostname) + || /^(www\.|maps\.)?google\.[a-z]{2,3}(\.[a-z]{2})?$/.test(hostname); +} + const WIKI_TIMEOUT_MS = 6000; // Tighter than the wiki calls, because this one sits at the FRONT of a chain: @@ -2038,9 +2052,8 @@ export class MapsService { // usually carries the !3d!4d data param we can then parse. Redirects are // followed manually so every hop is SSRF-re-checked. const parsed = new URL(url); - const GOOGLE_MAPS_HOSTS = ['goo.gl', 'maps.app.goo.gl', 'google.com', 'www.google.com', 'maps.google.com']; - const isShort = ['goo.gl', 'maps.app.goo.gl'].includes(parsed.hostname); - const isGoogleMaps = GOOGLE_MAPS_HOSTS.includes(parsed.hostname); + const isShort = GOOGLE_SHORT_HOSTS.includes(parsed.hostname); + const isGoogleMaps = isGoogleMapsHost(parsed.hostname); if (isShort || (isGoogleMaps && !extractCoords(url))) { resolvedUrl = (await followRedirects(url)).url || resolvedUrl; } @@ -2053,12 +2066,15 @@ export class MapsService { // short link that lands on maps.google.com still qualifies. let resolvedHost = ''; try { resolvedHost = new URL(resolvedUrl).hostname; } catch { /* keep the empty host, the branch is skipped */ } - if (!coords && GOOGLE_MAPS_HOSTS.includes(resolvedHost)) { + if (!coords && isGoogleMapsHost(resolvedHost)) { try { const pageRes = await followRedirects(resolvedUrl, { headers: { 'User-Agent': UA }, }); - if (!exceedsDeclaredLength(pageRes, MAX_MAPS_PAGE_BYTES)) { + if (exceedsDeclaredLength(pageRes, MAX_MAPS_PAGE_BYTES)) { + // Nothing here will read it, and an unread body keeps its socket. + discardBody(pageRes); + } else { // The map data sits near the top of the document, so a truncated read // still finds the coordinates; an oversized page degrades to the same // 400 an unparseable one already produced. diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index 67960dca3..70c9667d1 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -445,27 +445,39 @@ export class ReservationsService { */ referencesOutsideTrip(tripId: string | number, data: CreateReservationData | UpdateReservationData): string[] { const offenders: string[] = []; - const inTrip = (table: 'days' | 'places' | 'day_accommodations', id: unknown) => - !!this.db.get(`SELECT id FROM ${table} WHERE id = ? AND trip_id = ?`, id, tripId); + // An id that resolves to nothing is not an offender. accommodation_id in + // particular carries no foreign key, so shortening a trip's date range + // cascades the day_accommodations row away and leaves the reservation + // pointing at a gap; update() has always healed that by clearing the link, + // and refusing the whole write instead would make such a booking + // permanently uneditable. Only a row that exists in a DIFFERENT trip is one + // the caller must not have reached for. + const elsewhere = (table: 'days' | 'places' | 'day_accommodations', id: unknown) => { + const row = this.db.get<{ trip_id: number }>(`SELECT trip_id FROM ${table} WHERE id = ?`, id); + return !!row && String(row.trip_id) !== String(tripId); + }; - const check = (field: string, ok: boolean) => { if (!ok) offenders.push(field); }; + const check = (field: string, offending: boolean) => { if (offending) offenders.push(field); }; - if (data.day_id != null) check('day_id', inTrip('days', data.day_id)); - if (data.end_day_id != null) check('end_day_id', inTrip('days', data.end_day_id)); - if (data.place_id != null) check('place_id', inTrip('places', data.place_id)); - if (data.accommodation_id != null) check('accommodation_id', inTrip('day_accommodations', data.accommodation_id)); + if (data.day_id != null) check('day_id', elsewhere('days', data.day_id)); + if (data.end_day_id != null) check('end_day_id', elsewhere('days', data.end_day_id)); + if (data.place_id != null) check('place_id', elsewhere('places', data.place_id)); + if (data.accommodation_id != null) check('accommodation_id', elsewhere('day_accommodations', data.accommodation_id)); if (data.assignment_id != null) { - check('assignment_id', !!this.db.get( - 'SELECT da.id FROM day_assignments da JOIN days d ON da.day_id = d.id WHERE da.id = ? AND d.trip_id = ?', - data.assignment_id, tripId, - )); + // An assignment belongs to a trip through its day, so the join is the + // lookup — same shape as the MCP tool's getAssignmentForTrip. + const row = this.db.get<{ trip_id: number }>( + 'SELECT d.trip_id FROM day_assignments da JOIN days d ON da.day_id = d.id WHERE da.id = ?', + data.assignment_id, + ); + check('assignment_id', !!row && String(row.trip_id) !== String(tripId)); } const acc = data.create_accommodation; if (acc) { - if (acc.place_id != null) check('create_accommodation.place_id', inTrip('places', acc.place_id)); - if (acc.start_day_id != null) check('create_accommodation.start_day_id', inTrip('days', acc.start_day_id)); - if (acc.end_day_id != null) check('create_accommodation.end_day_id', inTrip('days', acc.end_day_id)); + if (acc.place_id != null) check('create_accommodation.place_id', elsewhere('places', acc.place_id)); + if (acc.start_day_id != null) check('create_accommodation.start_day_id', elsewhere('days', acc.start_day_id)); + if (acc.end_day_id != null) check('create_accommodation.end_day_id', elsewhere('days', acc.end_day_id)); } return offenders; diff --git a/server/src/nest/vacay/vacay.service.ts b/server/src/nest/vacay/vacay.service.ts index 9d6f8f26d..474ef7a7a 100644 --- a/server/src/nest/vacay/vacay.service.ts +++ b/server/src/nest/vacay/vacay.service.ts @@ -2,7 +2,7 @@ import { Injectable } from '@nestjs/common'; import { RealtimeService } from '../realtime/realtime.service'; import { DatabaseService } from '../database/database.service'; import { NotificationsService } from '../notifications/notifications.service'; -import { exceedsDeclaredLength } from '../../utils/cappedFetch'; +import { discardBody, readCappedJson } from '../../utils/cappedFetch'; // --------------------------------------------------------------------------- // Types @@ -465,8 +465,10 @@ export class VacayService { if (!holidays) { if (!COUNTRY_RE.test(country)) continue; const resp = await fetch(`https://date.nager.at/api/v3/PublicHolidays/${year}/${country}`, { signal: AbortSignal.timeout(FETCH_TIMEOUT_MS) }); - if (!resp.ok || exceedsDeclaredLength(resp, MAX_HOLIDAY_BYTES)) continue; - holidays = await resp.json() as Holiday[]; + if (!resp.ok) { discardBody(resp); continue; } + const parsed = await readCappedJson(resp, MAX_HOLIDAY_BYTES); + if (parsed === undefined) continue; + holidays = parsed; this.holidayCache.set(cacheKey, { data: holidays, time: Date.now() }); } const hasRegions = holidays.some((h: Holiday) => h.counties && h.counties.length > 0); @@ -1231,8 +1233,9 @@ export class VacayService { if (cached && Date.now() - cached.time < CACHE_TTL) return { data: cached.data }; try { const resp = await fetch('https://date.nager.at/api/v3/AvailableCountries', { signal: AbortSignal.timeout(FETCH_TIMEOUT_MS) }); - if (!resp.ok || exceedsDeclaredLength(resp, MAX_HOLIDAY_BYTES)) return { error: 'Failed to fetch countries' }; - const data = await resp.json(); + if (!resp.ok) { discardBody(resp); return { error: 'Failed to fetch countries' }; } + const data = await readCappedJson(resp, MAX_HOLIDAY_BYTES); + if (data === undefined) return { error: 'Failed to fetch countries' }; this.holidayCache.set(cacheKey, { data, time: Date.now() }); return { data }; } catch { @@ -1249,8 +1252,9 @@ export class VacayService { if (cached && Date.now() - cached.time < CACHE_TTL) return { data: cached.data }; try { const resp = await fetch(`https://date.nager.at/api/v3/PublicHolidays/${year}/${country}`, { signal: AbortSignal.timeout(FETCH_TIMEOUT_MS) }); - if (!resp.ok || exceedsDeclaredLength(resp, MAX_HOLIDAY_BYTES)) return { error: 'Failed to fetch holidays' }; - const data = await resp.json(); + if (!resp.ok) { discardBody(resp); return { error: 'Failed to fetch holidays' }; } + const data = await readCappedJson(resp, MAX_HOLIDAY_BYTES); + if (data === undefined) return { error: 'Failed to fetch holidays' }; this.holidayCache.set(cacheKey, { data, time: Date.now() }); return { data }; } catch { @@ -1269,14 +1273,15 @@ export class VacayService { fetch(`https://openholidaysapi.org/Groups?countryIsoCode=${country}&languageIsoCode=${normalizedLanguage}`, { headers: { accept: 'text/json' }, signal: AbortSignal.timeout(FETCH_TIMEOUT_MS) }), fetch(`https://openholidaysapi.org/Subdivisions?countryIsoCode=${country}&languageIsoCode=${normalizedLanguage}`, { headers: { accept: 'text/json' }, signal: AbortSignal.timeout(FETCH_TIMEOUT_MS) }), ]); - if (!groupsResp.ok || !subdivisionsResp.ok) return { error: 'Failed to fetch school holiday regions' }; - if (exceedsDeclaredLength(groupsResp, MAX_HOLIDAY_BYTES) || exceedsDeclaredLength(subdivisionsResp, MAX_HOLIDAY_BYTES)) { + if (!groupsResp.ok || !subdivisionsResp.ok) { + discardBody(groupsResp); + discardBody(subdivisionsResp); return { error: 'Failed to fetch school holiday regions' }; } - const data = { - groups: await groupsResp.json(), - subdivisions: await subdivisionsResp.json(), - }; + const groups = await readCappedJson(groupsResp, MAX_HOLIDAY_BYTES); + const subdivisions = await readCappedJson(subdivisionsResp, MAX_HOLIDAY_BYTES); + if (groups === undefined || subdivisions === undefined) return { error: 'Failed to fetch school holiday regions' }; + const data = { groups, subdivisions }; this.holidayCache.set(cacheKey, { data, time: Date.now() }); return { data }; } catch { @@ -1305,8 +1310,9 @@ export class VacayService { headers: { accept: 'text/json' }, signal: AbortSignal.timeout(FETCH_TIMEOUT_MS), }); - if (!resp.ok || exceedsDeclaredLength(resp, MAX_HOLIDAY_BYTES)) return { error: 'Failed to fetch school holidays' }; - const data = await resp.json(); + if (!resp.ok) { discardBody(resp); return { error: 'Failed to fetch school holidays' }; } + const data = await readCappedJson(resp, MAX_HOLIDAY_BYTES); + if (data === undefined) return { error: 'Failed to fetch school holidays' }; this.holidayCache.set(cacheKey, { data, time: Date.now() }); return { data }; } catch { diff --git a/server/src/nest/weather/weather.impl.ts b/server/src/nest/weather/weather.impl.ts index c9a98f57d..e9d4624f7 100644 --- a/server/src/nest/weather/weather.impl.ts +++ b/server/src/nest/weather/weather.impl.ts @@ -1,4 +1,4 @@ -import { exceedsDeclaredLength } from '../../utils/cappedFetch'; +import { readCappedJson } from '../../utils/cappedFetch'; // Open-Meteo sits on the request path as a third party: without a deadline a hung // connection holds the caller until the socket gives up on its own. Every other @@ -24,13 +24,21 @@ function hourIndexFromTime(time?: string): number | null { // misbehaving, not that the trip got longer. const MAX_WEATHER_BYTES = 1024 * 1024; -/** Open-Meteo GET with the shared deadline and the declared-size cap. */ -async function fetchOpenMeteo(url: string): Promise { +/** + * Open-Meteo GET with the shared deadline and the size cap. + * + * The body is read through the cap rather than buffered with .json(): Open-Meteo + * answers chunked, so there is no content-length to check and a declared-length + * test alone would let any amount of data through. The caller still gets the + * response, because the status and `reason` are what shape its error. + */ +async function fetchOpenMeteo(url: string): Promise<{ response: Response; data: OpenMeteoForecast }> { const response = await fetch(url, { signal: AbortSignal.timeout(WEATHER_TIMEOUT_MS) }); - if (exceedsDeclaredLength(response, MAX_WEATHER_BYTES)) { + const data = await readCappedJson(response, MAX_WEATHER_BYTES); + if (!data) { throw new ApiError(502, 'Open-Meteo API error'); } - return response; + return { response, data }; } /** @@ -241,8 +249,7 @@ async function _getWeatherImpl( // Forecast range (-1 .. +16 days) if (diffDays >= -1 && diffDays <= 16) { const url = `https://api.open-meteo.com/v1/forecast?latitude=${lat}&longitude=${lng}&daily=temperature_2m_max,temperature_2m_min,weathercode&timezone=auto&forecast_days=16`; - const response = await fetchOpenMeteo(url); - const data = await response.json() as OpenMeteoForecast; + const { response, data } = await fetchOpenMeteo(url); if (!response.ok || data.error) { throw new ApiError(response.status || 500, data.reason || 'Open-Meteo API error'); @@ -279,8 +286,7 @@ async function _getWeatherImpl( ? '&hourly=temperature_2m,weathercode' : ''; const url = `https://archive-api.open-meteo.com/v1/archive?latitude=${lat}&longitude=${lng}&start_date=${dateStr}&end_date=${dateStr}&daily=temperature_2m_max,temperature_2m_min,weathercode,precipitation_sum${hourParam}&timezone=auto`; - const response = await fetchOpenMeteo(url); - const data = await response.json() as OpenMeteoForecast; + const { response, data } = await fetchOpenMeteo(url); if (!response.ok || data.error) { throw new ApiError(response.status || 500, data.reason || 'Open-Meteo Archive API error'); @@ -336,8 +342,7 @@ async function _getWeatherImpl( const endStr = endDate.toISOString().slice(0, 10); const url = `https://archive-api.open-meteo.com/v1/archive?latitude=${lat}&longitude=${lng}&start_date=${startStr}&end_date=${endStr}&daily=temperature_2m_max,temperature_2m_min,precipitation_sum&timezone=auto`; - const response = await fetchOpenMeteo(url); - const data = await response.json() as OpenMeteoForecast; + const { response, data } = await fetchOpenMeteo(url); if (!response.ok || data.error) { throw new ApiError(response.status || 500, data.reason || 'Open-Meteo Climate API error'); @@ -389,8 +394,7 @@ async function _getWeatherImpl( if (cached) return cached; const url = `https://api.open-meteo.com/v1/forecast?latitude=${lat}&longitude=${lng}¤t=temperature_2m,weathercode&timezone=auto`; - const response = await fetchOpenMeteo(url); - const data = await response.json() as OpenMeteoForecast; + const { response, data } = await fetchOpenMeteo(url); if (!response.ok || data.error) { throw new ApiError(response.status || 500, data.reason || 'Open-Meteo API error'); @@ -464,8 +468,7 @@ async function _getDetailedWeatherImpl( + `&hourly=temperature_2m,precipitation,weathercode,windspeed_10m,relativehumidity_2m` + `&daily=temperature_2m_max,temperature_2m_min,weathercode,precipitation_sum,windspeed_10m_max,sunrise,sunset` + `&timezone=auto`; - const response = await fetchOpenMeteo(url); - const data = await response.json() as OpenMeteoForecast; + const { response, data } = await fetchOpenMeteo(url); if (!response.ok || data.error) { throw new ApiError(response.status || 500, data.reason || 'Open-Meteo Climate API error'); @@ -527,8 +530,7 @@ async function _getDetailedWeatherImpl( + `&daily=temperature_2m_max,temperature_2m_min,weathercode,sunrise,sunset,precipitation_probability_max,precipitation_sum,windspeed_10m_max` + `&timezone=auto&start_date=${dateStr}&end_date=${dateStr}`; - const response = await fetchOpenMeteo(url); - const data = await response.json() as OpenMeteoForecast; + const { response, data } = await fetchOpenMeteo(url); if (!response.ok || data.error) { throw new ApiError(response.status || 500, data.reason || 'Open-Meteo API error'); diff --git a/server/src/utils/cappedFetch.ts b/server/src/utils/cappedFetch.ts index 89ff4653c..927acc08b 100644 --- a/server/src/utils/cappedFetch.ts +++ b/server/src/utils/cappedFetch.ts @@ -16,9 +16,10 @@ interface CappedReader { } interface CappedResponse { headers?: { get(name: string): string | null } | null; - body?: { getReader(): CappedReader } | null; + body?: { getReader(): CappedReader; cancel?(): Promise } | null; arrayBuffer?(): Promise; text?(): Promise; + json?(): Promise; } /** True when the provider declares a body larger than the budget. */ @@ -26,6 +27,41 @@ export function exceedsDeclaredLength(res: CappedResponse, maxBytes: number): bo return Number(res.headers?.get('content-length') ?? 0) > maxBytes; } +/** + * Let go of a body nobody is going to read. undici keeps the connection + * reserved until the stream ends, so a bail-out that just returns leaves the + * socket pinned until the garbage collector gets around to it — which is the + * one thing a size cap is supposed to prevent. + */ +export function discardBody(res: CappedResponse): void { + void res.body?.cancel?.().catch(() => {}); +} + +/** + * readCappedText plus the parse, for the providers that only ever answer with a + * JSON document. Answers `undefined` when the body is over the cap, was cut off + * at it, or does not parse: every caller already has a "the provider + * misbehaved" branch, and none of them can do anything with half a document. + */ +export async function readCappedJson(res: CappedResponse, maxBytes: number): Promise { + if (exceedsDeclaredLength(res, maxBytes)) { + discardBody(res); + return undefined; + } + // Neither a stream nor text() means there is nothing to read incrementally + // and nothing to cap — a null-body status, or a partial stub in a test. + if (!res.body?.getReader && !res.text && !res.arrayBuffer && res.json) { + return await res.json() as T; + } + const { text, truncated } = await readCappedText(res, maxBytes); + if (truncated) return undefined; + try { + return JSON.parse(text) as T; + } catch { + return undefined; + } +} + /** * Read a body, stopping once maxBytes have arrived. Callers that must reject * an oversized payload look at `truncated`; callers that only scan the head of diff --git a/server/tests/integration/journey.test.ts b/server/tests/integration/journey.test.ts index 0f0f1f7f1..57790c6a9 100644 --- a/server/tests/integration/journey.test.ts +++ b/server/tests/integration/journey.test.ts @@ -428,11 +428,32 @@ describe('Journey share link', () => { .send({ share_timeline: true, share_gallery: true, share_map: true }); // The token is the whole credential and it outlives the contributor's - // access, so reading it takes the same owner check create and delete take. + // access, so reading it takes the same owner check create and delete take — + // and it refuses the same way, because a 200 with a null link would tell the + // editor this published journey is unpublished. const res = await request(app) .get(`/api/journeys/${journey.id}/share-link`) .set('Cookie', authCookie(helper.id)); + expect(res.status).toBe(403); + expect(res.body).toEqual({ error: 'Not allowed' }); + + // The owner still reads the link out of the same route. + const owned = await request(app) + .get(`/api/journeys/${journey.id}/share-link`) + .set('Cookie', authCookie(owner.id)); + expect(owned.status).toBe(200); + expect(owned.body.link.token).toBeTruthy(); + }); + + it('JOURNEY-INT-044b — an owner with nothing published still gets 200 and a null link', async () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + + const res = await request(app) + .get(`/api/journeys/${journey.id}/share-link`) + .set('Cookie', authCookie(user.id)); + expect(res.status).toBe(200); expect(res.body.link).toBeNull(); }); diff --git a/server/tests/integration/mcp.test.ts b/server/tests/integration/mcp.test.ts index 50e5e943e..f6b48ae68 100644 --- a/server/tests/integration/mcp.test.ts +++ b/server/tests/integration/mcp.test.ts @@ -256,14 +256,35 @@ describe('MCP session management', () => { const sessionsForUser = () => [...sessions.values()].filter((s) => s.userId === user.id).length; - for (let i = 0; i < 20; i++) await createSession(user.id); - expect(sessionsForUser()).toBe(20); + // What the race leaves behind: concurrent initializes all read the same + // pre-registration count, all pass the cap check, and all register — so the + // map ends up over the cap. Driving that through supertest depends on how + // the requests interleave, so the state itself is set up instead: four past + // the cap, none of them the newcomer's doing. Each entry is only ever + // counted and evicted, so a stub with closable halves is enough. + // Inside the TTL, or countSessionsForUser skips them and there is nothing + // over the cap to trim. + const stale = Date.now() - 10_000; + for (let i = 0; i < 24; i++) { + sessions.set(`overshoot-${i}`, { + server: { close() {} }, + transport: { close() {} }, + userId: user.id, + scopes: null, + clientId: null, + isStaticToken: false, + lastActivity: stale + i, + lastClientIp: null, + } as unknown as NonNullable>); + } + expect(sessionsForUser()).toBe(24); + + // The pre-check gives back one; without the trim at registration the other + // four stay in the map — each holding a full McpServer — until the TTL sweep. + const newSessionId = await createSession(user.id); - // All five read the same pre-registration count, so all five pass the cap - // check. Registration is where the overshoot has to be given back — nothing - // else drops these entries before the TTL sweep, and each holds a server. - await Promise.all([1, 2, 3, 4, 5].map(() => createSession(user.id))); expect(sessionsForUser()).toBe(20); + expect(sessions.has(newSessionId)).toBe(true); }); it('MCP — session resumption with valid mcp-session-id', async () => { diff --git a/server/tests/integration/memories-unified.test.ts b/server/tests/integration/memories-unified.test.ts index 9865512b4..3a06a4d9e 100644 --- a/server/tests/integration/memories-unified.test.ts +++ b/server/tests/integration/memories-unified.test.ts @@ -248,6 +248,42 @@ describe('Unified photo management', () => { expect(row).toBeUndefined(); }); + it('UNIFIED-009a — DELETE photos is held to the body contract, and still takes a numeric string', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + addTripPhoto(testDb, trip.id, user.id, 'asset-contract', 'immich'); + const trekRef = testDb.prepare(` + SELECT tp.photo_id FROM trip_photos tp + JOIN trek_photos tkp ON tkp.id = tp.photo_id + WHERE tp.trip_id = ? AND tkp.asset_id = ? + `).get(trip.id, 'asset-contract') as any; + + // A DELETE that reads a body validates it like any other write, so a + // photo_id that is neither a number nor a string never reaches the handler. + const bad = await request(app) + .delete(photosUrl(trip.id)) + .set('Cookie', authCookie(user.id)) + .send({ photo_id: { id: trekRef.photo_id } }); + + expect(bad.status).toBe(400); + expect(bad.body.error).toContain('photo_id'); + + // The contract is deliberately loose about the type, because the client has + // always been free to send the id as a string. + const ok = await request(app) + .delete(photosUrl(trip.id)) + .set('Cookie', authCookie(user.id)) + .send({ photo_id: String(trekRef.photo_id) }); + + expect(ok.status).toBe(200); + const row = testDb.prepare(` + SELECT tp.* FROM trip_photos tp + JOIN trek_photos tkp ON tkp.id = tp.photo_id + WHERE tkp.asset_id = ? + `).get('asset-contract'); + expect(row).toBeUndefined(); + }); + it('UNIFIED-010 — DELETE photos on non-member trip returns 404', async () => { const { user: owner } = createUser(testDb); const { user: stranger } = createUser(testDb); diff --git a/server/tests/unit/config.test.ts b/server/tests/unit/config.test.ts index a38567387..1c80db8e6 100644 --- a/server/tests/unit/config.test.ts +++ b/server/tests/unit/config.test.ts @@ -56,6 +56,26 @@ describe('config — encryption key resolution', () => { expect(write).not.toHaveBeenCalledWith(ENC_KEY_FILE, expect.anything(), expect.anything()); }); + it('CFGKEY-001b: names the errno, and the bind-mount case behind EISDIR', async () => { + vi.spyOn(process, 'exit').mockImplementation((() => { + throw new Error('process.exit called'); + }) as never); + vi.spyOn(fs, 'writeFileSync').mockImplementation(() => undefined); + const error = vi.spyOn(console, 'error').mockImplementation(() => undefined); + stubKeyFileRead(() => { + const err = new Error('EISDIR: illegal operation on a directory, read') as NodeJS.ErrnoException; + err.code = 'EISDIR'; + throw err; + }); + + await expect(import('../../src/config')).rejects.toThrow('process.exit called'); + // Without the code the operator sees a message and no way to tell a + // permission problem from a directory in the file's place. + const lines = error.mock.calls.map((c) => c.join(' ')); + expect(lines.some((l) => l.startsWith('FATAL:') && l.includes('EISDIR'))).toBe(true); + expect(lines.some((l) => l.includes('bind mount'))).toBe(true); + }); + it('CFGKEY-002: refuses to regenerate over an existing but empty key file', async () => { const exit = vi.spyOn(process, 'exit').mockImplementation((() => { throw new Error('process.exit called'); diff --git a/server/tests/unit/db/budget-ticket-json-migration.test.ts b/server/tests/unit/db/budget-ticket-json-migration.test.ts index 3b33821e0..4d5c091db 100644 --- a/server/tests/unit/db/budget-ticket-json-migration.test.ts +++ b/server/tests/unit/db/budget-ticket-json-migration.test.ts @@ -5,6 +5,10 @@ * migration lifts it into its own column and clears the note. The match has to * be case-SENSITIVE: LIKE is not, which would swallow a hand-written note that * happens to start with the same word. + * + * The second half covers the databases that ran the LIKE version before it was + * repaired — a fixed step never replays, so the damage is undone by a migration + * of its own at the end of the array. */ import { describe, it, expect } from 'vitest'; import Database from 'better-sqlite3'; @@ -55,3 +59,66 @@ describe('budget_items ticket_json migration', () => { } }); }); + +describe('recovering what the case-insensitive match destroyed', () => { + /** + * A database that already ran the buggy step: migrated to the tip, then wound + * back one version so only the recovery step is left to run — which is exactly + * what an existing install does on the next boot. + */ + function makeDamagedDb(): Database.Database { + const db = new Database(':memory:'); + db.exec('PRAGMA journal_mode = WAL'); + db.exec('PRAGMA busy_timeout = 5000'); + db.exec('PRAGMA foreign_keys = ON'); + createTables(db); + runMigrations(db); + db.prepare("INSERT INTO users (id, username, email, password_hash) VALUES (1, 'u', 'u@example.test', 'x')").run(); + db.prepare("INSERT INTO trips (id, user_id, title) VALUES (1, 1, 'T')").run(); + // Row 1: what the LIKE match left behind for "ticketjson: buy at the door". + // Row 2: an actual receipt. Row 3: a receipt on a row whose owner has since + // written a note. + db.prepare('INSERT INTO budget_items (id, trip_id, name, note, ticket_json) VALUES (?, ?, ?, ?, ?)') + .run(1, 1, 'Museum', null, ' buy at the door'); + db.prepare('INSERT INTO budget_items (id, trip_id, name, note, ticket_json) VALUES (?, ?, ?, ?, ?)') + .run(2, 1, 'Dinner', null, '{"items":[{"name":"Beer","price":"4.50","parts":[1]}]}'); + db.prepare('INSERT INTO budget_items (id, trip_id, name, note, ticket_json) VALUES (?, ?, ?, ?, ?)') + .run(3, 1, 'Taxi', 'split at the hotel', '{"items":[]}'); + + const version = (db.prepare('SELECT version FROM schema_version LIMIT 1').get() as { version: number }).version; + db.prepare('UPDATE schema_version SET version = ?').run(version - 1); + return db; + } + + const read = (db: Database.Database, id: number) => + db.prepare('SELECT note, ticket_json FROM budget_items WHERE id = ?').get(id) as { + note: string | null; + ticket_json: string | null; + }; + + it('puts a chopped note back where it came from', () => { + const db = makeDamagedDb(); + try { + runMigrations(db); + expect(read(db, 1)).toEqual({ note: ' buy at the door', ticket_json: null }); + } finally { + db.close(); + } + }); + + it('leaves a genuine receipt alone', () => { + const db = makeDamagedDb(); + try { + runMigrations(db); + expect(read(db, 2)).toEqual({ + note: null, + ticket_json: '{"items":[{"name":"Beer","price":"4.50","parts":[1]}]}', + }); + // A note written since the damage means the row was not chopped, so it is + // not a candidate at all. + expect(read(db, 3)).toEqual({ note: 'split at the hotel', ticket_json: '{"items":[]}' }); + } finally { + db.close(); + } + }); +}); diff --git a/server/tests/unit/demo/demo-reset-path.test.ts b/server/tests/unit/demo/demo-reset-path.test.ts index 4c25fa501..2ac204b02 100644 --- a/server/tests/unit/demo/demo-reset-path.test.ts +++ b/server/tests/unit/demo/demo-reset-path.test.ts @@ -1,40 +1,86 @@ /** - * Structural guard for demo-reset's DB path. + * demo-reset's DB path. * * demo-reset closes the live connection, copies the baseline over the database * file and reopens. It used to compute that file itself as data/travel.db, which * is wrong the moment TREK_DB_FILE moves the database elsewhere: the baseline * lands on an unrelated file and the real database is never reset. The path has - * to come from the connection that database.ts actually opened. + * to come from the connection that database.ts actually opened, so what these + * cases assert is which file each copy names. * - * It is pinned on the source rather than executed because demo-reset resolves - * ../db/database through a runtime require() (the module is a boot-time - * singleton), which vitest's module runner cannot resolve. + * demo-reset resolves ../db/database through a runtime require() — the module is + * a boot-time singleton — and vitest's mock registry never sees that call: it + * goes to Node's own resolver, which cannot load a .ts file and throws + * "Cannot find module '../db/database'". Hence the loader below, which is the + * seam that lets the module run at all. The file system is spied rather than + * written to, because the baseline path is fixed at data/travel-baseline.db and + * a developer running a demo instance has a real one sitting there. */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach, afterAll } from 'vitest'; import fs from 'node:fs'; import path from 'node:path'; +// eslint-disable-next-line @typescript-eslint/no-require-imports +const Module = require('node:module'); -const source = fs.readFileSync(path.join(__dirname, '..', '..', '..', 'src', 'demo', 'demo-reset.ts'), 'utf8'); +const BASELINE = path.resolve(__dirname, '..', '..', '..', 'data', 'travel-baseline.db'); +const LIVE_DB = path.join(path.sep, 'srv', 'trek', 'custom-name.db'); + +const dbStub = { + name: LIVE_DB, + exec: vi.fn(), + prepare: vi.fn(() => ({ get: () => undefined, all: () => [], run: () => undefined })), +}; +const databaseModule = { db: dbStub, closeDb: vi.fn(), reinitialize: vi.fn() }; + +// Teach Node's require how to answer the one runtime require demo-reset makes. +const previousLoader = Module._extensions['.ts']; +Module._extensions['.ts'] = (mod: { exports: unknown }, filename: string) => { + if (filename.endsWith(path.join('src', 'db', 'database.ts'))) { + mod.exports = databaseModule; + return; + } + throw new Error(`unexpected runtime require of ${filename}`); +}; +afterAll(() => { + if (previousLoader) Module._extensions['.ts'] = previousLoader; + else delete Module._extensions['.ts']; +}); + +import { resetDemoUser, saveBaseline } from '../../../src/demo/demo-reset'; describe('demo-reset DB path', () => { - it('DEMORESET-001: never hardcodes the default travel.db path', () => { - expect(source).not.toContain("'travel.db'"); + let copyFileSync: ReturnType; + + beforeEach(() => { + dbStub.name = LIVE_DB; + vi.spyOn(console, 'log').mockImplementation(() => undefined); + copyFileSync = vi.spyOn(fs, 'copyFileSync').mockImplementation(() => undefined); + vi.spyOn(fs, 'unlinkSync').mockImplementation(() => undefined); + vi.spyOn(fs, 'existsSync').mockImplementation(((p: fs.PathLike) => String(p) === BASELINE) as typeof fs.existsSync); }); - it('DEMORESET-002: takes the path from the open connection', () => { - expect(source).toContain('db.name'); - // Both entry points, not just one of them. - const uses = source.match(/liveDbPath\(db\)/g) ?? []; - expect(uses.length).toBe(2); + afterEach(() => { + vi.restoreAllMocks(); }); - it('DEMORESET-003: keeps the baseline next to the data directory', () => { - expect(source).toContain("path.join(dataDir, 'travel-baseline.db')"); + it('DEMORESET-001: saves the baseline from the file the connection has open', () => { + saveBaseline(); + expect(copyFileSync).toHaveBeenCalledWith(LIVE_DB, BASELINE); }); - it('DEMORESET-004: bails out on an in-memory database instead of copying files', () => { - const guards = source.match(/=== ':memory:'/g) ?? []; - expect(guards.length).toBe(2); + it('DEMORESET-002: restores the baseline onto that same file, not data/travel.db', () => { + resetDemoUser(); + expect(copyFileSync).toHaveBeenCalledWith(BASELINE, LIVE_DB); + // Closed before the copy and reopened after it — copying over an open + // SQLite file is how a database ends up half of each. + expect(databaseModule.closeDb).toHaveBeenCalled(); + expect(databaseModule.reinitialize).toHaveBeenCalled(); + }); + + it('DEMORESET-003: an in-memory database is left alone rather than copied around', () => { + dbStub.name = ':memory:'; + saveBaseline(); + resetDemoUser(); + expect(copyFileSync).not.toHaveBeenCalled(); }); }); diff --git a/server/tests/unit/middleware/globalMiddleware.csp.test.ts b/server/tests/unit/middleware/globalMiddleware.csp.test.ts index 0baaead11..09bc6a91e 100644 --- a/server/tests/unit/middleware/globalMiddleware.csp.test.ts +++ b/server/tests/unit/middleware/globalMiddleware.csp.test.ts @@ -38,8 +38,12 @@ describe('global CSP: script-src', () => { expect(await directiveSources('script-src')).toContain("'wasm-unsafe-eval'"); }); - it("does not allow 'unsafe-eval' — the policy the wiki documents, and nothing first-party evals", async () => { - expect(await directiveSources('script-src')).not.toContain("'unsafe-eval'"); + it("still allows 'unsafe-eval', which heic-to needs to decode an iPhone photo", async () => { + // Pinned so the next tidy-up of this list finds the reason before the + // consequence: libheif initialises embind with new Function(), and without + // this every .heic upload fails in the browser. See the comment on the + // directive for how to actually get rid of it. + expect(await directiveSources('script-src')).toContain("'unsafe-eval'"); }); }); diff --git a/server/tests/unit/nest/auth.controller.test.ts b/server/tests/unit/nest/auth.controller.test.ts index 0078e4196..a86b32d6f 100644 --- a/server/tests/unit/nest/auth.controller.test.ts +++ b/server/tests/unit/nest/auth.controller.test.ts @@ -249,25 +249,41 @@ describe('AuthController (authenticated)', () => { }); it('resource-token 503 when unavailable, else returns the token payload', () => { - expect(thrown(() => ac(asvc({}), rl(), { createResourceToken: vi.fn().mockReturnValue(null) }).resourceToken(user, {}, req))).toEqual({ status: 503, body: { error: 'Service unavailable' } }); - expect(ac(asvc({}), rl(), { createResourceToken: vi.fn().mockReturnValue({ token: 'rt' }) }).resourceToken(user, { purpose: 'download' }, req)).toEqual({ token: 'rt' }); + expect(thrown(() => ac(asvc({}), rl(), { createResourceToken: vi.fn().mockReturnValue(null) }).resourceToken(user, {}))).toEqual({ status: 503, body: { error: 'Service unavailable' } }); + expect(ac(asvc({}), rl(), { createResourceToken: vi.fn().mockReturnValue({ token: 'rt' }) }).resourceToken(user, { purpose: 'download' })).toEqual({ token: 'rt' }); }); it('ws/resource tokens throttle on their own buckets, so a mint loop cannot drain the store or block login', () => { const s = rl(); const now = Date.now(); - for (let i = 0; i < 120; i++) s.check('ws_token', '9.9.9.9', 120, 15 * 60 * 1000, now); - expect(thrown(() => ac(asvc({}), s, { createWsToken: vi.fn().mockReturnValue({ token: 'ws' }) }).wsToken(user, req))) + for (let i = 0; i < 120; i++) s.check('ws_token', String(user.id), 120, 15 * 60 * 1000, now); + expect(thrown(() => ac(asvc({}), s, { createWsToken: vi.fn().mockReturnValue({ token: 'ws' }) }).wsToken(user))) .toEqual({ status: 429, body: { error: 'Too many attempts. Please try again later.' } }); // The login bucket is untouched: an exhausted socket loop must not lock the account out. expect(s.check('login', '9.9.9.9', 5, 15 * 60 * 1000, now)).toBe(true); const s2 = rl(); - for (let i = 0; i < 120; i++) s2.check('resource_token', '9.9.9.9', 120, 15 * 60 * 1000, now); - expect(thrown(() => ac(asvc({}), s2, { createResourceToken: vi.fn().mockReturnValue({ token: 'rt' }) }).resourceToken(user, {}, req))) + for (let i = 0; i < 120; i++) s2.check('resource_token', String(user.id), 120, 15 * 60 * 1000, now); + expect(thrown(() => ac(asvc({}), s2, { createResourceToken: vi.fn().mockReturnValue({ token: 'rt' }) }).resourceToken(user, {}))) .toEqual({ status: 429, body: { error: 'Too many attempts. Please try again later.' } }); }); + it('the ws/resource ceilings are per account, not per address: one heavy user cannot 429 the office', () => { + const s = rl(); + const now = Date.now(); + const other = { ...user, id: 2 } as User; + // Both users sit behind the same NAT address, and the first one burns its + // whole ceiling. + for (let i = 0; i < 120; i++) { + ac(asvc({}), s, { createWsToken: vi.fn().mockReturnValue({ token: 'ws' }) }).wsToken(user); + } + expect(thrown(() => ac(asvc({}), s, { createWsToken: vi.fn().mockReturnValue({ token: 'ws' }) }).wsToken(user))) + .toEqual({ status: 429, body: { error: 'Too many attempts. Please try again later.' } }); + + expect(ac(asvc({}), s, { createWsToken: vi.fn().mockReturnValue({ token: 'ws2' }) }).wsToken(other)).toEqual({ token: 'ws2' }); + expect(ac(asvc({}), s, { createResourceToken: vi.fn().mockReturnValue({ token: 'rt2' }) }).resourceToken(other, {})).toEqual({ token: 'rt2' }); + }); + it('rate-limited account ops throw 429 once the bucket is exhausted', () => { const s = rl(); const now = Date.now(); @@ -400,8 +416,8 @@ describe('AuthController (authenticated)', () => { }); it('ws-token maps error, else returns the token', () => { - expect(thrown(() => ac(asvc({}), rl(), { createWsToken: vi.fn().mockReturnValue({ error: 'down', status: 503 }) }).wsToken(user, req))).toEqual({ status: 503, body: { error: 'down' } }); - expect(ac(asvc({}), rl(), { createWsToken: vi.fn().mockReturnValue({ token: 'ws' }) }).wsToken(user, req)).toEqual({ token: 'ws' }); + expect(thrown(() => ac(asvc({}), rl(), { createWsToken: vi.fn().mockReturnValue({ error: 'down', status: 503 }) }).wsToken(user))).toEqual({ status: 503, body: { error: 'down' } }); + expect(ac(asvc({}), rl(), { createWsToken: vi.fn().mockReturnValue({ token: 'ws' }) }).wsToken(user)).toEqual({ token: 'ws' }); }); it('avatar saves when not in demo mode (env present but email is not a demo email)', async () => { diff --git a/server/tests/unit/nest/collab.service.test.ts b/server/tests/unit/nest/collab.service.test.ts index 651561ea3..03187b0b9 100644 --- a/server/tests/unit/nest/collab.service.test.ts +++ b/server/tests/unit/nest/collab.service.test.ts @@ -231,6 +231,23 @@ describe('listMessages', () => { expect(reply.reply_text).toBe(''); }); + it('COLLAB-SVC-013c: a deleted message cannot be quoted, and the create path blanks too', () => { + const { user1, trip } = setup(); + const original = svc.createMessage(trip.id, user1.id, 'Original secret'); + svc.deleteMessage(trip.id, original.message!.id, user1.id); + + // Replying to something that is no longer there is refused outright. + expect(svc.createMessage(trip.id, user1.id, 'Too late', original.message!.id)).toEqual({ error: 'reply_not_found' }); + + // And the row the create path returns carries the same blanking listMessages + // does, for a message quoted before the original was deleted. + const second = svc.createMessage(trip.id, user1.id, 'Another secret'); + const quoting = svc.createMessage(trip.id, user1.id, 'Quoting it', second.message!.id); + expect(quoting.message!.reply_text).toBe('Another secret'); + svc.deleteMessage(trip.id, second.message!.id, user1.id); + expect(svc.listMessages(trip.id).find(m => m.text === 'Quoting it')!.reply_text).toBe(''); + }); + it('COLLAB-SVC-013: includes reactions grouped by emoji', () => { const { user1, trip } = setup(); const r = svc.createMessage(trip.id, user1.id, 'React me'); diff --git a/server/tests/unit/nest/idempotency.interceptor.test.ts b/server/tests/unit/nest/idempotency.interceptor.test.ts index 646e601dd..dcb5b2ed4 100644 --- a/server/tests/unit/nest/idempotency.interceptor.test.ts +++ b/server/tests/unit/nest/idempotency.interceptor.test.ts @@ -204,9 +204,17 @@ describe('IdempotencyInterceptor (parity with the legacy applyIdempotency middle const secondRes = makeRes(); const second = lastValueFrom(interceptor.intercept(ctx({ method: 'POST', headers: { 'x-idempotency-key': 'k' }, path: '/api/places', user: { id: 1 } }, secondRes), secondHandler)); + // The order Nest uses, and the one that makes this test worth having: the + // handler's observable completes FIRST, and the response - which is what + // stores the row - is written a few microtasks later. Release the waiter any + // earlier and it looks the key up, misses, and runs the handler again. + finish({ id: 'first' }); + // Nest is several microtask hops from the completion to the write + // (transformToResult -> lastValueFrom -> apply), so give the waiter the same + // room it gets in production to wake up too early. + for (let i = 0; i < 5; i++) await Promise.resolve(); firstRes.statusCode = 201; firstRes.json({ id: 'first' }); - finish({ id: 'first' }); expect(await first).toEqual({ id: 'first' }); expect(await second).toEqual({ id: 'first' }); @@ -231,6 +239,8 @@ describe('IdempotencyInterceptor (parity with the legacy applyIdempotency middle secondHandler, )); + // Nothing writes a response here, so the waiter is freed by the backstop in + // finalize, which defers a full tick past the response write. finish({ id: 'first' }); await first; expect(await second).toEqual({ id: 'second' }); diff --git a/server/tests/unit/nest/journey-share.service.test.ts b/server/tests/unit/nest/journey-share.service.test.ts index 6b5fee09e..d883e18ea 100644 --- a/server/tests/unit/nest/journey-share.service.test.ts +++ b/server/tests/unit/nest/journey-share.service.test.ts @@ -33,7 +33,7 @@ vi.mock('../../../src/config', () => ({ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; -import { createUser, createJourney, createJourneyEntry } from '../../helpers/factories'; +import { createUser, createJourney, createJourneyEntry, addJourneyContributor } from '../../helpers/factories'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { TrekPhotosRepository } from '../../../src/nest/photos/trek-photos.repository'; @@ -148,6 +148,25 @@ describe('createOrUpdateJourneyShareLink', () => { expect(link!.share_map).toBe(false); }); + it('JOURNEY-SHARE-029: reading the link separates "no link" from "not yours"', () => { + const { user: owner } = createUser(testDb); + const { user: helper } = createUser(testDb); + const journey = createJourney(testDb, owner.id); + addJourneyContributor(testDb, journey.id, helper.id, 'editor'); + + // Nothing published yet: an owner is allowed to look and finds nothing. + expect(svc.readJourneyShareLink(journey.id, owner.id)).toEqual({ allowed: true, link: null }); + + svc.createOrUpdateJourneyShareLink(journey.id, owner.id, {}); + const asOwner = svc.readJourneyShareLink(journey.id, owner.id); + expect(asOwner.allowed).toBe(true); + expect(asOwner.allowed && asOwner.link?.token).toBeTruthy(); + + // An editor is refused outright. Answering { link: null } here would tell a + // published journey's editor it is unpublished and offer to publish it. + expect(svc.readJourneyShareLink(journey.id, helper.id)).toEqual({ allowed: false }); + }); + it('JOURNEY-SHARE-027: an update leaves out flags it was not given', () => { const { user } = createUser(testDb); const journey = createJourney(testDb, user.id); diff --git a/server/tests/unit/nest/journey.controller.test.ts b/server/tests/unit/nest/journey.controller.test.ts index 3e8022711..704669fd1 100644 --- a/server/tests/unit/nest/journey.controller.test.ts +++ b/server/tests/unit/nest/journey.controller.test.ts @@ -187,7 +187,11 @@ describe('JourneyController', () => { it('preferences 403, share-link get/set/delete', () => { expect(thrown(() => ctl(svc({ updateJourneyPreferences: vi.fn().mockReturnValue(null) } as Partial)).preferences(user, '9', {}))).toEqual({ status: 403, body: { error: 'Not allowed' } }); - expect(ctl(svc({ getJourneyShareLink: vi.fn().mockReturnValue({ token: 'abc' }) } as Partial)).getShareLink(user, '9')).toEqual({ link: { token: 'abc' } }); + expect(ctl(svc({ getJourneyShareLink: vi.fn().mockReturnValue({ allowed: true, link: { token: 'abc' } }) } as Partial)).getShareLink(user, '9')).toEqual({ link: { token: 'abc' } }); + // An owner whose journey has no link gets the null; a non-owner gets the + // same refusal set/delete give, so the client can tell the two apart. + expect(ctl(svc({ getJourneyShareLink: vi.fn().mockReturnValue({ allowed: true, link: null }) } as Partial)).getShareLink(user, '9')).toEqual({ link: null }); + expect(thrown(() => ctl(svc({ getJourneyShareLink: vi.fn().mockReturnValue({ allowed: false }) } as Partial)).getShareLink(user, '9'))).toEqual({ status: 403, body: { error: 'Not allowed' } }); expect(thrown(() => ctl(svc({ createOrUpdateJourneyShareLink: vi.fn().mockReturnValue(null) } as Partial)).setShareLink(user, '9', {}))).toEqual({ status: 403, body: { error: 'Not allowed' } }); const setLink = vi.fn().mockReturnValue({ token: 'abc' }); expect(ctl(svc({ createOrUpdateJourneyShareLink: setLink } as Partial)).setShareLink(user, '9', { share_timeline: true })).toEqual({ token: 'abc' }); @@ -373,7 +377,11 @@ describe('JourneyController', () => { expect(thrown(() => ctl(svc(), { getBook: vi.fn().mockReturnValue(null), canOpen: vi.fn().mockReturnValue(false) }).getBook(user, '9'))).toEqual({ status: 404, body: { error: 'Journey not found' } }); expect(ctl(svc(), { getBook: vi.fn().mockReturnValue({ id: 3, version: 4 }), canOpen: vi.fn() }).getBook(user, '9')).toEqual({ book: { id: 3, version: 4 } }); - expect(thrown(() => ctl(svc(), { saveBook: vi.fn().mockReturnValue(null) }).saveBook(user, '9', { document: {} } as never))).toEqual({ status: 404, body: { error: 'Journey not found' } }); + // A refused save says which refusal it was. Out of reach entirely is still + // 404; a viewer who can open the journey but may not write it gets 403, so + // the editor can say why instead of claiming the journey vanished. + expect(thrown(() => ctl(svc(), { saveBook: vi.fn().mockReturnValue(null), canOpen: vi.fn().mockReturnValue(false) }).saveBook(user, '9', { document: {} } as never))).toEqual({ status: 404, body: { error: 'Journey not found' } }); + expect(thrown(() => ctl(svc(), { saveBook: vi.fn().mockReturnValue(null), canOpen: vi.fn().mockReturnValue(true) }).saveBook(user, '9', { document: {} } as never))).toEqual({ status: 403, body: { error: 'Not allowed' } }); // The record comes back bare, not in an envelope — the contract in // shared/src/book/book-store.schema.ts is the record itself. @@ -407,12 +415,13 @@ describe('JourneyController', () => { expect(broadcastSaved).not.toHaveBeenCalled(); }); - it('book: delete answers 204, or 404 out of reach', () => { + it('book: delete answers 204, 403 for a viewer, or 404 out of reach', () => { expect(ctl(svc(), { deleteBook: vi.fn().mockReturnValue(true) }).deleteBook(user, '9')).toBeUndefined(); // False means there was nothing to delete, which is still a 204 — deleting // a book that is already gone is not a failure. expect(ctl(svc(), { deleteBook: vi.fn().mockReturnValue(false) }).deleteBook(user, '9')).toBeUndefined(); - expect(thrown(() => ctl(svc(), { deleteBook: vi.fn().mockReturnValue(null) }).deleteBook(user, '9'))).toEqual({ status: 404, body: { error: 'Journey not found' } }); + expect(thrown(() => ctl(svc(), { deleteBook: vi.fn().mockReturnValue(null), canOpen: vi.fn().mockReturnValue(false) }).deleteBook(user, '9'))).toEqual({ status: 404, body: { error: 'Journey not found' } }); + expect(thrown(() => ctl(svc(), { deleteBook: vi.fn().mockReturnValue(null), canOpen: vi.fn().mockReturnValue(true) }).deleteBook(user, '9'))).toEqual({ status: 403, body: { error: 'Not allowed' } }); }); }); @@ -438,12 +447,12 @@ describe('JourneyPublicController', () => { }); it('legacy photo proxy: 404 invalid token, immich path streams', async () => { - expect(await thrownAsync(() => new JourneyPublicController(svc({ validateShareTokenForAsset: vi.fn().mockReturnValue(null) } as Partial), storageStub).legacyPhoto('tok', 'immich', 'a1', '2', 'thumbnail', {} as Response))).toEqual({ status: 404, body: { error: 'Not found' } }); + expect(await thrownAsync(() => new JourneyPublicController(svc({ validateShareTokenForAsset: vi.fn().mockReturnValue(null) } as Partial), storageStub).legacyPhoto('tok', 'immich', 'a1', 'thumbnail', {} as Response))).toEqual({ status: 404, body: { error: 'Not found' } }); // One call for every provider now, with the ids in a ref instead of in a // per-provider argument order. const streamProviderAsset = vi.fn().mockResolvedValue(undefined); const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }), streamProviderAsset } as Partial); - await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'immich', 'a1', '2', 'original', {} as Response); + await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'immich', 'a1', 'original', {} as Response); expect(streamProviderAsset).toHaveBeenCalledWith({}, 'immich', { userId: 5, ownerId: 5, assetId: 'a1' }, 'original'); }); @@ -457,14 +466,14 @@ describe('JourneyPublicController', () => { it('legacy photo proxy: synology streams, and a failure becomes a 404 json', async () => { const streamProviderAsset = vi.fn().mockResolvedValue(undefined); const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }), streamProviderAsset } as Partial); - await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'synologyphotos', 'a1', '2', 'thumbnail', {} as Response); + await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'synologyphotos', 'a1', 'thumbnail', {} as Response); expect(streamProviderAsset).toHaveBeenCalledWith({}, 'synologyphotos', { userId: 5, ownerId: 5, assetId: 'a1' }, 'thumbnail'); const status = vi.fn().mockReturnThis(); const json = vi.fn(); const res = { status, json } as unknown as Response; const failing = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 0 }), streamProviderAsset: vi.fn().mockRejectedValue(new Error('no synology')) } as Partial); - await new JourneyPublicController(failing, storageStub).legacyPhoto('tok', 'synologyphotos', 'a1', '6', 'original', res); + await new JourneyPublicController(failing, storageStub).legacyPhoto('tok', 'synologyphotos', 'a1', 'original', res); expect(status).toHaveBeenCalledWith(404); expect(json).toHaveBeenCalledWith({ error: 'Provider not supported' }); }); @@ -478,7 +487,7 @@ describe('JourneyPublicController', () => { const json = vi.fn(); const res = { status, json } as unknown as Response; const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }), streamProviderAsset } as Partial); - await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'photoprism', 'a1', '2', 'original', res); + await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'photoprism', 'a1', 'original', res); expect(status).toHaveBeenCalledWith(404); expect(json).toHaveBeenCalledWith({ error: 'Provider not supported' }); }); @@ -489,14 +498,14 @@ describe('JourneyPublicController', () => { // the journey owner when the photo row carries no owner_id. const streamProviderAsset = vi.fn().mockResolvedValue(undefined); const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 3 }), streamProviderAsset } as Partial); - await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'immich', 'a1', '8', 'original', {} as Response); + await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'immich', 'a1', 'original', {} as Response); expect(streamProviderAsset).toHaveBeenCalledWith({}, 'immich', { userId: 3, ownerId: 3, assetId: 'a1' }, 'original'); }); it('legacy photo proxy: local provider 404s when the object does not exist', async () => { storageExists.mockResolvedValue(false); const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }) } as Partial); - expect(await thrownAsync(() => new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'local', 'gone.jpg', '2', 'thumbnail', {} as Response))).toEqual({ status: 404, body: { error: 'Not found' } }); + expect(await thrownAsync(() => new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'local', 'gone.jpg', 'thumbnail', {} as Response))).toEqual({ status: 404, body: { error: 'Not found' } }); expect(storageSendToResponse).not.toHaveBeenCalled(); }); @@ -506,7 +515,7 @@ describe('JourneyPublicController', () => { const set = vi.fn(); const res = { set } as unknown as Response; const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }) } as Partial); - await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'local', 'photo.jpg', '2', 'original', res); + await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'local', 'photo.jpg', 'original', res); expect(storageExists).toHaveBeenCalledWith('journey', 'photo.jpg'); expect(set).toHaveBeenCalledWith('Cache-Control', 'public, max-age=86400'); expect(storageSendToResponse).toHaveBeenCalledWith('journey', 'photo.jpg', res); @@ -519,7 +528,7 @@ describe('JourneyPublicController', () => { const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }) } as Partial); // Express decodes %2F in a single path param to '/', so the handler sees this. - await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'local', '../../files/secret.pdf', '2', 'original', res); + await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'local', '../../files/secret.pdf', 'original', res); // basename() collapses the traversal before the storage lookup, and central // key validation (storage-keys.ts) rejects anything that still carries a @@ -533,7 +542,7 @@ describe('JourneyPublicController', () => { // which the handler reads as a miss. storageExists.mockRejectedValue(new Error('invalid storage key: journey/..')); const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }) } as Partial); - expect(await thrownAsync(() => new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'local', '..', '2', 'original', {} as Response))).toEqual({ status: 404, body: { error: 'Not found' } }); + expect(await thrownAsync(() => new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'local', '..', 'original', {} as Response))).toEqual({ status: 404, body: { error: 'Not found' } }); expect(storageSendToResponse).not.toHaveBeenCalled(); }); }); diff --git a/server/tests/unit/nest/maps.service.test.ts b/server/tests/unit/nest/maps.service.test.ts index 474a2324f..e44582c11 100644 --- a/server/tests/unit/nest/maps.service.test.ts +++ b/server/tests/unit/nest/maps.service.test.ts @@ -633,6 +633,39 @@ describe('resolveGoogleMapsUrl coordinate extraction (ReDoS guards)', () => { expect(fetchMock).toHaveBeenCalledTimes(1); }); + it.each([ + 'https://www.google.de/maps?cid=999', + 'https://maps.google.co.uk/?cid=999', + 'https://google.com.au/maps?cid=999', + ])('MAPS-CID-003b: still reads the page body of a country domain (%s)', async (link) => { + // Google Maps answers on every ccTLD, and those links carry no coordinates + // to fall back on — a host allow-list of .com spellings drops them. + const fetchMock = vi.fn(async (u: string) => { + if (u.includes('nominatim')) { + return { ok: true, json: async () => ({ display_name: 'Berlin, DE', name: null, address: {} }) }; + } + return { url: link, text: async () => 'x!3d52.5163!4d13.3777y' }; + }); + vi.stubGlobal('fetch', fetchMock); + + const result = await svc.resolveGoogleMapsUrl(link); + expect(result.lat).toBeCloseTo(52.5163, 3); + expect(result.lng).toBeCloseTo(13.3777, 3); + }); + + it('MAPS-CID-003c: a host that only looks like Google is still not read', async () => { + const fetchMock = vi.fn(async (u: string) => { + if (u.includes('nominatim')) { + return { ok: true, json: async () => ({ display_name: 'x', name: null, address: {} }) }; + } + return { url: 'https://google.evil.com/maps', text: async () => 'x!3d40.6892!4d-74.0445y' }; + }); + vi.stubGlobal('fetch', fetchMock); + + await expect(svc.resolveGoogleMapsUrl('https://google.evil.com/maps?cid=999')).rejects.toMatchObject({ status: 400 }); + expect(fetchMock).not.toHaveBeenCalled(); + }); + it('MAPS-CID-004: skips a page body that declares more than the size cap', async () => { const fetchMock = vi.fn(async (u: string) => { if (u.includes('cid=')) return { url: 'https://www.google.com/maps/place/Somewhere' }; diff --git a/server/tests/unit/nest/reservations.service.test.ts b/server/tests/unit/nest/reservations.service.test.ts index 89f4310ea..80f88137a 100644 --- a/server/tests/unit/nest/reservations.service.test.ts +++ b/server/tests/unit/nest/reservations.service.test.ts @@ -838,6 +838,25 @@ describe('ReservationsService — referenced ids stay inside the trip', () => { })).toEqual(['day_id', 'place_id', 'accommodation_id']); }); + it('RESV-SCOPE-005: a dangling id is not an offender, so the booking stays editable', () => { + const { mine } = twoTrips(); + const place = createPlace(testDb, mine.id); + const day = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').get(mine.id) as { id: number }; + const acc = createDayAccommodation(testDb, mine.id, place.id, day.id, day.id); + const res = createReservation(testDb, mine.id, { title: 'Hotel', type: 'hotel' }); + testDb.prepare('UPDATE reservations SET accommodation_id = ? WHERE id = ?').run(acc.id, res.id); + // What shortening the trip's date range does: the day goes, the cascade + // takes the accommodation with it, and the reservation keeps the id. + testDb.prepare('DELETE FROM day_accommodations WHERE id = ?').run(acc.id); + + expect(svc.referencesOutsideTrip(String(mine.id), { title: 'x', accommodation_id: acc.id })).toEqual([]); + + const current = svc.getReservation(String(res.id), String(mine.id))!; + svc.update(String(res.id), String(mine.id), { accommodation_id: acc.id, title: 'Hotel renamed' } as never, current); + expect(testDb.prepare('SELECT title, accommodation_id FROM reservations WHERE id = ?').get(res.id)) + .toEqual({ title: 'Hotel renamed', accommodation_id: null }); + }); + it('RESV-SCOPE-002: passes a body whose ids all live in the trip', () => { const { mine } = twoTrips(); const place = createPlace(testDb, mine.id); diff --git a/server/tests/unit/nest/vacay.service.test.ts b/server/tests/unit/nest/vacay.service.test.ts index 2c037c490..fc8b4019f 100644 --- a/server/tests/unit/nest/vacay.service.test.ts +++ b/server/tests/unit/nest/vacay.service.test.ts @@ -1553,6 +1553,32 @@ describe('quirk fixes', () => { expect(await fresh.getHolidays('2026', 'DE')).toEqual({ error: 'Failed to fetch holidays' }); }); + it('VACAY-SVC-070d: a chunked provider body past the cap reads as the usual fetch error', async () => { + // nager.at answers chunked, so there is no content-length for the declared + // check to look at — only the streaming read stops this being buffered whole. + const payload = `[${'{"date":"2026-01-01"},'.repeat(200_000)}{"date":"2026-12-24"}]`; + const fetchMock = vi.fn().mockImplementation(async () => { + let sent = false; + return { + ok: true, + headers: { get: () => null }, + body: { + getReader: () => ({ + read: async () => (sent ? { done: true } : ((sent = true), { done: false, value: new TextEncoder().encode(payload) })), + cancel: async () => undefined, + }), + cancel: async () => undefined, + }, + json: async () => JSON.parse(payload), + }; + }); + vi.stubGlobal('fetch', fetchMock); + const fresh = new VacayService(new DatabaseService(testDb), new RealtimeService(), notificationsStub()); + + expect(await fresh.getHolidays('2026', 'DE')).toEqual({ error: 'Failed to fetch holidays' }); + expect(await fresh.getCountries()).toEqual({ error: 'Failed to fetch countries' }); + }); + it('VACAY-SVC-071: applyHolidayCalendars honors the cache TTL', async () => { const { plan } = setupUserWithPlan(); testDb.prepare('UPDATE vacay_plans SET holidays_enabled = 1 WHERE id = ?').run(plan.id); diff --git a/server/tests/unit/nest/weather/weather.impl.test.ts b/server/tests/unit/nest/weather/weather.impl.test.ts index f3f8eb0f7..02a0d1dcf 100644 --- a/server/tests/unit/nest/weather/weather.impl.test.ts +++ b/server/tests/unit/nest/weather/weather.impl.test.ts @@ -868,3 +868,57 @@ describe('coordinate validation', () => { await expect(getWeather('42.40', '9.41', date, 'en')).rejects.toMatchObject({ status: 502 }); }); }); + +// ── response size cap ──────────────────────────────────────────────────────── + +describe('size cap on the Open-Meteo body', () => { + beforeEach(() => { + vi.mocked(fetch).mockReset(); + }); + + /** + * Response that streams `chunks` with no content-length, the way Open-Meteo + * answers. json() is there so the old buffer-it-all path would happily parse + * these bodies — the cap has to be what stops the oversized one. + */ + function chunked(chunks: string[]): Response { + let i = 0; + return { + ok: true, + status: 200, + headers: { get: () => null }, + json: async () => JSON.parse(chunks.join('')), + body: { + getReader: () => ({ + read: async () => + i < chunks.length + ? { done: false, value: new TextEncoder().encode(chunks[i++]) } + : { done: true, value: undefined }, + cancel: async () => undefined, + }), + cancel: async () => undefined, + }, + } as unknown as Response; + } + + it('WEATHER-COORD-004: a chunked body past the cap is a 502 — there is no content-length to catch it', async () => { + const date = dateOffset(5); + // Valid JSON, just far too much of it: the declared-length check sees nothing + // here, so only the streaming read stops this from being buffered whole. + const filler = '"x",'.repeat(400_000); + vi.mocked(fetch).mockResolvedValue(chunked([`{"daily":{"time":[${filler}"${date}"]}}`])); + + await expect(getWeather('43.40', '9.42', date, 'en')).rejects.toMatchObject({ status: 502 }); + }); + + it('WEATHER-COORD-005: a chunked body under the cap still parses', async () => { + const date = dateOffset(5); + const body = JSON.stringify({ + daily: { time: [date], temperature_2m_max: [20], temperature_2m_min: [10], weathercode: [0] }, + }); + vi.mocked(fetch).mockResolvedValue(chunked([body.slice(0, 20), body.slice(20)])); + + const result = await getWeather('44.40', '9.43', date, 'en'); + expect(result.temp).toBe(15); + }); +}); diff --git a/server/tests/unit/utils/cappedFetch.test.ts b/server/tests/unit/utils/cappedFetch.test.ts index e6a2c77ec..10cd2e626 100644 --- a/server/tests/unit/utils/cappedFetch.test.ts +++ b/server/tests/unit/utils/cappedFetch.test.ts @@ -1,14 +1,14 @@ import { describe, it, expect } from 'vitest'; -import { exceedsDeclaredLength, readCapped, readCappedText } from '../../../src/utils/cappedFetch'; +import { discardBody, exceedsDeclaredLength, readCapped, readCappedJson, readCappedText } from '../../../src/utils/cappedFetch'; /** Response stub that streams `chunks` through a reader, tracking cancellation. */ -function streamed(chunks: string[]) { +function streamed(chunks: string[], contentLength: string | null = null) { let i = 0; const state = { cancelled: false }; return { state, res: { - headers: { get: () => null }, + headers: { get: () => contentLength }, body: { getReader: () => ({ read: async () => @@ -17,6 +17,7 @@ function streamed(chunks: string[]) { : { done: true, value: undefined as unknown as Uint8Array }, cancel: async () => { state.cancelled = true; }, }), + cancel: async () => { state.cancelled = true; }, }, }, }; @@ -119,3 +120,48 @@ describe('readCappedText', () => { expect(text.length).toBe(20); }); }); + +describe('discardBody', () => { + it('CAP-012: cancels the stream of a response nobody is going to read', async () => { + const { res, state } = streamed(['unread']); + discardBody(res); + await Promise.resolve(); + expect(state.cancelled).toBe(true); + }); + + it('CAP-013: is a no-op for a response without a body, and swallows a rejecting cancel', async () => { + expect(() => discardBody({})).not.toThrow(); + expect(() => discardBody({ body: { getReader: () => ({ read: async () => ({ done: true }), cancel: async () => undefined }), cancel: async () => { throw new Error('nope'); } } })).not.toThrow(); + await Promise.resolve(); + }); +}); + +describe('readCappedJson', () => { + it('CAP-014: parses a streamed body that fits', async () => { + const { res } = streamed(['{"holidays"', ':[1,2]}']); + expect(await readCappedJson(res, 1024)).toEqual({ holidays: [1, 2] }); + }); + + it('CAP-015: a chunked body over the cap is undefined, not a truncated parse', async () => { + // No content-length at all — the case the declared-length check cannot see. + const { res, state } = streamed(['{"items":[', 'x'.repeat(200), ']}']); + expect(await readCappedJson(res, 32)).toBeUndefined(); + expect(state.cancelled).toBe(true); + }); + + it('CAP-016: a declared oversize is refused without reading the body', async () => { + const { res, state } = streamed(['{"ok":true}'], '999999'); + expect(await readCappedJson(res, 1024)).toBeUndefined(); + await Promise.resolve(); + expect(state.cancelled).toBe(true); + }); + + it('CAP-017: a body that is not JSON is undefined rather than a throw', async () => { + const { res } = streamed(['error page']); + expect(await readCappedJson(res, 1024)).toBeUndefined(); + }); + + it('CAP-018: falls back to json() when the response exposes nothing to stream', async () => { + expect(await readCappedJson({ json: async () => ({ from: 'json' }) }, 1024)).toEqual({ from: 'json' }); + }); +}); diff --git a/wiki/Audit-Log.md b/wiki/Audit-Log.md index c0e68409c..a51f9a606 100644 --- a/wiki/Audit-Log.md +++ b/wiki/Audit-Log.md @@ -115,7 +115,11 @@ The panel loads 100 entries at a time by default. Click **Load more** at the bot ## IP addresses -The client IP is read from the `X-Forwarded-For` header. When TREK is behind a reverse proxy, set `TRUST_PROXY=1` (the number of proxy hops) so the header is trusted and the real client IP is recorded. Without this setting, the proxy's own IP is logged instead. See [Environment-Variables](Environment-Variables). +The client IP is the one Express resolves after applying `TRUST_PROXY`, not whatever the `X-Forwarded-For` header happens to say. That distinction matters: the header is written by the caller, and an audit row an attacker can address to someone else's IP is worse than no row at all. + +`TRUST_PROXY` is **the number of proxy hops in front of TREK**, and it has to be accurate. With `TRUST_PROXY=1` (the default) TREK trusts exactly one hop, so a request that passed through two proxies is recorded as coming from the *inner* one. If your setup is Cloudflare in front of nginx in front of TREK, set `TRUST_PROXY=2`. Set it to `0` to trust nothing and always record the socket address. + +See [Environment-Variables](Environment-Variables). ## Log file diff --git a/wiki/Environment-Variables.md b/wiki/Environment-Variables.md index a32a23dcf..479069fb5 100644 --- a/wiki/Environment-Variables.md +++ b/wiki/Environment-Variables.md @@ -129,7 +129,7 @@ full explanation. |---------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------------| | `FORCE_HTTPS` | When `true`: 301-redirects HTTP→HTTPS, sends HSTS (`max-age=31536000`), adds CSP `upgrade-insecure-requests`, forces cookie `secure` flag. Only useful behind a TLS proxy. Requires `TRUST_PROXY`. | `false` | | `HSTS_INCLUDE_SUBDOMAINS` | When `true`: adds the `includeSubDomains` directive to the HSTS header, extending HTTPS enforcement to all subdomains. Only effective when HSTS is active (`FORCE_HTTPS=true` or `NODE_ENV=production`). Leave `false` if you run other services on sibling subdomains over plain HTTP. | `false` | -| `TRUST_PROXY` | Number of trusted proxy hops. Tells Express to read the real client IP from `X-Forwarded-For` and protocol from `X-Forwarded-Proto`. Defaults to `1` automatically in production. Required for `FORCE_HTTPS` to detect the forwarded protocol. | `1` (production) | +| `TRUST_PROXY` | Number of trusted proxy hops. Tells Express how far into `X-Forwarded-For` to look for the real client IP, and where to read `X-Forwarded-Proto`. Count your hops: with two proxies in front of TREK and `TRUST_PROXY=1`, the IP on every audit row is the inner proxy's. `0` trusts nothing and always uses the socket address. Required for `FORCE_HTTPS` to detect the forwarded protocol. | `1` (production) | | `COOKIE_SECURE` | Controls the `secure` flag on the `trek_session` cookie. Auto-derived as `true` when `NODE_ENV=production` or `FORCE_HTTPS=true`. Set to `false` only as an escape hatch for LAN testing without TLS — not recommended in production. | auto | > **Warning:** `FORCE_HTTPS=true` without `TRUST_PROXY` set causes a redirect loop.