docs: fold Task 7 security findings into the plan
Caret and control characters added to the filename guard, file-keyed in-flight registry replacing the row-scoped one, try/catch around spawn, single finish() exit path, UTF-8 chunk decoding, bounded output buffer.
This commit is contained in:
@@ -1447,6 +1447,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", () => {
|
test("isSafeFilename rejects path traversal", () => {
|
||||||
assert.equal(isSafeFilename("../secrets.md"), false);
|
assert.equal(isSafeFilename("../secrets.md"), false);
|
||||||
assert.equal(isSafeFilename("a/../../b.md"), false);
|
assert.equal(isSafeFilename("a/../../b.md"), false);
|
||||||
@@ -1476,9 +1484,17 @@ Insert into `main.js` below `reconcileConcepts`:
|
|||||||
```js
|
```js
|
||||||
// `shell: true` is required on Windows to resolve `claude.cmd`, which puts the
|
// `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
|
// 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
|
// interpret, plus every control character.
|
||||||
// present in real filenames, so they stay allowed.
|
//
|
||||||
const UNSAFE_CHARS = /["`$&|;<>%\r\n]/;
|
// `^` 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) {
|
function isSafeFilename(name) {
|
||||||
if (typeof name !== "string" || name.length === 0) return false;
|
if (typeof name !== "string" || name.length === 0) return false;
|
||||||
@@ -1496,7 +1512,7 @@ Add `isSafeFilename` to the `__test__` export object.
|
|||||||
node --test test/safety.test.js
|
node --test test/safety.test.js
|
||||||
```
|
```
|
||||||
|
|
||||||
Expected: PASS, 4 tests.
|
Expected: PASS, 5 tests.
|
||||||
|
|
||||||
- [ ] **Step 5: Add ingest-state styles**
|
- [ ] **Step 5: Add ingest-state styles**
|
||||||
|
|
||||||
@@ -1519,6 +1535,19 @@ Append to `styles.css`:
|
|||||||
|
|
||||||
`getBasePath()` exists on `FileSystemAdapter`; on mobile the adapter is a different class and `child_process` is unavailable, which is why the manifest sets `isDesktopOnly`.
|
`getBasePath()` exists on `FileSystemAdapter`; on mobile the adapter is a different class and `child_process` is unavailable, which is why the manifest sets `isDesktopOnly`.
|
||||||
|
|
||||||
|
```js
|
||||||
|
Add the in-flight registry as the first line of `onload()`, before the code-block
|
||||||
|
processor is registered. It lives on the plugin instance so it survives the row
|
||||||
|
re-renders that a DOM-scoped guard cannot:
|
||||||
|
|
||||||
|
```js
|
||||||
|
async onload() {
|
||||||
|
this.running = new Set();
|
||||||
|
// ... existing registerMarkdownCodeBlockProcessor call follows unchanged
|
||||||
|
```
|
||||||
|
|
||||||
|
Then add both methods to the class:
|
||||||
|
|
||||||
```js
|
```js
|
||||||
vaultPath() {
|
vaultPath() {
|
||||||
const adapter = this.app.vault.adapter;
|
const adapter = this.app.vault.adapter;
|
||||||
@@ -1527,11 +1556,19 @@ Append to `styles.css`:
|
|||||||
}
|
}
|
||||||
|
|
||||||
runIngest(file, rowEl) {
|
runIngest(file, rowEl) {
|
||||||
if (rowEl.dataset.wdRunning === "1") return;
|
|
||||||
|
|
||||||
const Notice = OB ? OB.Notice : null;
|
const Notice = OB ? OB.Notice : null;
|
||||||
const notify = (msg) => { if (Notice) new Notice(msg); };
|
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)) {
|
if (!isSafeFilename(file.name)) {
|
||||||
rowEl.createDiv({
|
rowEl.createDiv({
|
||||||
cls: "wd-output",
|
cls: "wd-output",
|
||||||
@@ -1554,54 +1591,76 @@ Append to `styles.css`:
|
|||||||
return;
|
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");
|
const button = rowEl.querySelector("button");
|
||||||
if (button) button.disabled = true;
|
if (button) button.disabled = true;
|
||||||
|
this.running.add(file.path);
|
||||||
|
|
||||||
const status = rowEl.createSpan({ cls: "wd-status wd-status-running", text: "running 0s" });
|
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 started = Date.now();
|
||||||
const timer = window.setInterval(() => {
|
const timer = window.setInterval(() => {
|
||||||
status.setText(`running ${Math.round((Date.now() - started) / 1000)}s`);
|
status.setText(`running ${Math.round((Date.now() - started) / 1000)}s`);
|
||||||
}, 1000);
|
}, 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 = "";
|
let buffered = "";
|
||||||
const append = (chunk) => {
|
const append = (text) => {
|
||||||
buffered += chunk.toString();
|
buffered = (buffered + text).slice(-8000);
|
||||||
output.setText(buffered.slice(-4000));
|
output.setText(buffered);
|
||||||
output.scrollTop = output.scrollHeight;
|
output.scrollTop = output.scrollHeight;
|
||||||
};
|
};
|
||||||
|
|
||||||
const child = spawn("claude", ["-p", `ingest "${file.name}"`], {
|
// 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,
|
cwd: base,
|
||||||
shell: true,
|
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.stdout.on("data", append);
|
||||||
child.stderr.on("data", append);
|
child.stderr.on("data", append);
|
||||||
|
|
||||||
child.on("error", (err) => {
|
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?`);
|
append(`\nCould not start claude: ${err.message}\nIs it on PATH?`);
|
||||||
rowEl.dataset.wdRunning = "0";
|
finish("wd-status-failed", "failed");
|
||||||
if (button) button.disabled = false;
|
|
||||||
});
|
});
|
||||||
|
|
||||||
child.on("close", (code) => {
|
child.on("close", (code) => {
|
||||||
window.clearInterval(timer);
|
|
||||||
const secs = Math.round((Date.now() - started) / 1000);
|
const secs = Math.round((Date.now() - started) / 1000);
|
||||||
if (code === 0) {
|
if (code === 0) {
|
||||||
status.className = "wd-status wd-status-done";
|
finish("wd-status-done", `done in ${secs}s`);
|
||||||
status.setText(`done in ${secs}s`);
|
|
||||||
notify(`Ingested ${file.name}. Reopen the dashboard to refresh.`);
|
notify(`Ingested ${file.name}. Reopen the dashboard to refresh.`);
|
||||||
} else {
|
} else {
|
||||||
status.className = "wd-status wd-status-failed";
|
finish("wd-status-failed", `failed - exit ${code}`);
|
||||||
status.setText(`failed - exit ${code}`);
|
|
||||||
}
|
}
|
||||||
rowEl.dataset.wdRunning = "0";
|
|
||||||
if (button) button.disabled = false;
|
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
```
|
```
|
||||||
@@ -1650,7 +1709,7 @@ Bare `node --test` auto-discovers the test files. Do not pass `test/` as an
|
|||||||
argument — Node 24 resolves a bare directory path as a module and fails with
|
argument — Node 24 resolves a bare directory path as a module and fails with
|
||||||
`MODULE_NOT_FOUND` before running anything.
|
`MODULE_NOT_FOUND` before running anything.
|
||||||
|
|
||||||
Expected: PASS, 26 tests across three files.
|
Expected: PASS, 27 tests across three files.
|
||||||
|
|
||||||
- [ ] **Step 11: Commit**
|
- [ ] **Step 11: Commit**
|
||||||
|
|
||||||
@@ -1683,4 +1742,4 @@ The spec's "v1 does not write the coverage file from the plugin" is honored —
|
|||||||
|
|
||||||
**Type consistency.** `rawFiles` items are `{path, name, size}` in Tasks 2 and 3. `sourcePages` items are `{path, name, rawPath}` in both. Coverage rows are `{concept, status, stations, pinned, line}` in Tasks 4, 5, and 6. `onIngest(file, rowEl)` is declared in Task 3 and implemented with the same signature in Task 7. `addIcon(parent, name)` is defined in Task 3 and reused in Task 6. `STATIONS` is defined in Task 1 and consumed by `groupByStation` in Task 4.
|
**Type consistency.** `rawFiles` items are `{path, name, size}` in Tasks 2 and 3. `sourcePages` items are `{path, name, rawPath}` in both. Coverage rows are `{concept, status, stations, pinned, line}` in Tasks 4, 5, and 6. `onIngest(file, rowEl)` is declared in Task 3 and implemented with the same signature in Task 7. `addIcon(parent, name)` is defined in Task 3 and reused in Task 6. `STATIONS` is defined in Task 1 and consumed by `groupByStation` in Task 4.
|
||||||
|
|
||||||
**Test count.** Task 2 adds 9, Task 4 adds 11, Task 6 adds 2, Task 7 adds 4 — 26 total, matching Step 10 of Task 7.
|
**Test count.** Task 2 adds 9, Task 4 adds 11, Task 6 adds 2, Task 7 adds 5 — 27 total, matching Step 10 of Task 7.
|
||||||
|
|||||||
Reference in New Issue
Block a user