fix(diagnostics): harden read-only agent — grep ReDoS, prototype-chain whitelist bypass, redaction gaps
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) <noreply@anthropic.com>
This commit is contained in:
parent
602e48cb01
commit
8d757a99dd
@ -15,7 +15,7 @@ function createAgent(collectors) {
|
|||||||
};
|
};
|
||||||
|
|
||||||
function handle(op, args) {
|
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}` };
|
if (typeof fn !== 'function') return { ok: false, error: `unknown or non-readonly op: ${op}` };
|
||||||
try {
|
try {
|
||||||
const data = fn(args || {});
|
const data = fn(args || {});
|
||||||
|
|||||||
@ -104,10 +104,12 @@ function createCollectors(deps) {
|
|||||||
let content = support.redactLogText(raw, _secrets());
|
let content = support.redactLogText(raw, _secrets());
|
||||||
let matchedLines;
|
let matchedLines;
|
||||||
if (a.grep && typeof a.grep === 'string' && a.grep.length <= 200) {
|
if (a.grep && typeof a.grep === 'string' && a.grep.length <= 200) {
|
||||||
let re;
|
const terms = a.grep.split('|').map(s => s.trim().toLowerCase()).filter(Boolean);
|
||||||
try { re = new RegExp(a.grep, 'i'); } catch { re = null; }
|
if (terms.length) {
|
||||||
if (re) {
|
const lines = content.split('\n').filter(l => {
|
||||||
const lines = content.split('\n').filter(l => re.test(l));
|
const low = l.toLowerCase();
|
||||||
|
return terms.some(t => low.includes(t));
|
||||||
|
});
|
||||||
matchedLines = lines.length;
|
matchedLines = lines.length;
|
||||||
content = lines.join('\n');
|
content = lines.join('\n');
|
||||||
}
|
}
|
||||||
|
|||||||
@ -43,10 +43,12 @@ function redactLogText(text, secrets) {
|
|||||||
}
|
}
|
||||||
out = out
|
out = out
|
||||||
.replace(/https?:\/\/(?:ptb\.|canary\.)?discord(?:app)?\.com\/api\/webhooks\/\d+\/[\w-]+/gi, 'https://discord.com/api/webhooks/' + REDACTED)
|
.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(/\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(/([?&](?: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(/(\bset-cookie:|\bcookie:)\s*\S[^\n]*/gi, '$1 ' + REDACTED)
|
||||||
.replace(/(\bsess(?:_?id)?\b["'=:\s]+)[A-Za-z0-9._\-]{8,}/gi, '$1' + REDACTED);
|
.replace(/(\bsess(?:_?id)?\b["'=:\s]+)[A-Za-z0-9._\-]{8,}/gi, '$1' + REDACTED);
|
||||||
return out;
|
return out;
|
||||||
|
|||||||
@ -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', () => {
|
test('agent maps each whitelisted op to its collector and is read-only only', () => {
|
||||||
const stub = stubCollectors();
|
const stub = stubCollectors();
|
||||||
const agent = createAgent(stub);
|
const agent = createAgent(stub);
|
||||||
|
|||||||
@ -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');
|
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', () => {
|
test('getQueueState flags stale=true for the persisted snapshot and counts by status', () => {
|
||||||
const { collectors } = makeFixture();
|
const { collectors } = makeFixture();
|
||||||
const q = collectors.getQueueState({});
|
const q = collectors.getQueueState({});
|
||||||
|
|||||||
@ -44,6 +44,37 @@ test('redactLogText leaves benign "token" prose alone', () => {
|
|||||||
assert.equal(redactLogText(benign, []), benign);
|
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', () => {
|
test('sanitizeConfig does not mutate input', () => {
|
||||||
const input = { hosters: { 'voe.sx': [{ password: 'secret' }] } };
|
const input = { hosters: { 'voe.sx': [{ password: 'secret' }] } };
|
||||||
const clone = JSON.parse(JSON.stringify(input));
|
const clone = JSON.parse(JSON.stringify(input));
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user