diff --git a/server/src/nest/accommodations/accommodations.mcp.ts b/server/src/nest/accommodations/accommodations.mcp.ts index f5e5cb6e7..3a62f2e8a 100644 --- a/server/src/nest/accommodations/accommodations.mcp.ts +++ b/server/src/nest/accommodations/accommodations.mcp.ts @@ -93,8 +93,7 @@ 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'), - // The place contract create_place uses: a bare host gains https (#2483). - website: placeWebsiteSchema.optional(), + website: z.string().max(500).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'), @@ -124,9 +123,13 @@ export class AccommodationsMcp { if (!this.guards.hasTripPermission('day_edit', tripId, ctx.userId)) return permissionDenied(); const dayErrors = this.accommodations.validateAccommodationRefs(tripId, undefined, start_day_id, end_day_id); if (dayErrors.length > 0) return { content: [{ type: 'text' as const, text: dayErrors.map(e => e.message).join(', ') }], isError: true }; + // The website takes the place contract create_place uses, so a bare host + // gains https (#2483). A value that contract refuses is left off instead of + // failing the booking: this tool always took any text here, '' as none. + const site = website ? placeWebsiteSchema.safeParse(website) : null; try { const result = this.db.transaction(() => { - const place = this.places.create(String(tripId), { name, description, lat, lng, address, category_id, google_place_id, google_ftid, osm_id, notes: place_notes, website, phone, price, currency }); + const place = this.places.create(String(tripId), { name, description, lat, lng, address, category_id, google_place_id, google_ftid, osm_id, notes: place_notes, website: site?.success ? site.data : undefined, phone, price, currency }); const { accommodation, mirror } = this.accommodations.createAccommodation(tripId, { place_id: place.id, start_day_id, end_day_id, check_in, check_in_end, check_out, confirmation, notes: accommodation_notes }); return { place, accommodation, mirror }; }); diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 3be75cd29..2ceacafac 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -672,6 +672,17 @@ export const PLACES_PROVIDER_SETTING = 'places_provider'; */ export const PLACES_GOOGLE_ONLY_SETTING = 'places_google_only'; +/** + * A details row as the cache holds it. A row written before #2483 still has a + * source's website as it came, `www.hotel.cn` from Amap for one, and keeps for a + * week (an expanded one until a refresh), so the website is normalized on the + * way out of the cache as well as on the way in. + */ +function cachedDetails(payload: string): Record | null { + const place = JSON.parse(payload) as Record | null; + return place && 'website' in place ? { ...place, website: normalizePlaceWebsite(place.website) } : place; +} + /** * Whoever holds the keyed slot beside the index for one request: Google's * credential, an Amap provider, or nobody (the OpenStreetMap stack alone). @@ -2532,7 +2543,7 @@ export class MapsService { placeId, langKey, ); - if (cached && Date.now() - cached.fetched_at < DETAILS_TTL) return { place: JSON.parse(cached.payload_json) }; + if (cached && Date.now() - cached.fetched_at < DETAILS_TTL) return { place: cachedDetails(cached.payload_json) }; // Closes the autocomplete session this lookup belongs to, so Google bills // the search once instead of per keystroke. A cache hit above never reaches @@ -2625,7 +2636,7 @@ export class MapsService { placeId, langKey, ); - if (cached && Date.now() - cached.fetched_at < DETAILS_TTL) return { place: JSON.parse(cached.payload_json) }; + if (cached && Date.now() - cached.fetched_at < DETAILS_TTL) return { place: cachedDetails(cached.payload_json) }; const place = await provider.placeDetails(placeId, lang); if (!place) return { place: null }; @@ -2681,7 +2692,7 @@ export class MapsService { placeId, langKey, ); - if (cached) return { place: JSON.parse(cached.payload_json) }; + if (cached) return { place: cachedDetails(cached.payload_json) }; } const response = await googleFetch( diff --git a/server/tests/unit/mcp/tools-days-accommodations.test.ts b/server/tests/unit/mcp/tools-days-accommodations.test.ts index 65d01dee2..67534c877 100644 --- a/server/tests/unit/mcp/tools-days-accommodations.test.ts +++ b/server/tests/unit/mcp/tools-days-accommodations.test.ts @@ -337,7 +337,7 @@ 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 () => { + it('MCP-ACCOM-2483-01: a website without a scheme is stored with https', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const day = createDay(testDb, trip.id); @@ -347,14 +347,29 @@ describe('Tool: 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 }); + }); + + // The tool always took any text as the website. It still does, so no call that + // went through before fails now; a value the place contract refuses is left + // off rather than stored, and '' stays "no website" as it is on REST. + it('MCP-ACCOM-2483-02: a website that is empty, free text or a script link books the stay without one', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + await withHarness(user.id, async (h) => { + for (const [name, website] of [['Empty', ''], ['Words', 'ask at the front desk'], ['Hostile', 'javascript:alert(1)']]) { + const result = await h.client.callTool({ + name: 'create_place_accommodation', + arguments: { tripId: trip.id, name, start_day_id: day.id, end_day_id: day.id, website }, + }); + expect(result.isError, name).toBeFalsy(); + const data = parseToolResult(result) as { place: { id: number; website: unknown }; accommodation: { id: number } }; + expect(data.place.website, name).toBeNull(); + expect(data.accommodation.id, name).toBeGreaterThan(0); + expect(testDb.prepare('SELECT website FROM places WHERE id = ?').get(data.place.id), name).toEqual({ website: null }); + } + }); }); it('creates the place and the accommodation atomically and broadcasts both', async () => { diff --git a/server/tests/unit/nest/maps.amap.test.ts b/server/tests/unit/nest/maps.amap.test.ts index 7cbc7b138..03a1a13b4 100644 --- a/server/tests/unit/nest/maps.amap.test.ts +++ b/server/tests/unit/nest/maps.amap.test.ts @@ -760,6 +760,22 @@ describe('MapsService.keyedProvider', () => { expect(place!.source).toBe('amap'); expect(calledUrl()).not.toContain('overpass'); }); + + // #2483: a details row cached before the fix holds Amap's website as it came, + // often a bare host, and keeps for a week. + it('AMAP-079: a cached details row with a bare website is served with https', async () => { + mockProviderGet.mockReturnValue({ value: 'amap' }); + const row = { payload_json: JSON.stringify({ name: '旧酒店', website: 'www.hotel.cn', source: 'amap' }), fetched_at: Date.now() }; + // The cache lookup binds the place id, the key lookup the user id. + mockDbGet.mockImplementation((...args: unknown[]) => + args[0] === 'amap:B7' ? row : { maps_api_key: null, amap_api_key: 'akey' }, + ); + const fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock); + const { place } = await svc.getPlaceDetails(1, 'amap:B7'); + expect(place).toMatchObject({ name: '旧酒店', website: 'https://www.hotel.cn' }); + expect(fetchMock).not.toHaveBeenCalled(); + }); }); describe('MapsService with Amap in the keyed slot', () => { diff --git a/server/tests/unit/nest/maps.service.test.ts b/server/tests/unit/nest/maps.service.test.ts index df825a422..e5eca1d6a 100644 --- a/server/tests/unit/nest/maps.service.test.ts +++ b/server/tests/unit/nest/maps.service.test.ts @@ -3587,4 +3587,36 @@ describe('websites from the map sources (#2483)', () => { const place = (await svc.getPlaceDetailsExpanded(1, 'ChIJWeb-expanded')).place as { website: unknown }; expect(place.website).toBeNull(); }); + + // A details row cached before the fix still holds the website as the source + // sent it, and keeps for a week; an expanded row keeps until a refresh. The + // cache hands it out through the same helper. + const cachedRow = (place: Record) => ({ payload_json: JSON.stringify(place), fetched_at: Date.now() }); + + it('MAPS-2483-06: a cached details row with a bare website is served with https, without a Google call', async () => { + const fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock); + mockDbGet + .mockReturnValueOnce({ maps_api_key: 'gkey' }) + .mockReturnValueOnce(cachedRow({ google_place_id: 'ChIJOld', name: 'Chapelle', website: 'example.fr/visite' })); + const { place } = await svc.getPlaceDetails(1, 'ChIJOld'); + expect(place).toMatchObject({ name: 'Chapelle', website: 'https://example.fr/visite' }); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('MAPS-2483-07: so is an expanded row, where a script link becomes null and a row without the field stays as it is', async () => { + const fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock); + mockDbGet + .mockReturnValueOnce({ maps_api_key: 'gkey' }) + .mockReturnValueOnce(cachedRow({ google_place_id: 'ChIJOldX', website: '//www.example.fr', reviews: [] })) + .mockReturnValueOnce({ maps_api_key: 'gkey' }) + .mockReturnValueOnce(cachedRow({ google_place_id: 'ChIJOldY', website: 'javascript:alert(1)' })) + .mockReturnValueOnce({ maps_api_key: 'gkey' }) + .mockReturnValueOnce(cachedRow({ google_place_id: 'ChIJOldZ', name: 'No site' })); + expect((await svc.getPlaceDetailsExpanded(1, 'ChIJOldX')).place).toMatchObject({ website: 'https://www.example.fr', reviews: [] }); + expect((await svc.getPlaceDetailsExpanded(1, 'ChIJOldY')).place).toMatchObject({ website: null }); + expect((await svc.getPlaceDetailsExpanded(1, 'ChIJOldZ')).place).toEqual({ google_place_id: 'ChIJOldZ', name: 'No site' }); + expect(fetchMock).not.toHaveBeenCalled(); + }); }); diff --git a/server/tests/unit/plugins/plugin-search.controller.test.ts b/server/tests/unit/plugins/plugin-search.controller.test.ts index 106f7641e..0fbb1edb5 100644 --- a/server/tests/unit/plugins/plugin-search.controller.test.ts +++ b/server/tests/unit/plugins/plugin-search.controller.test.ts @@ -165,6 +165,17 @@ describe('PluginSearchController', () => { expect(places.map((p) => p.website)).toEqual(['https://www.ristorante.example/menu', 'https://ristorante.example', null]); }); + // A plugin on the same network may answer with an intranet address. The old + // check kept every http(s) url a browser opens, and so does the helper. + it('PLUGIN-SEARCH-2483-02: an http(s) website keeps its host whatever it looks like', async () => { + const urls = ['http://localhost:8080/poi/1', 'http://intranet/poi/2', 'https://[2001:db8::1]/poi/3']; + const { c } = controller({ + searchPlaces: vi.fn(async () => urls.map((website, i) => hit({ id: `h${i}`, website }))) as unknown as PluginHooks['searchPlaces'], + }); + const { places } = await c.search('poi', undefined, undefined, undefined, undefined, req(5)); + expect(places.map((p) => p.website)).toEqual(urls); + }); + it('caps one provider at 20 hits', async () => { const { c } = controller({ searchPlaces: vi.fn(async () => diff --git a/shared/src/place/place-website.spec.ts b/shared/src/place/place-website.spec.ts index 042c346d7..9209c4edb 100644 --- a/shared/src/place/place-website.spec.ts +++ b/shared/src/place/place-website.spec.ts @@ -1,4 +1,5 @@ import { PLACE_WEBSITE_MAX_LENGTH, normalizePlaceWebsite } from './place-website'; +import { placeWebsiteSchema } from './place.schema'; import { describe, expect, it } from 'vitest'; @@ -66,8 +67,8 @@ describe('normalizePlaceWebsite (#2483)', () => { 'example-.com', 'example.com.', 'https://', - 'https://localhost:3000', 'https://exa mple.com', + 'http://%zz/', ]) { expect(normalizePlaceWebsite(value), value).toBeNull(); } @@ -103,4 +104,38 @@ describe('normalizePlaceWebsite (#2483)', () => { expect(normalizePlaceWebsite(42)).toBeNull(); expect(normalizePlaceWebsite({ href: 'https://example.com' })).toBeNull(); }); + + // The dot is how a bare host is told from a word. A value that names http(s) + // has settled that already, so it is kept whenever a browser opens it, the + // way the write contract keeps it: a plugin on the same network may answer + // with an intranet address. + it('SHARED-WEBSITE-013: an http(s) value the contract stores is kept, whatever its host looks like', () => { + for (const value of [ + 'https://localhost:3000', + 'http://intranet/wiki', + 'https://[2001:db8::1]:8443/x', + 'http://[::1]/', + 'https://example.com.', + 'https://ex%41mple.com', + ]) { + expect(normalizePlaceWebsite(value), value).toBe(value); + expect(placeWebsiteSchema.safeParse(value).data, value).toBe(value); + } + }); + + it('SHARED-WEBSITE-014: a backslash never turns a value into another scheme or a bare host into a kept one', () => { + // A browser reads the backslash as a slash, so this opens localhost over + // https. It is kept like any other intranet address: the scheme is http(s). + expect(normalizePlaceWebsite('https://localhost\\@example.com')).toBe('https://localhost\\@example.com'); + for (const value of [ + 'https:\\\\evil.example', + '\\\\evil.example', + '//\\evil.example', + 'example.com\\@evil.example', + 'intranet\\@example.com', + 'javascript:1\\@example.com', + ]) { + expect(normalizePlaceWebsite(value), value).toBeNull(); + } + }); }); diff --git a/shared/src/place/place-website.ts b/shared/src/place/place-website.ts index e2ae2ad87..522f18b76 100644 --- a/shared/src/place/place-website.ts +++ b/shared/src/place/place-website.ts @@ -35,7 +35,8 @@ function authorityOf(rest: string): string { /** * 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. + * or an IPv4 address. Without a scheme, "Chapelle", "localhost" and "3.5" are + * not websites. */ function isHostName(host: string): boolean { const labels = host.split('.'); @@ -55,10 +56,13 @@ function hasHost(authority: string): boolean { /** * 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. + * Trimmed; http and https stay as they are whenever a browser can open them, + * an intranet host or an IPv6 literal included, which is what the write + * contract keeps as well. A bare host or a protocol-relative `//host` gains + * https, and only there does the host need a dot: it is what tells a site + * from a word. Any other scheme (`javascript:`, `data:`, `mailto:`, `ftp:`), + * free text 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; @@ -67,9 +71,6 @@ export function normalizePlaceWebsite(value: unknown): string | 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;