From fa1e14c8d12ab8d79a2243b8190a09d84e8720ff Mon Sep 17 00:00:00 2001 From: meels Date: Tue, 28 Jul 2026 14:34:43 +0200 Subject: [PATCH] fix: reject cmd.exe caret and control chars; fix subprocess lifecycle bugs in runIngest --- .obsidian/plugins/webinar-dash/main.js | 91 +++++++++++++------ .../plugins/webinar-dash/test/safety.test.js | 8 ++ 2 files changed, 73 insertions(+), 26 deletions(-) diff --git a/.obsidian/plugins/webinar-dash/main.js b/.obsidian/plugins/webinar-dash/main.js index 02fe3bc..c40afe8 100644 --- a/.obsidian/plugins/webinar-dash/main.js +++ b/.obsidian/plugins/webinar-dash/main.js @@ -158,9 +158,17 @@ function reconcileConcepts(rows, conceptNames) { // `shell: true` is required on Windows to resolve `claude.cmd`, which puts the // filename into a shell string. Reject anything cmd.exe or a POSIX shell would -// interpret. Apostrophes and cyrillic are safe inside double quotes and are -// present in real filenames, so they stay allowed. -const UNSAFE_CHARS = /["`$&|;<>%\r\n]/; +// interpret, plus every control character. +// +// `^` is cmd.exe's escape character and belongs to the same class Node escapes +// in its CVE-2024-27980 mitigation for exactly this spawn-through-.cmd shape. +// Control characters are rejected because a NUL byte makes spawn() throw +// synchronously, which would otherwise strand the row mid-run. +// +// Apostrophes, spaces, cyrillic, em dashes and `!` stay allowed — they are safe +// inside double quotes and appear in real filenames in this vault. (`!` would +// matter only under `setlocal enabledelayedexpansion`, which is not in play.) +const UNSAFE_CHARS = /["`$&|;<>%^\u0000-\u001f\u007f]/; function isSafeFilename(name) { if (typeof name !== "string" || name.length === 0) return false; @@ -349,6 +357,7 @@ function renderRightPane(container, parsed, reconciled) { class WebinarDashPlugin extends PluginBase { async onload() { + this.running = new Set(); this.registerMarkdownCodeBlockProcessor("webinar-dash", async (source, el, ctx) => { const cfg = parseConfig(source); const root = el.createDiv({ cls: "webinar-dash" }); @@ -411,11 +420,19 @@ class WebinarDashPlugin extends PluginBase { } runIngest(file, rowEl) { - if (rowEl.dataset.wdRunning === "1") return; - const Notice = OB ? OB.Notice : null; const notify = (msg) => { if (Notice) new Notice(msg); }; + // In-flight state is keyed on the file, not the row. Obsidian rebuilds the + // row on every re-render, so a row-scoped guard would let a re-render hand + // out a fresh row whose guard is unset — and a second click would then run + // a second unsupervised agent against the same file, concurrently writing + // the same wiki pages as the first. + if (this.running.has(file.path)) { + notify(`Already ingesting ${file.name}.`); + return; + } + if (!isSafeFilename(file.name)) { rowEl.createDiv({ cls: "wd-output", @@ -438,54 +455,76 @@ class WebinarDashPlugin extends PluginBase { return; } - rowEl.dataset.wdRunning = "1"; + // A retry reuses the same row. Clear the previous run's status and output + // so they are replaced rather than stacked on top of each other. + rowEl.querySelectorAll(".wd-status, .wd-output").forEach((el) => el.remove()); + const button = rowEl.querySelector("button"); if (button) button.disabled = true; + this.running.add(file.path); const status = rowEl.createSpan({ cls: "wd-status wd-status-running", text: "running 0s" }); + const output = rowEl.createDiv({ cls: "wd-output", text: "" }); const started = Date.now(); const timer = window.setInterval(() => { status.setText(`running ${Math.round((Date.now() - started) / 1000)}s`); }, 1000); + this.registerInterval(timer); - const output = rowEl.createDiv({ cls: "wd-output", text: "" }); + // Bounded as it accumulates, not only when displayed, so a long or noisy + // run cannot grow this string without limit. let buffered = ""; - const append = (chunk) => { - buffered += chunk.toString(); - output.setText(buffered.slice(-4000)); + const append = (text) => { + buffered = (buffered + text).slice(-8000); + output.setText(buffered); output.scrollTop = output.scrollHeight; }; - const child = spawn("claude", ["-p", `ingest "${file.name}"`], { - cwd: base, - shell: true, - }); + // Single exit path: every way this run can end clears the timer, releases + // the file, and re-enables the button. + const finish = (cls, label) => { + window.clearInterval(timer); + this.running.delete(file.path); + status.className = `wd-status ${cls}`; + status.setText(label); + if (button) button.disabled = false; + }; + let child; + try { + child = spawn("claude", ["-p", `ingest "${file.name}"`], { + cwd: base, + shell: true, + }); + } catch (err) { + // spawn() throws synchronously for some argument shapes. Without this the + // timer would run forever and the row would stay disabled until reload. + append(`\nCould not start claude: ${err.message}`); + finish("wd-status-failed", "failed"); + return; + } + + // Decode as UTF-8 across chunk boundaries. Raw Buffer chunks split wherever + // the OS buffer ends, and this vault's output is full of Cyrillic and em + // dashes that would otherwise decode as replacement characters. + child.stdout.setEncoding("utf8"); + child.stderr.setEncoding("utf8"); child.stdout.on("data", append); child.stderr.on("data", append); child.on("error", (err) => { - window.clearInterval(timer); - status.className = "wd-status wd-status-failed"; - status.setText("failed"); append(`\nCould not start claude: ${err.message}\nIs it on PATH?`); - rowEl.dataset.wdRunning = "0"; - if (button) button.disabled = false; + finish("wd-status-failed", "failed"); }); child.on("close", (code) => { - window.clearInterval(timer); const secs = Math.round((Date.now() - started) / 1000); if (code === 0) { - status.className = "wd-status wd-status-done"; - status.setText(`done in ${secs}s`); + finish("wd-status-done", `done in ${secs}s`); notify(`Ingested ${file.name}. Reopen the dashboard to refresh.`); } else { - status.className = "wd-status wd-status-failed"; - status.setText(`failed - exit ${code}`); + finish("wd-status-failed", `failed - exit ${code}`); } - rowEl.dataset.wdRunning = "0"; - if (button) button.disabled = false; }); } } diff --git a/.obsidian/plugins/webinar-dash/test/safety.test.js b/.obsidian/plugins/webinar-dash/test/safety.test.js index 6dc2756..419dc1b 100644 --- a/.obsidian/plugins/webinar-dash/test/safety.test.js +++ b/.obsidian/plugins/webinar-dash/test/safety.test.js @@ -26,6 +26,14 @@ test("isSafeFilename rejects shell metacharacters", () => { } }); +test("isSafeFilename rejects the cmd.exe escape character and control characters", () => { + // `^` escapes the next character in cmd.exe, so it can defuse the closing + // quote. NUL additionally makes spawn() throw synchronously. + for (const bad of ["a^b.md", "a\u0000b.md", "a\u001bb.md", "a\u007fb.md"]) { + assert.equal(isSafeFilename(bad), false, `should reject: ${JSON.stringify(bad)}`); + } +}); + test("isSafeFilename rejects path traversal", () => { assert.equal(isSafeFilename("../secrets.md"), false); assert.equal(isSafeFilename("a/../../b.md"), false);