From 2d27b5aaa94e6d5e63cabb505516631765881227 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN Date: Wed, 19 Aug 2026 21:35:10 -0400 Subject: [PATCH] Assert optInt's three edges and the path guard's positive case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both checks could only be observed failing, so both could have been quietly inverted. `resolveWithinProfile` is `target !== root && !target.startsWith(root + "/")`. Turn that `&&` into `||` and every path except the profile root itself is refused — which the traversal cases could not see, because they expect a refusal either way. A check that refuses everything is not a working check, so the test now writes to a legitimate path inside the profile and confirms the file lands there. `optInt` guards length, period, digits, index, delay and the history target, and each of its three clauses was unasserted: a non-integer (1.5), a negative, and — the one that matters for the boundary — zero, which is NOT negative and must stay accepted. `n < 0` quietly becoming `n <= 0` would reject a clipboard delay of 0 and a derivation index of 0, both real choices. Records one thing found on the way and deliberately not fixed here: a nested export path answers 500 rather than a refusal naming the missing directory. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Vtx94nXDmNgXEeThUZP4QN --- TODO.md | 10 ++++++++ js/packages/cli/test/api.test.ts | 40 ++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/TODO.md b/TODO.md index f5611868..110b8f73 100644 --- a/TODO.md +++ b/TODO.md @@ -217,6 +217,16 @@ everything still open, in the order it should be tackled. It found one real defect — the config API accepting any key and any value, fixed in the same session — and one property worth recording rather than unilaterally changing, below. + - [ ] **A nested export path answers 500 instead of a refusal.** + `POST /api/v1/entry/:id/document/export` with + `{"path": "exports/nested/secret.txt"}` fails with an opaque + "internal error": `atomicWrite` does not create parent + directories, by design. The path is caller-supplied, so this + should be a 400 naming the missing directory — a 500 tells the + caller the server broke when they simply named somewhere that + does not exist. Small; left out of the commit that found it + because that commit was about the traversal guard. + (Found 2026-08-19 during the mutation sweep.) - [ ] **DECISION TO RECORD: an unencrypted portable backup is unauthenticated, and import accepts it silently.** With `encryption_mode: "none"` the payload is used verbatim; the diff --git a/js/packages/cli/test/api.test.ts b/js/packages/cli/test/api.test.ts index c54f9cc4..b4f0f6e9 100644 --- a/js/packages/cli/test/api.test.ts +++ b/js/packages/cli/test/api.test.ts @@ -201,6 +201,46 @@ describe("what the API refuses", () => { expect(res.status).toBe(400); expect(res.json.detail).toContain("inside the profile directory"); } + + // The POSITIVE half. The guard is `target !== root && !target.startsWith( + // root + "/")` — turn that `&&` into `||` and every path except the + // profile root itself is refused, which the traversal cases above cannot + // see because they expect a refusal either way. A check that refuses + // everything is not a working check. + const inside = await call("POST", "/api/v1/entry/0/document/export", { + password: PASSWORD, + // Flat, not nested: atomicWrite does not create parent directories, + // and a nested path answers 500 rather than a clear refusal — recorded + // in TODO.md rather than fixed inside a test for a different property. + body: { path: "exported-secret.txt" }, + }); + expect(inside.status).toBe(200); + expect(inside.json.path).toBe( + join(app.profileDir(FINGERPRINT), "exported-secret.txt"), + ); + expect(existsSync(inside.json.path)).toBe(true); + }); + + it("validates optional integers at every edge, not just the obvious one", async () => { + // optInt guards `length`, `period`, `digits`, `index`, `delay` and the + // history `target`. Each of its three clauses was unasserted. + for (const delay of [1.5, -1, "soon"]) { + const res = await call("POST", "/api/v1/secret-mode", { + password: PASSWORD, + body: { enabled: true, delay }, + }); + expect(res.status).toBe(400); + expect(res.json.detail).toContain("delay"); + } + + // Zero is NOT negative, and optInt says non-negative. A delay of 0 is a + // real choice, so `n < 0` must not quietly become `n <= 0`. + const zero = await call("POST", "/api/v1/secret-mode", { + password: PASSWORD, + body: { enabled: true, delay: 0 }, + }); + expect(zero.status).toBe(200); + expect((await call("GET", "/api/v1/config/clipboard_clear_delay")).json.value).toBe(0); }); it("refuses a non-loopback bind unless explicitly allowed", () => {