From 1c8e9067ee730e200b5bf3a7f8df2cb683e4142d Mon Sep 17 00:00:00 2001
From: Sucukdeluxe <259325684+Sucukdeluxe@users.noreply.github.com>
Date: Thu, 13 Aug 2026 22:48:21 +0200
Subject: [PATCH] fix: fail closed for diagnostic history reads

Require the decrypted configuration and its complete secret set before loading or returning any diagnostic history data. Propagate configuration failures so the agent response boundary falls back to its generic safe error instead of emitting unredacted history content.

Reject thrown and non-array dedicated history reader failures as unhealthy operations without consulting stale config history. Preserve redaction of reader errors at the agent boundary.

Keep caller-owned history snapshots unchanged by isolating the per-hoster summarizer from the reader array, and cover get_history, list_errors, and server_health with focused red-green regressions.
---
 lib/diagnostics-collectors.js        |  34 +++++----
 tests/diagnostics-collectors.test.js | 109 ++++++++++++++++++++++-----
 2 files changed, 110 insertions(+), 33 deletions(-)

diff --git a/lib/diagnostics-collectors.js b/lib/diagnostics-collectors.js
index 01d5bea..f1bd6f2 100644
--- a/lib/diagnostics-collectors.js
+++ b/lib/diagnostics-collectors.js
@@ -16,18 +16,24 @@ function createCollectors(deps) {
   const { loadConfig, loadHistory, getAllLogPaths, support, stats, appInfo, systemInfo, agentInfo } = deps;
 
   function _secrets() {
-    try { return support.collectSecretValues(loadConfig()); } catch { return []; }
+    return support.collectSecretValues(loadConfig());
   }
 
   function _currentHistory() {
     if (typeof loadHistory === 'function') {
       const history = loadHistory();
-      return Array.isArray(history) ? history : [];
+      if (!Array.isArray(history)) throw new Error('History reader returned invalid data');
+      return history;
     }
     const cfg = loadConfig();
     return Array.isArray(cfg && cfg.history) ? cfg.history : [];
   }
 
+  function _historyContext() {
+    const secrets = _secrets();
+    return { history: _currentHistory(), secrets };
+  }
+
   function _deepRedact(value, secrets) {
     return support.valueScrub(value, secrets || _secrets());
   }
@@ -139,10 +145,9 @@ function createCollectors(deps) {
     return { events: out.slice(-limit), truncated: out.length > limit };
   }
 
-  function _historyErrors(history, opts) {
+  function _historyErrors(history, opts, secrets) {
     const o = opts || {};
     const sinceMs = Number.isFinite(o.sinceMs) ? o.sinceMs : null;
-    const secrets = _secrets();
     const errors = [];
     const byCategory = {};
     for (const batch of (Array.isArray(history) ? history : [])) {
@@ -171,16 +176,17 @@ function createCollectors(deps) {
     return { errors, byCategory };
   }
 
-  function _listErrors(args, history) {
+  function _listErrors(args, history, secrets) {
     const a = args || {};
-    const { errors, byCategory } = _historyErrors(history, a);
+    const { errors, byCategory } = _historyErrors(history, a, secrets);
     const limit = Math.min(Math.max(Number(a.limit) || 100, 1), 1000);
     const window = Number.isFinite(a.sinceMs) ? `since ${new Date(a.sinceMs).toISOString()}` : 'all history';
     return { window, total: errors.length, byCategory, errors: errors.slice(-limit) };
   }
 
   function listErrors(args) {
-    return _listErrors(args, _currentHistory());
+    const { history, secrets } = _historyContext();
+    return _listErrors(args, history, secrets);
   }
 
   function getQueueState(args) {
@@ -212,12 +218,11 @@ function createCollectors(deps) {
     return result;
   }
 
-  function _getHistory(args, history) {
+  function _getHistory(args, history, secrets) {
     const a = args || {};
     const limit = Math.min(Math.max(Number(a.limit) || 20, 1), 200);
-    const perHoster = stats.summarizePerHoster(history);
+    const perHoster = stats.summarizePerHoster([...history]);
     const recent = [...history].slice(-limit).reverse();
-    const secrets = _secrets();
     const batches = recent.map(b => {
       const out = { timestamp: b.timestamp || null, fileCount: Array.isArray(b.files) ? b.files.length : 0 };
       if (a.includeFiles) {
@@ -237,7 +242,8 @@ function createCollectors(deps) {
   }
 
   function getHistory(args) {
-    return _getHistory(args, _currentHistory());
+    const { history, secrets } = _historyContext();
+    return _getHistory(args, history, secrets);
   }
 
   function getRotationState() {
@@ -259,10 +265,10 @@ function createCollectors(deps) {
     const a = args || {};
     const errorLimit = Math.min(Math.max(Number(a.errorLimit) || 20, 1), 200);
     const errArgs = Number.isFinite(a.errorSinceMs) ? { sinceMs: a.errorSinceMs, limit: errorLimit } : { limit: errorLimit };
-    const currentHistory = _currentHistory();
-    const errors = _listErrors(errArgs, currentHistory);
+    const { history: currentHistory, secrets } = _historyContext();
+    const errors = _listErrors(errArgs, currentHistory, secrets);
     const queue = getQueueState({ includeJobs: false });
-    const history = _getHistory({ limit: 5 }, currentHistory);
+    const history = _getHistory({ limit: 5 }, currentHistory, secrets);
     const warnings = [];
     if (queue.source === 'persisted' && queue.stale) warnings.push('queue state is from the persisted snapshot (may lag live state; UploadManager not introspected in this build).');
     if (errors.total > 0) warnings.push(`${errors.total} non-success result(s) in the error window.`);
diff --git a/tests/diagnostics-collectors.test.js b/tests/diagnostics-collectors.test.js
index e115e4b..b239563 100644
--- a/tests/diagnostics-collectors.test.js
+++ b/tests/diagnostics-collectors.test.js
@@ -6,6 +6,7 @@ const path = require('path');
 const support = require('../lib/support-bundle');
 const stats = require('../lib/stats');
 const { createCollectors } = require('../lib/diagnostics-collectors');
+const { createAgent } = require('../lib/diagnostics-agent');
 
 function makeFixture() {
   const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'mhu-diag-'));
@@ -116,30 +117,100 @@ test('getHistory, listErrors and serverHealth share loadHistory after migration'
   assert.equal(health.recentBatches[0].timestamp, health.errors.errors[0].ts, 'serverHealth must summarize one history snapshot');
 });
 
-test('history commands fail closed instead of reading stale config history', () => {
+test('getHistory, listErrors and serverHealth leave the supplied history snapshot unchanged', () => {
+  const original = [
+    { timestamp: '2026-01-02T00:00:00.000Z', files: [{ name: 'newer.mkv', results: [{ hoster: 'voe.sx', status: 'done' }] }] },
+    { timestamp: '2026-01-01T00:00:00.000Z', files: [{ name: 'older.mkv', results: [{ hoster: 'voe.sx', status: 'error', error: 'Not video file format' }] }] }
+  ];
+  const sortingStats = {
+    ...stats,
+    summarizePerHoster: history => {
+      history.sort((a, b) => Date.parse(a.timestamp) - Date.parse(b.timestamp));
+      return stats.summarizePerHoster(history);
+    }
+  };
+  for (const [name, invoke] of [
+    ['listErrors', c => c.listErrors({})],
+    ['getHistory', c => c.getHistory({ includeFiles: true })],
+    ['serverHealth', c => c.serverHealth({})]
+  ]) {
+    const snapshot = JSON.parse(JSON.stringify(original));
+    const c = createCollectors({
+      loadConfig: () => ({ hosters: {}, globalSettings: {}, history: [] }),
+      loadHistory: () => snapshot,
+      getAllLogPaths: () => ({ logDir: os.tmpdir() }),
+      support, stats: sortingStats,
+      appInfo: () => ({}), systemInfo: () => ({}), agentInfo: () => ({})
+    });
+    invoke(c);
+    assert.deepEqual(snapshot, original, `${name} must not mutate the supplied history snapshot`);
+  }
+});
+
+test('dedicated history reader failures never report healthy empty history or use stale config history', () => {
+  const sensitive = 'reader-private-value-38152';
+  const config = {
+    hosters: { 'voe.sx': [{ apiKey: sensitive }] },
+    globalSettings: {},
+    history: [{ timestamp: '2025-12-31T00:00:00.000Z', files: [{ name: 'stale.mkv', results: [{ hoster: 'voe.sx', status: 'error', error: 'stale error' }] }] }]
+  };
+  for (const [scenario, loadHistory] of [
+    ['throwing reader', () => { throw new Error(`history reader failed with ${sensitive}`); }],
+    ['invalid reader result', () => ({ history: [] })]
+  ]) {
+    const c = createCollectors({
+      loadConfig: () => JSON.parse(JSON.stringify(config)),
+      loadHistory,
+      getAllLogPaths: () => ({ logDir: os.tmpdir() }),
+      support, stats,
+      appInfo: () => ({}), systemInfo: () => ({}), agentInfo: () => ({})
+    });
+    for (const [name, invoke] of [
+      ['getHistory', () => c.getHistory({ includeFiles: true })],
+      ['listErrors', () => c.listErrors({})],
+      ['serverHealth', () => c.serverHealth({})]
+    ]) {
+      assert.throws(invoke, /history/i, `${name} must reject a ${scenario}`);
+    }
+    const agent = createAgent(c);
+    for (const op of ['get_history', 'list_errors', 'server_health']) {
+      const response = agent.handle(op, { includeFiles: true });
+      const json = JSON.stringify(response);
+      assert.equal(response.ok, false, `${op} must report the ${scenario} as unhealthy`);
+      assert.match(response.error, /history/i);
+      assert.ok(!json.includes(sensitive));
+      assert.ok(!json.includes('stale.mkv'));
+    }
+  }
+});
+
+test('history operations fail closed when configured secrets cannot be loaded', () => {
+  const sensitive = 'history-private-value-27491';
+  let historyReads = 0;
   const c = createCollectors({
-    loadConfig: () => ({
-      hosters: {},
-      globalSettings: {},
-      history: [{ timestamp: '2025-12-31T00:00:00.000Z', files: [{ name: 'stale.mkv', results: [{ hoster: 'voe.sx', status: 'error', error: 'stale error' }] }] }]
-    }),
-    loadHistory: () => null,
+    loadConfig: () => { throw new Error(`secret decryption failed near ${sensitive}`); },
+    loadHistory: () => {
+      historyReads++;
+      return [{ timestamp: '2026-01-04T00:00:00.000Z', files: [{ name: `${sensitive}.mkv`, results: [{ hoster: 'voe.sx', status: 'error', error: `opaque ${sensitive}` }] }] }];
+    },
     getAllLogPaths: () => ({ logDir: os.tmpdir() }),
     support, stats,
     appInfo: () => ({}), systemInfo: () => ({}), agentInfo: () => ({})
   });
-  const health = c.serverHealth({ errorLimit: 5 });
-  assert.deepEqual({
-    historyBatches: c.getHistory({ limit: 5 }).totalBatches,
-    listedErrors: c.listErrors({ limit: 5 }).total,
-    healthBatches: health.recentBatches.length,
-    healthErrors: health.errors.total
-  }, {
-    historyBatches: 0,
-    listedErrors: 0,
-    healthBatches: 0,
-    healthErrors: 0
-  });
+  for (const [name, invoke] of [
+    ['getHistory', () => c.getHistory({ includeFiles: true })],
+    ['listErrors', () => c.listErrors({})],
+    ['serverHealth', () => c.serverHealth({})]
+  ]) {
+    assert.throws(invoke, /secret decryption failed/, `${name} must fail without the configured redaction secrets`);
+  }
+  assert.equal(historyReads, 0, 'history must not be read before the redaction secrets are available');
+  const agent = createAgent(c);
+  for (const op of ['get_history', 'list_errors', 'server_health']) {
+    const response = agent.handle(op, { includeFiles: true });
+    assert.deepEqual(response, { ok: false, error: 'diagnostic response could not be safely returned' });
+    assert.ok(!JSON.stringify(response).includes(sensitive));
+  }
 });
 
 test('readLog redacts a planted token and a Bearer line; doodstream is NOT readable; unknown name rejected', () => {