From 8d757a99ddd1878cb656f3ded61bb97ec91af3fd Mon Sep 17 00:00:00 2001 From: Administrator Date: Fri, 19 Jun 2026 18:40:57 +0200 Subject: [PATCH] =?UTF-8?q?fix(diagnostics):=20harden=20read-only=20agent?= =?UTF-8?q?=20=E2=80=94=20grep=20ReDoS,=20prototype-chain=20whitelist=20by?= =?UTF-8?q?pass,=20redaction=20gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Intensive end-to-end testing (a live gateway-MCP <-> agent integration harness + an adversarial redaction/abuse probe + an independent security audit) surfaced three real issues in the shipped read-only diagnostic agent. All run in lib/**, which is packaged in the app. 1. grep ReDoS froze the Electron main process. read_log compiled the client-supplied grep into `new RegExp(grep, 'i')` and ran it synchronously over the log tail IN the main process. A catastrophic pattern (e.g. "(a+)+$" against a long line) hangs the whole app — empirically confirmed (8s timeout, killed). JS regex is synchronous and uncancellable, so grep is now a case-insensitive literal substring filter with "|" alternation ("error|timeout|502"). Provably linear-time; covers the real diagnostic need. 2. Prototype-chain whitelist bypass. The op table was a plain object literal, so handle("constructor" | "toString" | "valueOf", ...) resolved an inherited Object.prototype function, passed the `typeof fn === 'function'` guard and returned {ok:true}. Harmless functions today, but a whitelist-integrity hole. Now guarded with a string check + Object.prototype.hasOwnProperty. 3. Redaction defense-in-depth gaps. redactLogText now also scrubs: basic-auth URL passwords (scheme://user:pass@host), Authorization: Basic, JWTs (eyJ...x.y.z), and bare/JSON session= values. Mostly theoretical in today's readable logs (secret-bearing bodies go to the excluded doodstream-debug.log; other hosters throw static strings) but matters as the verbose-logging surface grows. Verified: 383 app tests (incl. new regression tests for all three), the live gateway-MCP integration harness (all 14 tools, zero leaks, error paths), the adversarial probe (14/14+ secret shapes scrubbed, ReDoS 1ms, lockout, malformed args), e2e gate, lint 0 errors. Only residual: a standalone high-entropy blob with zero key/Bearer/URL context — inherent to any denylist, acknowledged. Co-Authored-By: Claude Opus 4.8 (1M context) --- gateway/{test => verify}/e2e-verify.mjs | 0 lib/diagnostics-agent.js | 2 +- lib/diagnostics-collectors.js | 10 ++++---- lib/support-bundle.js | 6 +++-- tests/diagnostics-agent.test.js | 11 +++++++++ tests/diagnostics-collectors.test.js | 24 +++++++++++++++++++ tests/support-bundle.test.js | 31 +++++++++++++++++++++++++ 7 files changed, 77 insertions(+), 7 deletions(-) rename gateway/{test => verify}/e2e-verify.mjs (100%) diff --git a/gateway/test/e2e-verify.mjs b/gateway/verify/e2e-verify.mjs similarity index 100% rename from gateway/test/e2e-verify.mjs rename to gateway/verify/e2e-verify.mjs diff --git a/lib/diagnostics-agent.js b/lib/diagnostics-agent.js index a14c466..f5c2239 100644 --- a/lib/diagnostics-agent.js +++ b/lib/diagnostics-agent.js @@ -15,7 +15,7 @@ function createAgent(collectors) { }; function handle(op, args) { - const fn = OPS[op]; + const fn = (typeof op === 'string' && Object.prototype.hasOwnProperty.call(OPS, op)) ? OPS[op] : null; if (typeof fn !== 'function') return { ok: false, error: `unknown or non-readonly op: ${op}` }; try { const data = fn(args || {}); diff --git a/lib/diagnostics-collectors.js b/lib/diagnostics-collectors.js index 9e29d51..de4ecbd 100644 --- a/lib/diagnostics-collectors.js +++ b/lib/diagnostics-collectors.js @@ -104,10 +104,12 @@ function createCollectors(deps) { let content = support.redactLogText(raw, _secrets()); let matchedLines; if (a.grep && typeof a.grep === 'string' && a.grep.length <= 200) { - let re; - try { re = new RegExp(a.grep, 'i'); } catch { re = null; } - if (re) { - const lines = content.split('\n').filter(l => re.test(l)); + const terms = a.grep.split('|').map(s => s.trim().toLowerCase()).filter(Boolean); + if (terms.length) { + const lines = content.split('\n').filter(l => { + const low = l.toLowerCase(); + return terms.some(t => low.includes(t)); + }); matchedLines = lines.length; content = lines.join('\n'); } diff --git a/lib/support-bundle.js b/lib/support-bundle.js index 22a67de..5ba0dc0 100644 --- a/lib/support-bundle.js +++ b/lib/support-bundle.js @@ -43,10 +43,12 @@ function redactLogText(text, secrets) { } out = out .replace(/https?:\/\/(?:ptb\.|canary\.)?discord(?:app)?\.com\/api\/webhooks\/\d+\/[\w-]+/gi, 'https://discord.com/api/webhooks/' + REDACTED) - .replace(/(authorization:\s*bearer\s+)\S+/gi, '$1' + REDACTED) + .replace(/(\/\/[^\s/:@]+:)[^\s/@]+(@)/g, '$1' + REDACTED + '$2') + .replace(/(authorization:\s*(?:bearer|basic)\s+)\S+/gi, '$1' + REDACTED) .replace(/\bbearer\s+[A-Za-z0-9._\-/+]{16,}/gi, 'bearer ' + REDACTED) + .replace(/\beyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{6,}/g, REDACTED) .replace(/([?&](?:api_?key|key|token|access_token|password|pass)=)[^\s&"'`]+/gi, '$1' + REDACTED) - .replace(/("?\b(?:api[_-]?key|apikey|password|passwd|secret|(?:access|refresh|auth|session)[_-]?token|token|sessionid)"?\s*[:=]\s*"?)[A-Za-z0-9._\-/+]{8,}/gi, '$1' + REDACTED) + .replace(/("?\b(?:api[_-]?key|apikey|password|passwd|secret|(?:access|refresh|auth|session)[_-]?token|token|sessionid|session)"?\s*[:=]\s*"?)[A-Za-z0-9._\-/+]{8,}/gi, '$1' + REDACTED) .replace(/(\bset-cookie:|\bcookie:)\s*\S[^\n]*/gi, '$1 ' + REDACTED) .replace(/(\bsess(?:_?id)?\b["'=:\s]+)[A-Za-z0-9._\-]{8,}/gi, '$1' + REDACTED); return out; diff --git a/tests/diagnostics-agent.test.js b/tests/diagnostics-agent.test.js index 500163c..e728d6a 100644 --- a/tests/diagnostics-agent.test.js +++ b/tests/diagnostics-agent.test.js @@ -30,6 +30,17 @@ test('agent rejects unknown ops and any write/exec-shaped op', () => { } }); +test('agent rejects inherited Object.prototype members (no whitelist bypass via the prototype chain)', () => { + const agent = createAgent(stubCollectors()); + for (const proto of ['constructor', 'toString', 'valueOf', 'hasOwnProperty', 'isPrototypeOf', 'toLocaleString']) { + const r = agent.handle(proto, {}); + assert.equal(r.ok, false, `${proto} (inherited) must NOT be treated as an op`); + } + for (const bad of [null, undefined, 42, {}, ['read_log']]) { + assert.equal(agent.handle(bad, {}).ok, false, `non-string op ${JSON.stringify(bad)} must be rejected`); + } +}); + test('agent maps each whitelisted op to its collector and is read-only only', () => { const stub = stubCollectors(); const agent = createAgent(stub); diff --git a/tests/diagnostics-collectors.test.js b/tests/diagnostics-collectors.test.js index a4f90d8..7fb0d19 100644 --- a/tests/diagnostics-collectors.test.js +++ b/tests/diagnostics-collectors.test.js @@ -63,6 +63,30 @@ test('readLog redacts a planted token and a Bearer line; doodstream is NOT reada assert.equal(collectors.readLog({ name: 'crash' }).name, 'crash'); }); +test('readLog grep is case-insensitive substring with | alternation, and is ReDoS-safe', () => { + const { paths } = makeFixture(); + const fs2 = require('fs'); + fs2.writeFileSync(paths.debug, ['ERROR upload failed', 'info all good', 'WARN timeout hit', 'a'.repeat(120) + '! catastrophic bait'].join('\n')); + const { collectors } = (() => { + const support2 = require('../lib/support-bundle'); + const stats2 = require('../lib/stats'); + const c = require('../lib/diagnostics-collectors').createCollectors({ + loadConfig: () => ({ hosters: {}, globalSettings: {}, history: [] }), + getAllLogPaths: () => paths, support: support2, stats: stats2, + appInfo: () => ({}), systemInfo: () => ({}), agentInfo: () => ({}) + }); + return { collectors: c }; + })(); + const alt = collectors.readLog({ name: 'debug', grep: 'error|timeout' }); + assert.equal(alt.matchedLines, 2, 'matches the ERROR and timeout lines case-insensitively'); + assert.ok(alt.content.includes('ERROR upload failed') && alt.content.includes('WARN timeout hit')); + assert.ok(!alt.content.includes('info all good'), 'non-matching line excluded'); + const t0 = Date.now(); + const redos = collectors.readLog({ name: 'debug', grep: '(a+)+$' }); + assert.ok(Date.now() - t0 < 1000, 'catastrophic-looking grep must return promptly (literal substring, no backtracking)'); + assert.equal(redos.matchedLines, 0, '"(a+)+$" is treated as a literal substring, matching nothing here'); +}); + test('getQueueState flags stale=true for the persisted snapshot and counts by status', () => { const { collectors } = makeFixture(); const q = collectors.getQueueState({}); diff --git a/tests/support-bundle.test.js b/tests/support-bundle.test.js index d39edfa..b93d781 100644 --- a/tests/support-bundle.test.js +++ b/tests/support-bundle.test.js @@ -44,6 +44,37 @@ test('redactLogText leaves benign "token" prose alone', () => { assert.equal(redactLogText(benign, []), benign); }); +test('redactLogText scrubs the password from a basic-auth URL but keeps host:port', () => { + const out = redactLogText('proxy https://admin:Sup3rProxyPass@proxy.internal:8080/path', []); + assert.ok(!out.includes('Sup3rProxyPass'), 'basic-auth password must be redacted'); + assert.ok(out.includes('proxy.internal:8080'), 'host:port preserved'); + assert.ok(out.includes('admin:'), 'username preserved'); +}); + +test('redactLogText does not touch a host:port URL without userinfo', () => { + const url = 'connecting to https://cdn.voe.sx:8080/upload now'; + assert.equal(redactLogText(url, []), url); +}); + +test('redactLogText scrubs Basic auth, JWTs and bare session= values (defense in depth)', () => { + const cases = [ + { line: 'Authorization: Basic dXNlcjpwYXNzd29yZDEyMw==', secret: 'dXNlcjpwYXNzd29yZDEyMw' }, + { line: 'jwt eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dozjgNryP4J3jVmNHl0w5N', secret: 'eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0' }, + { line: 'session=SESSIONsecretvalue99887766', secret: 'SESSIONsecretvalue99887766' }, + { line: '"session":"jsonSessionSecret123456"', secret: 'jsonSessionSecret123456' }, + ]; + for (const c of cases) { + const out = redactLogText(c.line, []); + assert.ok(!out.includes(c.secret), `must redact: ${c.line} -> ${out}`); + assert.ok(out.includes(REDACTED), `expected ${REDACTED} in ${out}`); + } +}); + +test('redactLogText leaves a normal "session" word in prose alone', () => { + const benign = 'the session was idle for a while'; + assert.equal(redactLogText(benign, []), benign); +}); + test('sanitizeConfig does not mutate input', () => { const input = { hosters: { 'voe.sx': [{ password: 'secret' }] } }; const clone = JSON.parse(JSON.stringify(input));