diff --git a/client/src/utils/safeUrl.test.ts b/client/src/utils/safeUrl.test.ts index f908a67b1..6a19891be 100644 --- a/client/src/utils/safeUrl.test.ts +++ b/client/src/utils/safeUrl.test.ts @@ -27,6 +27,14 @@ describe('safeHttpUrl', () => { expect(safeHttpUrl('//evil.example')).toBeNull() }) + // #2483: the write contract now completes a bare host to https. What is read + // back here is the stored text, which is linked only when it is already final. + it('FE-UTIL-SAFEURL-007: a value the contract would complete is still not linked as stored', () => { + expect(safeHttpUrl('fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët')).toBeNull() + expect(safeHttpUrl(' https://louvre.fr')).toBeNull() + expect(safeHttpUrl('https://louvre.fr ')).toBe('https://louvre.fr ') + }) + it('FE-UTIL-SAFEURL-006: treats absent and empty as nothing to open', () => { expect(safeHttpUrl(null)).toBeNull() expect(safeHttpUrl(undefined)).toBeNull() diff --git a/client/src/utils/safeUrl.ts b/client/src/utils/safeUrl.ts index b6a21bef8..0b08d3238 100644 --- a/client/src/utils/safeUrl.ts +++ b/client/src/utils/safeUrl.ts @@ -14,7 +14,12 @@ import { placeWebsiteSchema } from '@trek/shared' * have not updated yet — the same split as safeHexColor. */ export function safeHttpUrl(value: string | null | undefined): string | null { - return typeof value === 'string' && placeWebsiteSchema.safeParse(value).success ? value : null + if (typeof value !== 'string') return null + const parsed = placeWebsiteSchema.safeParse(value) + // The contract completes a bare host to https on the way in (#2483), but the + // value handed to window.open here is the one stored, and "louvre.fr" would + // resolve against this app. So only a value already in its final form passes. + return parsed.success && parsed.data === value ? value : null } /** diff --git a/server/src/nest/accommodations/accommodations.mcp.ts b/server/src/nest/accommodations/accommodations.mcp.ts index 2dd6e0e54..f5e5cb6e7 100644 --- a/server/src/nest/accommodations/accommodations.mcp.ts +++ b/server/src/nest/accommodations/accommodations.mcp.ts @@ -4,6 +4,7 @@ import { demoDenied, ok, } from '../../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 { PlacesService } from '../places/places.service'; @@ -92,7 +93,8 @@ export class AccommodationsMcp { 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(), + // The place contract create_place uses: a bare host gains https (#2483). + website: placeWebsiteSchema.optional(), phone: z.string().max(50).optional(), start_day_id: z.number().int().positive().describe('Check-in day ID'), end_day_id: z.number().int().positive().describe('Check-out day ID'), diff --git a/server/src/nest/booking-import/booking-import.service.ts b/server/src/nest/booking-import/booking-import.service.ts index e436b17cc..487a5188e 100644 --- a/server/src/nest/booking-import/booking-import.service.ts +++ b/server/src/nest/booking-import/booking-import.service.ts @@ -12,7 +12,7 @@ import type { User } from '../../types'; import { KitineraryExtractorService } from './kitinerary-extractor.service'; import { LlmParseService } from '../llm-parse/llm-parse.service'; import { mapReservations } from './kitinerary-mapper'; -import { typeToCostCategory } from '@trek/shared'; +import { normalizePlaceWebsite, typeToCostCategory } from '@trek/shared'; import type { BookingImportPreviewItem, BookingImportPreviewResponse, BookingImportConfirmResponse, BookingImportMode, BookingImportFileReport, Reservation } from '@trek/shared'; import type { ParsedBookingItem, KiReservation } from './kitinerary.types'; @@ -257,7 +257,9 @@ export class BookingImportService { lat, lng, address: _venue.address, - website: _venue.website, + // A booking mail gives the venue's site however its sender wrote it; + // it lands as https or not at all (#2483). + website: normalizePlaceWebsite(_venue.website) ?? undefined, phone: _venue.phone, }); placeId = (place as any).id; diff --git a/server/src/nest/maps/maps.helpers.ts b/server/src/nest/maps/maps.helpers.ts index 0bec3b714..15f4f9f76 100644 --- a/server/src/nest/maps/maps.helpers.ts +++ b/server/src/nest/maps/maps.helpers.ts @@ -1,3 +1,4 @@ +import { normalizePlaceWebsite } from '@trek/shared'; import { readEnv, getAppUrl } from '../../app-config'; import { stripHtmlTags } from '../common/stripHtmlTags'; import { haversineMetres } from '../common/geo'; @@ -661,7 +662,7 @@ export function buildOsmDetails(tags: Record, osmType: string, o } } return { - website: tags['contact:website'] || tags.website || null, + website: normalizePlaceWebsite(tags['contact:website']) ?? normalizePlaceWebsite(tags.website), phone: tags['contact:phone'] || tags.phone || null, opening_hours, open_now, diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 5cfd651e4..3be75cd29 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -1,5 +1,5 @@ import { Injectable } from '@nestjs/common'; -import { isOutsideChina } from '@trek/shared'; +import { isOutsideChina, normalizePlaceWebsite } from '@trek/shared'; import type { MapsSearchResult, MapsAutocompleteResult, @@ -972,7 +972,7 @@ export class MapsService { category: labelFor(p.category ?? null, p.categoryPath ?? null) ?? wanted[0], poi_type: p.category ?? wanted[0], address: p.address?.freeform ?? null, - website: p.contact?.website ?? null, + website: normalizePlaceWebsite(p.contact?.website), phone: p.contact?.phone ?? null, opening_hours: p.hours?.osm ?? null, // The index carries the chain and its Wikidata item, which is what @@ -1493,7 +1493,7 @@ export class MapsService { category: categoryOfFilter.get(matched) ?? categories[0], poi_type: matched, address: addr, - website: tags.website || tags['contact:website'] || null, + website: normalizePlaceWebsite(tags.website) ?? normalizePlaceWebsite(tags['contact:website']), phone: tags.phone || tags['contact:phone'] || null, opening_hours: tags.opening_hours || null, cuisine: tags.cuisine || null, @@ -2208,7 +2208,7 @@ export class MapsService { lat: p.location?.latitude ?? null, lng: p.location?.longitude ?? null, rating: p.rating || null, - website: p.websiteUri || null, + website: normalizePlaceWebsite(p.websiteUri), phone: p.nationalPhoneNumber || null, types: p.types || [], source: 'google', @@ -2570,7 +2570,7 @@ export class MapsService { lng: data.location?.longitude ?? null, rating: data.rating || null, rating_count: data.userRatingCount || null, - website: data.websiteUri || null, + website: normalizePlaceWebsite(data.websiteUri), phone: data.nationalPhoneNumber || null, opening_hours: data.regularOpeningHours?.weekdayDescriptions || null, open_now: data.regularOpeningHours?.openNow ?? null, @@ -2715,7 +2715,7 @@ export class MapsService { lng: data.location?.longitude ?? null, rating: data.rating || null, rating_count: data.userRatingCount || null, - website: data.websiteUri || null, + website: normalizePlaceWebsite(data.websiteUri), phone: data.nationalPhoneNumber || null, opening_hours: data.regularOpeningHours?.weekdayDescriptions || null, open_now: data.regularOpeningHours?.openNow ?? null, diff --git a/server/src/nest/maps/providers/amap.provider.ts b/server/src/nest/maps/providers/amap.provider.ts index f16e41460..c085d1a71 100644 --- a/server/src/nest/maps/providers/amap.provider.ts +++ b/server/src/nest/maps/providers/amap.provider.ts @@ -29,7 +29,7 @@ * the same way a place found on OpenStreetMap does. */ import { createHash } from 'node:crypto'; -import { fromAmapLocation, gcj02ToWgs84, toAmapLocation } from '@trek/shared'; +import { fromAmapLocation, gcj02ToWgs84, normalizePlaceWebsite, toAmapLocation } from '@trek/shared'; import { readEnv } from '../../../app-config'; import { safeFetchFollow } from '../../../utils/ssrfGuard'; import { discardBody, exceedsDeclaredLength, readCappedJson } from '../../../utils/cappedFetch'; @@ -403,7 +403,8 @@ export class AmapPlacesProvider implements PlacesProvider { lng: coords?.lng ?? null, rating: amapNumber(business.rating ?? bizExt.rating), rating_count: null, - website: amapText(poi.website) || null, + // Free text, often a bare host: it gains https here like any other source's. + website: normalizePlaceWebsite(amapText(poi.website)), phone: amapText(business.tel ?? poi.tel) || null, // Amap's type is a slash-separated taxonomy ("餐饮服务;中餐厅;川菜"); split so // it reads like the string array every other provider returns. diff --git a/server/src/nest/maps/trek-places.client.ts b/server/src/nest/maps/trek-places.client.ts index 5913acf97..bd25d6a19 100644 --- a/server/src/nest/maps/trek-places.client.ts +++ b/server/src/nest/maps/trek-places.client.ts @@ -11,6 +11,7 @@ * to configure and no quota to exhaust, so the failure modes are the network * and the service being down, both of which fall back to what TREK did before. */ +import { normalizePlaceWebsite } from '@trek/shared'; import { readEnv, getAppUrl } from '../../app-config'; /** The public instance. An operator may point at their own copy instead. */ @@ -425,7 +426,8 @@ export function toPlaceRecord(p: TrekPlace): Record { lng: Number.isFinite(p.lng) ? p.lng : null, // Ratings exist nowhere in open data, and saying so with null is honest. rating: null, - website: p.contact?.website ?? null, + // The index keeps websites as Overture has them, often without a scheme. + website: normalizePlaceWebsite(p.contact?.website), phone: p.contact?.phone ?? null, email: p.contact?.email ?? null, category: p.category ?? null, diff --git a/server/src/nest/place-enrichment/place-enrichment.service.ts b/server/src/nest/place-enrichment/place-enrichment.service.ts index ce22188d6..6578c9006 100644 --- a/server/src/nest/place-enrichment/place-enrichment.service.ts +++ b/server/src/nest/place-enrichment/place-enrichment.service.ts @@ -192,9 +192,10 @@ function yesNo(value: unknown): 'yes' | 'no' | string | null { * dropped rather than linked. */ function googleMapsLink(value: unknown): string | null { - if (typeof value !== 'string' || !placeWebsiteSchema.safeParse(value).success) return null; + const parsed = placeWebsiteSchema.safeParse(value); + if (!parsed.success) return null; try { - return isGoogleMapsHost(new URL(value).hostname) ? value : null; + return isGoogleMapsHost(new URL(parsed.data).hostname) ? parsed.data : null; } catch { return null; } @@ -233,8 +234,8 @@ export function collectFacts(details: Record | null): PlaceFact // `menu_url` is a community-editable OSM tag that becomes an href on the // client, so it goes through the same allow-list as a place's website — // anything but http(s) is dropped rather than rendered as a link. - const menu = typeof details.menu_url === 'string' ? details.menu_url.trim() : ''; - if (placeWebsiteSchema.safeParse(menu).success) push('menu', null, menu); + const menu = placeWebsiteSchema.safeParse(typeof details.menu_url === 'string' ? details.menu_url.trim() : ''); + if (menu.success) push('menu', null, menu.data); if (yesNo(details.outdoor_seating) === 'yes') push('outdoorSeating', null); if (yesNo(details.takeaway) === 'yes') push('takeaway', null); @@ -635,11 +636,11 @@ export class PlaceEnrichmentService { // becomes an href on the client, and the value comes from whatever the // configured index answered with. Anything but http(s) loses the link and // keeps the text, rather than being rendered as one. - const rawUrl = typeof got?.sourceUrl === 'string' ? got.sourceUrl : null; + const url = placeWebsiteSchema.safeParse(got?.sourceUrl); return { text, source: 'website', - sourceUrl: rawUrl && placeWebsiteSchema.safeParse(rawUrl).success ? rawUrl : null, + sourceUrl: url.success ? url.data : null, // Not a licensed corpus: a quoted summary from the operator's own page, // credited and linked back. Saying "CC-something" here would be a claim // about terms nobody granted. diff --git a/server/src/nest/places/places.controller.ts b/server/src/nest/places/places.controller.ts index 2ee824448..45492e9be 100644 --- a/server/src/nest/places/places.controller.ts +++ b/server/src/nest/places/places.controller.ts @@ -79,9 +79,13 @@ function validateUrlFields(body: Record): void { 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) { + const parsed = placeWebsiteSchema.safeParse(website); + if (!parsed.success) { throw new HttpException({ error: 'website must be an http or https URL' }, 400); } + // The parsed form is the one stored: a bare host from a search result + // arrives here without its https (#2483). + body.website = parsed.data; } } diff --git a/server/src/nest/places/places.rpc.ts b/server/src/nest/places/places.rpc.ts index ecb1710d8..211562aa1 100644 --- a/server/src/nest/places/places.rpc.ts +++ b/server/src/nest/places/places.rpc.ts @@ -30,8 +30,11 @@ function capUrls(input: Record): void { 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'); + if (website !== undefined && website !== null && website !== '') { + const parsed = placeWebsiteSchema.safeParse(website); + if (!parsed.success) throw new BadParams('invalid place: website must be an http or https URL'); + // Stored in its parsed form, like the REST route: a bare host gains https. + input.website = parsed.data; } } diff --git a/server/src/nest/plugins/contributions/plugin-search.helpers.ts b/server/src/nest/plugins/contributions/plugin-search.helpers.ts index 5d518d7e1..416c80f7c 100644 --- a/server/src/nest/plugins/contributions/plugin-search.helpers.ts +++ b/server/src/nest/plugins/contributions/plugin-search.helpers.ts @@ -1,3 +1,4 @@ +import { normalizePlaceWebsite } from '@trek/shared'; import { stripEmoji } from '../text-sanitize'; /** @@ -45,20 +46,6 @@ const capOrNull = (v: unknown, n: number): string | null => { return s === '' ? null : s; }; -/** - * A url that may become an ``. Anything but http/https is dropped — a - * `javascript:` or `data:` url rendered as a link is click-XSS into the search list. - */ -function safeUrl(raw: unknown): string | null { - if (typeof raw !== 'string' || raw === '') return null; - try { - const u = new URL(raw); - return u.protocol === 'http:' || u.protocol === 'https:' ? raw.slice(0, 2048) : null; - } catch { - return null; - } -} - /** * A rating a plugin index carries, on the five-point scale every such index uses. * @@ -94,7 +81,10 @@ export function normalizeSearchHits(pluginId: string, raw: unknown): SearchHit[] lat, lng, rating: safeRating(h.rating), - website: safeUrl(h.website), + // Through the helper every other source uses (#2483): a `javascript:` or + // `data:` url rendered as a link is click-XSS into the search list, and a + // bare host gains https here as it does from the core search. + website: normalizePlaceWebsite(h.website), phone: capOrNull(h.phone, 60), category: capOrNull(h.category, 60), description: capOrNull(h.description, 500), diff --git a/server/tests/e2e/collections.e2e.test.ts b/server/tests/e2e/collections.e2e.test.ts index 623018455..cdb9ee785 100644 --- a/server/tests/e2e/collections.e2e.test.ts +++ b/server/tests/e2e/collections.e2e.test.ts @@ -139,6 +139,37 @@ describe('Collections e2e (real auth guard + real service + temp SQLite)', () => expect(placed.reservation_status).toBe('none'); // itinerary defaults }); + // #2483: a place from the TREK index can carry its website without a scheme, + // and the save dialog has no website field to correct it in. + it('COLLECTIONS-E2E-089: saving a place whose website has no scheme stores it as https', async () => { + const col = (await request(server).post('/api/addons/collections').set('Cookie', sessionCookie(ownerId)).send({ name: 'Bretagne' })).body; + const saved = await request(server).post('/api/addons/collections/places') + .set('Cookie', sessionCookie(ownerId)).send({ + collection_id: col.id, + name: 'Chapelle Sainte-Barbe', + lat: 48.0286, + lng: -3.4862, + osm_id: 'gers:ceba0e62-172b-4343-bb3b-78b915a18383', + website: 'fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët', + }); + expect(saved.status).toBe(200); + expect(saved.body.place.website).toBe('https://fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët'); + expect(db.prepare('SELECT website FROM collection_places WHERE id = ?').get(saved.body.place.id)).toEqual({ + website: 'https://fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët', + }); + }); + + it('COLLECTIONS-E2E-090: a script link on save is still a 400 and stores nothing', async () => { + const col = (await request(server).post('/api/addons/collections').set('Cookie', sessionCookie(ownerId)).send({ name: 'Script links' })).body; + for (const website of ['javascript:alert(1)', 'mailto:mairie@example.fr', 'Chapelle']) { + const res = await request(server).post('/api/addons/collections/places') + .set('Cookie', sessionCookie(ownerId)).send({ collection_id: col.id, name: 'Chapelle', website }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/^website: /); + } + expect(db.prepare('SELECT COUNT(*) AS n FROM collection_places WHERE collection_id = ?').get(col.id)).toEqual({ n: 0 }); + }); + // Regression for #1437: editing a place (PATCH without a status field) must NOT // reset a 'want'/'visited' place back to 'idea'. it('COLLECTIONS-E2E-012: PATCH without status leaves the saved status unchanged', async () => { diff --git a/server/tests/e2e/places.e2e.test.ts b/server/tests/e2e/places.e2e.test.ts index e181a7ef3..dd00e0d9c 100644 --- a/server/tests/e2e/places.e2e.test.ts +++ b/server/tests/e2e/places.e2e.test.ts @@ -235,6 +235,36 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { expect(bad.body).toEqual({ error: 'route_color must be a hex colour like #4f46e5' }); }); + // #2483: a place from the TREK index can carry its website without a scheme. + it('PLACES-E2E-2483-01: create and update take a website without a scheme and store it as https', async () => { + const created = await request(server).post('/api/trips/5/places').set('Cookie', sessionCookie(1)) + .send({ name: 'Chapelle Sainte-Barbe', website: 'fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët' }); + expect(created.status).toBe(201); + expect(created.body.place.website).toBe('https://fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët'); + + const id = created.body.place.id; + const updated = await request(server).put(`/api/trips/5/places/${id}`).set('Cookie', sessionCookie(1)) + .send({ website: '//www.example.fr/patrimoine' }); + expect(updated.status).toBe(200); + expect(db.prepare('SELECT website FROM places WHERE id = ?').get(id)).toEqual({ website: 'https://www.example.fr/patrimoine' }); + + // An explicit scheme is stored exactly as sent, and '' still clears the field. + const kept = await request(server).put(`/api/trips/5/places/${id}`).set('Cookie', sessionCookie(1)) + .send({ website: 'http://Example.fr/Pfad?q=1' }); + expect(kept.body.place.website).toBe('http://Example.fr/Pfad?q=1'); + const cleared = await request(server).put(`/api/trips/5/places/${id}`).set('Cookie', sessionCookie(1)).send({ website: '' }); + expect(cleared.status).toBe(200); + }); + + it('PLACES-E2E-2483-02: a script link, another scheme or a bare word is still a 400 with the same message', async () => { + for (const website of ['javascript:alert(1)', 'mailto:mairie@example.fr', 'Chapelle', 42]) { + const res = await request(server).post('/api/trips/5/places').set('Cookie', sessionCookie(1)).send({ name: 'Chapelle', website }); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'website must be an http or https URL' }); + } + expect(db.prepare('SELECT COUNT(*) AS n FROM places WHERE trip_id = 5').get()).toEqual({ n: 0 }); + }); + it('409 on a stale If-Match token (#1135)', async () => { db.prepare("INSERT INTO places (id, trip_id, name, updated_at) VALUES (9, 5, 'Walk', '2026-01-01 00:00:00')").run(); const res = await request(server) diff --git a/server/tests/unit/mcp/tools-collections.test.ts b/server/tests/unit/mcp/tools-collections.test.ts index da3efb6ba..e6b89a76f 100644 --- a/server/tests/unit/mcp/tools-collections.test.ts +++ b/server/tests/unit/mcp/tools-collections.test.ts @@ -338,6 +338,27 @@ describe('Tool: save_place_to_collection', () => { }); }); +// #2483: the tool spreads the REST save contract, so a bare host from +// search_place is saved with https here too. +describe('Tool: save_place_to_collection and a website without a scheme (#2483)', () => { + it('MCP-COLL-2483-01: stores https for a bare host and still refuses a script link', async () => { + const { user } = createUser(testDb); + const col = seedCollection(user.id, 'Bretagne'); + const site = 'fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët'; + await withHarness(user.id, async (h) => { + const saved = parseToolResult(await h.client.callTool({ + name: 'save_place_to_collection', + arguments: { collection_id: col, name: 'Chapelle Sainte-Barbe', website: site }, + })) as { place: { website: string } }; + expect(saved.place.website).toBe(`https://${site}`); + + const refused = await h.client.callTool({ name: 'save_place_to_collection', arguments: { collection_id: col, name: 'Hostile', website: 'javascript:alert(1)' } }); + expect(refused.isError).toBe(true); + }); + expect(testDb.prepare('SELECT website FROM collection_places WHERE collection_id = ?').all(col)).toEqual([{ website: `https://${site}` }]); + }); +}); + describe('Tool: save_trip_places_to_collection', () => { it('copies trip places, skipping duplicates', async () => { const { user } = createUser(testDb); diff --git a/server/tests/unit/mcp/tools-days-accommodations.test.ts b/server/tests/unit/mcp/tools-days-accommodations.test.ts index 833559683..65d01dee2 100644 --- a/server/tests/unit/mcp/tools-days-accommodations.test.ts +++ b/server/tests/unit/mcp/tools-days-accommodations.test.ts @@ -335,6 +335,28 @@ describe('Tool: delete_accommodation', () => { // --------------------------------------------------------------------------- describe('Tool: create_place_accommodation', () => { + // #2483: the place it creates takes its website through the same contract as + // create_place, so a bare host from search_place lands as https. + it('MCP-ACCOM-2483-01: a website without a scheme is stored with https, a script link is refused', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + await withHarness(user.id, async (h) => { + const data = parseToolResult(await h.client.callTool({ + name: 'create_place_accommodation', + arguments: { tripId: trip.id, name: 'Hôtel du Faouët', start_day_id: day.id, end_day_id: day.id, website: 'www.hotel-faouet.example' }, + })) as { place: { id: number } }; + expect(testDb.prepare('SELECT website FROM places WHERE id = ?').get(data.place.id)).toEqual({ website: 'https://www.hotel-faouet.example' }); + + const refused = await h.client.callTool({ + name: 'create_place_accommodation', + arguments: { tripId: trip.id, name: 'Hostile', start_day_id: day.id, end_day_id: day.id, website: 'javascript:alert(1)' }, + }); + expect(refused.isError).toBe(true); + }); + expect(testDb.prepare("SELECT COUNT(*) AS n FROM places WHERE trip_id = ? AND name = 'Hostile'").get(trip.id)).toEqual({ n: 0 }); + }); + it('creates the place and the accommodation atomically and broadcasts both', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); diff --git a/server/tests/unit/mcp/tools-places.test.ts b/server/tests/unit/mcp/tools-places.test.ts index d48ac5d7a..763a9d818 100644 --- a/server/tests/unit/mcp/tools-places.test.ts +++ b/server/tests/unit/mcp/tools-places.test.ts @@ -1271,3 +1271,62 @@ describe('journey hooks on the MCP delete paths', () => { expect(skeletonFor(journey.id, foreign.id)).toBeDefined(); }); }); + +// --------------------------------------------------------------------------- +// A website without a scheme (#2483): the tools parse it the way REST does +// --------------------------------------------------------------------------- + +describe('place tools and a website without a scheme (#2483)', () => { + const SITE = 'fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët'; + const websiteOf = (placeId: number) => + (testDb.prepare('SELECT website FROM places WHERE id = ?').get(placeId) as { website: string | null }).website; + + it('MCP-PLACES-2483-01: create_place, create_and_assign_place, update_place and bulk_update_places store https', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + + await withHarness(user.id, async (h) => { + const created = parseToolResult(await h.client.callTool({ name: 'create_place', arguments: { tripId: trip.id, name: 'Chapelle', website: SITE } })) as { place: { id: number } }; + expect(websiteOf(created.place.id)).toBe(`https://${SITE}`); + + const assigned = parseToolResult(await h.client.callTool({ + name: 'create_and_assign_place', + arguments: { tripId: trip.id, dayId: day.id, name: 'Halles', website: 'www.example.fr/halles' }, + })) as { place: { id: number } }; + expect(websiteOf(assigned.place.id)).toBe('https://www.example.fr/halles'); + + await h.client.callTool({ name: 'update_place', arguments: { tripId: trip.id, placeId: created.place.id, website: '//www.example.fr' } }); + expect(websiteOf(created.place.id)).toBe('https://www.example.fr'); + + await h.client.callTool({ name: 'bulk_update_places', arguments: { tripId: trip.id, placeIds: [created.place.id, assigned.place.id], website: 'example.fr:8080/x' } }); + expect([websiteOf(created.place.id), websiteOf(assigned.place.id)]).toEqual(['https://example.fr:8080/x', 'https://example.fr:8080/x']); + }); + }); + + it('MCP-PLACES-2483-02: a script link or another scheme is still refused and stores nothing', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + + await withHarness(user.id, async (h) => { + for (const website of ['javascript:alert(1)', 'mailto:mairie@example.fr', 'Chapelle']) { + const result = await h.client.callTool({ name: 'create_place', arguments: { tripId: trip.id, name: 'Hostile', website } }); + expect(result.isError, website).toBe(true); + expect((result.content as { text: string }[])[0].text).toMatch(/Invalid arguments/); + } + }); + + expect(testDb.prepare('SELECT COUNT(*) AS n FROM places WHERE trip_id = ?').get(trip.id)).toEqual({ n: 0 }); + }); + + it('MCP-PLACES-2483-03: tools/list still describes the field as a plain capped string', async () => { + const { user } = createUser(testDb); + await withHarness(user.id, async (h) => { + const { tools } = await h.client.listTools(); + for (const name of ['create_place', 'create_and_assign_place', 'update_place', 'bulk_update_places']) { + const tool = tools.find((t) => t.name === name); + expect((tool?.inputSchema.properties as Record).website, name).toEqual({ type: 'string', maxLength: 500 }); + } + }); + }); +}); diff --git a/server/tests/unit/nest/booking-import/booking-import.service.test.ts b/server/tests/unit/nest/booking-import/booking-import.service.test.ts index 9e9e471d8..8226db8a7 100644 --- a/server/tests/unit/nest/booking-import/booking-import.service.test.ts +++ b/server/tests/unit/nest/booking-import/booking-import.service.test.ts @@ -187,3 +187,32 @@ describe('BookingImportService.preview endpoint geocoding (#1969)', () => { expect(maps.geocodeQuery).not.toHaveBeenCalled(); }); }); + +// #2483: a venue's website from a booking mail is one more outside source, and it +// used to reach the place row exactly as the mail or the review form had it. +describe('BookingImportService.confirm venue website (#2483)', () => { + it('BOOKING-IMPORT-2483-01: a bare host is saved with https, a script link or nothing not at all', async () => { + const reservations = { create: vi.fn(() => ({ reservation: { id: 1 }, accommodationCreated: false })) }; + const places = { create: vi.fn((_tripId: string, _input: { website?: string }) => ({ id: 7 })) }; + const svc = new BookingImportService( + {} as never, {} as never, new DatabaseService(dbConn), reservations as never, permissionsStub as never, + undefined as never, { isAddonEnabled: () => false } as never, { broadcast: vi.fn() } as never, + { geocodeQuery: vi.fn() } as never, places as never, + ); + const item = (website?: string) => ({ + type: 'restaurant', + title: 'Dîner', + _venue: { name: 'Crêperie', lat: 48.03, lng: -3.49, website }, + source: { fileName: 'booking.eml', index: 0 }, + }); + + const res = await svc.confirm('5', [item('www.creperie.example/carte'), item('javascript:alert(1)'), item()], undefined); + + expect(res.created).toHaveLength(3); + expect(places.create.mock.calls.map(([, input]) => input.website)).toEqual([ + 'https://www.creperie.example/carte', + undefined, + undefined, + ]); + }); +}); diff --git a/server/tests/unit/nest/maps.amap.test.ts b/server/tests/unit/nest/maps.amap.test.ts index 5ed94774d..7cbc7b138 100644 --- a/server/tests/unit/nest/maps.amap.test.ts +++ b/server/tests/unit/nest/maps.amap.test.ts @@ -230,6 +230,23 @@ describe('AmapPlacesProvider.searchText', () => { expect(place.website).toBeNull(); }); + // #2483: a site in Amap is free text, and often a bare host. + it('AMAP-012b: a website without a scheme gains https, one that is no website becomes null', async () => { + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue( + ok({ + pois: [ + { id: 'B3', name: '故宫博物院', website: 'www.dpm.org.cn', location: TIANANMEN_LOCATION }, + { id: 'B4', name: '某小店', website: 'javascript:alert(1)', location: TIANANMEN_LOCATION }, + ], + }), + ), + ); + const places = await provider().searchText('故宫'); + expect(places.map((p) => p.website)).toEqual(['https://www.dpm.org.cn', null]); + }); + it('AMAP-013: uses place/around with a bias, because text search cannot be biased', async () => { vi.stubGlobal('fetch', vi.fn().mockResolvedValue(ok({ pois: [] }))); await provider().searchText('咖啡', 'zh', { lat: 31.2304, lng: 121.4737, radius: 3000 }); diff --git a/server/tests/unit/nest/maps.gers-details.test.ts b/server/tests/unit/nest/maps.gers-details.test.ts index 628b9bd5b..9d5c0ada5 100644 --- a/server/tests/unit/nest/maps.gers-details.test.ts +++ b/server/tests/unit/nest/maps.gers-details.test.ts @@ -120,6 +120,19 @@ describe('MapsService.getPlaceDetails for a gers: id', () => { expect(out.place?.opening_hours).toBeTruthy(); }); + // #2483: the place from the issue, whose index record carries its website + // without a scheme. Both halves of the merge hand it over completed. + it('MAPS-GERS-010: the merged website has its scheme, from the index or else from OSM', async () => { + const site = 'fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët'; + mockById.mockResolvedValue({ ...PLACE, gers: 'ceba0e62-172b-4343-bb3b-78b915a18383', contact: { ...PLACE.contact, website: site } }); + expect((await make(null).getPlaceDetails(1, 'gers:ceba0e62-172b-4343-bb3b-78b915a18383')).place?.website).toBe(`https://${site}`); + expect((await make({ website: 'www.example.fr' }).getPlaceDetails(1, 'gers:ceba0e62-172b-4343-bb3b-78b915a18383')).place?.website).toBe(`https://${site}`); + + mockById.mockResolvedValue({ ...PLACE, contact: { ...PLACE.contact, website: 'javascript:alert(1)' } }); + expect((await make({ website: 'www.example.fr' }).getPlaceDetails(1, 'gers:abc-123')).place?.website).toBe('https://www.example.fr'); + expect((await make(null).getPlaceDetails(1, 'gers:abc-123')).place?.website).toBeNull(); + }); + it('MAPS-GERS-009: with the index switched off, opening a saved place asks nobody', async () => { // TREK_PLACES_ENABLED=false is the operator's word that nothing leaves for // the index. Search, autocomplete and the area download honoured it; the diff --git a/server/tests/unit/nest/maps.pois.test.ts b/server/tests/unit/nest/maps.pois.test.ts index 80ffecb24..1586e826b 100644 --- a/server/tests/unit/nest/maps.pois.test.ts +++ b/server/tests/unit/nest/maps.pois.test.ts @@ -236,6 +236,20 @@ describe('MapsService.pois answered from the index', () => { }); }); + // #2483: the index keeps websites the way the operator typed them. + it('MAPS-POIS-015: a website without a scheme gains https, one that is no website becomes null', async () => { + mockNearby.mockResolvedValue([ + { ...FULL, contact: { ...FULL.contact, website: 'cafecentral.wien/de' } }, + { ...FULL, gers: 'a-2', contact: { ...FULL.contact, website: 'javascript:alert(1)' } }, + ]); + const svc = make(); + stubOverpass(svc); + + const out = await svc.pois('cafe', BOX); + + expect(out.pois.map((p) => p.website)).toEqual(['https://cafecentral.wien/de', null]); + }); + it('MAPS-POIS-004: asks the index for the categories behind the pill, around the viewport centre', async () => { mockNearby.mockResolvedValue([FULL]); const svc = make(); diff --git a/server/tests/unit/nest/maps.service.test.ts b/server/tests/unit/nest/maps.service.test.ts index 04aaa8196..df825a422 100644 --- a/server/tests/unit/nest/maps.service.test.ts +++ b/server/tests/unit/nest/maps.service.test.ts @@ -3516,3 +3516,75 @@ describe('brandLogo', () => { expect(await service().brandLogo('Q3')).toBeNull(); }); }) + +// ── Websites without a scheme (#2483) ───────────────────────────────────────── +// +// Every source hands its website through normalizePlaceWebsite, so a place that +// is picked from any of them saves without a 400 over a field nobody sees. + +describe('websites from the map sources (#2483)', () => { + it('MAPS-2483-01: OSM details complete a bare host and skip a contact:website that is no website', () => { + expect(buildOsmDetails({ website: 'www.example.fr/patrimoine' }, 'way', '1').website).toBe( + 'https://www.example.fr/patrimoine', + ); + expect(buildOsmDetails({ 'contact:website': 'mailto:mairie@example.fr', website: 'example.fr' }, 'node', '1').website).toBe( + 'https://example.fr', + ); + expect(buildOsmDetails({ website: 'javascript:alert(1)' }, 'node', '1').website).toBeNull(); + }); + + it('MAPS-2483-02: an Overpass POI gets https on a bare host and falls back past a script link', async () => { + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ + elements: [ + { type: 'node', id: 1, lat: 48.03, lon: -3.49, tags: { name: 'Chapelle', tourism: 'attraction', website: 'fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët' } }, + { type: 'node', id: 2, lat: 48.04, lon: -3.49, tags: { name: 'Mairie', tourism: 'attraction', website: 'javascript:alert(1)', 'contact:website': '//www.example.fr' } }, + { type: 'node', id: 3, lat: 48.05, lon: -3.49, tags: { name: 'Halles', tourism: 'attraction', website: 'Halles' } }, + ], + }), + }), + ); + const { pois } = await svc.searchOverpassPois('sights', { south: 48.0, west: -3.6, north: 48.1, east: -3.4 }, 'fr-FR'); + expect(pois.map((p) => p.website)).toEqual([ + 'https://fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët', + 'https://www.example.fr', + null, + ]); + }); + + it('MAPS-2483-03: a Google search result goes through the same helper', async () => { + mockDbGet.mockReturnValueOnce({ maps_api_key: 'ENCRYPTED' }).mockReturnValueOnce(null); + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ + places: [{ id: 'gid-web', displayName: { text: 'Chapelle' }, location: { latitude: 48, longitude: -3 }, websiteUri: 'www.example.fr' }], + }), + }), + ); + const search = await svc.searchPlaces(1, 'Chapelle'); + expect((search.places[0] as { website: unknown }).website).toBe('https://www.example.fr'); + }); + + const googleDetails = (websiteUri: string) => + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: true, json: async () => ({ id: 'ChIJWeb', websiteUri }) })); + + it('MAPS-2483-04: so does the website of a Google details lookup', async () => { + mockDbGet.mockReturnValueOnce({ maps_api_key: 'gkey' }); + googleDetails('example.fr/visite'); + const place = (await svc.getPlaceDetails(1, 'ChIJWeb-lean')).place as { website: unknown }; + expect(place.website).toBe('https://example.fr/visite'); + }); + + it('MAPS-2483-05: and of the expanded one, where a value that is no page becomes null', async () => { + mockDbGet.mockReturnValueOnce({ maps_api_key: 'gkey' }); + mockDbGet.mockReturnValueOnce(undefined); + googleDetails('data:text/html,x'); + const place = (await svc.getPlaceDetailsExpanded(1, 'ChIJWeb-expanded')).place as { website: unknown }; + expect(place.website).toBeNull(); + }); +}); diff --git a/server/tests/unit/nest/place-enrichment.description.test.ts b/server/tests/unit/nest/place-enrichment.description.test.ts index 86b4f2f14..6b694a4df 100644 --- a/server/tests/unit/nest/place-enrichment.description.test.ts +++ b/server/tests/unit/nest/place-enrichment.description.test.ts @@ -198,7 +198,7 @@ describe('description source order', () => { // The record and its link come with the request, and the answer is cached // for every user of the instance under the label "Google". A link that is // not a Google Maps address by shape is dropped; the text stays. - for (const bad of ['https://phish.example/maps', 'https://google.evil.example/x', 'javascript:alert(1)', 'maps.google.com/x']) { + for (const bad of ['https://phish.example/maps', 'https://google.evil.example/x', 'javascript:alert(1)', 'phish.example/maps']) { const maps = mapsStub({ getMapsKey: vi.fn(() => 'key'), fetchEditorialSummary: vi.fn(async () => 'Casual chain for wood-fired pizza.'), @@ -221,6 +221,15 @@ describe('description source order', () => { }); expect(out.description, ok).toMatchObject({ source: 'google', sourceUrl: ok }); } + + // #2483: the website contract completes a bare host, and the link is the + // completed one. The host check still decides, on the host it names. + const maps = mapsStub({ + getMapsKey: vi.fn(() => 'key'), + fetchEditorialSummary: vi.fn(async () => 'Casual chain for wood-fired pizza.'), + }); + const bare = await make(maps).enrich(1, { ...GOOGLE_REQ, details: { source: 'google', google_maps_url: 'maps.google.com/x' } }); + expect(bare.description).toMatchObject({ source: 'google', sourceUrl: 'https://maps.google.com/x' }); }); it('ENRICH-075: keeps the OpenStreetMap description ahead of everything', async () => { @@ -450,6 +459,14 @@ describe("the description on the place's own website", () => { }); const ok = await make(mapsStub()).enrich(1, GERS_REQ); expect(ok.description).toMatchObject({ sourceUrl: 'https://losteria.net/rostock' }); + + // #2483: the index keeps a page the way it found it, sometimes without a + // scheme. The link that is stored is the completed one, never the bare text. + mockTrekPlacesById.mockResolvedValue({ + description: { text: 'Pizza in Rostock.', sourceUrl: 'losteria.net/rostock' }, + }); + const bare = await make(mapsStub()).enrich(1, GERS_REQ); + expect(bare.description).toMatchObject({ sourceUrl: 'https://losteria.net/rostock' }); }); it('ENRICH-117: a description the request made up never reaches the shared cache', async () => { diff --git a/server/tests/unit/nest/place-enrichment.service.test.ts b/server/tests/unit/nest/place-enrichment.service.test.ts index 94256772d..944ac8e00 100644 --- a/server/tests/unit/nest/place-enrichment.service.test.ts +++ b/server/tests/unit/nest/place-enrichment.service.test.ts @@ -757,6 +757,14 @@ describe('collectFacts', () => { ]); }); + // #2483: an OSM menu tag is written the way people type a website. The link + // the client gets is the completed one, never the bare text. + it('ENRICH-052e: completes a menu_url without a scheme to https', () => { + expect(collectFacts(osm({ menu_url: 'www.example.org/speisekarte' }))).toEqual([ + { kind: 'menu', value: null, url: 'https://www.example.org/speisekarte' }, + ]); + }); + it('ENRICH-053: takes no chips at all from a Google place', () => { expect(collectFacts(null)).toEqual([]); // cuisine/outdoor_seating are OSM tagging and never appear on a Google blob; diff --git a/server/tests/unit/nest/places.controller.test.ts b/server/tests/unit/nest/places.controller.test.ts index 7dfa107eb..167ecdd79 100644 --- a/server/tests/unit/nest/places.controller.test.ts +++ b/server/tests/unit/nest/places.controller.test.ts @@ -410,7 +410,9 @@ describe('PlacesController (parity with the legacy /api/trips/:tripId/places rou it('400s a website that window.open would not treat as a page', async () => { const canEdit = vi.fn().mockReturnValue(false); - for (const website of ['javascript:fetch("/api/trips")', 'data:text/html,x', 'louvre.fr', 42]) { + // 'louvre' rather than 'louvre.fr': a bare host is completed since #2483, + // a single word is still no address. + for (const website of ['javascript:fetch("/api/trips")', 'data:text/html,x', 'mailto:info@louvre.fr', 'louvre', 42]) { expect(await thrownAsync(() => ctl({ canEdit }).update(user, '5', '9', { website }))).toEqual(siteErr); } expect(canEdit).not.toHaveBeenCalled(); @@ -420,8 +422,23 @@ describe('PlacesController (parity with the legacy /api/trips/:tripId/places rou const update = vi.fn().mockReturnValue({ id: 9 }); for (const website of ['https://louvre.fr', 'http://pension.at', '', null]) { expect(await ctl({ update } as Partial).update(user, '5', '9', { website })).toEqual({ place: { id: 9 } }); + expect(update).toHaveBeenLastCalledWith('5', '9', { website }, undefined); } }); + + // #2483: the value that reaches the service is the parsed one, on both write + // routes. The MCP tools parse the same schema (tools-places.test.ts), so an + // agent and the web app store the same string for the same input. + it('PLACES-CTRL-2483-01: hands the service a bare host with https, on create and on update', async () => { + const site = 'fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët'; + const create = vi.fn().mockReturnValue({ id: 9 }); + ctl({ create } as Partial).create(user, '5', { name: 'Chapelle', website: site }); + expect(create).toHaveBeenCalledWith('5', { name: 'Chapelle', website: `https://${site}` }); + + const update = vi.fn().mockReturnValue({ id: 9 }); + await ctl({ update } as Partial).update(user, '5', '9', { website: '//www.example.fr' }); + expect(update).toHaveBeenCalledWith('5', '9', { website: 'https://www.example.fr' }, undefined); + }); }); it('PUT /:id forwards the base-version token and 409s on a conflict (#1135)', async () => { diff --git a/server/tests/unit/nest/trek-places.client.test.ts b/server/tests/unit/nest/trek-places.client.test.ts index a93a72ca2..726c1a2c3 100644 --- a/server/tests/unit/nest/trek-places.client.test.ts +++ b/server/tests/unit/nest/trek-places.client.test.ts @@ -602,6 +602,16 @@ describe('toPlaceRecord', () => { expect(r.lat).toBeNull(); }); + // #2483: Overture keeps a website the way the operator typed it. + it('TREK-PLACES-2483-01: a website without a scheme gains https, one that is no website becomes null', () => { + const withSite = (website: string | null) => toPlaceRecord({ ...PLACE, contact: { ...PLACE.contact, website } }).website; + expect(withSite('fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët')).toBe( + 'https://fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët', + ); + expect(withSite('javascript:alert(1)')).toBeNull(); + expect(withSite(null)).toBeNull(); + }); + it('builds an address from whatever parts exist', () => { const r = toPlaceRecord({ ...PLACE, diff --git a/server/tests/unit/places/places.rpc.test.ts b/server/tests/unit/places/places.rpc.test.ts index e215d65d0..904338063 100644 --- a/server/tests/unit/places/places.rpc.test.ts +++ b/server/tests/unit/places/places.rpc.test.ts @@ -176,4 +176,22 @@ describe('PlacesRpc', () => { expect(res.error.message).toBe('no place 7 on trip 1'); expect(f.realtime.broadcast).not.toHaveBeenCalled(); }); + + // #2483: the same parse the REST controller and the MCP tools apply, so a + // plugin that hands over a bare host stores the https form, not the text. + it('PLACES-RPC-2483-01 a website without a scheme is stored with https, a script link is BAD_PARAMS', async () => { + const f = build(); + const host = f.host('db:write:places'); + const site = 'fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët'; + expect((await host.dispatch(req('places.create', { tripId: 1, input: { name: 'Chapelle', website: site } }), 42)).ok).toBe(true); + expect(f.places.create).toHaveBeenCalledWith('1', { name: 'Chapelle', website: `https://${site}` }); + + expect((await host.dispatch(req('places.update', { tripId: 1, placeId: 7, input: { website: '//www.example.fr' } }), 42)).ok).toBe(true); + expect(f.places.update).toHaveBeenCalledWith('1', '7', { website: 'https://www.example.fr' }); + + const refused = (await host.dispatch(req('places.create', { tripId: 1, input: { name: 'X', website: 'javascript:alert(1)' } }), 42)) as RpcError; + expect(refused.error.code).toBe('BAD_PARAMS'); + expect(refused.error.message).toBe('invalid place: website must be an http or https URL'); + expect(f.places.create).toHaveBeenCalledTimes(1); + }); }); diff --git a/server/tests/unit/plugins/plugin-search.controller.test.ts b/server/tests/unit/plugins/plugin-search.controller.test.ts index 01daa0c01..106f7641e 100644 --- a/server/tests/unit/plugins/plugin-search.controller.test.ts +++ b/server/tests/unit/plugins/plugin-search.controller.test.ts @@ -152,6 +152,19 @@ describe('PluginSearchController', () => { expect(places[2].rating).toBeNull(); }); + // #2483: a plugin index is one more source of websites typed without a scheme. + it('PLUGIN-SEARCH-2483-01: a website without a scheme gains https like one from the core search', async () => { + const { c } = controller({ + searchPlaces: vi.fn(async () => [ + hit({ id: 'a', website: 'www.ristorante.example/menu' }), + hit({ id: 'b', website: '//ristorante.example' }), + hit({ id: 'c', website: 'mailto:info@ristorante.example' }), + ]) as unknown as PluginHooks['searchPlaces'], + }); + const { places } = await c.search('milan', undefined, undefined, undefined, undefined, req(5)); + expect(places.map((p) => p.website)).toEqual(['https://www.ristorante.example/menu', 'https://ristorante.example', null]); + }); + it('caps one provider at 20 hits', async () => { const { c } = controller({ searchPlaces: vi.fn(async () => diff --git a/shared/src/collection/collection-file.schema.spec.ts b/shared/src/collection/collection-file.schema.spec.ts index e466a587f..6c47972ed 100644 --- a/shared/src/collection/collection-file.schema.spec.ts +++ b/shared/src/collection/collection-file.schema.spec.ts @@ -42,3 +42,17 @@ describe('collectionGpxReadRequestSchema (#2301)', () => { expect(accepts({})).toBe(false); }); }); + +describe('collectionFilePlaceSchema website (#2483)', () => { + // The importer stores the parsed place, so the completed form is the one that + // lands; a link that is no page is still dropped rather than costing the place. + it('SHARED-COLLFILE-2483-01: a bare host arrives with https, a script link as null', () => { + const place = (website: unknown) => collectionFilePlaceSchema.parse({ name: 'Chapelle', website }); + expect(place('fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët').website).toBe( + 'https://fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët', + ); + expect(place('https://example.fr').website).toBe('https://example.fr'); + expect(place('javascript:alert(1)').website).toBeNull(); + expect(place('Chapelle').website).toBeNull(); + }); +}); diff --git a/shared/src/index.ts b/shared/src/index.ts index e02483cd1..a60190922 100644 --- a/shared/src/index.ts +++ b/shared/src/index.ts @@ -42,6 +42,7 @@ export * from './day/stay-bookends'; export * from './assignment/assignment.schema'; export * from './place/place.schema'; export * from './place/place-match'; +export * from './place/place-website'; export * from './roadtrip/roadtrip.schema'; export * from './place/track-colors'; export * from './collection/collection.schema'; diff --git a/shared/src/place/place-website.spec.ts b/shared/src/place/place-website.spec.ts new file mode 100644 index 000000000..042c346d7 --- /dev/null +++ b/shared/src/place/place-website.spec.ts @@ -0,0 +1,106 @@ +import { PLACE_WEBSITE_MAX_LENGTH, normalizePlaceWebsite } from './place-website'; + +import { describe, expect, it } from 'vitest'; + +describe('normalizePlaceWebsite (#2483)', () => { + it('SHARED-WEBSITE-001: the value from the issue gains https and keeps its accented path as written', () => { + expect(normalizePlaceWebsite('fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët')).toBe( + 'https://fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët', + ); + }); + + it('SHARED-WEBSITE-002: http and https stay exactly as they came, case included', () => { + expect(normalizePlaceWebsite('https://louvre.fr/en/visit')).toBe('https://louvre.fr/en/visit'); + expect(normalizePlaceWebsite('http://pension-alpenblick.at')).toBe('http://pension-alpenblick.at'); + expect(normalizePlaceWebsite('HTTPS://Example.COM/Pfad?q=1#top')).toBe('HTTPS://Example.COM/Pfad?q=1#top'); + expect(normalizePlaceWebsite('https://user@example.com/')).toBe('https://user@example.com/'); + }); + + it('SHARED-WEBSITE-003: surrounding whitespace is trimmed, empty and blank are nothing', () => { + expect(normalizePlaceWebsite(' www.example.com \n')).toBe('https://www.example.com'); + expect(normalizePlaceWebsite(' https://example.com ')).toBe('https://example.com'); + expect(normalizePlaceWebsite('')).toBeNull(); + expect(normalizePlaceWebsite(' ')).toBeNull(); + }); + + it('SHARED-WEBSITE-004: a host with a port is a host, not a scheme', () => { + expect(normalizePlaceWebsite('example.com:8080/x')).toBe('https://example.com:8080/x'); + expect(normalizePlaceWebsite('www.example.com:443')).toBe('https://www.example.com:443'); + }); + + it('SHARED-WEBSITE-005: a protocol-relative address gains https:', () => { + expect(normalizePlaceWebsite('//www.example.fr/patrimoine')).toBe('https://www.example.fr/patrimoine'); + expect(normalizePlaceWebsite('//')).toBeNull(); + expect(normalizePlaceWebsite('//Chapelle')).toBeNull(); + }); + + it('SHARED-WEBSITE-006: every other scheme is dropped, a script one in any spelling included', () => { + for (const value of [ + 'javascript:alert(1)', + 'JavaScript:alert(1)', + ' javascript:alert(1)', + 'java\tscript:alert(1)', + 'javascript:80', + 'data:text/html,', + 'vbscript:msgbox(1)', + 'mailto:mairie@example.fr', + 'tel:+33297230000', + 'ftp://example.com/file', + 'file:///etc/passwd', + 'http:example.com', + ]) { + expect(normalizePlaceWebsite(value), value).toBeNull(); + } + }); + + it('SHARED-WEBSITE-007: text that is no address is dropped rather than linked', () => { + for (const value of [ + 'Chapelle', + 'localhost', + 'localhost:3000', + '3.5', + 'call the hotel', + 'exa mple.com', + 'mairie@example.fr', + '-example.com', + 'example-.com', + 'example.com.', + 'https://', + 'https://localhost:3000', + 'https://exa mple.com', + ]) { + expect(normalizePlaceWebsite(value), value).toBeNull(); + } + }); + + it('SHARED-WEBSITE-008: an IPv4 address is a host, a number that only looks like one is not', () => { + expect(normalizePlaceWebsite('192.168.1.10/admin')).toBe('https://192.168.1.10/admin'); + expect(normalizePlaceWebsite('http://203.0.113.7')).toBe('http://203.0.113.7'); + expect(normalizePlaceWebsite('256.1.1.1')).toBeNull(); + expect(normalizePlaceWebsite('1.2.3')).toBeNull(); + }); + + it('SHARED-WEBSITE-009: an internationalised host keeps its letters', () => { + expect(normalizePlaceWebsite('münchen.de/rathaus')).toBe('https://münchen.de/rathaus'); + expect(normalizePlaceWebsite('www.北京.cn')).toBe('https://www.北京.cn'); + }); + + it('SHARED-WEBSITE-010: a query or fragment right after the host still counts as a host', () => { + expect(normalizePlaceWebsite('example.com?lang=de')).toBe('https://example.com?lang=de'); + expect(normalizePlaceWebsite('example.com#kontakt')).toBe('https://example.com#kontakt'); + }); + + it('SHARED-WEBSITE-011: a value the contract could not store is dropped rather than cut', () => { + const path = 'a'.repeat(PLACE_WEBSITE_MAX_LENGTH); + expect(normalizePlaceWebsite(`example.com/${path}`)).toBeNull(); + const fits = `example.com/${'a'.repeat(PLACE_WEBSITE_MAX_LENGTH - 'https://example.com/'.length)}`; + expect(normalizePlaceWebsite(fits)).toHaveLength(PLACE_WEBSITE_MAX_LENGTH); + }); + + it('SHARED-WEBSITE-012: anything but a string is nothing', () => { + expect(normalizePlaceWebsite(null)).toBeNull(); + expect(normalizePlaceWebsite(undefined)).toBeNull(); + expect(normalizePlaceWebsite(42)).toBeNull(); + expect(normalizePlaceWebsite({ href: 'https://example.com' })).toBeNull(); + }); +}); diff --git a/shared/src/place/place-website.ts b/shared/src/place/place-website.ts new file mode 100644 index 000000000..e2ae2ad87 --- /dev/null +++ b/shared/src/place/place-website.ts @@ -0,0 +1,84 @@ +import { z } from 'zod'; + +/** + * A place's homepage as a source hands it over, made into an address a browser + * opens as a page, or nothing. + * + * The place index, OpenStreetMap and Amap carry websites the way people type + * them into a form: `fr.wikipedia.org/wiki/…`, `www.hotel.cn`, `//example.com`. + * The write contract wants http(s), so a place picked from search used to be + * refused on save over a field the dialog does not even show (#2483). Every + * path that brings a website in from outside goes through here, and a value + * either arrives with its scheme or does not arrive at all. + * + * Only the scheme is added. The rest of the text stays as it came, so an + * accented path is not percent-encoded on the way through. + */ + +/** The cap the write contract puts on the field; a longer value could not be saved. */ +export const PLACE_WEBSITE_MAX_LENGTH = 500; + +const HTTP_URL = /^https?:\/\//i; +// A host and an optional port. The port is what tells `example.com:8080/x` +// apart from a scheme such as `mailto:` or `javascript:`, which never match. +const HOST_AND_PORT = /^([^:/?#@\s]+)(?::\d{1,5})?$/; +const LABEL = /^[\p{L}\p{N}\p{M}_-]+$/u; +const OCTET = /^\d{1,3}$/; +const LETTER = /\p{L}/u; +const openable = z.url({ protocol: /^https?$/ }); + +/** Everything up to the path, the query or the fragment. */ +function authorityOf(rest: string): string { + const end = rest.search(/[/?#]/); + return end === -1 ? rest : rest.slice(0, end); +} + +/** + * A name with at least one dot and a top-level label that has a letter in it, + * or an IPv4 address. "Chapelle", "localhost" and "3.5" are not websites. + */ +function isHostName(host: string): boolean { + const labels = host.split('.'); + if (labels.length < 2) return false; + if (labels.every((label) => OCTET.test(label))) { + return labels.length === 4 && labels.every((label) => Number(label) <= 255); + } + const valid = labels.every((label) => LABEL.test(label) && !label.startsWith('-') && !label.endsWith('-')); + return valid && LETTER.test(labels[labels.length - 1] ?? ''); +} + +function hasHost(authority: string): boolean { + const host = HOST_AND_PORT.exec(authority)?.[1]; + return host !== undefined && isHostName(host); +} + +/** + * The website a place gets from `value`, or null. + * + * Trimmed; http and https stay as they are; a bare host or a protocol-relative + * `//host` gains https. Any other scheme (`javascript:`, `data:`, `mailto:`, + * `ftp:`), free text, a host without a dot and anything a browser cannot + * parse come back as null, as does a value longer than the contract allows. + */ +export function normalizePlaceWebsite(value: unknown): string | null { + if (typeof value !== 'string') return null; + const text = value.trim(); + if (!text) return null; + + let url: string; + if (HTTP_URL.test(text)) { + const authority = authorityOf(text.replace(HTTP_URL, '')); + // Credentials in front of the host are not part of the name being checked. + if (!hasHost(authority.slice(authority.lastIndexOf('@') + 1))) return null; + url = text; + } else if (text.startsWith('//')) { + if (!hasHost(authorityOf(text.slice(2)))) return null; + url = `https:${text}`; + } else { + if (!hasHost(authorityOf(text))) return null; + url = `https://${text}`; + } + + if (url.length > PLACE_WEBSITE_MAX_LENGTH) return null; + return openable.safeParse(url).success ? url : null; +} diff --git a/shared/src/place/place.schema.spec.ts b/shared/src/place/place.schema.spec.ts index 91b8bff65..674694ce2 100644 --- a/shared/src/place/place.schema.spec.ts +++ b/shared/src/place/place.schema.spec.ts @@ -4,9 +4,11 @@ import { placeImportListRequestSchema, placeSchema, placeUpdateRequestSchema, + placeWebsiteSchema, } from './place.schema'; import { describe, it, expect } from 'vitest'; +import { z } from 'zod'; describe('placeSchema route_color (#776)', () => { const place = { id: 1, trip_id: 1, name: 'Walk' }; @@ -77,3 +79,52 @@ describe('stop_type on the write routes', () => { expect(placeCreateRequestSchema.safeParse(body).success).toBe(true); }); }); + +describe('placeWebsiteSchema (#2483)', () => { + it('SHARED-PLACE-030: a bare host from a search result parses to its https form', () => { + const parsed = placeWebsiteSchema.safeParse('fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët'); + expect(parsed.success && parsed.data).toBe('https://fr.wikipedia.org/wiki/Chapelle_Sainte-Barbe_du_Faouët'); + const relative = placeWebsiteSchema.safeParse('//www.example.fr'); + expect(relative.success && relative.data).toBe('https://www.example.fr'); + }); + + it('SHARED-PLACE-031: a value that names http(s) comes back exactly as sent', () => { + for (const value of [ + 'https://louvre.fr/en/visit', + 'http://pension-alpenblick.at', + 'https://x.example ', + 'http://localhost:3000', + ]) { + const parsed = placeWebsiteSchema.safeParse(value); + expect(parsed.success && parsed.data, value).toBe(value); + } + }); + + it('SHARED-PLACE-032: every other scheme and free text still fail with the same message', () => { + for (const value of [ + 'javascript:alert(1)', + 'data:text/html,x', + 'mailto:a@example.fr', + 'Chapelle', + '', + 'localhost', + ]) { + const parsed = placeWebsiteSchema.safeParse(value); + expect(parsed.success, value).toBe(false); + expect(parsed.error?.issues.map((i) => i.message)).toContain('must be an http or https URL'); + } + }); + + it('SHARED-PLACE-033: the length cap applies to what is stored', () => { + expect(placeWebsiteSchema.safeParse(`https://example.com/${'a'.repeat(480)}`).success).toBe(true); + expect(placeWebsiteSchema.safeParse(`https://example.com/${'a'.repeat(481)}`).success).toBe(false); + // Measured after the scheme is added, which is the value that is stored. + expect(placeWebsiteSchema.safeParse(`example.com/${'a'.repeat(480)}`).success).toBe(true); + expect(placeWebsiteSchema.safeParse(`example.com/${'a'.repeat(481)}`).success).toBe(false); + }); + + it('SHARED-PLACE-034: the tool schema an MCP client reads is still a plain capped string', () => { + const shape = z.toJSONSchema(z.object({ website: placeWebsiteSchema.optional() }), { io: 'input' }); + expect(shape.properties?.website).toEqual({ type: 'string', maxLength: 500 }); + }); +}); diff --git a/shared/src/place/place.schema.ts b/shared/src/place/place.schema.ts index e202daeb6..1745221ef 100644 --- a/shared/src/place/place.schema.ts +++ b/shared/src/place/place.schema.ts @@ -1,4 +1,5 @@ import { tagSchema } from '../tag/tag.schema'; +import { PLACE_WEBSITE_MAX_LENGTH, normalizePlaceWebsite } from './place-website'; import { z } from 'zod'; @@ -41,14 +42,23 @@ export const placeImageUrlSchema = z { message: 'must be an uploaded path, a photo-proxy path, an inline image or an https URL' }, ); +const HTTP_URL = /^https?:\/\//i; + /** * A place's homepage. It reaches window.open() on the client, where a * javascript: value would run in this origin rather than opening a page. + * + * A bare host is completed to https before the check instead of refused + * (#2483): search results carry websites that way, and an older client, a + * cached offline result or an MCP agent must not be stopped by one. A value + * that already names http(s) is kept exactly as sent, and every other scheme + * still fails. Read the parsed value, not the input: that is the one to store. */ export const placeWebsiteSchema = z .string() - .max(500) - .refine((v) => /^https?:\/\//i.test(v), { message: 'must be an http or https URL' }); + .overwrite((v) => (HTTP_URL.test(v) ? v : (normalizePlaceWebsite(v) ?? v))) + .max(PLACE_WEBSITE_MAX_LENGTH) + .refine((v) => HTTP_URL.test(v), { message: 'must be an http or https URL' }); /** * Embedded category as returned on a place — a trimmed projection of the