From 121b1b6170c6261a70d7787da139ce597a4b8411 Mon Sep 17 00:00:00 2001 From: Epinephrine Date: Fri, 30 Oct 2026 10:04:56 +0900 Subject: [PATCH 1/2] fix(windows): snapshot ACL failure diagnostics --- src/lib/windows-secret-acl.ts | 76 ++++--- .../windows-secret-acl-diagnostics.test.ts | 196 ++++++++++++++++++ 2 files changed, 243 insertions(+), 29 deletions(-) diff --git a/src/lib/windows-secret-acl.ts b/src/lib/windows-secret-acl.ts index 7b423974d9e..c7d39f38df7 100644 --- a/src/lib/windows-secret-acl.ts +++ b/src/lib/windows-secret-acl.ts @@ -1207,37 +1207,55 @@ function isTimeoutError(error: unknown): boolean { * which is optional and simply omitted when it is not an allowlisted code. */ function sanitizeAclFailure(cause: unknown): AclFailureDiagnostics | undefined { - if (!cause || typeof cause !== "object" || !("aclFailure" in cause)) return undefined; - const raw = (cause as { aclFailure?: unknown }).aclFailure; - if (!raw || typeof raw !== "object") return undefined; - const f = raw as Record; - if (typeof f.step !== "string" || !ICACLS_STEPS.has(f.step)) return undefined; - if ( - f.stage !== "deadline_before_spawn" && - f.stage !== "spawn_failed" && - f.stage !== "runner_rejected" && - f.stage !== "runner_watchdog" && - f.stage !== "child_result" - ) { - return undefined; - } - if (typeof f.elapsedMs !== "number" || !Number.isFinite(f.elapsedMs)) return undefined; - if (typeof f.budgetMs !== "number" || !Number.isFinite(f.budgetMs)) return undefined; - if (typeof f.success !== "boolean" || typeof f.timedOut !== "boolean") return undefined; - if (f.exitCode !== null && (typeof f.exitCode !== "number" || !Number.isFinite(f.exitCode))) { + // Every read off the untrusted cause — the `in` check, the `aclFailure` + // access and each field — may be a throwing accessor or a getter that + // returns a different value on the next read. Each field is therefore read + // exactly once into a local, and only that local is validated and copied; + // a read that throws drops the whole record rather than letting a raw + // hostile exception replace the sanitized required-mode error. + try { + if (!cause || typeof cause !== "object" || !("aclFailure" in cause)) return undefined; + const raw = (cause as { aclFailure?: unknown }).aclFailure; + if (!raw || typeof raw !== "object") return undefined; + const f = raw as Record; + const step = f.step; + if (typeof step !== "string" || !ICACLS_STEPS.has(step)) return undefined; + const stage = f.stage; + if ( + stage !== "deadline_before_spawn" && + stage !== "spawn_failed" && + stage !== "runner_rejected" && + stage !== "runner_watchdog" && + stage !== "child_result" + ) { + return undefined; + } + const elapsedMs = f.elapsedMs; + if (typeof elapsedMs !== "number" || !Number.isFinite(elapsedMs)) return undefined; + const budgetMs = f.budgetMs; + if (typeof budgetMs !== "number" || !Number.isFinite(budgetMs)) return undefined; + const success = f.success; + if (typeof success !== "boolean") return undefined; + const timedOut = f.timedOut; + if (typeof timedOut !== "boolean") return undefined; + const exitCode = f.exitCode; + if (exitCode !== null && (typeof exitCode !== "number" || !Number.isFinite(exitCode))) { + return undefined; + } + const spawnErrorCode = allowlistedSpawnErrorCode(f.spawnErrorCode); + return { + step: step as IcaclsStep, + stage, + elapsedMs, + budgetMs, + success, + exitCode: exitCode as number | null, + timedOut, + ...(spawnErrorCode !== undefined ? { spawnErrorCode } : {}), + }; + } catch { return undefined; } - const spawnErrorCode = allowlistedSpawnErrorCode(f.spawnErrorCode); - return { - step: f.step as IcaclsStep, - stage: f.stage, - elapsedMs: f.elapsedMs, - budgetMs: f.budgetMs, - success: f.success, - exitCode: f.exitCode as number | null, - timedOut: f.timedOut, - ...(spawnErrorCode !== undefined ? { spawnErrorCode } : {}), - }; } /** Preserve only the bounded machine-readable cause on a sanitized public error. */ diff --git a/tests/windows/windows-secret-acl-diagnostics.test.ts b/tests/windows/windows-secret-acl-diagnostics.test.ts index c671ed9b1f7..6209b99f697 100644 --- a/tests/windows/windows-secret-acl-diagnostics.test.ts +++ b/tests/windows/windows-secret-acl-diagnostics.test.ts @@ -394,3 +394,199 @@ describe("aclFailure diagnostics on required harden errors", () => { expect(result.diagnostics).not.toContain(ownerSid); }); }); + +describe("aclFailure boundary against an untrusted runner-thrown cause", () => { + // A runner that throws instead of returning a result hands its raw error to + // the required-mode sanitizer. Every aclFailure field on that error is + // untrusted: a getter may return a different value on each read or throw, and + // a Proxy may trap `in` or `get`. The sanitizer must read each field once, + // validate and copy only that local value, and drop the whole record when any + // read throws — a hostile exception must never replace the sanitized error. + const throwingRunnerCause = (aclFailure: unknown): Error => + Object.assign(new Error("injected runner failure"), { code: "EICACLS", aclFailure }); + + test("a changing step getter cannot swap the validated value for a path at copy time", () => { + const filePath = secretFile(); + let stepReads = 0; + let exitCodeReads = 0; + const hostile = { + get step() { stepReads += 1; return stepReads <= 2 ? "/grant:r" : filePath; }, + stage: "child_result", + elapsedMs: 1, + budgetMs: 100, + success: false, + get exitCode() { exitCodeReads += 1; return exitCodeReads <= 3 ? 5 : `marker:${filePath}`; }, + timedOut: false, + }; + setIcaclsRunnerForTests(() => { throw throwingRunnerCause(hostile); }); + + const error = thrownAclError(() => hardenSecretPath(filePath, { required: true })); + + expect(error.code).toBe("EICACLS"); + // Validated values — never the later hostile reads — reach the record. + expect(error.aclFailure).toMatchObject({ step: "/grant:r", stage: "child_result", exitCode: 5 }); + expect(JSON.stringify(error.aclFailure)).not.toContain(filePath); + }); + + test("every copied field is read exactly once, keeping valid edge values", () => { + const filePath = secretFile(); + const reads: Record = {}; + const field = (name: string, value: T): PropertyDescriptor => ({ + enumerable: true, + get() { reads[name] = (reads[name] ?? 0) + 1; return value; }, + }); + const hostile = Object.defineProperties({}, { + step: field("step", "/grant:r"), + stage: field("stage", "child_result"), + elapsedMs: field("elapsedMs", -1), + budgetMs: field("budgetMs", 0), + success: field("success", false), + exitCode: field("exitCode", null), + timedOut: field("timedOut", false), + }); + setIcaclsRunnerForTests(() => { throw throwingRunnerCause(hostile); }); + + const error = thrownAclError(() => hardenSecretPath(filePath, { required: true })); + + expect(error.code).toBe("EICACLS"); + expect(error.aclFailure).toEqual({ + step: "/grant:r", + stage: "child_result", + elapsedMs: -1, + budgetMs: 0, + success: false, + exitCode: null, + timedOut: false, + }); + for (const name of ["step", "stage", "elapsedMs", "budgetMs", "success", "exitCode", "timedOut"]) { + expect(reads[name]).toBe(1); + } + }); + + test("an invalid first read drops the record even when a later read would pass", () => { + const filePath = secretFile(); + let stepReads = 0; + const hostile = { + get step() { stepReads += 1; return stepReads === 1 ? filePath : "/grant:r"; }, + stage: "child_result", + elapsedMs: 1, + budgetMs: 100, + success: false, + exitCode: 5, + timedOut: false, + }; + setIcaclsRunnerForTests(() => { throw throwingRunnerCause(hostile); }); + + const error = thrownAclError(() => hardenSecretPath(filePath, { required: true })); + + expect(error.code).toBe("EICACLS"); + expect(error.aclFailure).toBeUndefined(); + }); + + test("an invalid success field short-circuits before the timedOut getter runs", () => { + const filePath = secretFile(); + let timedOutReads = 0; + const hostile = { + step: "/grant:r", + stage: "child_result", + elapsedMs: 1, + budgetMs: 100, + success: "yes" as unknown as boolean, + get timedOut() { timedOutReads += 1; return false; }, + exitCode: 5, + }; + setIcaclsRunnerForTests(() => { throw throwingRunnerCause(hostile); }); + + const error = thrownAclError(() => hardenSecretPath(filePath, { required: true })); + + expect(error.code).toBe("EICACLS"); + expect(error.aclFailure).toBeUndefined(); + // Original guard read timedOut only after success passed; keep that order. + expect(timedOutReads).toBe(0); + }); + + test("a throwing aclFailure field getter drops the record but keeps the sanitized error", () => { + const filePath = secretFile(); + const hostile = Object.defineProperty({ + stage: "child_result", + elapsedMs: 1, + budgetMs: 100, + success: false, + exitCode: 5, + timedOut: false, + }, "step", { + enumerable: true, + get() { throw new Error(`getter-marker ${filePath}`); }, + }); + setIcaclsRunnerForTests(() => { throw throwingRunnerCause(hostile); }); + + const error = thrownAclError(() => hardenSecretPath(filePath, { required: true })); + + expect(error.code).toBe("EICACLS"); + expect(error.aclFailure).toBeUndefined(); + expect(error.message).not.toContain("getter-marker"); + expect(error.message).not.toContain(filePath); + }); + + test("proxy has/get traps on the untrusted cause drop the record without replacing the error", () => { + const wellFormed = { + step: "/grant:r", + stage: "child_result", + elapsedMs: 1, + budgetMs: 100, + success: false, + exitCode: 5, + timedOut: false, + }; + + // The record's own get trap throws when step is read. + const recordProxy = new Proxy({ ...wellFormed }, { + get(target, prop) { + if (prop === "step") throw new Error("record-get-marker"); + return (target as Record)[prop]; + }, + }); + const filePath1 = secretFile("secret-a.json"); + setIcaclsRunnerForTests(() => { throw throwingRunnerCause(recordProxy); }); + const first = thrownAclError(() => hardenSecretPath(filePath1, { required: true })); + expect(first.code).toBe("EICACLS"); + expect(first.aclFailure).toBeUndefined(); + expect(first.message).not.toContain("record-get-marker"); + + // The cause's own has trap throws only for the aclFailure key — a trap that + // also fires on the separate cause.code boundary is outside this fix. + const filePath2 = secretFile("secret-b.json"); + const causeProxy = new Proxy(throwingRunnerCause({ ...wellFormed }), { + has(target, prop) { + if (prop === "aclFailure") throw new Error(`cause-has-marker ${filePath2}`); + return prop in target; + }, + }); + setIcaclsRunnerForTests(() => { throw causeProxy; }); + const second = thrownAclError(() => hardenSecretPath(filePath2, { required: true })); + expect(second.code).toBe("EICACLS"); + expect(second.aclFailure).toBeUndefined(); + expect(second.message).not.toContain("cause-has-marker"); + expect(second.message).not.toContain(filePath2); + }); + + test("a well-formed plain record and its allowlisted spawn code survive verbatim", () => { + const filePath = secretFile(); + const clean = { + step: "/remove:g", + stage: "spawn_failed", + elapsedMs: 2, + budgetMs: 50, + success: false, + exitCode: null, + timedOut: false, + spawnErrorCode: "ENOENT", + }; + setIcaclsRunnerForTests(() => { throw throwingRunnerCause(clean); }); + + const error = thrownAclError(() => hardenSecretPath(filePath, { required: true })); + + expect(error.code).toBe("EICACLS"); + expect(error.aclFailure).toEqual({ ...clean }); + }); +}); From 372d7e73ead44f73edd0711bb347ce58f3673e9d Mon Sep 17 00:00:00 2001 From: JUN <243035832+lidge-jun@users.noreply.github.com> Date: Fri, 30 Oct 2026 20:18:39 +0900 Subject: [PATCH 2/2] docs(structure): record the single-read ACL diagnostic snapshot --- structure/ops/service-and-sidecars.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/structure/ops/service-and-sidecars.md b/structure/ops/service-and-sidecars.md index 71781fd0a71..00955b3e463 100644 --- a/structure/ops/service-and-sidecars.md +++ b/structure/ops/service-and-sidecars.md @@ -122,7 +122,7 @@ enumeration twice made a measured 12.3-second fallback cost roughly 25 seconds b and every async ACL hardening flight and deadline-surviving `icacls.exe` runner under that home have settled. `src/config/paths.ts` owns the barrier: a timeout verdict alone does not make the home removable. The contract is exercised by `tests/server/server-stop-config-hardening.test.ts`. -`src/lib/windows-secret-acl.ts` retains removal ownership for an entire async harden, including principal lookup, successive ACL commands, retries and diagnostics, as well as tracking runners that outlive their deadlines. The exact-path timeout indicator keeps its existing meaning for atomic-file fallback. The Bun regression in `tests/windows/windows-secret-acl-removal-flight.test.ts` holds a normal command and verifies that removal remains blocked between successive commands. Required-mode failures keep their sanitized ETIMEDOUT/EICACLS semantics and additionally carry a bounded `aclFailure` record — fixed step and stage enums (`deadline_before_spawn`/`spawn_failed`/`runner_rejected`/`runner_watchdog`/`child_result`), measured elapsed and requested budget, success, exit code, and an allowlisted spawn code — copied field-by-field so no path, argv, SID, or child output crosses into the thrown error; `tests/windows/windows-secret-acl-diagnostics.test.ts` covers the boundary. Lifecycle acquisition opts into `verifyExistingRuntimeDirectory` before mutating its shared Windows runtime root. `src/lib/windows-owner-acl.ts` verifies a protected private DACL with the serving user's inheritable Full Control; an Administrators-owned runtime directory also qualifies when that group is enabled in the serving token, independently of its default owner. The ordinary private secret-file/pairing policy is unchanged. Verification is bound to unchanged device/inode/ctime and the original deadline, creates no hardening memo, and never falls through to the legacy name-only shortcut. Refusal retains required hardening and fail-closed errors. The root check does not certify descendants; the lifecycle database is hardened separately. Regression coverage is in `tests/windows/windows-lifecycle-root-acl.test.ts`. +`src/lib/windows-secret-acl.ts` retains removal ownership for an entire async harden, including principal lookup, successive ACL commands, retries and diagnostics, as well as tracking runners that outlive their deadlines. The exact-path timeout indicator keeps its existing meaning for atomic-file fallback. The Bun regression in `tests/windows/windows-secret-acl-removal-flight.test.ts` holds a normal command and verifies that removal remains blocked between successive commands. Required-mode failures keep their sanitized ETIMEDOUT/EICACLS semantics and additionally carry a bounded `aclFailure` record — fixed step and stage enums (`deadline_before_spawn`/`spawn_failed`/`runner_rejected`/`runner_watchdog`/`child_result`), measured elapsed and requested budget, success, exit code, and an allowlisted spawn code — copied field-by-field so no path, argv, SID, or child output crosses into the thrown error. Each field is read from the cause exactly once and that same primitive is both validated and copied, so a changing getter cannot pass validation with one value and publish another; a getter or Proxy trap that throws drops the whole record while the sanitized required-mode error is still thrown; `tests/windows/windows-secret-acl-diagnostics.test.ts` covers the boundary. Lifecycle acquisition opts into `verifyExistingRuntimeDirectory` before mutating its shared Windows runtime root. `src/lib/windows-owner-acl.ts` verifies a protected private DACL with the serving user's inheritable Full Control; an Administrators-owned runtime directory also qualifies when that group is enabled in the serving token, independently of its default owner. The ordinary private secret-file/pairing policy is unchanged. Verification is bound to unchanged device/inode/ctime and the original deadline, creates no hardening memo, and never falls through to the legacy name-only shortcut. Refusal retains required hardening and fail-closed errors. The root check does not certify descendants; the lifecycle database is hardened separately. Regression coverage is in `tests/windows/windows-lifecycle-root-acl.test.ts`. Removal ownership captures lexical and canonical path identities before each work item or child starts, including Windows short names and directory aliases; deadline survivors retain the same identities. A deleted temporary file is captured through its existing parent. Unreadable identity at capture stays conservatively matched until settlement. Removal compares the requested root against those retained identities, so deleting or retargeting an alias cannot release the original root early. The same Bun regression verifies aliases changed mid-flight still block removal. Matching covers captured and current alias destinations; a second retarget by a writer of the protected directory is out of scope.