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.