From 5cecaa080d695c9e7e319bd496677f7cdc458c2f Mon Sep 17 00:00:00 2001 From: NuklearRabbit <145918611+NuklearRabbit@users.noreply.github.com> Date: Sun, 23 Aug 2026 14:44:33 +0200 Subject: [PATCH] fix: stop a large diff and a second SSH error from taking the app down Two failure modes that only show up under conditions the tests never reached. renderDiff built one span per diff line with no bound. A regenerated lock file is an ordinary change: 50,000 lines produce 4 MB of markup and 50,000 elements that then have to be parsed and laid out inside the full shell replacement, and 200,000 lines produce 16 MB. The rendered view now stops at 2,000 lines and says how many were left out; ui.diff keeps the whole change, so Copy diff, the editor and hunk staging are unaffected. The line scan also runs once now instead of three times. withClient registered the connection error handler with once(). A connection that fails and then emits a second error while it is being torn down - a reset during client.end() is the ordinary case - leaves that event unhandled, and an unhandled 'error' on an EventEmitter reaches the uncaughtException handler, which calls app.exit(1). The handler stays attached and ignores anything after the first failure. Both are covered by tests that were confirmed to fail without the fix, together with the SSH paths that had none: host key mismatch reporting, the trusted fingerprint requirement for exec and upload, and remote upload path validation. Co-Authored-By: Claude Opus 5 --- SOURCE_MANIFEST.txt | 21 +++---- reports/architecture-audit.json | 33 ++++++++--- reports/architecture-audit.md | 4 +- src/main/ssh-service.cjs | 6 +- src/renderer/views.js | 22 +++++-- tests/browser/forgeflow.spec.mjs | 31 ++++++++++ tests/ssh-connection.test.mjs | 98 ++++++++++++++++++++++++++++++++ 7 files changed, 188 insertions(+), 27 deletions(-) create mode 100644 tests/ssh-connection.test.mjs diff --git a/SOURCE_MANIFEST.txt b/SOURCE_MANIFEST.txt index 902687d..527d8d1 100644 --- a/SOURCE_MANIFEST.txt +++ b/SOURCE_MANIFEST.txt @@ -107,15 +107,15 @@ c230b931abf2293d2d44b7a69b94c35f1142c093cc46b88739a0de5cbd6d1896 1532 d7cae22a646c8f06f339db799f2b9921ba1799cdc44e5d05578c04aaace103ab 14357 main.cjs 91a984a89dd57a084b9a2331763cacdb061582fb590f13df379d92c1a77a2ee1 352 OVERLAY-INSTRUCTIONS.md 198a8a51705fcea2ddf716558a49e06df5a8181c5476709c474d183b64890ca0 179808 package-lock.json -c579d4a9df6ca10c32a7b8168567bfdb333745438747430329a7a64041f1a4c4 5971 package.json +fb12ca4da1105da46b4aacacb67260357c29aa362385c9cfaaeadeffd1bf82b3 6142 package.json 1237df9ddcbb5ac7dc4316f18c34ff4a7030e3e0d56216ade6dd07369e5e2a04 1353 playwright.config.mjs 16d6c37007c6c14dbf541060535c594308758477b867fc2a190c2946d74022b9 12196 preload.cjs abe5dd6fd68f2970cd19ef134094907c67219061d8fe9a1a08324c78de4ad437 484 PUBLISH-AND-ENABLE-UPDATE.cmd f018383f755352ca448e2ebb1e19b1dba412a3eb793d61e64b02953e300754fd 10538 Publish-ForgeFlow-Release.ps1 688fff7d2c989adb97ebb7fae38962656b70304a0aa5d27433c56adf7f136de0 4196 Publish-Missing-Binary-Release.ps1 42e2f375c2fa9bea138428b85eb2bb93fe8bf6e4a5fc0cd728f2c6713a019df7 10315 README.md -cac1e82069c0addef5cfc1fdb0dfed0a6b00408dbfe0ea95b24ed12e9e73ff13 14947 reports/architecture-audit.json -5f41108efe28200ec629d640e75bdf3ef326da98970ea0b941cbbf7ee9a83c61 1343 reports/architecture-audit.md +90e99e0fd3b92849215857137ca71307913b2558c159db86a4410e0466c249cd 15677 reports/architecture-audit.json +527aa0973a0059ae864d93a4f2393ccbda3ea725ab8036ae2d6ef323a5693fde 1372 reports/architecture-audit.md 509c7bcff5280349bd9f45ed6151f70372bad7010a9ea582c13e2ccab91fe0cd 6272 scripts/acceptance.mjs d0745072321aca2c80f44460974a7926715a9f429164aaf7660dced40b52c736 4790 scripts/apply-binary-update.ps1 404863bcbe7292355662e3a326455df864d7279badc29f90866a3b837420df54 10745 scripts/apply-source-update.ps1 @@ -159,7 +159,7 @@ e64f7257d478955c675a133b3735b6afe138a69d2ad090898061e56f557c43e5 9926 c629efa832a806409954ab5a4986bf88606ce5fa5278bb1885a76222d1cb6faf 6441 src/main/repository-monitor.cjs 6393583911263575c6e2a19d9baab6e638cce90252c386b0a5144f2fb6f81f15 12154 src/main/repository-service.cjs 52b6d88ed1f5c904a13cdde92e5f96d1e2b5971ceef49862152197353cdc6490 27928 src/main/server-inventory.cjs -5830cf038c2404272b743657bfb4ce57c700e2f17b4f063d06afb80c9130bcba 14863 src/main/ssh-service.cjs +c7921a2380e305abdabb94dc0d04c6060eaa09bbe01f18994786f20bfa02d0c3 15107 src/main/ssh-service.cjs 19538a3c40ea3489bbaee9a23af36a5e99962af6bb3d04259f05ece6588cbeb2 25901 src/main/unraid-access-methods.cjs 5ba5a90ad9b7a0d05dd64fa64968cbdd7271bec4df6971f9685a9dd411e4fad4 9536 src/main/unraid-deploy-key-host.cjs 6d9910dace52625f88e066a8485af2663c3735ff15e9ce9031441ce742710a21 30793 src/main/unraid-deployment-methods.cjs @@ -189,8 +189,8 @@ eef2f269ba4fbb76bf66ad328d481b461255d0acb753b30878dd4d4eaac57dc6 6924 81b0155dd51c7bc8871fd29bd7fdb28857cf11a86ed6d6953238c04c2774800c 20033 src/renderer/mock-repository-bridge.js 94fa265c2fe9ca8d644f0ce9b620b6f85d9b25dca5802c4e9195b66dcbe80120 6522 src/renderer/operations.js 21b8f3228bddd6da06613857763282d1f88f4eeab3d063f3a63395d7b567f2e4 80004 src/renderer/styles.css -48dbcdeb844ae991f02ee38f1a6993df27dcd7efe032c3dbc529ece315c68dfb 102537 src/renderer/views.js -c7c204360aba6a9a2cf1b0cd80f7aa13ddd212ecea0ed8b385e4c4cc40bfbefc 1383 src/shared/clone-target.cjs +c6e4bba94b5e4c26793c7e96f287e04e833ef490970bae02bc8dea71919342cb 103195 src/renderer/views.js +e9e72c072a5c5d04f59cd6763de0cfbf736c2a5ffa2f722143f3bad2bdbc630b 1411 src/shared/clone-target.cjs 5d425d5c2f939d0f6beebee7ebb0c77146cb7e318535ba7286ec7081a4dc2269 2497 src/shared/deployment-policy.cjs 029e600229714d033c28e2dcb77817aa8269847001782ae0012960e83ffd183f 3057 src/shared/git-status.cjs 2778ebcbdf60fdc1cb0749f15565e0e1bd66f3a0d31eb70ae7942a7511a3de75 1295 src/shared/repository-match.cjs @@ -203,14 +203,14 @@ f8853dce6fdf360d5df2fbe2b6df3e5687630c807fee5ba8436679b34ec737ea 2436 058aeaa5d9bfe377c7e322f213c7871ecc4151b5d08ef790992f4ee28d857658 743 START-FORGEFLOW-OVERLAY.ps1 f5b0ea887fcdeadec78c1ad49b0ec7979723562f5c0b730703acb77a37281ee0 1009 tests/acceptance.test.mjs a4e5947204ff6878e601e32477bc85b53cd0153baf95a161c8935b6e5466c257 1155 tests/audit-service.test.mjs -4018fa3ba329b02af38efc67e3e03cef4ff8f422eddacb47a18979fedd04e1c7 15344 tests/browser/forgeflow.spec.mjs -0d146d3b3428522df02bfacf80332ca6db065cb05666c0e02114885b8d51dea1 5858 tests/clone-target.test.mjs +64fd88e113f85a56090eb9943b6a3120e10ebbe98e6a1dd1c0da7f73599ba432 16645 tests/browser/forgeflow.spec.mjs +1728c0a7abd92f4d7d9e68df32e4a6b00730555f23795e9b36416795d9d127af 5978 tests/clone-target.test.mjs ac17f8bbe9e388b80abef7792c8b184a1fd482c93f13d23a478e433961020f75 17214 tests/config-store.test.mjs f1463326aee79842d265687ae628189ce54e92544600f2bd14073780287cfb14 2502 tests/configuration-backup.test.mjs 144c8e217a334cd69f502938d944e0f2dac61703d5df47e287b9ed542918c779 8129 tests/dependency-wiring.test.mjs -8ca5b62c691637f1ef35d7acff776f27652a6963bd7414b3cddf764cfd7db48d 9839 tests/deploy-key-host.test.mjs +caab598cd563454ed4281bfa89785eb4795f39da7fdb92b82ca73112714406f0 10033 tests/deploy-key-host.test.mjs b7e009fed4171d6dd6b4c3154ba1d3f7198e98f5b79b298687841fc8169447cd 9354 tests/deploy-key-lifecycle.test.mjs -57d93ac176cc6ff0850b38fcc6ec5be0e8eaca7e457a5a188bf86fa4a38d5b45 24378 tests/deployment-operations.test.mjs +49bf9cf9842e7899015013675208f83a95402065a082320927a677ee4bab0766 24875 tests/deployment-operations.test.mjs 1dc6477bd07de78be189e6e8195ec339eb9d75820c4dbd5b073b8520ee21f6b5 1938 tests/deployment-policy.test.mjs bf4576901e32662d832687a2761852aa1b2cffe256de5044f18c6637c189463b 9780 tests/deployment-status.test.mjs fae3634bae871abade4d487b94b4741b50e787804dbd6135249f634fdd83c6d0 3800 tests/diagnostics.test.mjs @@ -236,6 +236,7 @@ d49c772e3c7ddaa12dc5a1d4fc4cb474a4d99ae06fa5dab5a6cf1c44acb9ed6f 3463 bab853feb0e22aa25af17989baaa632c01efa636533ea67407fecfdd973c7024 627 tests/semver.test.mjs 12cb3b240bdd0922566323c0014838ca067ad10d9d4009943165ae2c4e93bc6f 11786 tests/server-inventory-branches.test.mjs 020eccfa9c4aef7a4ac4736d9af90518fcb6d1ad75aedcfaa1c92832a9e3d6d8 4609 tests/shell-verification.test.mjs +7ee9166327ed227d2b7c6929692dea5c5d7a41c3e566596fa92d9ec4f42e8677 4085 tests/ssh-connection.test.mjs 0d1bc4d623ce299337736c577ec61c8ffd6974ebe20335b72838d10eae35ecb1 7993 tests/ssh-service.test.mjs 8a6a8477eb94b85ccef18cddd2640afb0d1eafa679c96bc7de20428d5d69e1be 1794 tests/tool-invocation.test.mjs db0a347cdab1da11a04d41981695a65d7244367c46f0cdc868326e8d00d73cbf 56694 tests/unraid-deployment.test.mjs diff --git a/reports/architecture-audit.json b/reports/architecture-audit.json index 1e9f510..8bc9756 100644 --- a/reports/architecture-audit.json +++ b/reports/architecture-audit.json @@ -1,10 +1,27 @@ { - "generatedAt": "2026-08-23T12:21:43.185Z", + "generatedAt": "2026-08-23T12:44:33.101Z", "thresholds": { "preferredMaximumLines": 750, "justificationRequiredLines": 1000 }, - "over750": [], + "over750": [ + { + "file": "src/renderer/views.js", + "lines": 759, + "branches": 57, + "functions": 159, + "ipcHandlers": 0, + "responsibilities": [ + "inventory", + "deployment", + "git", + "renderer", + "security", + "updates" + ], + "hotspotScore": 97 + } + ], "over1000": [], "cyclomaticHotspots": [ { @@ -176,8 +193,8 @@ }, { "file": "src/renderer/views.js", - "lines": 749, - "branches": 56, + "lines": 759, + "branches": 57, "functions": 159, "ipcHandlers": 0, "responsibilities": [ @@ -188,7 +205,7 @@ "security", "updates" ], - "hotspotScore": 96 + "hotspotScore": 97 }, { "file": "src/main/ipc.cjs", @@ -374,8 +391,8 @@ }, { "file": "src/main/ssh-service.cjs", - "lines": 332, - "branches": 41, + "lines": 336, + "branches": 43, "functions": 69, "ipcHandlers": 0, "responsibilities": [ @@ -384,7 +401,7 @@ "security", "updates" ], - "hotspotScore": 61 + "hotspotScore": 63 }, { "file": "src/main/deploy-key-lifecycle-service.cjs", diff --git a/reports/architecture-audit.md b/reports/architecture-audit.md index f9c0b90..4f4d9be 100644 --- a/reports/architecture-audit.md +++ b/reports/architecture-audit.md @@ -1,12 +1,12 @@ # ForgeFlow architecture audit -Generated 2026-08-23T12:21:43.185Z. Complexity is a deterministic decision-point count used for hotspot ranking, not a claim of exact McCabe complexity. +Generated 2026-08-23T12:44:33.101Z. Complexity is a deterministic decision-point count used for hotspot ranking, not a claim of exact McCabe complexity. ## Files above 750 lines | File | Lines | Decisions | Functions | IPC handlers | Responsibilities | |---|---:|---:|---:|---:|---| -No findings. +| `src/renderer/views.js` | 759 | 57 | 159 | 0 | inventory, deployment, git, renderer, security, updates | ## Files above 1,000 lines diff --git a/src/main/ssh-service.cjs b/src/main/ssh-service.cjs index 386575f..37b31de 100644 --- a/src/main/ssh-service.cjs +++ b/src/main/ssh-service.cjs @@ -135,7 +135,11 @@ class SshService { finish(resolve, data); } catch (error) { finish(reject, error); } }); - client.once('error', async (error) => { + // Deliberately not `once`: a connection that already failed can emit a + // second error while it is being torn down, and an unhandled 'error' event + // on an EventEmitter terminates the main process. + client.on('error', async (error) => { + if (settled) return; const observed = connection.getObservedFingerprint(); const mismatch = Boolean(server.hostFingerprint && observed && server.hostFingerprint !== observed); const wrapped = new Error(mismatch diff --git a/src/renderer/views.js b/src/renderer/views.js index 33ec63f..ef1ffab 100644 --- a/src/renderer/views.js +++ b/src/renderer/views.js @@ -206,9 +206,14 @@ function linkedWorkloadsForRepository(repository) { ); } -function diffAtmosphere(diff) { +// A regenerated lock file is an ordinary change and runs into tens of thousands +// of lines. One element per line freezes the window while it is parsed and laid +// out, so the tail is left out of the rendered view only. +const DIFF_RENDER_LINE_LIMIT = 2000; + +function diffAtmosphere(diff, allLines = null) { if (!ui.selectedFile) return ""; - const lines = String(diff || "").split("\n"); + const lines = allLines || String(diff || "").split("\n"); const additions = lines.filter( (line) => line.startsWith("+") && !line.startsWith("+++"), ).length; @@ -224,8 +229,9 @@ function diffAtmosphere(diff) { function renderDiff(diff) { if (!diff) return '
↔

No textual diff

Select another file or open the project folder for binary changes.

'; - const rendered = escapeHtml(diff) - .split("\n") + const lines = String(diff).split("\n"); + const rendered = lines + .slice(0, DIFF_RENDER_LINE_LIMIT) .map((line) => { const type = line.startsWith("+") && !line.startsWith("+++") @@ -235,10 +241,14 @@ function renderDiff(diff) { : line.startsWith("@@") ? "hunk" : ""; - return `${line || " "}`; + return `${escapeHtml(line) || " "}`; }) .join(""); - return `${rendered}${diffAtmosphere(diff)}`; + const hidden = Math.max(0, lines.length - DIFF_RENDER_LINE_LIMIT); + const notice = hidden + ? `… ${hidden.toLocaleString()} more line${hidden === 1 ? "" : "s"} are not shown. Copy diff and the editor still give you the complete change.` + : ""; + return `${rendered}${notice}${diffAtmosphere(diff, lines)}`; } function fileStatusCode(file) { if (file.conflict) return "U"; diff --git a/tests/browser/forgeflow.spec.mjs b/tests/browser/forgeflow.spec.mjs index 102315b..1ecafd6 100644 --- a/tests/browser/forgeflow.spec.mjs +++ b/tests/browser/forgeflow.spec.mjs @@ -272,6 +272,37 @@ test("sections that used to be injected after render are part of the rendered ma await expect(auditPanel.locator("table.data-table")).toContainText("deployment.requested"); }); +test("a very large diff is capped instead of freezing the window", async ({ page }) => { + const selected = await page.evaluate(() => { + const withChanges = ui.repositories.find((repository) => repository.localStatus?.counts?.changed); + if (!withChanges) return null; + selectRepository(withChanges.id); + return withChanges.fullName; + }); + expect(selected, "the demo needs a repository with local changes").not.toBeNull(); + await expect(page.locator(".diff-view")).toBeVisible(); + + const measured = await page.evaluate(() => { + const newline = String.fromCharCode(10); + const lines = ["diff --git a/package-lock.json b/package-lock.json"]; + for (let index = 0; index < 40_000; index += 1) lines.push(`+ "package-${index}": "^1.2.3",`); + ui.diff = lines.join(newline); + ui.repositoryTab = "changes"; + const started = performance.now(); + render(); + return { + renderMs: performance.now() - started, + rendered: document.querySelectorAll(".diff-line").length, + storedLines: ui.diff.split(newline).length, + }; + }); + + expect(measured.storedLines).toBe(40_001); + expect(measured.rendered).toBeLessThan(2100); + expect(measured.renderMs).toBeLessThan(3000); + await expect(page.locator(".diff-view")).toContainText("more lines are not shown"); +}); + test("an unchanged render leaves the existing DOM in place", async ({ page }) => { await page.locator('[data-action="select-repo"]').first().click(); const marked = await page.evaluate(() => { diff --git a/tests/ssh-connection.test.mjs b/tests/ssh-connection.test.mjs new file mode 100644 index 0000000..d4cc084 --- /dev/null +++ b/tests/ssh-connection.test.mjs @@ -0,0 +1,98 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { EventEmitter } from "node:events"; +import { createRequire } from "node:module"; + +const require = createRequire(import.meta.url); + +// SshService resolves ssh2 lazily, so replacing the cached module is enough to +// drive a real connection lifecycle without a server. +const ssh2Path = require.resolve("ssh2"); +const realSsh2 = require("ssh2"); + +function withFakeSsh2(Client, run) { + require.cache[ssh2Path] = { id: ssh2Path, filename: ssh2Path, loaded: true, exports: { ...realSsh2, Client } }; + try { + return run(); + } finally { + require.cache[ssh2Path] = { id: ssh2Path, filename: ssh2Path, loaded: true, exports: realSsh2 }; + } +} + +const { SshService } = require("../src/main/ssh-service.cjs"); + +function store(server = {}) { + return { + getServer: () => ({ id: "unraid", host: "tower", port: 22, username: "root", authType: "password", basePath: "/mnt/user/appdata", hostFingerprint: "SHA256:trusted", ...server }), + getServerCredentials: () => ({ password: "secret", passphrase: "" }), + }; +} + +test("a connection that fails twice rejects once and never terminates the process", async () => { + class DoubleFailingClient extends EventEmitter { + connect() { + setImmediate(() => this.emit("error", Object.assign(new Error("connect ECONNREFUSED"), { code: "ECONNREFUSED" }))); + } + end() { + // The socket resets shortly after teardown. An unhandled 'error' event on + // an EventEmitter takes the whole main process down. + setImmediate(() => this.emit("error", new Error("read ECONNRESET"))); + } + } + + const service = withFakeSsh2(DoubleFailingClient, () => new SshService({ store: store(), diagnostics: null })); + await assert.rejects( + () => withFakeSsh2(DoubleFailingClient, () => service.exec("unraid", "true")), + (error) => { + assert.equal(error.code, "ECONNREFUSED"); + assert.match(error.message, /SSH connection failed/); + return true; + }, + ); + + // Give the delayed teardown error time to land while the test is still running. + await new Promise((resolve) => setTimeout(resolve, 50)); +}); + +test("a host key that does not match the trusted fingerprint is reported as an identity change", async () => { + class MismatchingClient extends EventEmitter { + connect(options) { + options.hostVerifier(Buffer.from("a different host key")); + setImmediate(() => this.emit("error", new Error("handshake failed"))); + } + end() {} + } + + const service = withFakeSsh2(MismatchingClient, () => new SshService({ store: store(), diagnostics: null })); + await assert.rejects( + () => withFakeSsh2(MismatchingClient, () => service.exec("unraid", "true")), + (error) => { + assert.equal(error.code, "SSH_HOST_KEY_MISMATCH"); + assert.match(error.message, /SSH host identity changed/); + assert.equal(error.expectedFingerprint, "SHA256:trusted"); + assert.ok(error.observedFingerprint.startsWith("SHA256:")); + return true; + }, + ); +}); + +test("running a command requires a trusted host fingerprint", async () => { + const service = new SshService({ store: store({ hostFingerprint: "" }), diagnostics: null }); + await assert.rejects(() => service.exec("unraid", "true"), (error) => { + assert.equal(error.code, "SSH_HOST_NOT_TRUSTED"); + return true; + }); + await assert.rejects(() => service.uploadBuffer("unraid", "/mnt/user/appdata/x", "data"), (error) => { + assert.equal(error.code, "SSH_HOST_NOT_TRUSTED"); + return true; + }); +}); + +test("a remote upload path may not escape into an arbitrary location", () => { + const service = new SshService({ store: store(), diagnostics: null }); + assert.equal(service.ensureUploadTarget("/mnt/user/appdata/app/file.tar"), "/mnt/user/appdata/app/file.tar"); + assert.equal(service.ensureUploadTarget("\\mnt\\user\\appdata\\app"), "/mnt/user/appdata/app"); + for (const value of ["relative/path", "/mnt/../etc/passwd", "/mnt/user/../../etc", "", null]) { + assert.throws(() => service.ensureUploadTarget(value), /absolute safe Unix path/); + } +});