From ddf394dfd11bd2134043ba96b7045734139cff93 Mon Sep 17 00:00:00 2001 From: Rin <58572875+TurtIeSocks@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:12:31 -0400 Subject: [PATCH] fix(geocoder): stop a provider mismatch from crashing the server A webhook pointing nominatimUrl at Photon without setting geocoderProvider took down the process rather than failing the request. node-geocoder decides how to read a response by its shape: an array is a result list, anything else is a single result. Photon answers with a GeoJSON object, so the entire FeatureCollection was handed to _formatResult as though it were one place. node-geocoder 4.4.1 guards its own address lookup and returns undefined fields, but the patch ReactMap layers on top did not, so result.address.suburb threw. That throw never reached geocoder()'s catch. node-geocoder resolves through bluebird's asCallback, so a throw inside _formatResult surfaces as an uncaught exception and kills the process. The try/catch reads as though every failure returns {}, and this one could not be caught there at all. Three changes. The patch now optional-chains the address, so a response without one cannot throw. nominatimGeocoder awaits its results and rejects with a message naming the fix when the body is a GeoJSON FeatureCollection, which rejects normally and is caught. photonGeocoder does the mirror check for a JSON array, so the opposite mismatch reports itself instead of returning an empty result set with no reason. Four tests drive geocoder() over a real HTTP server for both mismatches and both matched pairs. Removing the optional chaining hangs the runner rather than failing it, which is the same fatality seen in production. --- server/src/services/geocoder.js | 42 ++++++++-- server/src/services/photonGeocoder.js | 8 ++ server/test/geocoder.test.js | 106 ++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 5 deletions(-) diff --git a/server/src/services/geocoder.js b/server/src/services/geocoder.js index 94e32a114..1f431d162 100644 --- a/server/src/services/geocoder.js +++ b/server/src/services/geocoder.js @@ -20,6 +20,31 @@ function formatter(addressFormat, result) { .trim() } +/** + * Fails loudly when the configured URL answered with something that is not a + * Nominatim response. + * + * node-geocoder decides how to read a body by its shape: an array is treated as + * a result list, and anything else is treated as a single result. Photon + * answers with a GeoJSON object, so the whole FeatureCollection gets handed to + * _formatResult as though it were one place. Nothing throws, because + * node-geocoder guards the address lookup, so the caller silently receives one + * entry with every field undefined. + * + * That is a misconfiguration rather than a geocoding failure, and it is + * invisible in the results, so it is worth an error naming the fix. + * @param {any} results + * @param {string} url + */ +function assertNominatimResponse(results, url) { + const raw = results?.raw + if (raw && !Array.isArray(raw) && raw.type === 'FeatureCollection') { + throw new Error( + `${url} answered with GeoJSON, which is Photon's format rather than Nominatim's. Set "geocoderProvider": "photon" on this webhook, or point the URL at a Nominatim instance.`, + ) + } +} + /** * Nominatim, via node-geocoder's `openstreetmap` provider. * @param {string} url @@ -34,13 +59,20 @@ async function nominatimGeocoder(url, search, isReverse) { }) stockGeocoder._geocoder._formatResult = ((original) => (result) => ({ ...original(result), - suburb: result.address.suburb || '', - town: result.address.town || '', - village: result.address.village || '', + suburb: result.address?.suburb || '', + town: result.address?.town || '', + village: result.address?.village || '', }))(stockGeocoder._geocoder._formatResult) - return isReverse && typeof search === 'object' + // Awaited rather than returned so the shape check runs here. A throw inside + // _formatResult would not reach geocoder()'s catch at all: node-geocoder + // resolves through bluebird's asCallback, so it surfaces as an uncaught + // exception and takes the process down. Anything thrown from this function + // rejects normally and is caught. + const results = await (isReverse && typeof search === 'object' ? stockGeocoder.reverse(search) - : stockGeocoder.geocode(String(search)) + : stockGeocoder.geocode(String(search))) + assertNominatimResponse(results, url) + return results } /** diff --git a/server/src/services/photonGeocoder.js b/server/src/services/photonGeocoder.js index 2759ce9e4..db368cc9f 100644 --- a/server/src/services/photonGeocoder.js +++ b/server/src/services/photonGeocoder.js @@ -257,6 +257,14 @@ async function photonGeocoder(photonUrl, search, isReverse) { }) const response = await fetchJson(url) + // The mirror of the Nominatim check: a JSON array is Nominatim's search + // shape, so the URL and the provider disagree. Without this the caller just + // gets an empty result set and no reason for it. + if (Array.isArray(response)) { + throw new Error( + `${photonUrl} answered with a JSON array, which is Nominatim's format rather than Photon's. Remove "geocoderProvider": "photon" from this webhook, or point the URL at a Photon instance.`, + ) + } // fetchJson answers a failed request with the Response rather than throwing, // so an absent features array covers both a network failure and an empty // result set. diff --git a/server/test/geocoder.test.js b/server/test/geocoder.test.js index 797093151..0e2aeca99 100644 --- a/server/test/geocoder.test.js +++ b/server/test/geocoder.test.js @@ -3,7 +3,10 @@ const { test } = require('node:test') const NodeGeocoder = require('node-geocoder') +const http = require('node:http') + const { PoracleAPI } = require('../src/services/Poracle') +const { geocoder } = require('../src/services/geocoder') const { formatPhotonFeature, joinComponents, @@ -420,3 +423,106 @@ test('PoracleAPI leaves the provider undefined when it is not configured', () => assert.equal(api.geocoderProvider, undefined) assert.equal(api.nominatimUrl, 'http://127.0.0.1:2322') }) + +// A misconfigured webhook -- a Photon URL left on the Nominatim provider, or the +// reverse -- used to reach node-geocoder, which reads a GeoJSON object as a +// single result and hands the whole FeatureCollection to _formatResult. The +// unguarded address lookup in ReactMap's patch then threw, and because +// node-geocoder resolves through bluebird's asCallback the throw never reached +// geocoder()'s catch: it surfaced as an uncaught exception and killed the +// process. +const serveOnce = async (body) => { + const server = http.createServer((_, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }) + res.end(typeof body === 'string' ? body : JSON.stringify(body)) + }) + await new Promise((resolve) => { + server.listen(0, '127.0.0.1', resolve) + }) + return { + url: `http://127.0.0.1:${server.address().port}`, + close: () => + new Promise((resolve) => { + server.close(resolve) + }), + } +} + +const PHOTON_BODY = { + type: 'FeatureCollection', + features: [ + { + geometry: { type: 'Point', coordinates: [-104.9903, 39.7392] }, + properties: { name: 'Denver', type: 'city', countrycode: 'US' }, + }, + ], +} + +const NOMINATIM_BODY = [ + { + lat: '39.7392', + lon: '-104.9903', + display_name: 'Denver, Colorado, United States', + address: { city: 'Denver', state: 'Colorado', country_code: 'us' }, + }, +] + +test('a Photon URL on the Nominatim provider fails without crashing', async () => { + const server = await serveOnce(PHOTON_BODY) + try { + // geocoder() catches and returns {}. What matters is that the process + // survives to get here at all. + const result = await geocoder(server.url, 'Denver', false, '{{city}}') + assert.deepEqual(result, {}) + } finally { + await server.close() + } +}) + +test('a Nominatim URL on the Photon provider fails without returning nothing silently', async () => { + const server = await serveOnce(NOMINATIM_BODY) + try { + const result = await geocoder( + server.url, + 'Denver', + false, + '{{city}}', + 'photon', + ) + assert.deepEqual(result, {}) + } finally { + await server.close() + } +}) + +// The matched pairs still work, so the checks above are not rejecting valid +// responses. +test('a correctly configured Photon webhook still geocodes', async () => { + const server = await serveOnce(PHOTON_BODY) + try { + const result = await geocoder( + server.url, + 'Denver', + false, + '{{city}}', + 'photon', + ) + assert.deepEqual(result, [ + { formatted: 'Denver', latitude: 39.7392, longitude: -104.9903 }, + ]) + } finally { + await server.close() + } +}) + +test('a correctly configured Nominatim webhook still geocodes', async () => { + const server = await serveOnce(NOMINATIM_BODY) + try { + const result = await geocoder(server.url, 'Denver', false, '{{city}}') + assert.deepEqual(result, [ + { formatted: 'Denver', latitude: 39.7392, longitude: -104.9903 }, + ]) + } finally { + await server.close() + } +})