openvibers/openvibe.food Commits

Food: only http(s) OpenStreetMap links become links; /near has the search budget; the geo cache is pruned hourly (#5)

OP OpenVibers committed 10 Oct 2026 · fd0cac2469 · 7 files, +172 −16

From an independent security review of Food.

- **HIGH:** an OpenStreetMap element's `website` became an `<a href>` with escaping only. Anyone can edit OSM, so a `javascript:` URL would have run on openvibe.food. URLs are now kept only when they parse as http(s), both at the source (`places.js` `safeUrl`) and again where rendered.
- **MEDIUM:** the HTML `/near` search didn't have the `food.place.search` budget that `/api/v1/places` uses. It does now.
- **LOW:** the OSM answer cache is pruned on an hourly timer, not only at boot.

Tests: `test/places.test.js` and `test/service-kit.test.js`. Suite: 18/18 files pass.

Changes

ddiff --git a/server/app.js b/server/app.js
iindex b03b6d0..d260776 100644
---- a/server/app.js
++++ b/server/app.js
@@@ -59,8 +59,9 @@ async function createApp(opts = {}) {
// it from here, so every test points it at a stand-in through FOOD_NOMINATIM_URL / FOOD_OVERPASS_URL.
// Its own wall clock, not the store's: the pace between two calls must not depend on an injected test clock.
ctx.upstream = createUpstream({ config, s, fetchImpl, log });
-- // Drop expired geocode/area answers; an expired row is ignored anyway, this only keeps the table small.
-- Promise.resolve(foodStore.cachePrune(s)).catch((err) => log.warn('[OpenVibe.Food] geo cache prune:', err && err.message));
++ // Drop expired geocode/area answers now and hourly; an expired row is ignored anyway, this only keeps the table
++ // small. The interval is unref'd and kept on ctx so the entry point clears it on shutdown (server/index.js).
++ ctx.geoPruneTimer = foodStore.startCachePrune(s, { log });
// Account export and deletion (ADR-033, ./identity/account-data.js): the two tables that hold a person's rows.
// The sender posts to Network's internal export/deletion routes with this service's own client-credentials token;
ddiff --git a/server/food/places.js b/server/food/places.js
iindex c95abcd..53b55e4 100644
---- a/server/food/places.js
++++ b/server/food/places.js
@@@ -60,6 +60,21 @@ const isBudgetStore = (tags) => {
return BUDGET_BRANDS.some((re) => re.test(brand));
};
++/**
++ * An OpenStreetMap-supplied URL, kept only when it parses and speaks http(s). A mapped element's website is
++ * attacker-controlled (anyone may edit OSM): a `javascript:` or `data:` URL must never become an <a href>, so
++ * anything but http/https is dropped at the source here, and again where the href is rendered (server/http/pages.js).
++ */
++function safeUrl(v) {
++ if (typeof v !== 'string' || !v.trim()) return null;
++ try {
++ const u = new URL(v.trim());
++ return u.protocol === 'http:' || u.protocol === 'https:' ? u.href : null;
++ } catch {
++ return null;
++ }
++}
++
/** The Overpass query for one kind, bounded to `radiusM` metres around one point. Never a bulk download. */
function queryFor(kind, lat, lon, radiusM) {
const around = `(around:${radiusM},${lat},${lon})`;
@@@ -102,7 +117,7 @@ function toResult(el, kind, origin) {
opening_hours: tags.opening_hours || null,
operator: tags.operator || null,
phone: tags.phone || tags['contact:phone'] || null,
-- website: tags.website || tags['contact:website'] || null,
++ website: safeUrl(tags.website || tags['contact:website'] || tags.url),
wheelchair: tags.wheelchair === 'yes' ? true : tags.wheelchair === 'no' ? false : null,
budget: kind === 'grocery' ? isBudgetStore(tags) : (tags.fee === 'yes' ? false : null),
};
@@@ -184,4 +199,4 @@ async function searchPlaces(upstream, { kind = 'foodbank', q = null, lat = null,
};
}
--module.exports = { searchPlaces, haversineKm, formatAddress, isBudgetStore, queryFor, elementsToResults, ATTRIBUTION, SOURCE, FOOD_BANK_TYPES, KINDS };
++module.exports = { searchPlaces, haversineKm, formatAddress, isBudgetStore, safeUrl, queryFor, elementsToResults, ATTRIBUTION, SOURCE, FOOD_BANK_TYPES, KINDS };
ddiff --git a/server/food/store.js b/server/food/store.js
iindex afe31d4..93d223b 100644
---- a/server/food/store.js
++++ b/server/food/store.js
@@@ -29,6 +29,20 @@ async function cachePut(s, key, body, ttlMs) {
/** Drop expired rows (called at boot; the table stays small because every answer is bounded). */
const cachePrune = (s) => s.db.exec('DELETE FROM food_geo_cache WHERE expires_at <= $1', [s.iso()]);
++/**
++ * Prune the geo cache now, then hourly: an expired row is ignored anyway, this only keeps the table small. The
++ * interval is unref'd so it never holds the process open; the returned timer is what the entry point clears on
++ * shutdown (server/index.js). This repository has no restore-drill mode; if one is added it must skip this timer,
++ * because a drill reads a restored database and must not write to it.
++ */
++function startCachePrune(s, { intervalMs = 60 * 60 * 1000, log = console } = {}) {
++ const tick = () => Promise.resolve(cachePrune(s)).catch((err) => log.warn('[OpenVibe.Food] geo cache prune:', err && err.message));
++ tick();
++ const timer = setInterval(tick, intervalMs);
++ if (typeof timer.unref === 'function') timer.unref();
++ return timer;
++}
++
// ── food_plans ───────────────────────────────────────────────
async function insertPlan(s, p) {
await s.db.query(
@@@ -61,7 +75,7 @@ async function setPantry(s, owner, items) {
}
module.exports = {
-- cacheGet, cachePut, cachePrune,
++ cacheGet, cachePut, cachePrune, startCachePrune,
insertPlan, planRow, listPlans, deletePlan, planToWire,
getPantry, pantryRows, setPantry,
};
ddiff --git a/server/http/pages.js b/server/http/pages.js
iindex 90ce9a8..78a3862 100644
---- a/server/http/pages.js
++++ b/server/http/pages.js
@@@ -26,7 +26,7 @@ const { html, raw, table, notice, time, badge } = require('../render/html');
const { send } = require('../render/layout');
const foods = require('../food/foods');
const { planMeals } = require('../food/planner');
--const { searchPlaces, ATTRIBUTION } = require('../food/places');
++const { searchPlaces, safeUrl, ATTRIBUTION } = require('../food/places');
const { matchPantry } = require('../food/pantry-match');
const store = require('../food/store');
@@@ -39,7 +39,7 @@ const km = (n) => `${Number(n).toFixed(1)} km`;
const str = (v, max = 120) => (typeof v === 'string' && v.trim() ? v.trim().slice(0, max) : null);
function createPageRoutes(ctx) {
-- const { config, s } = ctx;
++ const { config, s, limits } = ctx;
const r = asyncRouter();
const PUBLIC_CACHE = cache.htmlHeaders({ maxAge: 300 });
const page = (req, res, o, status = 200) => send(res, status, { viewer: req.viewer, config, path: req.originalUrl, ...o });
@@@ -65,12 +65,15 @@ ${panel ? html`<p class="food-search-note">Free, no account. Places from OpenStr
}
function placeRow(res_) {
++ // The source already drops a non-http(s) website from OSM (server/food/places.js); checked again here, so a
++ // hostile or stale value can never reach an href. Anything but a safe URL simply shows no website link.
++ const website = safeUrl(res_.website);
return html`<li class="place" id="${res_.id}">
<p class="place-name"><b>${res_.name}</b> ${badge(res_.type_label)} ${res_.budget === true ? badge('budget', 'ok') : ''}</p>
<p class="muted small">${km(res_.distance_km)} away${res_.address ? html` · ${res_.address}` : ''}${res_.operator ? html` · run by ${res_.operator}` : ''}</p>
${res_.opening_hours ? html`<p class="small">Opening hours: ${res_.opening_hours}</p>` : ''}
${res_.phone ? html`<p class="small">Phone: <a href="tel:${String(res_.phone).replace(/[^\d+]/g, '')}">${res_.phone}</a></p>` : ''}
--<p class="small">${res_.website ? html`<a href="${res_.website}" rel="noopener nofollow">Their website</a> · ` : ''}<a href="${res_.osm_url}" rel="noopener nofollow">View on OpenStreetMap</a>${res_.wheelchair === true ? html` · wheelchair accessible` : ''}</p>
++<p class="small">${website ? html`<a href="${website}" rel="noopener nofollow">Their website</a> · ` : ''}<a href="${res_.osm_url}" rel="noopener nofollow">View on OpenStreetMap</a>${res_.wheelchair === true ? html` · wheelchair accessible` : ''}</p>
</li>`;
}
@@@ -119,15 +122,20 @@ ${raw(showcase.cta({
});
// ── Food near you ────────────────────────────────────────
-- r.get('/near', async (req, res) => {
++ const queryNum = (v) => (v != null && v !== '' && Number.isFinite(Number(v)) ? Number(v) : null);
++ // A page load only costs the shared OpenStreetMap budget when it will actually search.
++ const wantsSearch = (req) => !!str(req.query.q) || (queryNum(req.query.lat) != null && queryNum(req.query.lon) != null);
++
++ async function nearPage(req, res, { limited = false } = {}) {
const q = str(req.query.q);
const kind = KINDS[req.query.kind] ? req.query.kind : 'foodbank';
-- const num = (v) => (v != null && v !== '' && Number.isFinite(Number(v)) ? Number(v) : null);
let out = null;
let problem = null;
-- if (q || (num(req.query.lat) != null && num(req.query.lon) != null)) {
++ if (limited) {
++ problem = 'Too many searches from you just now. Try again in a minute.';
++ } else if (wantsSearch(req)) {
try {
-- out = await searchPlaces(ctx.upstream, { kind, q, lat: num(req.query.lat), lon: num(req.query.lon), radiusKm: num(req.query.radius) }, config);
++ out = await searchPlaces(ctx.upstream, { kind, q, lat: queryNum(req.query.lat), lon: queryNum(req.query.lon), radiusKm: queryNum(req.query.radius) }, config);
} catch (err) {
problem = err && err.code ? err.message : 'The place search is unavailable right now. Try again in a few minutes.';
}
@@@ -146,7 +154,27 @@ ${out ? html`<section class="near-results">
${resultsBlock(out)}
${osmNote(out.fetched_at)}
</section>` : html`<p class="muted">Search for a town, a city or a postcode above — or link here with <code>?lat=</code> and <code>?lon=</code> from a map.</p>`}`,
-- });
++ }, limited ? 429 : 200);
++ }
++
++ // The search on this page spends the same per-caller budget the API does (server/http/caller-limits.js), so a
++ // script cannot reach OpenStreetMap through the HTML route while the API is limited. The budget runs before the
++ // search; over it, the limiter's refusal is turned into the page's normal message (a 429 page), not its JSON.
++ const placeSearch = limits.budget('food.place.search');
++ r.get('/near', (req, res) => {
++ if (!wantsSearch(req)) return nearPage(req, res);
++ let settled = false;
++ let retryAfter = '60';
++ const allowed = () => { if (!settled) { settled = true; nearPage(req, res); } };
++ const refused = () => {
++ if (settled) return;
++ settled = true;
++ res.set('Retry-After', retryAfter);
++ nearPage(req, res, { limited: true });
++ };
++ // The limiter calls next when the caller is under budget, or ends the response itself when over it; it is
++ // handed a stand-in response so a refusal cannot write its JSON problem onto this HTML route.
++ placeSearch(req, { statusCode: 200, setHeader: (k, v) => { if (String(k).toLowerCase() === 'retry-after') retryAfter = String(v); }, end: refused }, allowed);
});
// ── Meal plan ────────────────────────────────────────────
ddiff --git a/server/index.js b/server/index.js
iindex 2e26506..af7eb1e 100644
---- a/server/index.js
++++ b/server/index.js
@@@ -10,12 +10,14 @@ const { gracefulStop } = require('openvibe-sdk/service');
/**
* The process stop (openvibe-sdk/service): the HTTP drain runs, then the timers clear, the Events subscriptions stop,
* the JWKS refresher stops and the store closes. Exported so a test can inject `exit` and `signals: false`. `extra` is
-- * what the product adds (the Events subscriptions); a test that passes none keeps the skeleton's order.
++ * what the product adds (the Events subscriptions); `timers` defaults to the app's own (ctx.geoPruneTimer) and a test
++ * that passes none keeps the skeleton's order.
*/
--function createLifecycle({ server, ctx, exit, signals, timers = [], extra = [] }) {
++function createLifecycle({ server, ctx, exit, signals, timers, extra = [] }) {
++ const clearTimers = timers || [ctx.geoPruneTimer].filter(Boolean);
return gracefulStop({
name: 'OpenVibe.Food', server, deadlineExitCode: 0, exit, signals, deadlineMs: 10_000,
-- close: [() => { for (const t of timers) clearInterval(t); }, () => ctx.keys.client.stop(), () => ctx.s.close(), ...extra],
++ close: [() => { for (const t of clearTimers) clearInterval(t); }, () => ctx.keys.client.stop(), () => ctx.s.close(), ...extra],
});
}
ddiff --git a/test/places.test.js b/test/places.test.js
iindex 0ba0d13..d154065 100644
---- a/test/places.test.js
++++ b/test/places.test.js
@@@ -9,11 +9,26 @@ const assert = require('assert');
const { boot, check, done } = require('./helpers/boot');
const { startOsm } = require('./helpers/osm');
const { createPacer } = require('../server/food/upstream');
++const { safeUrl } = require('../server/food/places');
const geo = require('../server/food/store');
const OVERPASS = '/api/interpreter';
(async () => {
++ await check('only an http(s) URL from OpenStreetMap is kept: javascript, data and friends are dropped', () => {
++ assert.strictEqual(safeUrl('javascript:alert(1)'), null);
++ assert.strictEqual(safeUrl(' JavaScript:x'), null, 'leading space does not smuggle a scheme');
++ assert.strictEqual(safeUrl(' javascript:alert(1)'), null);
++ assert.strictEqual(safeUrl('java\nscript:alert(1)'), null, 'an embedded newline does not smuggle a scheme');
++ assert.strictEqual(safeUrl('data:text/html,<script>alert(1)</script>'), null);
++ assert.strictEqual(safeUrl('ftp://example.org/x'), null);
++ assert.strictEqual(safeUrl('//example.org/x'), null, 'a scheme-relative URL is not an absolute http(s) one');
++ assert.strictEqual(safeUrl(''), null);
++ assert.strictEqual(safeUrl(null), null);
++ assert.strictEqual(safeUrl('https://example.org/pantry'), 'https://example.org/pantry');
++ assert.strictEqual(safeUrl('http://example.org/pantry'), 'http://example.org/pantry');
++ });
++
await check('the pace is a queue: concurrent callers are serialised, and a failure does not stall it', async () => {
const started = [];
const pacer = createPacer({ minIntervalMs: 50 });
@@@ -43,6 +58,23 @@ const OVERPASS = '/api/interpreter';
assert.strictEqual(await geo.cacheGet(s, 'nominatim:stale:1'), null, 'a row past its TTL is not a hit');
});
++ await check('the geo cache is pruned on a timer, not only at boot', async () => {
++ const s = t.ctx.s;
++ assert.ok(t.ctx.geoPruneTimer, 'the app starts a prune timer');
++ assert.strictEqual(t.ctx.geoPruneTimer.hasRef(), false, 'the app\'s prune timer is unref\'d');
++ await s.db.query(
++ `INSERT INTO food_geo_cache (key, body, fetched_at, expires_at) VALUES ($1, $2::jsonb, $3, $4)
++ ON CONFLICT (key) DO UPDATE SET body = EXCLUDED.body, expires_at = EXCLUDED.expires_at`,
++ ['nominatim:timer:1', JSON.stringify([]), '2020-01-01T00:00:00.000Z', '2020-01-08T00:00:00.000Z']);
++ const timer = geo.startCachePrune(s, { intervalMs: 25, log: { warn() {} } });
++ assert.strictEqual(timer.hasRef(), false, 'the prune timer is unref\'d');
++ const gone = async () => (await s.db.maybe('SELECT 1 AS x FROM food_geo_cache WHERE key = $1', ['nominatim:timer:1'])) == null;
++ try {
++ for (let i = 0; i < 20 && !(await gone()); i++) await new Promise((r) => setTimeout(r, 25));
++ assert.ok(await gone(), 'the expired row was deleted by the timer, not just ignored');
++ } finally { clearInterval(timer); }
++ });
++
await check('the place search geocodes a name, lists food banks nearest first, and carries the ODbL attribution', async () => {
const r = await t.get(`/api/v1/places?q=${encodeURIComponent('Springfield, Illinois')}&kind=foodbank`);
assert.strictEqual(r.status, 200, r.text);
@@@ -115,6 +147,29 @@ const OVERPASS = '/api/interpreter';
assert.ok(!page.text.includes('<script>alert(1)</script>'), 'no live script reaches the page');
});
++ await check('a hostile website, contact:website or url from OpenStreetMap never becomes an href', async () => {
++ osm.state.overpass = { ...osm.state.overpass, foodbank: { elements: [
++ { type: 'node', id: 401, lat: 42.10, lon: -92.20, tags: { amenity: 'social_facility', social_facility: 'food_bank', name: 'Script Link', website: 'javascript:alert(1)' } },
++ { type: 'node', id: 402, lat: 42.11, lon: -92.21, tags: { amenity: 'social_facility', social_facility: 'food_bank', name: 'Sneaky', 'contact:website': ' JavaScript:x' } },
++ { type: 'node', id: 403, lat: 42.12, lon: -92.22, tags: { amenity: 'social_facility', social_facility: 'food_bank', name: 'Data Link', url: 'data:text/html,<script>alert(1)</script>' } },
++ { type: 'node', id: 404, lat: 42.13, lon: -92.23, tags: { amenity: 'social_facility', social_facility: 'food_bank', name: 'Real Website', website: 'https://example.org/pantry' } },
++ ] } };
++ const api = await t.get('/api/v1/places?lat=42.1&lon=-92.2&kind=foodbank');
++ assert.strictEqual(api.status, 200, api.text);
++ const websiteOf = Object.fromEntries(api.json().results.map((r) => [r.name, r.website]));
++ assert.strictEqual(websiteOf['Script Link'], null, 'a javascript: website is dropped at the source');
++ assert.strictEqual(websiteOf.Sneaky, null, 'a whitespace-smuggled javascript: contact:website is dropped');
++ assert.strictEqual(websiteOf['Data Link'], null, 'a data: url is dropped');
++ assert.strictEqual(websiteOf['Real Website'], 'https://example.org/pantry', 'an https URL is kept');
++
++ const page = await t.get('/near?lat=42.1&lon=-92.2&kind=foodbank');
++ assert.strictEqual(page.status, 200);
++ assert.ok(!/href="javascript:/i.test(page.text), 'no javascript: href reaches the page');
++ assert.ok(!/href="data:text\/html/i.test(page.text), 'no data: href from OSM reaches the page');
++ assert.match(page.text, /href="https:\/\/example\.org\/pantry"/, 'the real website is linked');
++ assert.match(page.text, /Their website/);
++ });
++
await check('public reads need no token, and bad input is refused with a stable code', async () => {
assert.strictEqual((await t.get('/api/v1/places?kind=foodbank')).status, 422, 'no q and no coordinates');
const bad = await t.get('/api/v1/places?q=Springfield&kind=restaurants');
@@@ -180,6 +235,28 @@ const OVERPASS = '/api/interpreter';
assert.strictEqual((await limited.get('/api/v1/foods', from)).status, 200);
} finally { await limited.close(); }
});
++
++ await check('the /near page spends the same place-search budget: over it, it answers its normal message with 429', async () => {
++ const limited = await boot({ env: { ...osm2.env, FOOD_OSM_MIN_INTERVAL_MS: '1' }, callerLimits: true, limitsNow: () => Date.UTC(2026, 9, 8, 13, 0, 0) });
++ try {
++ const from = { headers: { 'x-forwarded-for': '203.0.113.61' } };
++ for (let i = 0; i < 10; i++) assert.strictEqual((await limited.get('/api/v1/places?lat=39.7817&lon=-89.6501&kind=foodbank', from)).status, 200, `search ${i + 1} is allowed`);
++ assert.strictEqual((await limited.get('/api/v1/places?lat=39.7817&lon=-89.6501&kind=foodbank', from)).status, 429, 'the eleventh API search is refused');
++
++ // The HTML route shares those counters: the same caller's search is over the budget too.
++ const before = osm2.requests.length;
++ const page = await limited.get('/near?lat=39.7817&lon=-89.6501&kind=foodbank', from);
++ assert.strictEqual(page.status, 429);
++ assert.match(page.headers.get('content-type'), /text\/html/);
++ assert.match(page.text, /Too many searches from you just now\. Try again in a minute\./);
++ assert.ok(Number(page.headers.get('retry-after')) > 0);
++ assert.strictEqual(osm2.requests.length, before, 'the refused page search made no upstream request');
++
++ // Another caller is unaffected, and a page load with nothing to search costs no budget.
++ assert.strictEqual((await limited.get('/near?lat=39.7817&lon=-89.6501&kind=foodbank', { headers: { 'x-forwarded-for': '203.0.113.62' } })).status, 200);
++ assert.strictEqual((await limited.get('/near', from)).status, 200);
++ } finally { await limited.close(); }
++ });
} finally {
await t2.close();
await osm2.close();
ddiff --git a/test/service-kit.test.js b/test/service-kit.test.js
iindex 2a82b23..a487a1f 100644
---- a/test/service-kit.test.js
++++ b/test/service-kit.test.js
@@@ -52,5 +52,24 @@ const listen = (server) => new Promise((resolve) => server.listen(0, '127.0.0.1'
assert.strictEqual(exits, 1);
});
++ await check('the app\'s prune timer on ctx is cleared on stop, even when no timers are passed', async () => {
++ let ticks = 0;
++ const timer = setInterval(() => { ticks += 1; }, 20);
++ const ctx = {
++ geoPruneTimer: timer,
++ keys: { client: { stop: () => {} } },
++ s: { close: async () => {} },
++ };
++ const server = http.createServer((req, res) => res.end('ok'));
++ await listen(server);
++
++ const lifecycle = createLifecycle({ server, ctx, exit: () => {}, signals: false });
++ await lifecycle.stop('SIGTERM');
++ const stopped = ticks;
++ await new Promise((r) => setTimeout(r, 80));
++ assert.strictEqual(ticks, stopped, 'the prune timer no longer ticks after the stop');
++ assert.strictEqual(server.listening, false);
++ });
++
done();
})().catch((err) => { console.error(err); process.exit(1); });