From c8cb88133f72d1bb94b4414f8db598af9da2d994 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 13 Sep 2026 23:01:36 +0200 Subject: [PATCH] fix(maps): the projection, the labels and two unbounded reads A map keeps the datum it was built with, and Leaflet cannot change one afterwards. The basemap could still turn into Amap's after the fact: a first settings request that fails leaves the store on defaults for the session and is healed later, and the raster layer then drew GCJ-02 tiles under a map still projecting WGS-84, putting every marker, route and cluster a few hundred metres off the street beneath it until the planner was left and reopened. The datum is part of the element's identity now, so a flip rebuilds the map. Only the datum: keying on the whole tile URL would throw the view away on every template edit. Index POIs were labelled by an exact lookup of a term the index matches as a substring, so any leaf that is not literally a term missed and took the first category of the request instead. In a corridor search that is whichever pill was tapped first, and the panel groups and colours on that field: a trattoria came back as a petrol station, or a petrol station as tonight's hotel. The label now comes from the term that actually matched, longest first. Two outbound reads were missing what the house rule asks for. The brand-logo fetch had no deadline at all next to a Wikidata hop that allows six seconds, so a stalled CDN held a request context and a socket for undici's five-minute default, and its size cap only applied after the bytes were in memory. The Wikimedia photo proxy beside it had the same shape. And the Amap provider buffered every answer uncapped and trusted a cast for its arrays, so a `pois` object rather than a list threw a TypeError that surfaced as a 500 instead of the OpenStreetMap answer. --- client/src/components/Map/MapView.tsx | 9 +++ server/src/nest/maps/maps.service.ts | 56 +++++++++++++++---- .../src/nest/maps/providers/amap.provider.ts | 29 +++++++++- server/tests/unit/nest/maps.amap.test.ts | 23 ++++++++ server/tests/unit/nest/maps.pois.test.ts | 33 +++++++++++ 5 files changed, 137 insertions(+), 13 deletions(-) diff --git a/client/src/components/Map/MapView.tsx b/client/src/components/Map/MapView.tsx index 5b9e38ece..96e0b9412 100644 --- a/client/src/components/Map/MapView.tsx +++ b/client/src/components/Map/MapView.tsx @@ -1006,6 +1006,15 @@ export const MapView = memo(function MapView({ <>
BRAND_LOGO_MAX_BYTES) return remember(null); - const bytes = Buffer.from(await imgRes.arrayBuffer()); - // Checked again after reading: a chunked response has no length to check first. - if (bytes.byteLength === 0 || bytes.byteLength > BRAND_LOGO_MAX_BYTES) return remember(null); + if (exceedsDeclaredLength(imgRes, BRAND_LOGO_MAX_BYTES)) { + discardBody(imgRes); + return remember(null); + } + // Streamed rather than buffered whole: a chunked answer declares no length, + // so the post-check only ever ran after the bytes were already in memory. + const { bytes, truncated } = await readCapped(imgRes, BRAND_LOGO_MAX_BYTES); + if (truncated || bytes.byteLength === 0) return remember(null); const contentType = imgRes.headers.get('content-type') ?? ''; if (!contentType.startsWith('image/')) return remember(null); @@ -872,6 +880,23 @@ export class MapsService { // by Overpass, and a mixed answer would silently drop it. const indexKnowsAll = wanted.length > 0 && wanted.every(key => POI_CATEGORY_TO_TREK[key]?.length); const terms = [...categoryOfTerm.keys()]; + // The index matches a term as a SUBSTRING of `category` and `category_path` + // (see POI_CATEGORY_TO_TREK), so an exact lookup misses every leaf that is + // not literally a term: `italian_restaurant` is a hit for `restaurant` and + // finds nothing here. Falling through to wanted[0] then labelled it with + // whichever pill the user happened to tap first, and the corridor panel + // groups and colours on exactly that field, so a trattoria came back as a + // petrol station. Longest match wins, because `fast_food` must not lose to + // `food` when both are terms of different categories. + const labelFor = (leaf: string | null, path: string | null): string | undefined => { + const haystack = `${leaf ?? ''} ${path ?? ''}`; + let best: string | undefined; + for (const [term, key] of categoryOfTerm) { + if (!haystack.includes(term)) continue; + if (best === undefined || term.length > best.length) best = term; + } + return best === undefined ? undefined : categoryOfTerm.get(best); + }; if (this.trekPlacesEnabled() && indexKnowsAll) { try { const lat = (bbox.south + bbox.north) / 2; @@ -916,7 +941,7 @@ export class MapsService { // The category that produced the hit, not the list that was // asked for: a mixed search must not label a petrol station as // "fuel,charging,restaurant". - category: categoryOfTerm.get(p.category ?? '') ?? wanted[0], + 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, @@ -2702,12 +2727,23 @@ export class MapsService { if (!wiki) return null; // Follow redirects manually so each hop (the image URL can 3xx to a CDN // host) is re-validated against the SSRF guard, not just the first URL. - const imgRes = await safeFetchFollow(wiki.photoUrl, undefined, { bypassInternalIpAllowed: true }); + const imgRes = await safeFetchFollow( + wiki.photoUrl, { signal: AbortSignal.timeout(WIKI_TIMEOUT_MS) }, { bypassInternalIpAllowed: true }, + ); if (!imgRes.ok) { providerFailed = true; return null; } - const bytes = Buffer.from(await imgRes.arrayBuffer()); + if (exceedsDeclaredLength(imgRes, WIKIMEDIA_PHOTO_MAX_BYTES)) { + discardBody(imgRes); + providerFailed = true; + return null; + } + const { bytes, truncated } = await readCapped(imgRes, WIKIMEDIA_PHOTO_MAX_BYTES); + if (truncated || bytes.byteLength === 0) { + providerFailed = true; + return null; + } const cached = await this.photoCache.put(placeId, bytes, wiki.attribution); return { attribution: cached.attribution }; } catch { diff --git a/server/src/nest/maps/providers/amap.provider.ts b/server/src/nest/maps/providers/amap.provider.ts index 9e799105c..83dca9f13 100644 --- a/server/src/nest/maps/providers/amap.provider.ts +++ b/server/src/nest/maps/providers/amap.provider.ts @@ -32,6 +32,7 @@ import { createHash } from 'node:crypto'; import { fromAmapLocation, gcj02ToWgs84, toAmapLocation } from '@trek/shared'; import { readEnv } from '../../../app-config'; import { safeFetchFollow } from '../../../utils/ssrfGuard'; +import { discardBody, exceedsDeclaredLength, readCappedJson } from '../../../utils/cappedFetch'; import { UA, parseOpeningHours } from '../maps.helpers'; import type { PlacesProvider, @@ -44,6 +45,13 @@ import type { /** The upstream every Web Service call is written against. */ const AMAP_UPSTREAM = 'https://restapi.amap.com'; +/** A search answer is a few dozen POIs; anything past this is not the endpoint we think it is. */ +const AMAP_MAX_RESPONSE_BYTES = 1_000_000; + +/** An Amap list field, or nothing. The envelope is checked; its arrays never were. */ +function asArray(value: T[] | undefined | null): T[] { + return Array.isArray(value) ? value : []; +} /** * The prefix that makes an Amap POI id recognisable anywhere in TREK. @@ -226,7 +234,18 @@ export class AmapPlacesProvider implements PlacesProvider { this.fail(label, response.status, `Amap ${label} failed with HTTP ${response.status}`); } - const data = (await response.json()) as T; + // Capped, like the places index client next door: the base URL is + // configurable (AMAP_API_BASE points at an operator's own gateway), and an + // answer of arbitrary size was buffered into the heap in full before + // anything looked at it. + if (exceedsDeclaredLength(response, AMAP_MAX_RESPONSE_BYTES)) { + discardBody(response); + this.fail(label, 502, `Amap ${label} answered with more than ${AMAP_MAX_RESPONSE_BYTES} bytes`); + } + const data = await readCappedJson(response, AMAP_MAX_RESPONSE_BYTES); + if (!data || typeof data !== 'object') { + this.fail(label, 502, `Amap ${label} answered with something that is not a JSON object`); + } if (data.status !== '1') { const infocode = data.infocode ?? ''; const hint = AMAP_INFOCODE_HINTS[infocode] || data.info || 'Amap API error'; @@ -320,7 +339,11 @@ export class AmapPlacesProvider implements PlacesProvider { ) : await this.call('/v3/place/text', common, 'place/text'); - return (data.pois ?? []).map((poi) => this.toPlace(poi)); + // The envelope is validated, its arrays are not: `as T` only ever said what + // the answer was meant to look like. A `pois` object rather than an array + // threw a TypeError nobody caught between here and the controller, so the + // user got a 500 instead of the OpenStreetMap answer. + return asArray(data.pois).map((poi) => this.toPlace(poi)); } // Amap does not bill per keystroke and has no session concept, so the token @@ -377,7 +400,7 @@ export class AmapPlacesProvider implements PlacesProvider { `place/detail(${poiId})`, ); - const poi = data.pois?.[0]; + const poi = asArray(data.pois)[0]; return poi ? { ...this.toPlace(poi), cached_at: Date.now() } : null; } diff --git a/server/tests/unit/nest/maps.amap.test.ts b/server/tests/unit/nest/maps.amap.test.ts index dc3a26664..0a2f9f172 100644 --- a/server/tests/unit/nest/maps.amap.test.ts +++ b/server/tests/unit/nest/maps.amap.test.ts @@ -256,6 +256,29 @@ describe('AmapPlacesProvider.searchText', () => { expect(calledUrl()).toContain('/v3/place/text'); }); + it('AMAP-015b: a pois field that is not a list answers empty instead of throwing a 500', async () => { + // `as T` only ever described what the answer was meant to look like. The + // base URL is configurable, and an object here used to reach `.map` and + // throw a TypeError nobody caught between the provider and the controller. + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(ok({ pois: { id: 'B3' } }))); + await expect(provider().searchText('外滩')).resolves.toEqual([]); + }); + + it('AMAP-015c: a body larger than the cap is refused before it is read', async () => { + const cancel = vi.fn().mockResolvedValue(undefined); + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ + ok: true, + status: 200, + headers: { get: (n: string) => (n === 'content-length' ? String(50 * 1024 * 1024) : null) }, + body: { getReader: () => ({ read: async () => ({ done: true }), cancel }), cancel }, + json: async () => ({ status: '1', pois: [] }), + })); + + await expect(provider().searchText('外滩')).rejects.toThrow(/more than/); + // The socket goes back rather than staying pinned on a body nobody reads. + expect(cancel).toHaveBeenCalled(); + }); + it('AMAP-016: keeps a POI that has no geometry rather than dropping it', async () => { vi.stubGlobal('fetch', vi.fn().mockResolvedValue(ok({ pois: [{ id: 'B3', name: '无坐标', location: [] }] }))); const [place] = await provider().searchText('无坐标'); diff --git a/server/tests/unit/nest/maps.pois.test.ts b/server/tests/unit/nest/maps.pois.test.ts index 1be08ddb4..80ffecb24 100644 --- a/server/tests/unit/nest/maps.pois.test.ts +++ b/server/tests/unit/nest/maps.pois.test.ts @@ -283,6 +283,39 @@ describe('MapsService.pois answered from the index', () => { expect(out.pois.map(p => p.category)).toEqual(['fuel', 'charging']); }); + it('MAPS-POIS-013b: a leaf category that is only a substring of a term is still labelled by it', async () => { + // The index matches a term inside `category` and `category_path`, so most + // real leaves are not literally a term. Looked up exactly, they missed and + // took the first category of the request instead — whichever pill the user + // tapped first — and the corridor panel groups and colours on that field. + mockNearby.mockResolvedValue([ + { ...FULL, gers: 'r-1', name: 'Trattoria', category: 'italian_restaurant', categoryPath: 'eat_and_drink>restaurant>italian_restaurant' }, + { ...FULL, gers: 'f-1', name: 'Aral', category: 'gas_station', categoryPath: 'automotive>gas_station' }, + ]); + const svc = make(); + stubOverpass(svc); + + const out = await svc.pois('fuel,restaurant', BOX); + + // Fuel was tapped first, so the old fallback made the trattoria a petrol + // station: orange pin, listed under Fuel. + expect(out.pois.map(p => [p.name, p.category])).toEqual([['Trattoria', 'restaurant'], ['Aral', 'fuel']]); + // The true leaf is still reported, unchanged. + expect(out.pois[0].poi_type).toBe('italian_restaurant'); + }); + + it('MAPS-POIS-013c: the longest matching term wins, so fast_food does not answer as a cafe', async () => { + mockNearby.mockResolvedValue([ + { ...FULL, gers: 'q-1', name: 'Imbiss', category: 'fast_food_restaurant', categoryPath: 'eat_and_drink>fast_food>fast_food_restaurant' }, + ]); + const svc = make(); + stubOverpass(svc); + + const out = await svc.pois('cafe,restaurant', BOX); + + expect(out.pois[0].category).toBe('restaurant'); + }); + it('MAPS-POIS-014: a category the index has no terms for sends the whole query to Overpass', async () => { // All or nothing. Answering the half it knows would silently drop the rest, // and the caller counts a finished search either way.