fix(places): pin the image and website URL fields on the way in

image_url and website were bare strings in the contract, and the create/update
bodies are open records, so the Zod pipe never looked at either. Both leave the
database for something that treats them as a URL: the thumbnail is interpolated
into hand-built marker HTML, and the homepage is handed to window.open, where a
javascript: value would run in this origin rather than opening a page.

Both are allow-listed on the way in now, next to hexColorSchema and checked in
the handler the way validateRouteColor already is, on all three write surfaces
so REST, the plugin RPC and MCP agree on what they accept. The entity schemas
stay open: rows written before this still have to round-trip.

safeHttpUrl covers the read side for exactly those rows, the same split
safeHexColor uses, and the window.open calls it guards pick up
noopener,noreferrer while they are being touched.
This commit is contained in:
Maurice
2026-08-16 21:37:52 +02:00
committed by Maurice
parent 0f0bf50cd3
commit c8607ba234
15 changed files with 205 additions and 19 deletions
+22 -1
View File
@@ -20,7 +20,7 @@ import type { Response } from 'express';
import { isDemoWriteBlocked, DEMO_WRITE_ERROR } from '../common/demo-write';
import { RuntimeEnvService } from '../app-config/runtime-env.service';
import { memoryStorage } from 'multer';
import { hexColorSchema } from '@trek/shared';
import { hexColorSchema, placeImageUrlSchema, placeWebsiteSchema } from '@trek/shared';
import type { User } from '../../types';
import { PlacesService } from './places.service';
import { isUpdateConflict } from '../common/conflictResult';
@@ -63,6 +63,25 @@ function validateRouteColor(body: Record<string, unknown>): void {
}
}
// Same reason as route_color: these two leave the database for a renderer that
// treats them as a URL — the thumbnail into marker HTML, the homepage into
// window.open — and the open record means the pipe never sees them.
function validateUrlFields(body: Record<string, unknown>): void {
const image = body.image_url;
if (image !== undefined && image !== null) {
if (typeof image !== 'string' || !placeImageUrlSchema.safeParse(image).success) {
throw new HttpException({ error: 'image_url must be an uploaded path, a photo-proxy path, an inline image or an https URL' }, 400);
}
}
const website = body.website;
// '' is how the UI clears the field; treat it like an absent value.
if (website !== undefined && website !== null && website !== '') {
if (typeof website !== 'string' || !placeWebsiteSchema.safeParse(website).success) {
throw new HttpException({ error: 'website must be an http or https URL' }, 400);
}
}
}
function parseBool(v: unknown, defaultVal: boolean): boolean {
return v === undefined || v === null ? defaultVal : String(v) === 'true';
}
@@ -131,6 +150,7 @@ export class PlacesController {
const trip = this.requireTrip(tripId, user);
validateLengths(body);
validateRouteColor(body);
validateUrlFields(body);
this.requireEdit(trip, user);
const place = this.places.create(tripId, body as never);
this.places.broadcast(tripId, 'place:created', { place }, socketId);
@@ -428,6 +448,7 @@ export class PlacesController {
const trip = this.requireTrip(tripId, user);
validateLengths(body);
validateRouteColor(body);
validateUrlFields(body);
this.requireEdit(trip, user);
const result = this.places.update(tripId, id, body as never, ifMatch);
if (!result) {
+5 -4
View File
@@ -5,6 +5,7 @@ import {
demoDenied, ok,
} from '@trek/nest-mcp';
import { McpToolGuardsService } from '../mcp-shared/mcp-tool-guards.service';
import { placeWebsiteSchema } from '@trek/shared';
import { z } from 'zod';
import { AuthService } from '../auth/auth.service';
import { AssignmentsService } from '../assignments/assignments.service';
@@ -63,7 +64,7 @@ export class PlacesMcp {
google_ftid: z.string().optional().describe('Google Maps feature ID from search_place — enables direct Google Maps links'),
osm_id: z.string().optional().describe('OpenStreetMap ID from search_place (e.g. "way:12345") — enables opening hours if no Google ID'),
notes: z.string().max(2000).optional(),
website: z.string().max(500).optional(),
website: placeWebsiteSchema.optional(),
phone: z.string().max(50).optional(),
price: z.number().nonnegative().optional().describe('Cost of this place/activity (e.g. ticket price, entry fee)'),
currency: z.string().length(3).optional().describe('ISO 4217 currency code (e.g. "EUR", "USD")'),
@@ -103,7 +104,7 @@ export class PlacesMcp {
google_ftid: z.string().optional().describe('Google Maps feature ID from search_place — enables direct Google Maps links'),
osm_id: z.string().optional().describe('OpenStreetMap ID from search_place (e.g. "way:12345")'),
place_notes: z.string().max(2000).optional().describe('Notes for the place'),
website: z.string().max(500).optional(),
website: placeWebsiteSchema.optional(),
phone: z.string().max(50).optional(),
assignment_notes: z.string().max(500).optional().describe('Notes for this day assignment'),
price: z.number().nonnegative().optional().describe('Cost of this place/activity (e.g. ticket price, entry fee)'),
@@ -158,7 +159,7 @@ export class PlacesMcp {
end_time: z.string().max(50).optional().describe('End time (e.g. "11:00")'),
duration_minutes: z.number().int().positive().optional(),
notes: z.string().max(2000).optional(),
website: z.string().max(500).optional(),
website: placeWebsiteSchema.optional(),
phone: z.string().max(50).optional(),
transport_mode: z.enum(['walking', 'driving', 'cycling', 'transit', 'flight']).optional(),
osm_id: z.string().optional().describe('OpenStreetMap ID (e.g. "way:12345")'),
@@ -366,7 +367,7 @@ export class PlacesMcp {
end_time: z.string().max(50).optional().describe('End time (e.g. "11:00")'),
duration_minutes: z.number().int().positive().optional(),
notes: z.string().max(2000).optional(),
website: z.string().max(500).optional(),
website: placeWebsiteSchema.optional(),
phone: z.string().max(50).optional(),
description: z.string().max(2000).optional(),
},
+20 -1
View File
@@ -1,4 +1,4 @@
import { placeCreateRequestSchema, placeUpdateRequestSchema } from '@trek/shared';
import { placeCreateRequestSchema, placeImageUrlSchema, placeUpdateRequestSchema, placeWebsiteSchema } from '@trek/shared';
import { PluginController, PluginMethod } from '../plugins/host/rpc-kit/decorators';
import { PluginGuards } from '../plugins/host/plugin-guards.service';
import { BadParams, ForbiddenResource } from '../plugins/host/rpc-errors';
@@ -18,6 +18,23 @@ const PLACE_EDIT_ACTION = 'place_edit';
*/
const PLACE_STR_LIMITS: Record<string, number> = { name: 200, description: 2000, address: 500, notes: 2000 };
/**
* The two URL fields the REST controller pins in validateUrlFields. Same reason
* they are checked there rather than in the contract: the write schemas are open
* records, so a plugin could otherwise store a thumbnail or a homepage the web
* app refuses — and both end up somewhere that treats them as a URL.
*/
function capUrls(input: Record<string, unknown>): void {
const image = input.image_url;
if (image !== undefined && image !== null && !placeImageUrlSchema.safeParse(image).success) {
throw new BadParams('invalid place: image_url must be an uploaded path, a photo-proxy path, an inline image or an https URL');
}
const website = input.website;
if (website !== undefined && website !== null && website !== '' && !placeWebsiteSchema.safeParse(website).success) {
throw new BadParams('invalid place: website must be an http or https URL');
}
}
/**
* The place surface a plugin may reach (#plugins).
*
@@ -41,6 +58,7 @@ export class PlacesRpc {
const parsed = placeCreateRequestSchema.safeParse(params.input);
if (!parsed.success) throw new BadParams(`invalid place: ${schemaMessage(parsed.error)}`);
this.guards.capStrings(parsed.data as Record<string, unknown>, PLACE_STR_LIMITS);
capUrls(parsed.data as Record<string, unknown>);
this.guards.requireTripEdit(tripId, actor, PLACE_EDIT_ACTION);
const place = this.places.create(String(tripId), parsed.data as unknown as PlaceCreateInput);
this.realtime.broadcast(tripId, 'place:created', { place });
@@ -56,6 +74,7 @@ export class PlacesRpc {
const parsed = placeUpdateRequestSchema.safeParse(params.input);
if (!parsed.success) throw new BadParams(`invalid place: ${schemaMessage(parsed.error)}`);
this.guards.capStrings(parsed.data as Record<string, unknown>, PLACE_STR_LIMITS);
capUrls(parsed.data as Record<string, unknown>);
this.guards.requireTripEdit(tripId, actor, PLACE_EDIT_ACTION);
const place = this.places.update(String(tripId), String(placeId), parsed.data as PlaceUpdateInput);
if (place === null) throw new ForbiddenResource(`no place ${placeId} on trip ${tripId}`);
@@ -301,6 +301,59 @@ describe('PlacesController (parity with the legacy /api/trips/:tripId/places rou
});
});
// image_url and website leave the database for something that treats them as a
// URL — the thumbnail into hand-built marker HTML, the homepage into
// window.open. The write body is an open record, so the Zod pipe never sees
// either one; they are checked here for the same reason route_color is.
describe('image_url and website', () => {
const imageErr = { status: 400, body: { error: 'image_url must be an uploaded path, a photo-proxy path, an inline image or an https URL' } };
const siteErr = { status: 400, body: { error: 'website must be an http or https URL' } };
const ctl = (over: Partial<PlacesService> = {}) => new PlacesController(svc(over), new RuntimeEnvService());
it('400s an image_url the marker builders were never meant to receive, before the permission check', () => {
const canEdit = vi.fn().mockReturnValue(false); // would 403 if it got that far
for (const image_url of [
'javascript:alert(1)',
'data:text/html,<script>alert(1)</script>',
'http://insecure.example/p.png',
'//evil.example/p.png',
123,
]) {
expect(thrown(() => ctl({ canEdit }).update(user, '5', '9', { image_url }))).toEqual(imageErr);
}
expect(thrown(() => ctl({ canEdit }).create(user, '5', { name: 'T', image_url: 'javascript:alert(1)' }))).toEqual(imageErr);
expect(canEdit).not.toHaveBeenCalled();
});
it('keeps accepting every shape the app actually stores', () => {
const update = vi.fn().mockReturnValue({ id: 9 });
for (const image_url of [
'/uploads/places/eiffel.jpg',
'/api/maps/place-photo/abc123',
'data:image/jpeg;base64,/9j/4AAQSkZJRg==',
'https://images.example/photo.jpg',
null,
]) {
expect(ctl({ update } as Partial<PlacesService>).update(user, '5', '9', { image_url })).toEqual({ place: { id: 9 } });
}
});
it('400s a website that window.open would not treat as a page', () => {
const canEdit = vi.fn().mockReturnValue(false);
for (const website of ['javascript:fetch("/api/trips")', 'data:text/html,x', 'louvre.fr', 42]) {
expect(thrown(() => ctl({ canEdit }).update(user, '5', '9', { website }))).toEqual(siteErr);
}
expect(canEdit).not.toHaveBeenCalled();
});
it('accepts http and https, and treats the empty string as clearing the field', () => {
const update = vi.fn().mockReturnValue({ id: 9 });
for (const website of ['https://louvre.fr', 'http://pension.at', '', null]) {
expect(ctl({ update } as Partial<PlacesService>).update(user, '5', '9', { website })).toEqual({ place: { id: 9 } });
}
});
});
it('PUT /:id forwards the base-version token and 409s on a conflict (#1135)', () => {
const update = vi.fn().mockReturnValue({ conflict: true, server: { id: 9, name: 'Theirs' } });
const onUpdated = vi.fn(); const broadcast = vi.fn();