From 077a7ecb94ecb30f7e1e884c30e48e5dfb873d0d Mon Sep 17 00:00:00 2001 From: SandObserver <260779319+SandObserver@users.noreply.github.com> Date: Mon, 10 Aug 2026 17:33:34 -0400 Subject: [PATCH] Security: log only the host when a ping fails The failure record wrote the origin and the request path. Some services carry an API key as a path segment rather than in the query string, so a failed ping could write a credential into the container log, where it persists for the life of the log. The origin alone answers the operator's question, which is which service is unreachable. Credentials in the authority and the query string were already excluded; the path is the remaining half. --- api/src/proxy.js | 8 +++++--- api/test/ping-log.test.js | 20 ++++++++++++++++---- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/api/src/proxy.js b/api/src/proxy.js index ffaba192..3791f3d9 100644 --- a/api/src/proxy.js +++ b/api/src/proxy.js @@ -387,9 +387,11 @@ function pingUrl(raw, ms = PING_MS, skipTls, pinIp) { /* This result reaches the browser as-is. The message names the address it failed to reach; the code does not, so only the code is kept. */ req.on('error', (/** @type {unknown} */ e) => { - /* u.origin excludes credentials in the authority, and the query string - is left off because it can carry an API key. */ - log.warn('ping failed', { url: u.origin + (opts.path || ''), error: errMessage(e) }); + /* u.origin only: it excludes credentials in the authority, and the path + and query are both left off because either can carry an API key. The + operator's question is which service is unreachable, which the origin + answers on its own. */ + log.warn('ping failed', { url: u.origin, error: errMessage(e) }); dl.settle(resolve, { ok: false, status: 0, error: pingErrorText(e), code: errCode(e) }); }); req.end(); diff --git a/api/test/ping-log.test.js b/api/test/ping-log.test.js index bf462adc..1fcfeb5b 100644 --- a/api/test/ping-log.test.js +++ b/api/test/ping-log.test.js @@ -52,7 +52,7 @@ test('a failed ping logs a usable URL', async t => { const w = pingFailure(seen); assert.ok(w, 'the failure should be logged'); - assert.equal(w.data.url, `http://127.0.0.1:${port}/health`); + assert.equal(w.data.url, `http://127.0.0.1:${port}`); assert.ok(!String(w.data.url).includes('undefined'), 'no part of the URL may be undefined'); }); @@ -64,11 +64,11 @@ test('the logged URL names the scheme and the port', async t => { const { url } = pingFailure(seen).data; assert.match(url, /^http:\/\//, 'the scheme distinguishes http from https'); - assert.ok(url.includes(`:${port}/`), `the port must be named: ${url}`); + assert.ok(url.endsWith(`:${port}`), `the port must be named: ${url}`); }); /* A URL may carry an API key in its query string and credentials in its - authority. u.origin excludes both, and the query is deliberately left off. */ + authority. u.origin excludes both. */ test('the logged URL carries neither credentials nor query string', async t => { const port = await closedPort(); const seen = warnings(t); @@ -77,7 +77,19 @@ test('the logged URL carries neither credentials nor query string', async t => { const { url } = pingFailure(seen).data; assert.ok(!url.includes('hunter2'), `credentials must not be logged: ${url}`); assert.ok(!url.includes('SECRET'), `the query string must not be logged: ${url}`); - assert.equal(url, `http://127.0.0.1:${port}/api`); + assert.equal(url, `http://127.0.0.1:${port}`); +}); + +/* Not every service puts its key in the query string. Several carry it as a + path segment instead, which the record used to write out in full. */ +test('a secret in a path segment cannot reach the log', async t => { + const port = await closedPort(); + const seen = warnings(t); + await pingUrl(`http://127.0.0.1:${port}/api/APIKEYINPATH/status`, 2000); + + const { url } = pingFailure(seen).data; + assert.ok(!url.includes('APIKEYINPATH'), `the path must not be logged: ${url}`); + assert.equal(url, `http://127.0.0.1:${port}`); }); /* The error text is logged in full, since the response no longer carries it. */