From ddaac24a593c27fef4d94ca776a3ea27d1f32e10 Mon Sep 17 00:00:00 2001 From: SandObserver <260779319+SandObserver@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:08:05 -0400 Subject: [PATCH] Security: stop the Connections widget printing the address it failed to reach Three catch blocks wrote a caught error's message into a per-service error field. That field travels in a 200 response, where the api-error sanitiser never runs, so a failed fetch drew "connect ECONNREFUSED 172.17.0.2:8181" on the dashboard. The widget's own sentences go through ctx.fail now, which marks them as vouched, and the catches choose between that message and a phrase read from the error code. Nothing else changes: one dead service still leaves the others rendering, and the tile reads better for it. connections no longer needs its exemption from the plain-Error guard. --- CHANGELOG.md | 6 +++ api/test/connections-url.test.js | 17 +++++++- api/test/widget-error.test.js | 40 +++++++++++++++---- ui/widgets/connections/data.js | 68 +++++++++++++++++++++----------- 4 files changed, 97 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf1aa0a8..6a3ee083 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The Books, Now Playing and Backup widgets now show why they failed, such as a missing API key or a rejected password, instead of a generic error. +### Security + +- A Connections tile that cannot reach a service now says so in plain words + instead of printing the address it tried, which put an internal host and port + on the dashboard. + ## [1.5.2] - 2026-08-09 ### Changed diff --git a/api/test/connections-url.test.js b/api/test/connections-url.test.js index 618fb60d..c8826626 100644 --- a/api/test/connections-url.test.js +++ b/api/test/connections-url.test.js @@ -18,6 +18,7 @@ const assert = require('node:assert/strict'); const data = require(path.join(__dirname, '../../ui/widgets/connections/data.js')); const { normBase } = data; +const { WidgetError, KIND } = require('../src/api-error'); /* ── the normaliser ───────────────────────────────────────────────────────── */ @@ -56,14 +57,26 @@ test('the string "undefined" never appears in the result', () => { /* ── through the widget ───────────────────────────────────────────────────── */ -/* fetchJSON is passed in, so a call can be observed without any network. */ +/* fetchJSON is passed in, so a call can be observed without any network. The + rest of the context is the surface widget-data.js really hands a data + function: ctx.fail throws a WidgetError, which is what marks a message as one + the widget wrote rather than one that escaped from Node. */ function run(config, endpoint = 'map') { const requested = []; const fetchJSON = async url => { requested.push(url); return { status: 200, data: {} }; }; - return data({ config, endpoint, fetchJSON }).then(result => ({ result, requested })); + const ctx = { + config, + endpoint, + fetchJSON, + KIND, + fail: (message, opts) => { + throw new WidgetError(message, opts); + }, + }; + return data(ctx).then(result => ({ result, requested })); } test('a service with no address is not fetched at all', async () => { diff --git a/api/test/widget-error.test.js b/api/test/widget-error.test.js index fa8e2aa9..d97277f2 100644 --- a/api/test/widget-error.test.js +++ b/api/test/widget-error.test.js @@ -183,17 +183,12 @@ test('every widget frontend shows the message the server vouched for', () => { still throws, and the sanitiser is right to replace it. What is banned is a sentence written for the user that never reaches them. - connections is exempt and has to be. It reports several services in one - result, catches each service's throw itself, and writes the message into that - service's own `error` field, which is an error inside a successful response - rather than a failed one. Its sentences do reach the user, by the other route - the convention allows. */ -const THROWS_INTERNALLY = new Set(['connections']); - + connections used to be exempt, because it catches its own throws and reports + each service inside a successful result. It uses ctx.fail too now, which is + what lets its catch tell a sentence it wrote from one Node threw. */ test('no widget reports a user-facing failure with a plain Error', () => { const offenders = []; for (const [name, p] of dataFiles) { - if (THROWS_INTERNALLY.has(name)) continue; const src = fs.readFileSync(p, 'utf8').replace(/\/\*[\s\S]*?\*\//g, ''); for (const m of src.matchAll(/throw new Error\(([^)]*)\)/g)) { offenders.push(`${name}: throw new Error(${m[1].slice(0, 50)})`); @@ -227,3 +222,32 @@ test('no widget rebuilds the base-URL normalisation ctx already provides', () => } assert.deepEqual(offenders, [], `Use ctx.normalizeBase:\n${offenders.join('\n')}`); }); + +/* An error field inside a successful result is allowed, and is how a widget + reporting several services marks the one that failed. It bypasses the + api-error sanitiser entirely, though: the response is a 200, so nothing + rewrites what goes in it. + + A caught error's message names what it failed to reach, so writing one into a + result puts an internal host and port on the dashboard. The field has to be + given a phrase the widget chose. */ +test('no widget writes a caught message into a successful result', () => { + const offenders = []; + for (const [name, p] of dataFiles) { + const src = fs + .readFileSync(p, 'utf8') + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/ctx\.log\.\w+\([^;]*?\);/g, ''); + for (const m of src.matchAll(/(\w+)\.error\s*=\s*([^;]+);/g)) { + const value = m[2]; + if (/\be\.(message|stack)\b|String\(e\)/.test(value)) { + offenders.push(`${name}: ${m[0].trim().slice(0, 60)}`); + } + } + } + assert.deepEqual( + offenders, + [], + `A caught message reaches the browser in a 200 body. Map it to a phrase first:\n${offenders.join('\n')}`, + ); +}); diff --git a/ui/widgets/connections/data.js b/ui/widgets/connections/data.js index 56815045..54084dfa 100644 --- a/ui/widgets/connections/data.js +++ b/ui/widgets/connections/data.js @@ -96,6 +96,24 @@ const MAP_DEFAULT_COLOR = { }; /* Raw GET: Conduit exposes Prometheus-style text that needs the unparsed body. */ +/* What a failed service shows on the tile. This string travels in a successful + response, so the api-error sanitiser never sees it: a caught error's message + names the host and port it failed to reach. Our own sentence when we wrote + one, a phrase from the error code otherwise. */ +const TIMEOUT_CODES = new Set(['ETIMEDOUT', 'ESOCKETTIMEDOUT', 'UND_ERR_HEADERS_TIMEOUT']); +function errorText(e) { + if (e && e.vouchedMessage) return e.vouchedMessage; + /* Already a sentence this repo wrote, and the reason is the point of it. */ + if (e && e.name === 'SsrfBlockedError') return e.message; + const code = e && e.code; + if (code === 'ECONNREFUSED') return 'Connection refused'; + if (code === 'ENOTFOUND' || code === 'EAI_AGAIN') return 'Host not found'; + if (TIMEOUT_CODES.has(code)) return 'Timed out'; + if (code === 'ECONNRESET' || code === 'EPIPE') return 'Connection lost'; + if (typeof code === 'string' && (code.startsWith('CERT_') || code.includes('_CERT_'))) return 'Certificate rejected'; + return 'Unreachable'; +} + function authErr(r) { return r.status === 401 || r.status === 403; } @@ -130,13 +148,14 @@ function parseConduitText(raw) { return { regions, limit, connected, live }; } -const run = async function ({ config, endpoint, fetchJSON }) { - if (endpoint === 'vpn') return vpnView(config, fetchJSON); - return mapView(config, fetchJSON); +const run = async function (ctx) { + if (ctx.endpoint === 'vpn') return vpnView(ctx); + return mapView(ctx); }; /* ── VPN view ── */ -async function vpnView(config, fetchJSON) { +async function vpnView(ctx) { + const { config, fetchJSON } = ctx; const vpn = config.vpn || {}; const svc = vpn.service || 'gluetun'; const out = { @@ -151,13 +170,13 @@ async function vpnView(config, fetchJSON) { try { if (svc === 'gluetun') { const base = normBase(vpn.url); - if (!base) throw new Error('No control server URL configured'); + if (!base) ctx.fail('No control server URL configured', { kind: ctx.KIND.INVALID }); const headers = vpn.apiKey ? { 'X-API-Key': vpn.apiKey } : {}; let ipRes = null; try { ipRes = await fetchJSON(base + '/v1/publicip/ip', { headers, timeout: 7000 }); } catch (e) { - out.error = e.code || e.message || 'unreachable'; + out.error = errorText(e); } if (ipRes) { if (authErr(ipRes)) out.error = 'Auth required — set the API key'; @@ -192,12 +211,12 @@ async function vpnView(config, fetchJSON) { } } else { const base = normBase(vpn.url).replace(/\/+$/, ''); - if (!base) throw new Error('No management API URL configured'); + if (!base) ctx.fail('No management API URL configured', { kind: ctx.KIND.INVALID }); const apiBase = /\/api$/.test(base) ? base : base + '/api'; const headers = { Authorization: `Token ${vpn.token || ''}`, Accept: 'application/json' }; const r = await fetchJSON(apiBase + '/peers', { headers, timeout: 8000 }); - if (authErr(r)) throw new Error('Auth failed — check the access token'); - if (r.status >= 400) throw new Error('Management API HTTP ' + r.status); + if (authErr(r)) ctx.fail('Auth failed — check the access token', { kind: ctx.KIND.AUTH }); + if (r.status >= 400) ctx.fail('Management API HTTP ' + r.status); const peers = Array.isArray(r.data) ? r.data : []; const connected = peers.filter(p => p && p.connected); out.peersTotal = peers.length; @@ -219,14 +238,15 @@ async function vpnView(config, fetchJSON) { } } } catch (e) { - out.error = e.message; + out.error = errorText(e); } return out; } /* ── Map view ── */ -async function mapView(config, fetchJSON) { +async function mapView(ctx) { + const { config, fetchJSON } = ctx; const wc = config || {}; const services = (Array.isArray(wc.services) ? wc.services : []).filter(s => s && s.enabled !== false && s.url); const results = await Promise.all( @@ -242,15 +262,15 @@ async function mapView(config, fetchJSON) { try { /* Same as the VPN path above: say the address is missing rather than letting a doomed request report itself as a network fault. */ - if (!base) throw new Error('No URL configured'); + if (!base) ctx.fail('No URL configured', { kind: ctx.KIND.INVALID }); if (s.type === 'conduit') { const r = await fetchJSON(new URL('/metrics', base).href, { raw: true }); - if (r.status >= 400) throw new Error('HTTP ' + r.status); + if (r.status >= 400) ctx.fail('HTTP ' + r.status); Object.assign(o, { kind: 'regions' }, parseConduitText(r.data)); } else if (s.type === 'gluetun') { const r = await fetchJSON(base + '/v1/publicip/ip', { headers: s.apiKey ? { 'X-API-Key': s.apiKey } : {} }); - if (r.status === 401) throw new Error('Auth required — set the API key'); - if (r.status >= 400) throw new Error('HTTP ' + r.status); + if (r.status === 401) ctx.fail('Auth required — set the API key', { kind: ctx.KIND.AUTH }); + if (r.status >= 400) ctx.fail('HTTP ' + r.status); const d = r.data || {}; const L = d.location || d.loc; o.kind = 'point'; @@ -265,8 +285,8 @@ async function mapView(config, fetchJSON) { const r = await fetchJSON(base + '/api/peers', { headers: s.token ? { Authorization: 'Token ' + s.token } : {}, }); - if (authErr(r)) throw new Error('Auth required — check the token'); - if (r.status >= 400) throw new Error('HTTP ' + r.status); + if (authErr(r)) ctx.fail('Auth required — check the token', { kind: ctx.KIND.AUTH }); + if (r.status >= 400) ctx.fail('HTTP ' + r.status); const peers = Array.isArray(r.data) ? r.data : []; const regions = {}; let conn = 0; @@ -294,8 +314,8 @@ async function mapView(config, fetchJSON) { headers: { 'Content-Type': 'application/json', Authorization: s.apiKey ? 'Bearer ' + s.apiKey : '' }, body, }); - if (authErr(r)) throw new Error('Auth required — check the API key'); - if (r.status >= 400) throw new Error('HTTP ' + r.status); + if (authErr(r)) ctx.fail('Auth required — check the API key', { kind: ctx.KIND.AUTH }); + if (r.status >= 400) ctx.fail('HTTP ' + r.status); const rows = (r.data && r.data.results) || []; const regions = {}; let total = 0; @@ -317,10 +337,10 @@ async function mapView(config, fetchJSON) { headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ username: s.username || '', password: s.password || '' }), }); - if (authErr(lg)) throw new Error('Auth required — check username/password'); - if (lg.status >= 400) throw new Error('Login HTTP ' + lg.status); + if (authErr(lg)) ctx.fail('Auth required — check username/password', { kind: ctx.KIND.AUTH }); + if (lg.status >= 400) ctx.fail('Login HTTP ' + lg.status); const token = lg.data && lg.data.token; - if (!token) throw new Error('Login failed'); + if (!token) ctx.fail('Login failed', { kind: ctx.KIND.AUTH }); const end = Date.now(), start = end - 7 * 24 * 3600 * 1000; const r = await fetchJSON( @@ -333,7 +353,7 @@ async function mapView(config, fetchJSON) { end, { headers: { Authorization: 'Bearer ' + token } }, ); - if (r.status >= 400) throw new Error('HTTP ' + r.status); + if (r.status >= 400) ctx.fail('HTTP ' + r.status); const rows = Array.isArray(r.data) ? r.data : []; const regions = {}; let total = 0; @@ -353,7 +373,7 @@ async function mapView(config, fetchJSON) { o.error = 'Unsupported service type'; } } catch (e) { - o.error = e.message || String(e); + o.error = errorText(e); } return o; }),