openvibers/openvibe.tools Commits
Filter food maps queries and suppress upstream 5xx bodies (#9)
Changes
@@@ -124,13 +124,19 @@ app.use(express.static(path.join(__dirname, '..', 'public'), { maxAge: '1d', ind// Proxy food-related API calls to openvibe-maps backend. The visitor's address goes along, so maps// rate-limits each person instead of putting every food visitor in one 127.0.0.1 bucket.--function proxyToMaps(apiPath) {++function proxyToMaps(apiPath, allowedParams) {return async (req, res) => {-- const qs = new URL(req.url, `http://localhost`).search;-- const url = `${MAPS_API}${apiPath}${qs}`;++ const incoming = new URL(req.url, 'http://localhost').searchParams;++ const query = new URLSearchParams();++ for (const key of allowedParams) {++ for (const value of incoming.getAll(key)) query.append(key, value);++ }++ const qs = query.toString();++ const url = `${MAPS_API}${apiPath}${qs ? `?${qs}` : ''}`;try {const headers = req.ip ? { 'X-Forwarded-For': req.ip, 'X-Real-IP': req.ip } : {};const response = await fetch(url, { headers, signal: AbortSignal.timeout(15000) });++ if (response.status >= 500 && response.status < 600) return res.status(502).json({ error: 'Backend unavailable' });const data = await response.json();res.status(response.status).json(data);} catch (e) {@@@ -139,11 +145,12 @@ function proxyToMaps(apiPath) {};}--app.get('/api/food-banks', proxyToMaps('/api/food-banks'));--app.get('/api/stores', proxyToMaps('/api/stores'));--app.get('/api/foods', proxyToMaps('/api/foods'));--app.get('/api/meal-plan', proxyToMaps('/api/meal-plan'));--app.get('/api/geocode', proxyToMaps('/api/geocode'));++// Keep these in step with the query fields read by the matching maps routes.++app.get('/api/food-banks', proxyToMaps('/api/food-banks', ['lat', 'lon', 'radius']));++app.get('/api/stores', proxyToMaps('/api/stores', ['lat', 'lon']));++app.get('/api/foods', proxyToMaps('/api/foods', ['group', 'campFriendly', 'shelfStable', 'search']));++app.get('/api/meal-plan', proxyToMaps('/api/meal-plan', ['budget', 'days', 'campFriendly', 'shelfStable', 'randomize']));++app.get('/api/geocode', proxyToMaps('/api/geocode', ['q']));// ── Internal Analytics API ────────────────────────────────────const internalAccess = requireInternalAccess({ keys: guard.keys, issuer: guard.issuer, audience: guard.audience });@@@ -1,14 +1,5 @@'use strict';--// Two behaviours of food's maps proxy (apps/food/server/index.js, proxyToMaps) that are wrong today.--// Each is written as the check it should pass; while it fails it prints the repo runner's--// `<label>: skipped (<why>)` line — the file is reported as skipped, and under --strict it fails — so--// the defect is visible instead of silently green. No production code is changed here (this job does--// not touch it); when the proxy is fixed, no skip line is printed and the file passes.--// 1. a maps 5xx with a JSON body is forwarded verbatim: food answers the upstream's status and body,--// so an upstream URL or key in that body reaches the caller (it should be a clean 502).--// apps/food/server/index.js:134 forwards response.status and the parsed body unchanged, and the--// catch that answers { error: 'Backend unavailable' } (line 137) only runs when the body is not JSON.--// 2. every query parameter is forwarded (line 129), so parameters the route does not take reach maps.++// Food's maps proxy suppresses upstream 5xx bodies and forwards only each route's query fields.const assert = require('assert');const fs = require('fs');const os = require('os');@@@ -17,7 +8,6 @@ const { startApp } = require('../../_shared/test/spawn');const { startMapsStub } = require('./maps-stub');const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'ov-food-upstream-todo-'));--const ALLOWED_FOODS = new Set(['group', 'campFriendly', 'shelfStable', 'search']);(async () => {const maps = await startMapsStub();@@@ -29,38 +19,36 @@ const ALLOWED_FOODS = new Set(['group', 'campFriendly', 'shelfStable', 'search']try { body = JSON.parse(text); } catch { /* not JSON */ }return { status: r.status, headers: r.headers, text, body };};-- const todo = [];-- // The repo runner only reads a skip line that is one line of its own, so the reason is flattened.-- const expect = (label, fn) => {-- try { fn(); } catch (err) { todo.push(`${label}: skipped (${String(err.message).replace(/\s+/g, ' ').slice(0, 200)})`); }-- };try {// 1. maps answers 5xx with a JSON body → a clean 502 that carries nothing of the upstreammaps.setMode('boom');const r = await call('/api/foods?search=rice');-- expect('food maps 5xx answers a clean 502 with no upstream body', () => {-- assert.strictEqual(r.status, 502, `food answered ${r.status} ${r.text.slice(0, 200)}`);-- assert.deepStrictEqual(r.body, { error: 'Backend unavailable' }, 'the upstream body must not be passed through');-- assert.ok(!/maps\.internal|sk-live|upstream blew up/.test(r.text), `the answer leaks the upstream: ${r.text.slice(0, 200)}`);-- assert.strictEqual(r.headers.get('x-upstream-secret'), null, 'no upstream header is passed through');-- });++ assert.strictEqual(r.status, 502, `food answered ${r.status} ${r.text.slice(0, 200)}`);++ assert.deepStrictEqual(r.body, { error: 'Backend unavailable' }, 'the upstream body must not be passed through');++ assert.ok(!/maps\.internal|sk-live|upstream blew up/.test(r.text), `the answer leaks the upstream: ${r.text.slice(0, 200)}`);++ assert.strictEqual(r.headers.get('x-upstream-secret'), null, 'no upstream header is passed through');-- // 2. only the query parameters the route allows reach maps++ // 2. every route keeps its supported fields and drops unrelated onesmaps.setMode('ok');-- maps.requests.length = 0;-- const q = await call('/api/foods?search=rice&evil=1&lat=5');-- assert.strictEqual(q.status, 200);-- const fwd = maps.requests.filter((s) => s.path === '/api/foods').pop();-- assert.ok(fwd, 'food forwarded the call to maps');-- expect('food forwards only the query parameters the route allows', () => {-- const extra = fwd.params.filter((k) => !ALLOWED_FOODS.has(k));-- assert.deepStrictEqual(extra, [], `these parameters reached maps and are not part of the route: ${extra.join(', ')}`);-- });++ const routes = [++ ['/api/food-banks', 'lat=40&lon=-74&radius=10', ['lat', 'lon', 'radius']],++ ['/api/stores', 'lat=40&lon=-74', ['lat', 'lon']],++ ['/api/foods', 'group=grains&campFriendly=true&shelfStable=true&search=rice', ['group', 'campFriendly', 'shelfStable', 'search']],++ ['/api/meal-plan', 'budget=20&days=3&campFriendly=true&shelfStable=true&randomize=true', ['budget', 'days', 'campFriendly', 'shelfStable', 'randomize']],++ ['/api/geocode', 'q=Seattle', ['q']],++ ];++ for (const [route, allowedQuery, expected] of routes) {++ maps.requests.length = 0;++ const result = await call(`${route}?${allowedQuery}&evil=1&unexpected%5Bkey%5D=2`);++ assert.strictEqual(result.status, 200, `${route} answered ${result.status}`);++ const fwd = maps.requests.find((s) => s.path === route);++ assert.ok(fwd, `${route} reached maps`);++ assert.deepStrictEqual(fwd.params.sort(), expected.sort(), `${route} forwarded unexpected query fields`);++ }} finally {await food.kill();await maps.close();}-- for (const line of todo) console.log(line);-- if (!todo.length) console.log('food → maps todo: the proxy now answers cleanly and forwards only the allowed parameters');++ console.log('food → maps: 5xx bodies are suppressed and only route query fields reach maps');process.exit(0);})().catch((err) => { console.error(err); process.exit(1); });@@@ -6,7 +6,7 @@// • with no provider key configured, maps' "source skipped" answer is passed through (200), and food// sends no key or credential of its own upstream// • the parameters the route allows are forwarded--// The two known defects this uncovered stay as todo tests in maps-upstream-todo.test.js.++// Maps 5xx responses and query filtering are checked in maps-upstream-todo.test.js.const assert = require('assert');const fs = require('fs');const os = require('os');
@@ -124,13 +124,19 @@ app.use(express.static(path.join(__dirname, '..', 'public'), { maxAge: '1d', ind
@@ -124,13 +124,19 @@ app.use(express.static(path.join(__dirname, '..', 'public'), { maxAge: '1d', ind
// Proxy food-related API calls to openvibe-maps backend. The visitor's address goes along, so maps
// Proxy food-related API calls to openvibe-maps backend. The visitor's address goes along, so maps
// rate-limits each person instead of putting every food visitor in one 127.0.0.1 bucket.
// rate-limits each person instead of putting every food visitor in one 127.0.0.1 bucket.
-function proxyToMaps(apiPath) {
+function proxyToMaps(apiPath, allowedParams) {
return async (req, res) => {
return async (req, res) => {
- const qs = new URL(req.url, `http://localhost`).search;
- const url = `${MAPS_API}${apiPath}${qs}`;
+ const incoming = new URL(req.url, 'http://localhost').searchParams;
+ const query = new URLSearchParams();
+ for (const key of allowedParams) {
+ for (const value of incoming.getAll(key)) query.append(key, value);
+ }
+ const qs = query.toString();
+ const url = `${MAPS_API}${apiPath}${qs ? `?${qs}` : ''}`;
try {
try {
const headers = req.ip ? { 'X-Forwarded-For': req.ip, 'X-Real-IP': req.ip } : {};
const headers = req.ip ? { 'X-Forwarded-For': req.ip, 'X-Real-IP': req.ip } : {};
const response = await fetch(url, { headers, signal: AbortSignal.timeout(15000) });
const response = await fetch(url, { headers, signal: AbortSignal.timeout(15000) });
+ if (response.status >= 500 && response.status < 600) return res.status(502).json({ error: 'Backend unavailable' });
const data = await response.json();
const data = await response.json();
res.status(response.status).json(data);
res.status(response.status).json(data);
} catch (e) {
} catch (e) {
@@ -139,11 +145,12 @@ function proxyToMaps(apiPath) {
@@ -139,11 +145,12 @@ function proxyToMaps(apiPath) {
};
};
}
}
-app.get('/api/food-banks', proxyToMaps('/api/food-banks'));
-app.get('/api/stores', proxyToMaps('/api/stores'));
-app.get('/api/foods', proxyToMaps('/api/foods'));
-app.get('/api/meal-plan', proxyToMaps('/api/meal-plan'));
-app.get('/api/geocode', proxyToMaps('/api/geocode'));
+// Keep these in step with the query fields read by the matching maps routes.
+app.get('/api/food-banks', proxyToMaps('/api/food-banks', ['lat', 'lon', 'radius']));
+app.get('/api/stores', proxyToMaps('/api/stores', ['lat', 'lon']));
+app.get('/api/foods', proxyToMaps('/api/foods', ['group', 'campFriendly', 'shelfStable', 'search']));
+app.get('/api/meal-plan', proxyToMaps('/api/meal-plan', ['budget', 'days', 'campFriendly', 'shelfStable', 'randomize']));
+app.get('/api/geocode', proxyToMaps('/api/geocode', ['q']));
// ── Internal Analytics API ────────────────────────────────────
// ── Internal Analytics API ────────────────────────────────────
const internalAccess = requireInternalAccess({ keys: guard.keys, issuer: guard.issuer, audience: guard.audience });
const internalAccess = requireInternalAccess({ keys: guard.keys, issuer: guard.issuer, audience: guard.audience });
@@ -1,14 +1,5 @@
@@ -1,14 +1,5 @@
'use strict';
'use strict';
-// Two behaviours of food's maps proxy (apps/food/server/index.js, proxyToMaps) that are wrong today.
-// Each is written as the check it should pass; while it fails it prints the repo runner's
-// `<label>: skipped (<why>)` line — the file is reported as skipped, and under --strict it fails — so
-// the defect is visible instead of silently green. No production code is changed here (this job does
-// not touch it); when the proxy is fixed, no skip line is printed and the file passes.
-// 1. a maps 5xx with a JSON body is forwarded verbatim: food answers the upstream's status and body,
-// so an upstream URL or key in that body reaches the caller (it should be a clean 502).
-// apps/food/server/index.js:134 forwards response.status and the parsed body unchanged, and the
-// catch that answers { error: 'Backend unavailable' } (line 137) only runs when the body is not JSON.
-// 2. every query parameter is forwarded (line 129), so parameters the route does not take reach maps.
+// Food's maps proxy suppresses upstream 5xx bodies and forwards only each route's query fields.
const assert = require('assert');
const assert = require('assert');
const fs = require('fs');
const fs = require('fs');
const os = require('os');
const os = require('os');
@@ -17,7 +8,6 @@ const { startApp } = require('../../_shared/test/spawn');
@@ -17,7 +8,6 @@ const { startApp } = require('../../_shared/test/spawn');
const { startMapsStub } = require('./maps-stub');
const { startMapsStub } = require('./maps-stub');
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'ov-food-upstream-todo-'));
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'ov-food-upstream-todo-'));
-const ALLOWED_FOODS = new Set(['group', 'campFriendly', 'shelfStable', 'search']);
(async () => {
(async () => {
const maps = await startMapsStub();
const maps = await startMapsStub();
@@ -29,38 +19,36 @@ const ALLOWED_FOODS = new Set(['group', 'campFriendly', 'shelfStable', 'search']
@@ -29,38 +19,36 @@ const ALLOWED_FOODS = new Set(['group', 'campFriendly', 'shelfStable', 'search']
try { body = JSON.parse(text); } catch { /* not JSON */ }
try { body = JSON.parse(text); } catch { /* not JSON */ }
return { status: r.status, headers: r.headers, text, body };
return { status: r.status, headers: r.headers, text, body };
};
};
- const todo = [];
- // The repo runner only reads a skip line that is one line of its own, so the reason is flattened.
- const expect = (label, fn) => {
- try { fn(); } catch (err) { todo.push(`${label}: skipped (${String(err.message).replace(/\s+/g, ' ').slice(0, 200)})`); }
- };
try {
try {
// 1. maps answers 5xx with a JSON body → a clean 502 that carries nothing of the upstream
// 1. maps answers 5xx with a JSON body → a clean 502 that carries nothing of the upstream
maps.setMode('boom');
maps.setMode('boom');
const r = await call('/api/foods?search=rice');
const r = await call('/api/foods?search=rice');
- expect('food maps 5xx answers a clean 502 with no upstream body', () => {
- assert.strictEqual(r.status, 502, `food answered ${r.status} ${r.text.slice(0, 200)}`);
- assert.deepStrictEqual(r.body, { error: 'Backend unavailable' }, 'the upstream body must not be passed through');
- assert.ok(!/maps\.internal|sk-live|upstream blew up/.test(r.text), `the answer leaks the upstream: ${r.text.slice(0, 200)}`);
- assert.strictEqual(r.headers.get('x-upstream-secret'), null, 'no upstream header is passed through');
- });
+ assert.strictEqual(r.status, 502, `food answered ${r.status} ${r.text.slice(0, 200)}`);
+ assert.deepStrictEqual(r.body, { error: 'Backend unavailable' }, 'the upstream body must not be passed through');
+ assert.ok(!/maps\.internal|sk-live|upstream blew up/.test(r.text), `the answer leaks the upstream: ${r.text.slice(0, 200)}`);
+ assert.strictEqual(r.headers.get('x-upstream-secret'), null, 'no upstream header is passed through');
- // 2. only the query parameters the route allows reach maps
+ // 2. every route keeps its supported fields and drops unrelated ones
maps.setMode('ok');
maps.setMode('ok');
- maps.requests.length = 0;
- const q = await call('/api/foods?search=rice&evil=1&lat=5');
- assert.strictEqual(q.status, 200);
- const fwd = maps.requests.filter((s) => s.path === '/api/foods').pop();
- assert.ok(fwd, 'food forwarded the call to maps');
- expect('food forwards only the query parameters the route allows', () => {
- const extra = fwd.params.filter((k) => !ALLOWED_FOODS.has(k));
- assert.deepStrictEqual(extra, [], `these parameters reached maps and are not part of the route: ${extra.join(', ')}`);
- });
+ const routes = [
+ ['/api/food-banks', 'lat=40&lon=-74&radius=10', ['lat', 'lon', 'radius']],
+ ['/api/stores', 'lat=40&lon=-74', ['lat', 'lon']],
+ ['/api/foods', 'group=grains&campFriendly=true&shelfStable=true&search=rice', ['group', 'campFriendly', 'shelfStable', 'search']],
+ ['/api/meal-plan', 'budget=20&days=3&campFriendly=true&shelfStable=true&randomize=true', ['budget', 'days', 'campFriendly', 'shelfStable', 'randomize']],
+ ['/api/geocode', 'q=Seattle', ['q']],
+ ];
+ for (const [route, allowedQuery, expected] of routes) {
+ maps.requests.length = 0;
+ const result = await call(`${route}?${allowedQuery}&evil=1&unexpected%5Bkey%5D=2`);
+ assert.strictEqual(result.status, 200, `${route} answered ${result.status}`);
+ const fwd = maps.requests.find((s) => s.path === route);
+ assert.ok(fwd, `${route} reached maps`);
+ assert.deepStrictEqual(fwd.params.sort(), expected.sort(), `${route} forwarded unexpected query fields`);
+ }
} finally {
} finally {
await food.kill();
await food.kill();
await maps.close();
await maps.close();
}
}
- for (const line of todo) console.log(line);
- if (!todo.length) console.log('food → maps todo: the proxy now answers cleanly and forwards only the allowed parameters');
+ console.log('food → maps: 5xx bodies are suppressed and only route query fields reach maps');
process.exit(0);
process.exit(0);
})().catch((err) => { console.error(err); process.exit(1); });
})().catch((err) => { console.error(err); process.exit(1); });
@@ -6,7 +6,7 @@
@@ -6,7 +6,7 @@
// • with no provider key configured, maps' "source skipped" answer is passed through (200), and food
// • with no provider key configured, maps' "source skipped" answer is passed through (200), and food
// sends no key or credential of its own upstream
// sends no key or credential of its own upstream
// • the parameters the route allows are forwarded
// • the parameters the route allows are forwarded
-// The two known defects this uncovered stay as todo tests in maps-upstream-todo.test.js.
+// Maps 5xx responses and query filtering are checked in maps-upstream-todo.test.js.
const assert = require('assert');
const assert = require('assert');
const fs = require('fs');
const fs = require('fs');
const os = require('os');
const os = require('os');