From 7ea3861e25562d454cb5ff509cf64a6f5e448999 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 17:27:05 +0000 Subject: [PATCH 1/5] chore(deps): bump actions/deploy-pages in the actions group Bumps the actions group with 1 update: [actions/deploy-pages](https://github.com/actions/deploy-pages). Updates `actions/deploy-pages` from 5.0.0 to 5.0.1 - [Release notes](https://github.com/actions/deploy-pages/releases) - [Commits](https://github.com/actions/deploy-pages/compare/cd2ce8fcbc39b97be8ca5fce6e763baed58fa128...368f82528645a54fb793d4d04e342629a3f51346) --- updated-dependencies: - dependency-name: actions/deploy-pages dependency-version: 5.0.1 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions ... Signed-off-by: dependabot[bot] --- .github/workflows/pages.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pages.yml b/.github/workflows/pages.yml index c8503e84..9b7dc989 100644 --- a/.github/workflows/pages.yml +++ b/.github/workflows/pages.yml @@ -51,4 +51,4 @@ jobs: - name: Deploy id: deployment - uses: actions/deploy-pages@cd2ce8fcbc39b97be8ca5fce6e763baed58fa128 # v5.0.0 + uses: actions/deploy-pages@368f82528645a54fb793d4d04e342629a3f51346 # v5.0.1 From 8739325c706dc91cb97256c8d821363441c6feab Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 3 Sep 2026 10:30:22 -0700 Subject: [PATCH 2/5] test(ci): bound dispatch fixture subprocess lifetime (#219) Replace unbounded synchronous fixture execution with async process-group supervision, an explicit deadline, bounded output, and close-before-cleanup. Cover descendants retaining pipes after a successful shell exit. The full serial suite passes; preserve separately recorded local parallel-run timeouts. --- CHANGELOG.md | 1 + test/clawsweeper-dispatch-workflow.test.ts | 131 ++++++++++++++++++--- 2 files changed, 113 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a5b377d..5cc08e91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## 0.7.3 - Unreleased +- Bound workflow-dispatch test subprocesses and terminate their process groups before fixture cleanup, so stuck shell descendants fail validation instead of hanging the suite. - Retry Windows Root-backed sidecar exclusive-create denials within the existing eight-retry and caller budgets, using per-call provenance while preserving callback errors and rejecting replayed failure evidence. - Assign cross-platform sidecar contention proof liveness to its whole-worker watchdog instead of false-failing healthy unfair acquisition; production lock timeout behavior is unchanged. - Enforce portable FileStore keys consistently across methods: async reads, `exists`, and `remove` now reject parent-segment and backslash aliases with `invalid-path` for existing roots, matching sync and write methods while preserving missing-root error precedence and Root's confined existing-object compatibility. diff --git a/test/clawsweeper-dispatch-workflow.test.ts b/test/clawsweeper-dispatch-workflow.test.ts index 63a08484..61f02181 100644 --- a/test/clawsweeper-dispatch-workflow.test.ts +++ b/test/clawsweeper-dispatch-workflow.test.ts @@ -1,4 +1,5 @@ -import { spawnSync } from "node:child_process"; +import { spawn } from "node:child_process"; +import { once } from "node:events"; import { createHash } from "node:crypto"; import { chmodSync, @@ -10,9 +11,56 @@ import { import { readFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; const behaviorIt = process.platform === "win32" ? it.skip : it; +const fixtureTimeoutMs = 10_000; + +function startFixture( + scriptPath: string, + env: NodeJS.ProcessEnv, + armDeadline: (callback: () => void, ms: number) => NodeJS.Timeout = setTimeout, +) { + const child = spawn("bash", [scriptPath], { + cwd: process.cwd(), env, detached: true, stdio: ["ignore", "pipe", "pipe"], + }); + let closed = false; + let stopping = false; + let timedOut = false; + let error: Error | undefined; + let bytes = 0; + const output = { stdout: [] as Buffer[], stderr: [] as Buffer[] }; + const stop = () => { + if (closed || stopping || child.pid === undefined) return; + stopping = true; + try { process.kill(-child.pid, "SIGKILL"); } + catch (reason) { + if ((reason as NodeJS.ErrnoException).code !== "ESRCH") error = reason as Error; + } + }; + const deadline = armDeadline(() => { timedOut = true; stop(); }, fixtureTimeoutMs); + for (const name of ["stdout", "stderr"] as const) { + child[name].on("data", (chunk: Buffer) => { + bytes += chunk.length; + if (bytes <= 1_048_576) output[name].push(chunk); + else { error ??= new Error("dispatch fixture output exceeded 1 MiB"); stop(); } + }); + } + child.on("error", (reason) => { error = reason; stop(); }); + // A descendant may hold the pipes after Bash exits; keep the deadline until close. + const result = new Promise<{ + status: number | null; signal: NodeJS.Signals | null; timedOut: boolean; + error?: Error; stdout: string; stderr: string; + }>((resolve) => child.once("close", (status, signal) => { + closed = true; + clearTimeout(deadline); + resolve({ status, signal, timedOut, error, + stdout: Buffer.concat(output.stdout).toString("utf8"), + stderr: Buffer.concat(output.stderr).toString("utf8"), + }); + })); + return { child, result, stop }; +} function exactReviewBlock(workflow: string) { const start = workflow.indexOf(" - name: Dispatch exact ClawSweeper review"); @@ -32,7 +80,7 @@ function exactReviewRun(workflow: string) { .trimEnd(); } -function executeExactReview(run: string, event: object, environment: Record) { +async function executeExactReview(run: string, event: object, environment: Record) { const directory = mkdtempSync(join(tmpdir(), "fs-safe-clawsweeper-dispatch-")); const eventPath = join(directory, "event.json"); const capturePath = join(directory, "dispatch.json"); @@ -61,19 +109,17 @@ function executeExactReview(run: string, event: object, environment: Record { updated_at: "2026-08-10T22:00:00Z", body: "proof body", }; - const prPayload = executeExactReview( + const prPayload = await executeExactReview( run, { pull_request: pullRequest, label: { name: "proof: sufficient" } }, { @@ -165,7 +211,7 @@ describe("ClawSweeper dispatch workflow", () => { }, }); - const issuePayload = executeExactReview( + const issuePayload = await executeExactReview( run, { issue: { number: 446 } }, { @@ -186,5 +232,52 @@ describe("ClawSweeper dispatch workflow", () => { source_action: "opened", supersedes_in_progress: false, }); - }); + }, 2 * fixtureTimeoutMs + 1_000); + + behaviorIt("kills descendants holding pipes after the shell exits and reports a timeout", async () => { + const directory = mkdtempSync(join(tmpdir(), "fs-safe-dispatch-watchdog-")); + const scriptPath = join(directory, "hang.sh"); + writeFileSync(scriptPath, '\"$FIXTURE_NODE\" -e \'process.on("SIGTERM", () => {}); console.log("READY"); setInterval(() => {}, 1000)\' &\n'); + let expire = () => {}; + const armDeadline = vi.fn((callback: () => void, ms: number) => { + expire = callback; + return setTimeout(callback, ms); + }); + const fixture = startFixture(scriptPath, { ...process.env, FIXTURE_NODE: process.execPath }, armDeadline); + try { + await Promise.race([ + Promise.all([once(fixture.child, "exit"), once(fixture.child.stdout, "data")]), + fixture.result.then(() => { throw new Error("fixture closed before readiness"); }), + ]); + expect(fixture.child.exitCode).toBe(0); + expect(armDeadline).toHaveBeenCalledWith(expect.any(Function), fixtureTimeoutMs); + expire(); + const result = await fixture.result; + expect(result.timedOut).toBe(true); + expect(result.status).toBe(0); + expect(result.stdout).toContain("READY"); + expect(result.error).toBeUndefined(); + } finally { + fixture.stop(); + await fixture.result; + rmSync(directory, { recursive: true, force: true }); + } + }, fixtureTimeoutMs + 1_000); + + behaviorIt("bounds fixture output and reaps an overflowing child", async () => { + const directory = mkdtempSync(join(tmpdir(), "fs-safe-dispatch-output-")); + const scriptPath = join(directory, "overflow.sh"); + writeFileSync(scriptPath, '\"$FIXTURE_NODE\" -e \'process.stdout.write("x".repeat(2 * 1024 * 1024)); setInterval(() => {}, 1000)\'\n'); + const fixture = startFixture(scriptPath, { ...process.env, FIXTURE_NODE: process.execPath }); + try { + const result = await fixture.result; + expect(result.error?.message).toBe("dispatch fixture output exceeded 1 MiB"); + expect(result.timedOut).toBe(false); + expect(Buffer.byteLength(result.stdout)).toBeLessThanOrEqual(1_048_576); + } finally { + fixture.stop(); + await fixture.result; + rmSync(directory, { recursive: true, force: true }); + } + }, fixtureTimeoutMs + 1_000); }); From b4322efc07e296f91d33e3eabc071bd6e01f4149 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 3 Sep 2026 11:24:48 -0700 Subject: [PATCH 3/5] fix(queue): propagate enqueue parent sync failures (#221) * fix(queue): propagate enqueue parent sync failures * fix(queue): retain atomic ownership through directory sync --- CHANGELOG.md | 1 + docs/store.md | 2 + src/json-durable-queue.ts | 26 +--- src/replace-file-descriptor.ts | 36 ++++++ src/replace-file.ts | 53 +++----- ...n-durable-queue-enqueue-durability.test.ts | 114 ++++++++++++++++++ test/json-durable-queue-publication.test.ts | 70 +++++++++++ 7 files changed, 243 insertions(+), 59 deletions(-) create mode 100644 test/json-durable-queue-enqueue-durability.test.ts create mode 100644 test/json-durable-queue-publication.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cc08e91..a4557285 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## 0.7.3 - Unreleased +- Propagate durable queue enqueue parent-sync failures and keep published-file identity checks after synchronization, sharing the guarded writer with migrations while retaining retry state. - Bound workflow-dispatch test subprocesses and terminate their process groups before fixture cleanup, so stuck shell descendants fail validation instead of hanging the suite. - Retry Windows Root-backed sidecar exclusive-create denials within the existing eight-retry and caller budgets, using per-call provenance while preserving callback errors and rejecting replayed failure evidence. - Assign cross-platform sidecar contention proof liveness to its whole-worker watchdog instead of false-failing healthy unfair acquisition; production lock timeout behavior is unchanged. diff --git a/docs/store.md b/docs/store.md index 17b5568d..2b4ff367 100644 --- a/docs/store.md +++ b/docs/store.md @@ -72,6 +72,8 @@ Loading serializes consumers for one ID through a sidecar lock, then creates `pr Queue and failed directory creation fsyncs every newly-created parent edge from the leaf toward the trusted root. Enqueue and migration writes fsync the temp file and parent; claim, acknowledgement, quarantine, delivered-marker cleanup, and retirement transitions fsync every affected directory and propagate real sync failures. A transition may already be visible when a post-mutation sync fails, so retry the same operation to complete its crash-recovery state. Acknowledgement retries resync the queue directory even when both `.processing` and `.delivered` marker names are already absent, before reporting completion or rejecting a newer pending generation; quarantine retries with only failed evidence resync that destination before repairing the vanished queue source. +`writeJsonDurableQueueEntry()` and migrations share strict parent synchronization inside the atomic writer's retained descriptor and per-path serialization lifetime, followed by published-file identity verification. If sync fails after publication, the write rejects without rolling back the published JSON; retrying writes the entry again and must complete its own sync. This is not a rollback, deduplication, or exactly-once guarantee. The generic `replaceFileAtomic({ syncParentDir: true })` option remains best-effort. + Batch loading skips invalid entry names, malformed, oversized, or unreadable entry content, and caller `read` callback failures. Initially unowned pending entries (hardlinks or unverifiable identities), symlinks, non-files, and absent pending entries are also skipped. Claim, transfer-lock, retirement, and migration write/publication/durability failures reject the batch with the original error, even if earlier entries succeeded. Migration in both loaders strictly syncs the parent directory after successful publication. Visible transitions and earlier processing claims remain for retry; a rejected batch does not acknowledge or roll them back. Failed destinations are create-only. Quarantine publishes the claimed file by hardlink, so the queue and failed directories must share a filesystem with hardlink support. If `failed/.json` already exists, quarantine rejects while preserving both that earlier evidence and the current claimed entry instead of overwriting either file. The `read` callback continues to receive the logical `.json` path even though bytes are read and migrations are written through the claimed path. diff --git a/src/json-durable-queue.ts b/src/json-durable-queue.ts index e72bf3cc..c294e670 100644 --- a/src/json-durable-queue.ts +++ b/src/json-durable-queue.ts @@ -12,7 +12,7 @@ import { } from "./json-durable-queue-ownership.js"; import { stringifyJsonDocument } from "./json-stringify.js"; import { resolveReadOpenFlags } from "./read-open-flags.js"; -import { replaceFileAtomic } from "./replace-file.js"; +import { replaceFileAtomicWithDirectorySync } from "./replace-file.js"; import { assertSafePathSegment } from "./safe-path-segment.js"; import { inspectFileIdentity } from "./strict-file-identity.js"; @@ -285,29 +285,13 @@ export async function writeJsonDurableQueueEntry(params: { entry: unknown; tempPrefix: string; }): Promise { - await replaceFileAtomic({ + await replaceFileAtomicWithDirectorySync({ filePath: params.filePath, content: stringifyJsonDocument(params.entry, null, 2), mode: 0o600, tempPrefix: params.tempPrefix, syncTempFile: true, - syncParentDir: true, - }); -} - -async function replaceJsonDurableQueueEntry(params: { - filePath: string; - entry: unknown; - tempPrefix: string; -}): Promise { - await replaceFileAtomic({ - filePath: params.filePath, - content: stringifyJsonDocument(params.entry, null, 2), - mode: 0o600, - tempPrefix: params.tempPrefix, - syncTempFile: true, - }); - await syncDirectory(path.dirname(params.filePath)); + }, syncDirectory); } async function inspectQueueEntry( @@ -401,7 +385,7 @@ export async function loadJsonDurableQueueEntry(params: { }); const result = params.read ? await params.read(raw, params.paths.jsonPath) : { entry: raw }; if (result.migrated) { - await replaceJsonDurableQueueEntry({ + await writeJsonDurableQueueEntry({ filePath: claimedPath, entry: result.entry, tempPrefix: params.tempPrefix, @@ -471,7 +455,7 @@ export async function loadPendingJsonDurableQueueEntries( continue; } if (result.migrated) { - await replaceJsonDurableQueueEntry({ + await writeJsonDurableQueueEntry({ filePath: claimedPath, entry: result.entry, tempPrefix: options.tempPrefix, diff --git a/src/replace-file-descriptor.ts b/src/replace-file-descriptor.ts index 0f29bd31..a60c230b 100644 --- a/src/replace-file-descriptor.ts +++ b/src/replace-file-descriptor.ts @@ -13,6 +13,42 @@ type SyncTempFileSystem = Pick< export type SyncFchmod = (fd: number, mode: number) => void; +export async function syncDirectoryBestEffort( + fsModule: Pick, + dirPath: string, +): Promise { + let handle: FileHandle | undefined; + try { + handle = await fsModule.open(dirPath, "r"); + await handle.sync(); + } catch { + // Best-effort on platforms/filesystems that do not support directory fsync. + } finally { + await handle?.close().catch(() => undefined); + } +} + +export function syncDirectoryBestEffortSync( + fsModule: Pick, + dirPath: string, +): void { + let fd: number | undefined; + try { + fd = fsModule.openSync(dirPath, "r"); + fsModule.fsyncSync(fd); + } catch { + // Best-effort on platforms/filesystems that do not support directory fsync. + } finally { + if (fd !== undefined) { + try { + fsModule.closeSync(fd); + } catch { + // Best-effort close after directory fsync. + } + } + } +} + function directoryOpenFlags(): number { return ( syncFs.constants.O_RDONLY | diff --git a/src/replace-file.ts b/src/replace-file.ts index a4967c7a..b9e0076a 100644 --- a/src/replace-file.ts +++ b/src/replace-file.ts @@ -15,6 +15,8 @@ import { import { applyDirectoryMode, applyDirectoryModeSync, + syncDirectoryBestEffort, + syncDirectoryBestEffortSync, type SyncFchmod, writeTempFile, writeTempFileSync, @@ -273,44 +275,16 @@ function missingFchmodSyncError(): TypeError { ); } -async function syncDirectoryBestEffort( - fsModule: ReplaceFileAtomicFileSystem["promises"], - dirPath: string, -): Promise { - let handle: Awaited> | undefined; - try { - handle = await fsModule.open(dirPath, "r"); - await handle.sync(); - } catch { - // Best-effort on platforms/filesystems that do not support directory fsync. - } finally { - await handle?.close().catch(() => undefined); - } -} - -function syncDirectoryBestEffortSync( - fsModule: ReplaceFileAtomicSyncFileSystem, - dirPath: string, -): void { - let fd: number | undefined; - try { - fd = fsModule.openSync(dirPath, "r"); - fsModule.fsyncSync(fd); - } catch { - // Best-effort on platforms/filesystems that do not support directory fsync. - } finally { - if (fd !== undefined) { - try { - fsModule.closeSync(fd); - } catch { - // Best-effort close after directory fsync. - } - } - } +export async function replaceFileAtomic( + options: ReplaceFileAtomicOptions, +): Promise { + return await replaceFileAtomicWithDirectorySync(options); } -export async function replaceFileAtomic( +// Internal owner hook: keep directory durability inside publication verification and serialization. +export async function replaceFileAtomicWithDirectorySync( options: ReplaceFileAtomicOptions, + syncParent?: (directoryPath: string) => Promise, ): Promise { const filePath = options.filePath; validateReplaceFilePath(filePath); @@ -318,19 +292,20 @@ export async function replaceFileAtomic( validateRenameIdentity(options.renameIdentity); return await serializePathWrite(path.resolve(filePath), async () => { if (options.renameIdentity !== "verify-content-with-lock") { - return await replaceFileAtomicUnserialized(options); + return await replaceFileAtomicUnserialized(options, syncParent); } await (options.fileSystem?.promises ?? fs).mkdir(path.dirname(filePath), { recursive: true, mode: options.dirMode ?? 0o700, }); return await withAtomicRenameIdentityLock(filePath, async () => - await replaceFileAtomicUnserialized(options)); + await replaceFileAtomicUnserialized(options, syncParent)); }); } async function replaceFileAtomicUnserialized( options: ReplaceFileAtomicOptions, + syncParent?: (directoryPath: string) => Promise, ): Promise { const filePath = options.filePath; const fsModule = options.fileSystem?.promises ?? fs; @@ -379,7 +354,9 @@ async function replaceFileAtomicUnserialized( } else { await tempOwner.assertCurrent(fsModule); } - if (options.syncParentDir) { + if (syncParent) { + await syncParent(dir); + } else if (options.syncParentDir) { await syncDirectoryBestEffort(fsModule, dir); } if (result.method === "rename") { diff --git a/test/json-durable-queue-enqueue-durability.test.ts b/test/json-durable-queue-enqueue-durability.test.ts new file mode 100644 index 00000000..8fc3f888 --- /dev/null +++ b/test/json-durable-queue-enqueue-durability.test.ts @@ -0,0 +1,114 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + ensureJsonDurableQueueDirs, + resolveJsonDurableQueueEntryPaths, + writeJsonDurableQueueEntry, +} from "../src/json-durable-queue.js"; +import { useTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useTempDirs(); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +async function fixture(existing = false) { + const root = await fs.realpath(await tempRoot("fs-safe-queue-enqueue-durability-")); + const queueDir = path.join(root, "queue"); + await ensureJsonDurableQueueDirs({ queueDir, failedDir: path.join(root, "failed") }); + const paths = resolveJsonDurableQueueEntryPaths(queueDir, "job"); + if (existing) await fs.writeFile(paths.jsonPath, '{"generation":1}\n', { mode: 0o600 }); + const write = () => writeJsonDurableQueueEntry({ + filePath: paths.jsonPath, + entry: { generation: 2 }, + tempPrefix: "queue", + }); + return { queueDir, paths, write }; +} + +function observeWrite(filePath: string, failAt?: "temp-sync" | "parent-sync" | "parent-open", failure?: Error) { + const events: string[] = []; + const realOpen = fs.open.bind(fs); + const realRename = fs.rename.bind(fs); + vi.spyOn(fs, "open").mockImplementation(async (...args) => { + if (failAt === "parent-open" && events.includes("publish") && args[0].toString() === path.dirname(filePath)) { + throw failure; + } + const handle = await realOpen(...args); + const realSync = handle.sync.bind(handle); + const event = (await handle.stat()).isDirectory() ? "parent-sync" : "temp-sync"; + vi.spyOn(handle, "sync").mockImplementation(async () => { + events.push(event); + if (event === failAt) throw failure; + await realSync(); + }); + return handle; + }); + vi.spyOn(fs, "rename").mockImplementation(async (source, target) => { + await realRename(source, target); + if (target === filePath) events.push("publish"); + }); + return events; +} + +describe("durable JSON queue enqueue durability", () => { + it("syncs the temp file, publishes, then syncs the parent exactly once", async () => { + const { paths, write } = await fixture(); + const events = observeWrite(paths.jsonPath); + + await write(); + + expect(events).toEqual(["temp-sync", "publish", "parent-sync"]); + await expect(fs.readFile(paths.jsonPath, "utf8")).resolves.toBe('{\n "generation": 2\n}'); + if (process.platform !== "win32") { + expect((await fs.stat(paths.jsonPath)).mode & 0o777).toBe(0o600); + } + }); + + it.each([false, true])("propagates parent sync errors and retries after publication (existing=%s)", async (existing) => { + const { queueDir, paths, write } = await fixture(existing); + for (const phase of ["initial", "retry"]) { + const failure = Object.assign(new Error(`${phase} enqueue directory sync failed`), { code: "EIO" }); + const events = observeWrite(paths.jsonPath, "parent-sync", failure); + + await expect(write()).rejects.toBe(failure); + + expect(events).toEqual(["temp-sync", "publish", "parent-sync"]); + await expect(fs.readFile(paths.jsonPath, "utf8")).resolves.toBe('{\n "generation": 2\n}'); + await expect(fs.readdir(queueDir)).resolves.toEqual(["job.json"]); + vi.restoreAllMocks(); + } + const events = observeWrite(paths.jsonPath); + await expect(write()).resolves.toBeUndefined(); + expect(events).toEqual(["temp-sync", "publish", "parent-sync"]); + await expect(fs.readdir(queueDir)).resolves.toEqual(["job.json"]); + }); + + it.each([false, true])("does not publish or leak a temp file after temp sync failure (existing=%s)", async (existing) => { + const { queueDir, paths, write } = await fixture(existing); + const failure = Object.assign(new Error("enqueue temp sync failed"), { code: "EIO" }); + const events = observeWrite(paths.jsonPath, "temp-sync", failure); + + await expect(write()).rejects.toBe(failure); + + expect(events).toEqual(["temp-sync"]); + await expect(fs.readdir(queueDir)).resolves.toEqual(existing ? ["job.json"] : []); + if (existing) { + await expect(fs.readFile(paths.jsonPath, "utf8")).resolves.toBe('{"generation":1}\n'); + } + }); + + it.each(["EIO", "EACCES"])("propagates parent open %s after publication", async (code) => { + const { queueDir, paths, write } = await fixture(); + const failure = Object.assign(new Error("enqueue parent open failed"), { code }); + const events = observeWrite(paths.jsonPath, "parent-open", failure); + + await expect(write()).rejects.toBe(failure); + + expect(events).toEqual(["temp-sync", "publish"]); + await expect(fs.readFile(paths.jsonPath, "utf8")).resolves.toBe('{\n "generation": 2\n}'); + await expect(fs.readdir(queueDir)).resolves.toEqual(["job.json"]); + }); +}); diff --git a/test/json-durable-queue-publication.test.ts b/test/json-durable-queue-publication.test.ts new file mode 100644 index 00000000..f5200748 --- /dev/null +++ b/test/json-durable-queue-publication.test.ts @@ -0,0 +1,70 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, vi } from "vitest"; +import { + loadJsonDurableQueueEntry, + loadPendingJsonDurableQueueEntries, + resolveJsonDurableQueueEntryPaths, + writeJsonDurableQueueEntry, +} from "../src/json-durable-queue.js"; +import { itPosix, useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +afterEach(() => vi.restoreAllMocks()); + +describe("durable queue post-sync publication identity", () => { + for (const operation of ["enqueue", "single-migration", "batch-migration"] as const) { + itPosix.each([false, true])(`${operation} rejects a swap during directory sync (same bytes=%s)`, async (sameBytes) => { + const root = await tempRoot("fs-safe-queue-post-sync-"); + const queueDir = path.join(root, "queue"); + await fs.mkdir(queueDir, { mode: 0o700 }); + const paths = resolveJsonDurableQueueEntryPaths(queueDir, "job"); + const target = operation === "enqueue" ? paths.jsonPath : paths.processingPath!; + if (operation !== "enqueue") await fs.writeFile(target, '{"generation":1}'); + const published = JSON.stringify({ generation: 2 }, null, 2); + const replacement = sameBytes ? published : '{"generation":"foreign"}'; + const foreign = path.join(root, "foreign"); + const saved = path.join(root, "saved"); + await fs.writeFile(foreign, replacement); + const foreignIdentity = await fs.lstat(foreign, { bigint: true }); + const realOpen = fs.open.bind(fs); + const realRename = fs.rename.bind(fs); + let didPublish = false; + let didSwap = false; + vi.spyOn(fs, "rename").mockImplementation(async (source, destination) => { + await realRename(source, destination); + if (destination === target) didPublish = true; + }); + vi.spyOn(fs, "open").mockImplementation(async (...args) => { + const handle = await realOpen(...args); + if (args[0].toString() === queueDir) { + const realSync = handle.sync.bind(handle); + vi.spyOn(handle, "sync").mockImplementation(async () => { + await realSync(); + if (didPublish && !didSwap) { + didSwap = true; + await realRename(target, saved); + await realRename(foreign, target); + } + }); + } + return handle; + }); + const read = async () => ({ entry: { generation: 2 }, migrated: true }); + const write = operation === "enqueue" + ? () => writeJsonDurableQueueEntry({ filePath: target, entry: { generation: 2 }, tempPrefix: "queue" }) + : operation === "single-migration" + ? () => loadJsonDurableQueueEntry({ paths, tempPrefix: "queue", read }) + : () => loadPendingJsonDurableQueueEntries({ queueDir, tempPrefix: "queue", read }); + + await expect(write()).rejects.toMatchObject({ code: "path-mismatch" }); + + expect(didSwap).toBe(true); + await expect(fs.readFile(saved, "utf8")).resolves.toBe(published); + await expect(fs.readFile(target, "utf8")).resolves.toBe(replacement); + const current = await fs.lstat(target, { bigint: true }); + expect({ dev: current.dev, ino: current.ino }).toEqual({ dev: foreignIdentity.dev, ino: foreignIdentity.ino }); + expect((await fs.readdir(queueDir)).some((name) => name.endsWith(".tmp"))).toBe(false); + }); + } +}); From 128bf1f863cb6cbf03edca555d8dea56eee2e980 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 3 Sep 2026 21:00:43 -0700 Subject: [PATCH 4/5] fix(secret): preserve admitted directory permissions and identity (#222) Restore the documented non-repair policy for secret roots and parents, including EEXIST winners. Initialize newly created directories through retained descriptor authority and effective-UID checks, preserve full directory mode bits, and retain the admitted guard chain through write and private-lock handoff. Carry exact bigint identities through private Root capabilities and native/fallback writes without relaxing identity comparisons. Make async Root-backed lock normalization read-only so a deleted admitted parent is not recreated before refusal. Preserve external arbitration keys with explicit in-root sidecars, normal lock defaults, and ordinary Root-backed descendant creation. Invalid modes reject before mutation; unpinnable new directories fail closed, including the documented non-root macOS umask(0777) case. Creation and descriptor admission are not an atomic create-and-pin guarantee; callers own cleanup of empty created directories after refusal. Add regressions for permission repair, initialization ownership, late replacements, large/same-number identities, bounded Windows unknown-identity checks, private JSON lock preparation/deletion, and stale-Root reentrant reuse. Extend existing root-only npm/pnpm package smoke with public-API traces, real identities, separate-process mutations, actual native-load evidence, and artifact/source provenance. Keep metadata collection portable for Windows namespace paths and Git-less build containers; preserve explicit output-directory overrides. Proof: 7,235 tests passed with 80 skipped in the final serial suite; security and package checks passed. Hosted packaged consumers passed 144 observations across Windows, macOS, Linux glibc, and Linux musl in off/require modes. CI 33832592064 and coverage 33832592009 passed. Complete-scope Codex autoreview and final ClawSweeper review found no actionable code/security issues; the Windows authority-chain proof was accepted. Failed parallel/CI/provisioning attempts remain documented in https://github.com/openclaw/fs-safe/pull/222. The separate explicit file-mode parity follow-up is not claimed fixed here. --- CHANGELOG.md | 3 + docs/errors.md | 2 + docs/private-file-store.md | 13 +- docs/secret-file.md | 10 +- docs/sidecar-lock.md | 9 + scripts/check-release-packages.mjs | 7 +- scripts/consumer-install-smoke.mjs | 11 +- scripts/consumer-proof-metadata.mjs | 22 ++ scripts/consumer-secret-probe.mjs | 179 +++++++++++++ src/directory-guard.ts | 30 ++- src/directory-mode-node.ts | 21 +- src/file-store-boundary.ts | 16 ++ src/file-store.ts | 4 + src/guarded-mutation.ts | 4 +- src/json-document-store.ts | 5 + src/native-pinned-write-windows.ts | 4 +- src/native-pinned-write.ts | 17 +- src/native-staged-file.ts | 4 +- src/pinned-write.ts | 12 +- src/root-context.ts | 7 +- src/root-impl.ts | 2 +- src/root-write-verification.ts | 4 +- src/secret-file.ts | 179 +++++++++---- src/sidecar-lock-acquire.ts | 13 +- test/consumer-pnpm-lifecycle.test.ts | 21 +- test/consumer-proof-metadata.test.ts | 54 ++++ test/deepsec-regression.test.ts | 13 +- test/directory-guard-exact-identity.test.ts | 66 +++++ test/private-json-directory-admission.test.ts | 131 ++++++++++ test/secret-directory-admission.test.ts | 235 ++++++++++++++++++ test/secret-directory-receipt.test.ts | 56 +++++ test/secret-directory-wide-identity.test.ts | 130 ++++++++++ test/secret-file-failure.test.ts | 2 +- test/secret-write-publication.test.ts | 2 +- test/sidecar-lock-root-normalization.test.ts | 125 ++++++++++ 35 files changed, 1311 insertions(+), 102 deletions(-) create mode 100644 scripts/consumer-proof-metadata.mjs create mode 100644 scripts/consumer-secret-probe.mjs create mode 100644 test/consumer-proof-metadata.test.ts create mode 100644 test/directory-guard-exact-identity.test.ts create mode 100644 test/private-json-directory-admission.test.ts create mode 100644 test/secret-directory-admission.test.ts create mode 100644 test/secret-directory-receipt.test.ts create mode 100644 test/secret-directory-wide-identity.test.ts create mode 100644 test/sidecar-lock-root-normalization.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index a4557285..bf0e574f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## 0.7.3 - Unreleased +- Preserve existing secret-directory permissions instead of repairing them; retain lossless directory identities through private locks and native writes, initialize new directories through guarded descriptor authority, honor full directory mode bits, and fail closed for unpinnable parents, including non-root macOS directories created under `umask(0o777)`. +- Keep async Root-backed lock normalization read-only, rejecting deleted or replaced admitted parents without recreating them while preserving explicit in-root sidecars for external target keys. +- Exercise secret-directory admission from isolated npm/pnpm consumer installs, retain real-identity and native-load proof, and honor explicit package-proof output paths. - Propagate durable queue enqueue parent-sync failures and keep published-file identity checks after synchronization, sharing the guarded writer with migrations while retaining retry state. - Bound workflow-dispatch test subprocesses and terminate their process groups before fixture cleanup, so stuck shell descendants fail validation instead of hanging the suite. - Retry Windows Root-backed sidecar exclusive-create denials within the existing eight-retry and caller budgets, using per-call provenance while preserving callback errors and rejecting replayed failure evidence. diff --git a/docs/errors.md b/docs/errors.md index d3c58e92..cd38dcc5 100644 --- a/docs/errors.md +++ b/docs/errors.md @@ -134,6 +134,8 @@ type FsSafeErrorCode = | `too-large` | A read or bounded walk exceeded its configured budget. | Caller gave a too-permissive file or traversal limit. | | `unsupported-platform` | Reserved compatibility code for a platform-specific operation. | No current public helper emits this `FsSafeError` code. Platform-specific APIs currently return a typed unsupported result or use `helper-unavailable`; keep the union member when exhaustively switching across supported package versions. | +Secret writes reject invalid `mode` / `dirMode` values with `invalid-path` before directory creation. Existing secret directories with a mode different from the requested `dirMode` report `insecure-permissions` without chmod; a created directory whose descriptor ownership no longer matches its initializing effective user reports `not-owned`. + Pathname `sha256File()` also reports `path-mismatch` when pre-open, descriptor, or current-path identity remains unknown after one bounded Windows retry, even if the file is benign. It never reopens to recover identity. Preview symlinks diff --git a/docs/private-file-store.md b/docs/private-file-store.md index d9b74b37..b6d61d93 100644 --- a/docs/private-file-store.md +++ b/docs/private-file-store.md @@ -17,8 +17,17 @@ const loaded = await store.readJsonIfExists("state.json"); - Writes create parent directories at `0o700` and files at `0o600` unless you pass stricter `dirMode` / `mode` options. -- Private-mode writes route through the secret-file atomic path, which refuses - symlink parent components and re-asserts mode after rename. +- Async private-mode writes route through the secret-file atomic path, which refuses + symlink parent components and re-asserts mode after rename. Existing directories + must already have the requested mode; writes do not repair their permissions. + New-directory initialization requires guarded descriptor authority and may + fail closed under restrictive platform/umask combinations; see the + [secret-directory policy](secret-file.md#parameters). +- Locked JSON mutations prepare private directories before acquiring their + sidecar and bind the lock to the admitted parent identity. Lock normalization + is read-only: a deleted or replaced admitted parent is rejected, not recreated. + The writer still revalidates directory admission afterward; reads do not create + directories. - `readText()` and `readJson()` are strict and throw on missing files. - `readTextIfExists()` and `readJsonIfExists()` return `null` on missing files. - `write()`, `writeText()`, `writeJson()`, `writeStream()`, and `copyIn()` all diff --git a/docs/secret-file.md b/docs/secret-file.md index f7291a44..63919ab5 100644 --- a/docs/secret-file.md +++ b/docs/secret-file.md @@ -159,7 +159,15 @@ type WriteSecretFileParams = { }; ``` -The directory mode is asserted on each component along the path: `rootDir`, then any intermediate dirs, then the parent. The helper enforces that every component matches `dirMode` — wider permissions on an existing directory cause the write to fail. Audit and tighten existing secret directories yourself. +The full POSIX directory mode is asserted on each component along the path: `rootDir`, then any intermediate dirs, then the parent. Existing directories, including another creator's `EEXIST` winner, must already match `dirMode` exactly or the write fails with `insecure-permissions`; they are never chmod-repaired. An explicitly requested directory mode such as `0o2750` preserves its setgid bit. Audit and adjust existing secret directories yourself. The admitted directory guards are retained through traversal and the final writer/lock handoff; a fresh pathname lookup cannot silently authorize a replacement. The caller must still trust the selected root and its owners; matching permission bits alone do not establish that trust. + +Directory admission and its retained guards use lossless bigint identities, including through private locks and native writes. On Windows, an unknown zero device or inode gets one reinspection that retains known components; a definite mismatch or persistent ambiguity fails with `path-mismatch` rather than authorizing a replacement. + +Both mode options must resolve to integers between `0o0000` and `0o7777`; invalid values fail with `invalid-path` before directory creation or file publication. Windows validates the options but does not enforce POSIX permission bits. + +After this operation wins directory creation, initialization uses a pinned descriptor bound to the admitted identity and effective user, with ancestor checks before chmod. It does not chmod the caller's pathname. Creation and descriptor admission are separate operations, not an atomic create-and-pin guarantee. A raced directory that has not reached its requested mode yet is rejected rather than repaired; callers may retry after its creator finishes initialization. + +Initialization fails closed if the platform cannot safely pin a created directory. In particular, a non-root macOS process cannot pin a new `000` directory produced by `umask(0o777)`; the write fails without repairing that directory or writing a secret. Restrictive masks retaining owner search permission remain usable. Linux x64/arm64 can use the guarded `O_PATH`/procfs descriptor route where available. There is no unguarded pathname-chmod fallback, and a failure may leave a created directory for caller-managed cleanup. ### `createSecretFileAtomic(params)` diff --git a/docs/sidecar-lock.md b/docs/sidecar-lock.md index 2bcf3800..7e70e76b 100644 --- a/docs/sidecar-lock.md +++ b/docs/sidecar-lock.md @@ -178,6 +178,15 @@ an existing `Root` capability. `lockPath` must resolve inside that root. Identity-conditioned removal remains the only release and reclaim deletion path. +Async Root-backed acquisition normalizes the target's parent without creating +it, checking the retained Root before and after normalization. A deleted or +replaced Root fails before payload execution or held-entry reuse. Missing lock +subdirectories are still created through `Root.create`, never by target-key +normalization. The target is an arbitration key and may be outside the Root +when an explicit in-root `lockPath` is supplied; normalization does not follow a +target-leaf symlink. Non-Root acquisition retains its existing parent-creation +behavior. + An owner can finish releasing while another async acquirer inspects its record. Create-only Root writes do not open an existing record merely to inherit its mode. Once a pathname sample and opened descriptor agree, a failed acquisition diff --git a/scripts/check-release-packages.mjs b/scripts/check-release-packages.mjs index fd4d41ae..1da9778d 100644 --- a/scripts/check-release-packages.mjs +++ b/scripts/check-release-packages.mjs @@ -15,9 +15,10 @@ import { fileURLToPath } from "node:url"; import { hostNativeTarget, nativePackageDirectory, nativeTargets } from "./native-targets.mjs"; import { normalizePackResult } from "./npm-pack-result.mjs"; import { consumerInstallSmoke, isolatedConsumerEnv, resolvePnpmCli } from "./consumer-install-smoke.mjs"; +import { packageProofSource } from "./consumer-proof-metadata.mjs"; const pnpmCli = resolvePnpmCli(); -const outputIndex = process.argv.indexOf("--output"); +const outputIndex = process.argv.lastIndexOf("--output"); const outputDir = resolve(outputIndex >= 0 ? process.argv[outputIndex + 1] : "release-artifacts"); const allowHostOnly = process.argv.includes("--allow-host-only"); mkdirSync(outputDir, { recursive: true }); @@ -140,7 +141,9 @@ async function main() { if (!host || !targets.some((target) => target.label === host.label)) { throw new Error(`release smoke requires the host target ${host?.label ?? "unknown"}`); } - await consumerInstallSmoke({ rootPkg, manifest, outputDir, npmCli, pnpmCli, allowHostOnly }); + const source = packageProofSource(); + if (source.unavailable) console.warn(`source revision unavailable: ${source.unavailable}; artifact and behavior proof remain separate`); + await consumerInstallSmoke({ rootPkg, manifest, outputDir, npmCli, pnpmCli, allowHostOnly, source }); for (const artifact of manifest) { console.log(`${artifact.name}: ${artifact.size} bytes gzipped, ${artifact.unpackedSize} bytes unpacked`); diff --git a/scripts/consumer-install-smoke.mjs b/scripts/consumer-install-smoke.mjs index c4ea2f43..2fc47b4f 100644 --- a/scripts/consumer-install-smoke.mjs +++ b/scripts/consumer-install-smoke.mjs @@ -65,7 +65,7 @@ const hashScript = ` } `; -export async function consumerInstallSmoke({ rootPkg, manifest, outputDir, npmCli, pnpmCli, allowHostOnly }) { +export async function consumerInstallSmoke({ rootPkg, manifest, outputDir, npmCli, pnpmCli, allowHostOnly, source }) { const temporary = mkdtempSync(join(tmpdir(), "fs-safe-consumer-proof-")); let server; try { @@ -93,7 +93,7 @@ export async function consumerInstallSmoke({ rootPkg, manifest, outputDir, npmCl } const host = hostNativeTarget(); const proof = { - host: host.label, node: process.version, + host: host.label, node: process.version, source, root: manifest.find((artifact) => artifact.name === rootPkg.name), syntheticForeignPackages: synthetic, managers: [], }; @@ -134,6 +134,13 @@ export async function consumerInstallSmoke({ rootPkg, manifest, outputDir, npmCl cases.auto = await hash("auto"); cases.off = await hash("off"); if (!omitted) { + const secretProbe = join(directory, "secret-probe.mjs"); + writeFileSync(secretProbe, readFileSync(new URL("./consumer-secret-probe.mjs", import.meta.url))); + writeFileSync(join(directory, "consumer-proof-metadata.mjs"), readFileSync(new URL("./consumer-proof-metadata.mjs", import.meta.url))); + cases.secretDirectories = []; + for (const mode of ["off", "require"]) { + cases.secretDirectories.push(JSON.parse(await run(secretProbe, [mode], directory, env))); + } renameSync(installed.binary, `${installed.binary}.removed`); cases.missingBinaryAuto = await hash("auto"); cases.missingBinaryRequire = await hash("require", true); diff --git a/scripts/consumer-proof-metadata.mjs b/scripts/consumer-proof-metadata.mjs new file mode 100644 index 00000000..dc46733a --- /dev/null +++ b/scripts/consumer-proof-metadata.mjs @@ -0,0 +1,22 @@ +import { execFileSync } from "node:child_process"; +import { realpathSync } from "node:fs"; + +export function packageProofSource(cwd = process.cwd()) { + try { + const git = (args) => execFileSync("git", args, { + cwd, encoding: "utf8", timeout: 10_000, stdio: "pipe", + }).trim(); + const [commit, tree] = git(["rev-parse", "HEAD", "HEAD^{tree}"]).split(/\r?\n/); + return { commit, tree, dirty: git(["status", "--porcelain"]) !== "" }; + } catch (error) { + // Source archives and minimal build containers need not have Git metadata. + return { unavailable: error.code === "ENOENT" ? "git-not-installed" : "git-metadata-unavailable" }; + } +} + +export function nativeBinaryLoaded(binary, sharedObjects = process.report.getReport().sharedObjects) { + // The native resolver accepts Windows loader namespace paths without walking "C:". + const expected = realpathSync.native(binary); + return sharedObjects.filter((file) => file.endsWith(".node")) + .some((file) => realpathSync.native(file) === expected); +} diff --git a/scripts/consumer-secret-probe.mjs b/scripts/consumer-secret-probe.mjs new file mode 100644 index 00000000..12aa574e --- /dev/null +++ b/scripts/consumer-secret-probe.mjs @@ -0,0 +1,179 @@ +import assert from "node:assert/strict"; +import { execFile } from "node:child_process"; +import { createHash } from "node:crypto"; +import fsSync from "node:fs"; +import fs from "node:fs/promises"; +import { createRequire } from "node:module"; +import path from "node:path"; +import { promisify } from "node:util"; +import { configureFsSafeNative } from "@openclaw/fs-safe/config"; +import { createSecretFileAtomic, writeSecretFileAtomic } from "@openclaw/fs-safe/secret"; +import { fileStore } from "@openclaw/fs-safe/store"; +import { nativeBinaryLoaded } from "./consumer-proof-metadata.mjs"; + +const mode = process.argv[2]; +assert.ok(mode === "off" || mode === "require"); +configureFsSafeNative({ mode }); +const require = createRequire(import.meta.url); +const expected = JSON.parse(await fs.readFile("expected.json", "utf8")); +const sandbox = await fs.realpath(await fs.mkdtemp(path.join(process.cwd(), "secret-proof-"))); +const original = { lstat: fs.lstat, realpath: fs.realpath, open: fs.open }; +const exec = promisify(execFile); +const rows = []; + +async function identity(target) { + const stat = await original.lstat(target, { bigint: true }).catch((error) => { + if (error.code === "ENOENT") return null; + throw error; + }); + return stat ? { dev: String(stat.dev), ino: String(stat.ino), directory: stat.isDirectory() } : null; +} + +async function replaceDirectory(target, moved) { + await exec(process.execPath, ["-e", ` + const fs = require('node:fs'); + fs.renameSync(process.argv[1], process.argv[2]); + fs.mkdirSync(process.argv[1], { mode: 0o700 }); + fs.writeFileSync(process.argv[1] + '/replacement', 'unchanged'); + `, target, moved], { timeout: 10_000, killSignal: "SIGKILL" }); +} + +function observeOpens(events) { + fs.open = async (...args) => { + events.push({ event: "open", path: path.relative(sandbox, String(args[0])), flags: args[1] }); + return await original.open(...args); + }; +} + +try { + for (const [operation, write] of [["write", writeSecretFileAtomic], ["create", createSecretFileAtomic]]) { + const rootDir = path.join(sandbox, `${operation}-stable`); + const parent = path.join(rootDir, "parent"); + await fs.mkdir(parent, { recursive: true, mode: 0o700 }); + const before = { root: await identity(rootDir), parent: await identity(parent) }; + await write({ rootDir, filePath: path.join(parent, "token"), content: "synthetic package proof" }); + assert.equal(await fs.readFile(path.join(parent, "token"), "utf8"), "synthetic package proof"); + assert.deepEqual({ root: await identity(rootDir), parent: await identity(parent) }, before); + rows.push({ operation, scenario: "stable", before, published: true }); + + for (const component of ["root", "parent"]) { + const rootDir = path.join(sandbox, `${operation}-${component}`); + const parent = path.join(rootDir, "parent"); + await fs.mkdir(parent, { recursive: true, mode: 0o700 }); + const target = component === "root" ? rootDir : parent; + const moved = `${target}-admitted`; + await fs.writeFile(path.join(target, "sentinel"), "unchanged"); + const before = await identity(target); + let inspections = 0; + let swapped = false; + const events = []; + fs.lstat = async (...args) => { + const stat = await original.lstat(...args); + if (String(args[0]) === target && args[1]?.bigint) inspections++; + return stat; + }; + fs.realpath = async (...args) => { + // Replace after initial inspection and exact guard capture, before write admission. + if (String(args[0]) === target && inspections >= 2 && !swapped) { + await replaceDirectory(target, moved); + swapped = true; + events.push({ event: "replace-directory", before, after: await identity(target) }); + } + return await original.realpath(...args); + }; + observeOpens(events); + let failure; + try { + await write({ rootDir, filePath: path.join(parent, "token"), content: "must not publish" }); + } catch (error) { + failure = { name: error.name, code: error.code }; + } finally { + Object.assign(fs, original); + } + assert.ok(swapped, "replacement witness must execute"); + assert.equal(failure?.code, "path-mismatch"); + assert.equal(events.filter((event) => event.event === "open").length, 0); + assert.deepEqual(await fs.readdir(target), ["replacement"]); + assert.equal(await fs.readFile(path.join(moved, "sentinel"), "utf8"), "unchanged"); + assert.equal(await identity(path.join(parent, "token")), null); + rows.push({ operation, scenario: `replace-${component}`, failure, events, published: false }); + } + } + + for (const scenario of ["stable", "replacement", "deletion"]) { + const rootDir = path.join(sandbox, `json-${scenario}`); + const parent = path.join(rootDir, "parent"); + await fs.mkdir(parent, { recursive: true, mode: 0o700 }); + const before = await identity(parent); + const events = []; + let inspections = 0; + let changed = false; + let callbacks = 0; + // Queue-key inspection plus preparation; Windows skips the POSIX mode inspection. + const admittedAfter = process.platform === "win32" ? 5 : 6; + fs.lstat = async (...args) => { + if (scenario !== "stable" && String(args[0]) === parent && inspections >= admittedAfter && !changed) { + if (scenario === "deletion") { + await exec(process.execPath, ["-e", "require('node:fs').rmdirSync(process.argv[1])", parent], { + timeout: 10_000, killSignal: "SIGKILL", + }); + } else { + await replaceDirectory(parent, `${parent}-admitted`); + } + changed = true; + events.push({ event: scenario, before, after: await identity(parent) }); + } + const stat = await original.lstat(...args); + if (String(args[0]) === parent) inspections++; + return stat; + }; + observeOpens(events); + let failure; + try { + const state = fileStore({ rootDir, private: true }).json("parent/state.json", { lock: true }); + await state.updateOr({ count: 0 }, (value) => { + callbacks++; + return { count: value.count + 1 }; + }); + } catch (error) { + failure = { name: error.name, code: error.code }; + } finally { + Object.assign(fs, original); + } + if (scenario === "stable") { + assert.equal(failure, undefined); + assert.equal(callbacks, 1); + assert.deepEqual(JSON.parse(await fs.readFile(path.join(parent, "state.json"), "utf8")), { count: 1 }); + assert.deepEqual(await fs.readdir(parent), ["state.json"]); + } else { + assert.ok(changed, "parent mutation witness must execute"); + assert.equal(failure?.code, "path-mismatch"); + assert.equal(callbacks, 0); + assert.equal(events.filter((event) => event.event === "open").length, 0); + if (scenario === "deletion") assert.equal(await identity(parent), null); + else assert.deepEqual(await fs.readdir(parent), ["replacement"]); + } + rows.push({ operation: "private-json", scenario, before, failure: failure ?? null, callbacks, events }); + } + + const rootManifest = require.resolve("@openclaw/fs-safe/package.json"); + const rootRequire = createRequire(rootManifest); + const binary = fsSync.realpathSync.native(rootRequire.resolve(expected.host.package)); + const loaded = nativeBinaryLoaded(binary); + assert.equal(loaded, mode === "require"); + const hash = (file) => createHash("sha256").update(fsSync.readFileSync(file)).digest("hex"); + const modulePath = require.resolve("@openclaw/fs-safe/secret"); + console.log(JSON.stringify({ + platform: process.platform, arch: process.arch, node: process.version, mode, + module: path.relative(process.cwd(), modulePath), moduleSha256: hash(modulePath), + compiledModules: Object.fromEntries([ + "secret-file.js", "directory-guard.js", "root-context.js", "native-pinned-write.js", "pinned-write.js", "sidecar-lock-acquire.js", + ].map((name) => [name, hash(path.join(path.dirname(rootManifest), "dist", name))])), + binary: path.relative(process.cwd(), binary), binarySha256: hash(binary), nativeLoaded: loaded, + probeSha256: hash(new URL(import.meta.url)), metadataHelperSha256: hash(new URL("./consumer-proof-metadata.mjs", import.meta.url)), + metadataProjection: false, separateProcessMutations: true, rows, + })); +} finally { + Object.assign(fs, original); + await fs.rm(sandbox, { recursive: true, force: true }); +} diff --git a/src/directory-guard.ts b/src/directory-guard.ts index 1fe6326d..f76c63e4 100644 --- a/src/directory-guard.ts +++ b/src/directory-guard.ts @@ -8,28 +8,35 @@ import { inspectFileIdentity } from "./strict-file-identity.js"; import { isNotFoundPathError } from "./path.js"; import { directoryComponentNotDirectoryError } from "./root-errors.js"; -export type AsyncDirectoryGuard = { +export type AsyncDirectoryGuard = { dir: string; realPath: string; - stat: Stats; + stat: T; }; +export type AnyAsyncDirectoryGuard = AsyncDirectoryGuard; + export type SyncDirectoryGuard = { dir: string; realPath: string; stat: Stats; }; -export async function createAsyncDirectoryGuard(dir: string): Promise { - const stat = await fs.lstat(dir); +export function createAsyncDirectoryGuard(dir: string, options: { bigint: true }): Promise>; +export function createAsyncDirectoryGuard(dir: string, options?: { bigint?: false }): Promise; +export function createAsyncDirectoryGuard(dir: string, options: { bigint: boolean }): Promise; +export async function createAsyncDirectoryGuard(dir: string, options?: { bigint?: boolean }): Promise { + const stat = options?.bigint ? await inspectDirectoryIdentity(dir) : await fs.lstat(dir); if (stat.isSymbolicLink() || !stat.isDirectory()) { throw directoryComponentNotDirectoryError(); } return { dir, realPath: await fs.realpath(dir), stat }; } -export async function assertAsyncDirectoryGuard(guard: AsyncDirectoryGuard): Promise { - const stat = await fs.lstat(guard.dir); +export async function assertAsyncDirectoryGuard(guard: AnyAsyncDirectoryGuard): Promise { + const stat = typeof guard.stat.dev === "bigint" && typeof guard.stat.ino === "bigint" + ? await inspectDirectoryIdentity(guard.dir, { dev: guard.stat.dev, ino: guard.stat.ino }) + : await fs.lstat(guard.dir); if (stat.isSymbolicLink() || !stat.isDirectory()) { throw directoryComponentNotDirectoryError(); } @@ -56,15 +63,18 @@ export function assertSyncDirectoryGuard(guard: SyncDirectoryGuard): void { } } +export function createNearestExistingDirectoryGuard(rootReal: string, targetPath: string): Promise; +export function createNearestExistingDirectoryGuard(rootReal: string, targetPath: string, options: { bigint: boolean }): Promise; export async function createNearestExistingDirectoryGuard( rootReal: string, targetPath: string, -): Promise { + options = { bigint: false }, +): Promise { let current = path.resolve(targetPath); const root = path.resolve(rootReal); while (current !== root) { try { - return await createAsyncDirectoryGuard(current); + return await createAsyncDirectoryGuard(current, options); } catch (error) { if (!isNotFoundPathError(error)) { throw error; @@ -72,7 +82,7 @@ export async function createNearestExistingDirectoryGuard( current = path.dirname(current); } } - return await createAsyncDirectoryGuard(root); + return await createAsyncDirectoryGuard(root, options); } export function createNearestExistingSyncDirectoryGuard( @@ -95,7 +105,7 @@ export function createNearestExistingSyncDirectoryGuard( } // Recovery receipts must retain every identity bit, including on Windows. -export async function inspectDirectoryIdentity(dir: string, expected?: BigIntStats): Promise { +export async function inspectDirectoryIdentity(dir: string, expected?: Pick): Promise { return await inspectFileIdentity(async () => { const stat = await fs.lstat(dir, { bigint: true }); if (stat.isSymbolicLink() || !stat.isDirectory()) throw directoryComponentNotDirectoryError(); diff --git a/src/directory-mode-node.ts b/src/directory-mode-node.ts index 0288943b..75258ec8 100644 --- a/src/directory-mode-node.ts +++ b/src/directory-mode-node.ts @@ -1,4 +1,4 @@ -import { constants } from "node:fs"; +import { constants, type BigIntStats } from "node:fs"; import fs from "node:fs/promises"; import { FsSafeError } from "./errors.js"; import { inspectDirectoryIdentity } from "./directory-guard.js"; @@ -15,12 +15,22 @@ function searchOnlyFlags(): { flags: number; proc: boolean } | undefined { } /** Real Node only: injected filesystem adapters must retain descriptor-chmod semantics. */ -export async function pinNodeDirectoryForMode(dirPath: string): Promise { - const expected = await inspectDirectoryIdentity(dirPath); +export async function pinNodeDirectoryForMode( + dirPath: string, + options: { expectedIdentity?: BigIntStats; ownerUid?: number } = {}, +): Promise { + const { ownerUid } = options; + const assertOwner = (stat: BigIntStats) => { + if (ownerUid !== undefined && stat.uid !== BigInt(ownerUid)) { + throw new FsSafeError("not-owned", "directory mode target must retain its expected owner"); + } + }; + const expected = await inspectDirectoryIdentity(dirPath, options.expectedIdentity); + assertOwner(expected); if (process.platform === "win32") { // POSIX mode enforcement is unsupported; retain strict path identity checks. return ownDirectoryMode({ - inspect: async () => { await inspectDirectoryIdentity(dirPath, expected); return 0; }, + inspect: async () => { assertOwner(await inspectDirectoryIdentity(dirPath, expected)); return 0; }, chmod: async () => undefined, close: async () => undefined, ignoreChmodError: true, }); } @@ -41,6 +51,7 @@ export async function pinNodeDirectoryForMode(dirPath: string): Promise { const opened = await inspectFileIdentity(() => handle.stat({ bigint: true }), expected); assertOwnedDirectory(expected, opened); + assertOwner(opened); await inspectDirectoryIdentity(dirPath, expected); return Number(opened.mode & 0o7777n); }; @@ -55,6 +66,8 @@ export async function pinNodeDirectoryForMode(dirPath: string): Promise handle.stat({ bigint: true }), expected); const followed = await inspectFileIdentity(() => fs.stat(procPath, { bigint: true }), expected); assertOwnedDirectory(opened, followed); + assertOwner(opened); + assertOwner(followed); }; const owner = ownDirectoryMode({ inspect, diff --git a/src/file-store-boundary.ts b/src/file-store-boundary.ts index ed4a87a2..3b550dc6 100644 --- a/src/file-store-boundary.ts +++ b/src/file-store-boundary.ts @@ -13,6 +13,9 @@ import { FsSafeError } from "./errors.js"; import { sameFileIdentity } from "./file-identity.js"; import { isPathInside, isPathRelativeEscape } from "./path.js"; import { resolveOpenedFileRealPathForHandle, root, type Root } from "./root.js"; +import { ensureTrailingSep } from "./root-context.js"; +import { RootHandle } from "./root-impl.js"; +import { prepareSecretFileWrite } from "./secret-file.js"; import { resolveSecureTempRoot } from "./secure-temp-dir.js"; export type SyncParentGuard = SyncDirectoryGuard; @@ -46,6 +49,19 @@ export async function openWritableStoreRoot(params: { return await root(params.rootDir, { hardlinks: "reject", maxBytes }); } +export async function openPrivateStoreLockRoot( + params: Parameters[0], +): Promise { + const { parentGuard } = await prepareSecretFileWrite(params); + // Bind to the admitted parent, never resolve a replacement into a fresh capability. + return new RootHandle({ + rootDir: parentGuard.dir, + rootReal: parentGuard.realPath, + rootWithSep: ensureTrailingSep(parentGuard.realPath), + rootIdentity: { dev: parentGuard.stat.dev, ino: parentGuard.stat.ino }, + }, { hardlinks: "reject" }); +} + async function chmodDirectoryInRootBestEffort( scopedRoot: Root, relativePath: string, diff --git a/src/file-store.ts b/src/file-store.ts index e2e439eb..a81d8b29 100644 --- a/src/file-store.ts +++ b/src/file-store.ts @@ -10,6 +10,7 @@ import { pruneExpiredStoreEntries, type FileStorePruneOptions } from "./file-sto export type { FileStorePruneOptions } from "./file-store-prune.js"; import { ensureParentInRoot, + openPrivateStoreLockRoot, openWritableStoreRoot, writeStreamToTempSource, } from "./file-store-boundary.js"; @@ -388,6 +389,9 @@ export function fileStore(options: FileStoreOptions): FileStore { return createJsonStore( { filePath, + ...(privateMode ? { + prepareLock: () => openPrivateStoreLockRoot({ rootDir, filePath, mode, dirMode }), + } : {}), readIfExists: async () => { try { return await (await openRoot()).readJson(assertRelativePath(relativePath)); diff --git a/src/guarded-mutation.ts b/src/guarded-mutation.ts index 4a09d43b..d8c01331 100644 --- a/src/guarded-mutation.ts +++ b/src/guarded-mutation.ts @@ -8,12 +8,12 @@ import { createNearestExistingDirectoryGuard, createNearestExistingSyncDirectoryGuard, createSyncDirectoryGuard, - type AsyncDirectoryGuard, + type AnyAsyncDirectoryGuard, type SyncDirectoryGuard, } from "./directory-guard.js"; export async function withAsyncDirectoryGuards( - guards: readonly AsyncDirectoryGuard[], + guards: readonly AnyAsyncDirectoryGuard[], mutate: () => Promise, options: { verifyAfter?: boolean; diff --git a/src/json-document-store.ts b/src/json-document-store.ts index e83e7ce3..89837b5c 100644 --- a/src/json-document-store.ts +++ b/src/json-document-store.ts @@ -3,6 +3,7 @@ import { canonicalPathFromExistingAncestor } from "./absolute-path.js"; import { FsSafeError } from "./errors.js"; import type { FileLockRetryOptions } from "./file-lock.js"; import { getFsSafeLockConfig } from "./lock-config.js"; +import type { Root } from "./root.js"; import { createSidecarLockManager } from "./sidecar-lock.js"; import type { SidecarLockStaleRecovery } from "./sidecar-lock.js"; import { serializePathWrite } from "./write-queue.js"; @@ -38,6 +39,8 @@ export type JsonStore = { export type JsonStoreAdapter = { filePath: string; + /** Internal admission before lock I/O, within the serialized mutation. */ + prepareLock?: () => Promise; readIfExists(): Promise; readRequired(): Promise; write(value: T, options?: { trailingNewline?: boolean }): Promise; @@ -110,8 +113,10 @@ export function createJsonStore( if (!locks || !lockOptions) { return await run(); } + const lockRoot = adapter.prepareLock ? await adapter.prepareLock() : undefined; return await locks.withLock( { + ...(lockRoot ? { lockRoot } : {}), targetPath: adapter.filePath, staleMs: lockOptions.staleMs, timeoutMs: lockOptions.timeoutMs, diff --git a/src/native-pinned-write-windows.ts b/src/native-pinned-write-windows.ts index 9206d2e7..bb4ffab8 100644 --- a/src/native-pinned-write-windows.ts +++ b/src/native-pinned-write-windows.ts @@ -1,7 +1,7 @@ import { randomUUID } from "node:crypto"; import fsSync, { type Stats } from "node:fs"; import type { FileHandle } from "node:fs/promises"; -import type { AsyncDirectoryGuard } from "./directory-guard.js"; +import type { AnyAsyncDirectoryGuard } from "./directory-guard.js"; import { FsSafeError } from "./errors.js"; import type { FileIdentityStat } from "./file-identity.js"; import { @@ -34,7 +34,7 @@ export async function runPinnedWriteWindows( params: PinnedWriteParams, root: FileHandle, parentFd: number, - parentGuard: AsyncDirectoryGuard, + parentGuard: AnyAsyncDirectoryGuard, ): Promise { const parentPath = parentGuard.realPath; let tempFd: number | undefined; diff --git a/src/native-pinned-write.ts b/src/native-pinned-write.ts index fe2d049d..5d96ceea 100644 --- a/src/native-pinned-write.ts +++ b/src/native-pinned-write.ts @@ -1,6 +1,7 @@ import fsSync from "node:fs"; import fs from "node:fs/promises"; import path from "node:path"; +import { inspectDirectoryIdentity } from "./directory-guard.js"; import { FsSafeError } from "./errors.js"; import type { FileIdentityStat } from "./file-identity.js"; import { runPinnedWriteWindows, sameNativeIdentity } from "./native-pinned-write-windows.js"; @@ -8,6 +9,7 @@ import { assertNativeStaging, writeNativeStage, type NativeStagingBinding } from import type { NativeBinding } from "./native.js"; import type { PinnedWriteParams } from "./pinned-write.js"; import { describeStagedDirectory, exactIdentityMatches } from "./staged-directory.js"; +import { inspectFileIdentitySync } from "./strict-file-identity.js"; export async function runPinnedWriteNative(binding: NativeBinding, params: PinnedWriteParams): Promise { const windows = process.platform === "win32"; @@ -29,8 +31,13 @@ export async function runPinnedWriteNative(binding: NativeBinding, params: Pinne }, }; try { + const exactRoot = typeof params.rootIdentity?.dev === "bigint" && typeof params.rootIdentity.ino === "bigint" + ? { dev: params.rootIdentity.dev, ino: params.rootIdentity.ino } : undefined; let rootMatches: boolean; - if (windows) { + if (exactRoot) { + inspectFileIdentitySync(() => fsSync.fstatSync(root.fd, { bigint: true }), exactRoot); + rootMatches = true; + } else if (windows) { const identity = binding.fstatIdentity(root.fd); rootMatches = !params.rootIdentity || sameNativeIdentity(params.rootIdentity, identity); } else { @@ -54,13 +61,15 @@ export async function runPinnedWriteNative(binding: NativeBinding, params: Pinne : params.rootPath, ); const directory = windows ? undefined : describeStagedDirectory(parentFd, parentPath); - const parentPathStat = await fs.lstat(parentPath); - if (windows) { + const parentPathStat = exactRoot + ? await inspectDirectoryIdentity(parentPath, inspectFileIdentitySync(() => fsSync.fstatSync(parentFd!, { bigint: true }))) + : await fs.lstat(parentPath); + if (windows && !exactRoot) { const parentIdentity = binding.fstatIdentity(parentFd); if (parentPathStat.isSymbolicLink() || !sameNativeIdentity(parentPathStat, parentIdentity)) { throw new FsSafeError("path-mismatch", "native write parent changed during resolution"); } - } else if (parentPathStat.isSymbolicLink() || !exactIdentityMatches(parentPathStat, directory!.identity)) { + } else if (!windows && (parentPathStat.isSymbolicLink() || !exactIdentityMatches(parentPathStat, directory!.identity))) { throw new FsSafeError("path-mismatch", "native write parent changed during resolution"); } const verificationGuard = { dir: parentPath, realPath: parentPath, stat: parentPathStat }; diff --git a/src/native-staged-file.ts b/src/native-staged-file.ts index a3809ed8..68f5a85c 100644 --- a/src/native-staged-file.ts +++ b/src/native-staged-file.ts @@ -1,6 +1,6 @@ import { randomUUID } from "node:crypto"; import fs from "node:fs"; -import type { AsyncDirectoryGuard } from "./directory-guard.js"; +import type { AnyAsyncDirectoryGuard } from "./directory-guard.js"; import { FsSafeError } from "./errors.js"; import type { FileIdentityStat } from "./file-identity.js"; import type { NativeBinding } from "./native-binding.js"; @@ -115,7 +115,7 @@ class NativeStagedFile implements StagedFile { parentFd: number, directory: StagedFileReceipt["directory"], params: PinnedWriteParams, - parentGuard: AsyncDirectoryGuard, + parentGuard: AnyAsyncDirectoryGuard, ): Promise { // This owner never escapes. Only the internal verifier borrows its fd; // public descriptor methods remain await-free and cannot race disposal. diff --git a/src/pinned-write.ts b/src/pinned-write.ts index 8c46241e..566e4237 100644 --- a/src/pinned-write.ts +++ b/src/pinned-write.ts @@ -5,7 +5,7 @@ import fs from "node:fs/promises"; import path from "node:path"; import type { Readable } from "node:stream"; import { normalizeMaxBytes } from "./byte-budget.js"; -import { createAsyncDirectoryGuard, createNearestExistingDirectoryGuard, type AsyncDirectoryGuard } from "./directory-guard.js"; +import { createAsyncDirectoryGuard, createNearestExistingDirectoryGuard, inspectDirectoryIdentity, type AnyAsyncDirectoryGuard } from "./directory-guard.js"; import { FsSafeError } from "./errors.js"; import { syncDirectoryBestEffort } from "./fsync.js"; import type { FileIdentityStat } from "./file-identity.js"; @@ -106,7 +106,7 @@ export type PinnedWriteParams = { verifyPublished?: ( fd: number, identity: PublishedWriteIdentity, - parentGuard: AsyncDirectoryGuard, + parentGuard: AnyAsyncDirectoryGuard, ) => Promise; }; @@ -163,6 +163,10 @@ export async function runPinnedWriteWithRenamePolicy( } async function runPinnedWriteFallback(params: PinnedWriteParams): Promise { + const exactRoot = typeof params.rootIdentity?.dev === "bigint" && typeof params.rootIdentity.ino === "bigint" + ? { dev: params.rootIdentity.dev, ino: params.rootIdentity.ino } : undefined; + if (exactRoot) await inspectDirectoryIdentity(params.rootPath, exactRoot); + const guardOptions = { bigint: exactRoot !== undefined }; let parentPath = params.relativeParentPath ? path.join(params.rootPath, ...params.relativeParentPath.split("/")) : params.rootPath; @@ -179,8 +183,8 @@ async function runPinnedWriteFallback(params: PinnedWriteParams): Promise { let current: Awaited>; try { + if (typeof root.rootIdentity.dev === "bigint" && typeof root.rootIdentity.ino === "bigint") { + await inspectDirectoryIdentity(root.rootReal, { dev: root.rootIdentity.dev, ino: root.rootIdentity.ino }); + return; + } current = await fs.lstat(root.rootReal); } catch (error) { throw new FsSafeError("path-mismatch", "root path changed during operation", { diff --git a/src/root-impl.ts b/src/root-impl.ts index 66612f58..cb0abc2c 100644 --- a/src/root-impl.ts +++ b/src/root-impl.ts @@ -391,7 +391,7 @@ export interface Root { walk(relativePath: string, options: RootWalkOptions): AsyncIterableIterator; } -class RootHandle implements Root { +export class RootHandle implements Root { private readonly rootIdentity: RootContext["rootIdentity"]; readonly rootDir: string; readonly rootReal: string; diff --git a/src/root-write-verification.ts b/src/root-write-verification.ts index c5161aee..4cd9d654 100644 --- a/src/root-write-verification.ts +++ b/src/root-write-verification.ts @@ -1,6 +1,6 @@ import fsSync, { type BigIntStats } from "node:fs"; import fs from "node:fs/promises"; -import { assertAsyncDirectoryGuard, type AsyncDirectoryGuard } from "./directory-guard.js"; +import { assertAsyncDirectoryGuard, type AnyAsyncDirectoryGuard } from "./directory-guard.js"; import { FsSafeError } from "./errors.js"; import { sameFileIdentity } from "./file-identity.js"; import { resolveOpenedFileRealPathForFd } from "./opened-realpath.js"; @@ -16,7 +16,7 @@ export async function verifyAtomicWriteResult(params: { fd: number; expectedIdentity: PublishedWriteIdentity; expectedMode?: number; - parentGuard: AsyncDirectoryGuard; + parentGuard: AnyAsyncDirectoryGuard; }): Promise { let needsPathOpen = false; const assertFile = (stat: BigIntStats) => { diff --git a/src/secret-file.ts b/src/secret-file.ts index e55450b9..3d32c3aa 100644 --- a/src/secret-file.ts +++ b/src/secret-file.ts @@ -1,10 +1,12 @@ -import fs from "node:fs"; +import fs, { type BigIntStats } from "node:fs"; import fsp from "node:fs/promises"; import path from "node:path"; import { canonicalPathFromExistingAncestor } from "./absolute-path.js"; import { readFileDescriptorBoundedSync } from "./bounded-read.js"; import { normalizeMaxBytes } from "./byte-budget.js"; -import { assertAsyncDirectoryGuard, createAsyncDirectoryGuard, type AsyncDirectoryGuard } from "./directory-guard.js"; +import { assertAsyncDirectoryGuard, createAsyncDirectoryGuard, inspectDirectoryIdentity, type AsyncDirectoryGuard } from "./directory-guard.js"; +import { pinNodeDirectoryForMode } from "./directory-mode-node.js"; +import { assertOwnedDirectory } from "./directory-mode-owner.js"; import { FsSafeError } from "./errors.js"; import { resolveHomeRelativePath } from "./home-dir.js"; import { openPinnedFileSync } from "./pinned-open.js"; @@ -19,7 +21,7 @@ import { trimSecretFileContent, type SecretFileReadOptions, } from "./secret-read-policy.js"; -import { inspectFileIdentitySync } from "./strict-file-identity.js"; +import { inspectFileIdentity, inspectFileIdentitySync } from "./strict-file-identity.js"; import { serializePathWrite } from "./write-queue.js"; export const PRIVATE_SECRET_DIR_MODE = 0o700; @@ -137,89 +139,142 @@ function assertRealPathWithinRoot(rootDir: string, targetPath: string): void { } } -async function enforcePrivatePathMode( - resolvedPath: string, - expectedMode: number, - kind: "directory" | "file", -): Promise { - if (process.platform === "win32") { +async function createPrivateDirectory(directory: string, mode: number): Promise { + try { + await fsp.mkdir(directory, { mode }); + return true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "EEXIST") return false; + throw error; + } +} + +async function enforcePrivateDirectoryMode(params: { + realPath: string; + identity: BigIntStats; + mode: number; + created: boolean; + beforeChmod: () => Promise; +}): Promise { + if (process.platform === "win32") return; + const stat = await inspectDirectoryIdentity(params.realPath, params.identity); + if (!params.created) { + const actualMode = Number(stat.mode & 0o7777n); + if (actualMode !== params.mode) { + throw new FsSafeError( + "insecure-permissions", + `Private secret directory ${JSON.stringify(params.realPath)} has insecure permissions ${actualMode.toString(8)}.`, + ); + } return; } - await fsp.chmod(resolvedPath, expectedMode); - const stat = await fsp.stat(resolvedPath); - const actualMode = stat.mode & 0o777; - if (actualMode !== expectedMode) { - throw new Error( - `Private secret ${kind} ${resolvedPath} has insecure permissions ${actualMode.toString(8)}.`, - ); + const ownerUid = process.geteuid?.(); + if (ownerUid === undefined) { + throw new FsSafeError("helper-unavailable", "secret directory initialization requires owner identity"); } + const owner = await pinNodeDirectoryForMode(params.realPath, { + expectedIdentity: params.identity, + ownerUid, + }); + try { + await owner.apply(params.mode, { beforeChmod: params.beforeChmod }); + } finally { + await owner.close(); + } +} + +async function inspectPrivateDirectory(directory: string, kind: "root" | "directory component"): Promise { + return await inspectFileIdentity(async () => { + const stat = await fsp.lstat(directory, { bigint: true }); + if (stat.isSymbolicLink()) { + throw new Error(`Private secret ${kind} ${directory} must not be a symlink.`); + } + if (!stat.isDirectory()) { + throw new Error(`Private secret ${kind} ${directory} must be a directory.`); + } + return stat; + }); } async function ensurePrivateDirectory( rootDir: string, targetDir: string, mode: number, -): Promise<{ rootGuard: AsyncDirectoryGuard; targetReal: string }> { +): Promise<{ rootGuard: AsyncDirectoryGuard; parentGuard: AsyncDirectoryGuard }> { const resolvedRoot = path.resolve(rootDir); const resolvedTarget = path.resolve(targetDir); - await fsp.mkdir(resolvedRoot, { recursive: true, mode }); - const rootStat = await fsp.lstat(resolvedRoot); - if (rootStat.isSymbolicLink()) { - throw new Error(`Private secret root ${resolvedRoot} must not be a symlink.`); - } - if (!rootStat.isDirectory()) { - throw new Error(`Private secret root ${resolvedRoot} must be a directory.`); + let rootStat = await inspectPrivateDirectory(resolvedRoot, "root").catch((error) => { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; + return undefined; + }); + let createdRoot = false; + if (!rootStat) { + try { + createdRoot = await createPrivateDirectory(resolvedRoot, mode); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; + await fsp.mkdir(path.dirname(resolvedRoot), { recursive: true, mode }); + createdRoot = await createPrivateDirectory(resolvedRoot, mode); + } + rootStat = await inspectPrivateDirectory(resolvedRoot, "root"); } - const rootGuard = await createAsyncDirectoryGuard(resolvedRoot); - await enforcePrivatePathMode(rootGuard.realPath, mode, "directory"); + const rootGuard = await createAsyncDirectoryGuard(resolvedRoot, { bigint: true }); + assertOwnedDirectory(rootStat, rootGuard.stat); + await enforcePrivateDirectoryMode({ + realPath: rootGuard.realPath, identity: rootStat, mode, created: createdRoot, + beforeChmod: () => assertAsyncDirectoryGuard(rootGuard), + }); await assertAsyncDirectoryGuard(rootGuard); if (resolvedTarget === resolvedRoot) { - return { rootGuard, targetReal: rootGuard.realPath }; + return { rootGuard, parentGuard: rootGuard }; } assertPathWithinRoot(resolvedRoot, resolvedTarget); const resolvedRootReal = rootGuard.realPath; let current = resolvedRoot; + let targetGuard = rootGuard; for (const segment of path .relative(resolvedRoot, resolvedTarget) .split(path.sep) .filter(Boolean)) { current = path.join(current, segment); - const parentGuard = await createAsyncDirectoryGuard(path.dirname(current)); + const parentGuard = targetGuard; + let created = false; + let identity: BigIntStats; while (true) { await assertAsyncDirectoryGuard(rootGuard); await assertAsyncDirectoryGuard(parentGuard); try { - const stat = await fsp.lstat(current); - if (stat.isSymbolicLink()) { - throw new Error(`Private secret directory component ${current} must not be a symlink.`); - } - if (!stat.isDirectory()) { - throw new Error(`Private secret directory component ${current} must be a directory.`); - } + identity = await inspectPrivateDirectory(current, "directory component"); break; } catch (error) { if (!error || typeof error !== "object" || !("code" in error) || error.code !== "ENOENT") { throw error; } await assertAsyncDirectoryGuard(parentGuard); - try { - await fsp.mkdir(current, { mode }); - } catch (mkdirError) { - if ((mkdirError as NodeJS.ErrnoException).code !== "EEXIST") throw mkdirError; - } - // A successful mkdir or a competing creator still requires fresh type checks. + created = await createPrivateDirectory(current, mode); + // EEXIST grants no initialization authority; both outcomes need fresh type checks. } } - const currentReal = await fsp.realpath(current); - assertRealPathWithinRoot(resolvedRootReal, currentReal); - await enforcePrivatePathMode(currentReal, mode, "directory"); + const currentGuard = await createAsyncDirectoryGuard(current, { bigint: true }); + assertOwnedDirectory(identity, currentGuard.stat); + assertRealPathWithinRoot(resolvedRootReal, currentGuard.realPath); + await enforcePrivateDirectoryMode({ + realPath: currentGuard.realPath, identity, mode, created, + beforeChmod: async () => { + await assertAsyncDirectoryGuard(parentGuard); + await assertAsyncDirectoryGuard(rootGuard); + await assertAsyncDirectoryGuard(currentGuard); + }, + }); await assertAsyncDirectoryGuard(parentGuard); await assertAsyncDirectoryGuard(rootGuard); + await assertAsyncDirectoryGuard(currentGuard); + targetGuard = currentGuard; } - return { rootGuard, targetReal: await fsp.realpath(resolvedTarget) }; + return { rootGuard, parentGuard: targetGuard }; } type SecretFileWriteParams = { @@ -239,27 +294,45 @@ async function secretFileWriteQueueKey(filePath: string): Promise { } } -async function materializeSecretFileAtomic( - params: SecretFileWriteParams, - createOnly: boolean, -): Promise { +// Internal preparation for private writers and their pre-write locks; not lasting authorization. +export async function prepareSecretFileWrite( + params: Omit, +): Promise<{ + mode: number; + rootGuard: AsyncDirectoryGuard; + parentGuard: AsyncDirectoryGuard; + fileName: string; + finalFilePath: string; +}> { const mode = params.mode ?? PRIVATE_SECRET_FILE_MODE; const dirMode = params.dirMode ?? PRIVATE_SECRET_DIR_MODE; const resolvedRoot = path.resolve(params.rootDir); const resolvedFile = path.resolve(params.filePath); assertPathWithinRoot(resolvedRoot, resolvedFile); + for (const [kind, value] of [["file", mode], ["directory", dirMode]] as const) { + if (!Number.isInteger(value) || value < 0 || value > 0o7777) { + throw new FsSafeError("invalid-path", `Private secret ${kind} mode must be an integer between 0o0000 and 0o7777.`); + } + } const intendedParentDir = path.dirname(resolvedFile); - const { rootGuard, targetReal } = await ensurePrivateDirectory( + const { rootGuard, parentGuard } = await ensurePrivateDirectory( resolvedRoot, intendedParentDir, dirMode, ); await assertAsyncDirectoryGuard(rootGuard); - assertRealPathWithinRoot(rootGuard.realPath, targetReal); - const parentGuard = await createAsyncDirectoryGuard(targetReal); + await assertAsyncDirectoryGuard(parentGuard); + assertRealPathWithinRoot(rootGuard.realPath, parentGuard.realPath); const fileName = path.basename(resolvedFile); - const finalFilePath = path.join(targetReal, fileName); + const finalFilePath = path.join(parentGuard.realPath, fileName); + return { mode, rootGuard, parentGuard, fileName, finalFilePath }; +} +async function materializeSecretFileAtomic( + params: SecretFileWriteParams, + createOnly: boolean, +): Promise { + const { mode, rootGuard, parentGuard, fileName, finalFilePath } = await prepareSecretFileWrite(params); try { const stat = await fsp.lstat(finalFilePath); if (createOnly) { diff --git a/src/sidecar-lock-acquire.ts b/src/sidecar-lock-acquire.ts index 3225b1a0..4dcba000 100644 --- a/src/sidecar-lock-acquire.ts +++ b/src/sidecar-lock-acquire.ts @@ -1,5 +1,6 @@ import fs from "node:fs/promises"; import path from "node:path"; +import { canonicalPathFromExistingAncestor } from "./absolute-path.js"; import { FsSafeError } from "./errors.js"; import { fileObservation } from "./file-observation.js"; import { readFileHandleBounded } from "./bounded-read.js"; @@ -58,9 +59,17 @@ type SidecarLockAcquisitionContext = { ): Promise; }; -async function resolveNormalizedTargetPath(targetPath: string): Promise { +async function resolveNormalizedTargetPath(targetPath: string, lockRoot?: Root): Promise { const resolved = path.resolve(targetPath); const dir = path.dirname(resolved); + if (lockRoot) { + // The target is an arbitration key, not necessarily inside the lock Root. + // Leave parent creation to the Root-backed sidecar write. + await lockRoot.resolve("."); + const parent = await canonicalPathFromExistingAncestor(dir); + await lockRoot.resolve("."); + return path.join(parent, path.basename(resolved)); + } await fs.mkdir(dir, { recursive: true }); try { return path.join(await fs.realpath(dir), path.basename(resolved)); @@ -77,7 +86,7 @@ export async function acquireSidecarLock resolvePnpmCli(join(directory, "pnpm.mjs"))).toThrow("pnpm lifecycle CLI"); }); +it("lets a caller override the package script's default output directory", () => { + const directory = temporary(); + const first = join(directory, "default-artifacts"); + const last = join(directory, "requested-artifacts"); + writeFileSync(join(directory, "package.json"), JSON.stringify({ name: "fixture-not-fs-safe" })); + try { + execFileSync(process.execPath, [resolve("scripts/check-release-packages.mjs"), "--output", first, "--output", last], { + cwd: directory, env: { ...isolatedConsumerEnv(join(directory, "config")), npm_execpath: process.env.npm_execpath }, + encoding: "utf8", timeout: 10_000, stdio: "pipe", + }); + expect.fail("the fixture must stop at package validation"); + } catch (error) { + expect(error).toMatchObject({ status: 1 }); + expect(String((error as { stderr: string }).stderr)).toContain("unexpected package name fixture-not-fs-safe"); + } + expect(existsSync(first)).toBe(false); + expect(existsSync(last)).toBe(true); +}); + it("rejects direct collection before creating artifacts when the lifecycle is absent", () => { const directory = temporary(); const output = join(directory, "artifacts"); diff --git a/test/consumer-proof-metadata.test.ts b/test/consumer-proof-metadata.test.ts new file mode 100644 index 00000000..34a37acd --- /dev/null +++ b/test/consumer-proof-metadata.test.ts @@ -0,0 +1,54 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, expect, it, vi } from "vitest"; +import { nativeBinaryLoaded, packageProofSource } from "../scripts/consumer-proof-metadata.mjs"; +import { useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +afterEach(() => vi.unstubAllEnvs()); + +it("records the actual checkout rather than workflow head metadata", () => { + const source = packageProofSource(); + expect(source.commit).toMatch(/^[0-9a-f]{40}$/); + expect(source.tree).toMatch(/^[0-9a-f]{40}$/); + expect(typeof source.dirty).toBe("boolean"); +}); + +it("marks source metadata unavailable when Git is absent", async () => { + const directory = await tempRoot("fs-safe-proof-no-git-"); + vi.stubEnv("PATH", ""); + expect(packageProofSource(directory)).toEqual({ unavailable: "git-not-installed" }); +}); + +it("marks source archives without a Git checkout explicitly", async () => { + const directory = await tempRoot("fs-safe-proof-no-checkout-"); + expect(packageProofSource(directory)).toEqual({ unavailable: "git-metadata-unavailable" }); +}); + +it("requires the actual binary in the loaded-object list, not mere file presence", async () => { + const directory = await tempRoot("fs-safe-proof-loaded-"); + const binary = path.join(directory, "host.node"); + const other = path.join(directory, "other.node"); + await fs.writeFile(binary, "synthetic metadata fixture"); + await fs.writeFile(other, "synthetic metadata fixture"); + expect(nativeBinaryLoaded(binary, [])).toBe(false); + expect(nativeBinaryLoaded(binary, [other])).toBe(false); + expect(nativeBinaryLoaded(binary, [binary])).toBe(true); +}); + +it("accepts a Windows loader namespace spelling of the same binary", async () => { + const directory = await tempRoot("fs-safe-proof-loader-path-"); + const binary = path.join(directory, "host.node"); + await fs.writeFile(binary, "synthetic metadata fixture"); + const reported = path.toNamespacedPath(binary); + if (process.platform === "win32") expect(reported).toMatch(/^\\\\\?\\/); + expect(nativeBinaryLoaded(binary, [reported])).toBe(true); +}); + +it("does not silently accept an unreadable reported native object", async () => { + const directory = await tempRoot("fs-safe-proof-missing-object-"); + const binary = path.join(directory, "host.node"); + await fs.writeFile(binary, "synthetic metadata fixture"); + expect(() => nativeBinaryLoaded(binary, [path.join(directory, "missing.node")])) + .toThrowError(expect.objectContaining({ code: "ENOENT" })); +}); diff --git a/test/deepsec-regression.test.ts b/test/deepsec-regression.test.ts index e4eb0c9c..c3617642 100644 --- a/test/deepsec-regression.test.ts +++ b/test/deepsec-regression.test.ts @@ -226,7 +226,7 @@ describe("deepsec regressions", () => { const rootDir = path.join(base, "root"); const originalRoot = path.join(base, "root-original"); const outside = path.join(base, "outside"); - await fsp.mkdir(rootDir); + await fsp.mkdir(rootDir, { mode: 0o700 }); await fsp.mkdir(outside); const secretPath = path.join(rootDir, "nested", "secret.txt"); const realRealpath = fsp.realpath; @@ -243,6 +243,7 @@ describe("deepsec regressions", () => { await expect( writeSecretFileAtomic({ rootDir, filePath: secretPath, content: "secret" }), ).rejects.toBeTruthy(); + expect(swapped).toBe(true); await expect(fsp.lstat(path.join(outside, "nested"))).rejects.toMatchObject({ code: "ENOENT" }); }); @@ -250,7 +251,7 @@ describe("deepsec regressions", () => { configureFsSafeNative({ mode: "off" }); const base = await tempRoot("fs-safe-secret-native-off-"); const rootDir = path.join(base, "root"); - await fsp.mkdir(rootDir); + await fsp.mkdir(rootDir, { mode: 0o700 }); const secretPath = path.join(rootDir, "secret.txt"); await expect( @@ -265,7 +266,7 @@ describe("deepsec regressions", () => { const rootDir = path.join(base, "root"); const outside = path.join(base, "outside"); const movedParent = path.join(base, "nested-original"); - await fsp.mkdir(path.join(rootDir, "nested"), { recursive: true }); + await fsp.mkdir(path.join(rootDir, "nested"), { recursive: true, mode: 0o700 }); await fsp.mkdir(outside); const secretPath = path.join(rootDir, "nested", "secret.txt"); const realLstat = fsp.lstat; @@ -282,6 +283,7 @@ describe("deepsec regressions", () => { await expect( writeSecretFileAtomic({ rootDir, filePath: secretPath, content: "secret" }), ).rejects.toBeTruthy(); + expect(swapped).toBe(true); await expect(fsp.lstat(path.join(outside, "secret.txt"))).rejects.toMatchObject({ code: "ENOENT" }); }); @@ -291,7 +293,7 @@ describe("deepsec regressions", () => { const outside = path.join(base, "outside"); const movedParent = path.join(base, "nested-original"); const parentDir = path.join(rootDir, "nested"); - await fsp.mkdir(parentDir, { recursive: true }); + await fsp.mkdir(parentDir, { recursive: true, mode: 0o700 }); await fsp.mkdir(outside); const secretPath = path.join(parentDir, "secret.txt"); const realLstat = fsp.lstat; @@ -313,6 +315,7 @@ describe("deepsec regressions", () => { await expect( writeSecretFileAtomic({ rootDir, filePath: secretPath, content: "secret" }), ).rejects.toBeTruthy(); + expect(swapped).toBe(true); await expect(fsp.lstat(path.join(outside, "secret.txt"))).rejects.toMatchObject({ code: "ENOENT" }); }); @@ -321,7 +324,7 @@ describe("deepsec regressions", () => { const rootDir = path.join(base, "root"); const outside = path.join(base, "outside"); const outsideFile = path.join(outside, "outside.txt"); - await fsp.mkdir(rootDir); + await fsp.mkdir(rootDir, { mode: 0o700 }); await fsp.mkdir(outside); await fsp.writeFile(outsideFile, "outside"); await fsp.chmod(outsideFile, 0o600); diff --git a/test/directory-guard-exact-identity.test.ts b/test/directory-guard-exact-identity.test.ts new file mode 100644 index 00000000..d2bf4d0e --- /dev/null +++ b/test/directory-guard-exact-identity.test.ts @@ -0,0 +1,66 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { assertAsyncDirectoryGuard, createAsyncDirectoryGuard } from "../src/directory-guard.js"; +import { prepareSecretFileWrite } from "../src/secret-file.js"; +import { useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +const platform = Object.getOwnPropertyDescriptor(process, "platform")!; +afterEach(() => { + vi.restoreAllMocks(); + Object.defineProperty(process, "platform", platform); +}); + +describe.each(["capture", "verify", "secret preparation"] as const)("Windows exact directory %s", (stage) => { + it.each(["transient device", "transient inode", "persistent", "changed known component", "alternating", "symlink"])( + "retains bounded identity admission: %s", + async (scenario) => { + const directory = await tempRoot("fs-safe-directory-exact-"); + const guard = await createAsyncDirectoryGuard(directory, { bigint: true }); + Object.defineProperty(process, "platform", { value: "win32" }); + const lstat = fs.lstat.bind(fs); + let inspections = 0; + vi.spyOn(fs, "lstat").mockImplementation((async (...args: Parameters) => { + const stat = await lstat(...args); + if (String(args[0]) !== directory) return stat; + expect(typeof stat.ino).toBe("bigint"); + const attempt = ++inspections; + const first = attempt === 1; + if (scenario === "symlink" && !first) return Object.assign(Object.create(stat), { isSymbolicLink: () => true }); + if (scenario === "alternating") return Object.assign(Object.create(stat), first ? { dev: 0n } : { ino: 0n }); + if (scenario === "changed known component" && first) { + return Object.assign(Object.create(stat), { dev: 0n, ino: guard.stat.ino + 1n }); + } + if (first || scenario === "persistent") { + return Object.assign(Object.create(stat), scenario === "transient inode" ? { ino: 0n } : { dev: 0n }); + } + return stat; + }) as typeof fs.lstat); + const pending = stage === "capture" ? createAsyncDirectoryGuard(directory, { bigint: true }) + : stage === "verify" ? assertAsyncDirectoryGuard(guard) + : prepareSecretFileWrite({ rootDir: directory, filePath: path.join(directory, "secret") }); + if (scenario.startsWith("transient")) { + await pending; + expect(inspections).toBeGreaterThanOrEqual(2); + if (stage !== "secret preparation") expect(inspections).toBe(2); + } else if (scenario === "symlink") { + await expect(pending).rejects.toThrow(/directory|symlink/); + expect(inspections).toBe(2); + } else { + await expect(pending).rejects.toMatchObject({ code: "path-mismatch" }); + expect(inspections).toBe(stage === "verify" && scenario === "changed known component" ? 1 : 2); + } + }, + ); +}); + +it("does not retry an exact guard filesystem failure", async () => { + const directory = await tempRoot("fs-safe-directory-exact-error-"); + const guard = await createAsyncDirectoryGuard(directory, { bigint: true }); + Object.defineProperty(process, "platform", { value: "win32" }); + const failure = Object.assign(new Error("inspection denied"), { code: "EACCES" }); + const lstat = vi.spyOn(fs, "lstat").mockRejectedValue(failure); + await expect(assertAsyncDirectoryGuard(guard)).rejects.toBe(failure); + expect(lstat).toHaveBeenCalledTimes(1); +}); diff --git a/test/private-json-directory-admission.test.ts b/test/private-json-directory-admission.test.ts new file mode 100644 index 00000000..e296fb0f --- /dev/null +++ b/test/private-json-directory-admission.test.ts @@ -0,0 +1,131 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import * as privateBoundary from "../src/file-store-boundary.js"; +import { createJsonStore } from "../src/json-document-store.js"; +import { root } from "../src/root.js"; +import { fileStore, jsonStore } from "../src/store.js"; +import { itPosix, useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +afterEach(() => vi.restoreAllMocks()); + +describe("private JSON directory admission before locking", () => { + it.each(["write", "update", "updateOr"] as const)("prepares missing private directories before locked %s", async (operation) => { + const sandbox = await tempRoot("fs-safe-private-json-prepare-"); + const rootDir = path.join(sandbox, "root"); + const state = fileStore({ rootDir, private: true }).json<{ count: number }>("café/state.json", { lock: true }); + if (operation === "write") await state.write({ count: 1 }); + else if (operation === "update") await state.update(() => ({ count: 1 })); + else await state.updateOr({ count: 0 }, (current) => ({ count: current.count + 1 })); + + expect(await state.readRequired()).toEqual({ count: 1 }); + expect(await fs.readdir(path.join(rootDir, "café"))).toEqual(["state.json"]); + if (process.platform !== "win32") { + expect((await fs.stat(rootDir)).mode & 0o7777).toBe(0o700); + expect((await fs.stat(path.join(rootDir, "café"))).mode & 0o7777).toBe(0o700); + } + }); + + it("prepares the standalone jsonStore's missing root without implicit repair", async () => { + const sandbox = await tempRoot("fs-safe-json-store-prepare-"); + const filePath = path.join(sandbox, "nested", "state.json"); + const state = jsonStore<{ count: number }>({ filePath, lock: true }); + await state.updateOr({ count: 0 }, (current) => ({ count: current.count + 1 })); + expect(await state.readRequired()).toEqual({ count: 1 }); + if (process.platform !== "win32") expect((await fs.stat(path.dirname(filePath))).mode & 0o7777).toBe(0o700); + }); + + itPosix("rejects an existing non-private parent before acquiring a lock or running the update", async () => { + const rootDir = await tempRoot("fs-safe-private-json-refuse-"); + const parent = path.join(rootDir, "parent"); + await fs.mkdir(parent, { mode: 0o750 }); + await fs.chmod(parent, 0o750); + const state = fileStore({ rootDir, private: true }).json("parent/state.json", { lock: true }); + const update = vi.fn(() => ({ count: 1 })); + const mkdir = vi.spyOn(fs, "mkdir"); + const chmod = vi.spyOn(fs, "chmod"); + + await expect(state.update(update)).rejects.toMatchObject({ code: "insecure-permissions" }); + + expect(update).not.toHaveBeenCalled(); + expect(mkdir).not.toHaveBeenCalled(); + expect(chmod).not.toHaveBeenCalled(); + expect((await fs.stat(parent)).mode & 0o7777).toBe(0o750); + expect(await fs.readdir(parent)).toEqual([]); + }); + + it("keeps the admitted parent identity when lock normalization sees a replacement", async () => { + const rootDir = await tempRoot("fs-safe-private-json-parent-swap-"); + const parent = path.join(rootDir, "parent"); + const moved = path.join(rootDir, "moved"); + const state = fileStore({ rootDir, private: true }).json("parent/state.json", { lock: true }); + const prepare = privateBoundary.openPrivateStoreLockRoot; + let swapped = false; + const prepareSpy = vi.spyOn(privateBoundary, "openPrivateStoreLockRoot").mockImplementation(async (params) => { + const admitted = await prepare(params); + await fs.rename(parent, moved); + await fs.mkdir(parent, { mode: 0o700 }); + swapped = true; + return admitted; + }); + const update = vi.fn(() => ({ count: 1 })); + + await expect(state.update(update)).rejects.toMatchObject({ code: "path-mismatch" }); + + expect(swapped).toBe(true); + expect(update).not.toHaveBeenCalled(); + expect(await fs.readdir(parent)).toEqual([]); + expect(await fs.readdir(moved)).toEqual([]); + prepareSpy.mockRestore(); + await state.write({ count: 2 }); + expect(await state.readRequired()).toEqual({ count: 2 }); + }); + + it("does not recreate an admitted parent deleted before lock acquisition", async () => { + const rootDir = await tempRoot("fs-safe-private-json-parent-deleted-"); + const parent = path.join(rootDir, "parent"); + const state = fileStore({ rootDir, private: true }).json("parent/state.json", { lock: true }); + const prepare = privateBoundary.openPrivateStoreLockRoot; + let removed = false; + vi.spyOn(privateBoundary, "openPrivateStoreLockRoot").mockImplementation(async (params) => { + const admitted = await prepare(params); + await fs.rmdir(parent); + removed = true; + return admitted; + }); + const update = vi.fn(() => ({ count: 1 })); + + await expect(state.update(update)).rejects.toMatchObject({ code: "path-mismatch" }); + + expect(removed).toBe(true); + expect(update).not.toHaveBeenCalled(); + await expect(fs.lstat(parent)).rejects.toMatchObject({ code: "ENOENT" }); + expect(await fs.readdir(rootDir)).toEqual([]); + }); + + it("does not prepare directories on reads", async () => { + const sandbox = await tempRoot("fs-safe-private-json-read-"); + const state = fileStore({ rootDir: path.join(sandbox, "missing"), private: true }) + .json("parent/state.json", { lock: true }); + await expect(state.read()).resolves.toBeUndefined(); + await expect(state.readOr({ count: 0 })).resolves.toEqual({ count: 0 }); + expect(await fs.readdir(sandbox)).toEqual([]); + }); + + it("does not invoke adapter preparation when locking is disabled", async () => { + const directory = await tempRoot("fs-safe-json-unlocked-prepare-"); + const prepareLock = vi.fn(async () => await root(directory)); + const write = vi.fn(async () => undefined); + const state = createJsonStore({ + filePath: path.join(directory, "state.json"), + prepareLock, + readIfExists: async () => undefined, + readRequired: async () => ({}), + write, + }, { lock: false }); + await state.write({}); + expect(prepareLock).not.toHaveBeenCalled(); + expect(write).toHaveBeenCalledOnce(); + }); +}); diff --git a/test/secret-directory-admission.test.ts b/test/secret-directory-admission.test.ts new file mode 100644 index 00000000..5c8fd69e --- /dev/null +++ b/test/secret-directory-admission.test.ts @@ -0,0 +1,235 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { createSecretFileAtomic, writeSecretFileAtomic } from "../src/secret.js"; +import { itPosix, useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +const writers = [ + { operation: "write", write: writeSecretFileAtomic }, + { operation: "create", write: createSecretFileAtomic }, +] as const; +afterEach(() => vi.restoreAllMocks()); + +async function directoryWithMode(directory: string, mode: number): Promise { + await fs.mkdir(directory, { mode: 0o700 }); + await fs.chown(directory, process.geteuid!(), process.getegid!()); + await fs.chmod(directory, mode); + expect((await fs.lstat(directory)).mode & 0o7777).toBe(mode); +} + +describe("secret directory admission", () => { + for (const component of ["root", "parent"] as const) { + for (const mode of [0o500, 0o750, 0o2750]) { + itPosix.each(writers)(`$operation rejects existing ${component} mode ${mode.toString(8)} without repairing it`, async ({ write }) => { + const sandbox = await tempRoot("fs-safe-secret-mode-admission-"); + const rootDir = path.join(sandbox, "root"); + await directoryWithMode(rootDir, 0o700); + const parent = component === "root" ? rootDir : path.join(rootDir, "parent"); + if (parent !== rootDir) await directoryWithMode(parent, 0o700); + const filePath = path.join(parent, "token"); + await fs.chmod(parent, mode); + const before = await fs.lstat(parent, { bigint: true }); + const chmod = fs.chmod.bind(fs); + const chmodSpy = vi.spyOn(fs, "chmod"); + try { + await expect(write({ rootDir, filePath, content: "synthetic" })) + .rejects.toMatchObject({ code: "insecure-permissions" }); + expect(chmodSpy).not.toHaveBeenCalled(); + const after = await fs.lstat(parent, { bigint: true }); + expect({ dev: after.dev, ino: after.ino, mode: after.mode }).toEqual({ dev: before.dev, ino: before.ino, mode: before.mode }); + await expect(fs.lstat(filePath)).rejects.toMatchObject({ code: "ENOENT" }); + } finally { + await chmod(parent, 0o700); + } + }); + } + } + + for (const dirMode of [0o700, 0o750, 0o2750]) { + itPosix.each(writers)(`$operation accepts matching mode ${dirMode.toString(8)} without chmod`, async ({ write }) => { + const sandbox = await tempRoot("fs-safe-secret-mode-match-"); + const rootDir = path.join(sandbox, "root"); + await directoryWithMode(rootDir, dirMode); + const chmod = vi.spyOn(fs, "chmod"); + const filePath = path.join(rootDir, "token"); + + await write({ rootDir, filePath, dirMode, content: "synthetic" }); + + expect(chmod).not.toHaveBeenCalled(); + expect((await fs.lstat(rootDir)).mode & 0o7777).toBe(dirMode); + expect(await fs.readFile(filePath, "utf8")).toBe("synthetic"); + }); + } + + itPosix.each(writers)("$operation initializes an explicitly requested setgid root", async ({ write }) => { + const sandbox = await tempRoot("fs-safe-secret-mode-setgid-"); + await fs.chown(sandbox, process.geteuid!(), process.getegid!()); + const rootDir = path.join(sandbox, "root"); + const filePath = path.join(rootDir, "token"); + const chmod = vi.spyOn(fs, "chmod"); + + await write({ rootDir, filePath, dirMode: 0o2750, content: "synthetic" }); + + expect(chmod).not.toHaveBeenCalled(); + expect((await fs.lstat(rootDir)).mode & 0o7777).toBe(0o2750); + expect(await fs.readFile(filePath, "utf8")).toBe("synthetic"); + }); + + for (const component of ["root", "parent"] as const) { + itPosix.each(writers)(`$operation does not repair a ${component} created by an EEXIST winner`, async ({ write }) => { + const sandbox = await tempRoot("fs-safe-secret-mode-eexist-"); + const rootDir = path.join(sandbox, "root"); + if (component === "parent") await directoryWithMode(rootDir, 0o700); + const parent = component === "root" ? rootDir : path.join(rootDir, "parent"); + const mkdir = fs.mkdir.bind(fs); + const chmod = fs.chmod.bind(fs); + let raced = false; + vi.spyOn(fs, "mkdir").mockImplementation(async (candidate, options) => { + if (String(candidate) === parent && !raced) { + raced = true; + await mkdir(parent, { mode: 0o750 }); + await chmod(parent, 0o750); + } + return await mkdir(candidate, options); + }); + const chmodSpy = vi.spyOn(fs, "chmod"); + + await expect(write({ rootDir, filePath: path.join(parent, "token"), content: "synthetic" })) + .rejects.toMatchObject({ code: "insecure-permissions" }); + + expect(raced).toBe(true); + expect(chmodSpy).not.toHaveBeenCalled(); + expect((await fs.lstat(parent)).mode & 0o7777).toBe(0o750); + expect(await fs.readdir(parent)).toEqual([]); + }); + } + + itPosix.each(writers)("$operation requires descriptor authority for an umask777 parent", async ({ write }) => { + const rootDir = await tempRoot("fs-safe-secret-mode-created-"); + const parent = path.join(rootDir, "parent"); + const filePath = path.join(parent, "token"); + const chmod = vi.spyOn(fs, "chmod"); + const previous = process.umask(0o777); + try { + if (process.platform === "darwin" && process.geteuid?.() !== 0) { + await expect(write({ rootDir, filePath, content: "synthetic" })) + .rejects.toMatchObject({ code: "EACCES" }); + expect(chmod).not.toHaveBeenCalled(); + expect((await fs.lstat(parent)).mode & 0o7777).toBe(0o000); + expect(await fs.readdir(rootDir)).toEqual(["parent"]); + } else { + await write({ rootDir, filePath, content: "synthetic" }); + expect(chmod.mock.calls.every(([target]) => String(target).startsWith("/proc/self/fd/"))).toBe(true); + expect((await fs.lstat(parent)).mode & 0o7777).toBe(0o700); + expect((await fs.lstat(filePath)).mode & 0o7777).toBe(0o600); + } + } finally { + process.umask(previous); + await fs.chmod(parent, 0o700).catch(() => undefined); + } + if (process.platform === "darwin" && process.geteuid?.() !== 0) { + expect(await fs.readdir(parent)).toEqual([]); + } + }); + + itPosix.each(writers)("$operation initializes a missing nested root without widening its bootstrap parent", async ({ write }) => { + const sandbox = await tempRoot("fs-safe-secret-mode-bootstrap-"); + const bootstrap = path.join(sandbox, "bootstrap"); + const rootDir = path.join(bootstrap, "root"); + const previous = process.umask(0o400); + try { + await write({ rootDir, filePath: path.join(rootDir, "token"), content: "synthetic" }); + expect((await fs.lstat(bootstrap)).mode & 0o7777).toBe(0o300); + expect((await fs.lstat(rootDir)).mode & 0o7777).toBe(0o700); + } finally { + process.umask(previous); + await fs.chmod(bootstrap, 0o700).catch(() => undefined); + } + }); + + for (const option of ["mode", "dirMode"] as const) { + for (const value of [-1, 0o10000, 1.5, Infinity, NaN]) { + it.each(writers)(`$operation rejects ${option}=${value} before creating directories`, async ({ write }) => { + const sandbox = await tempRoot("fs-safe-secret-invalid-mode-"); + const rootDir = path.join(sandbox, "root"); + const mkdir = vi.spyOn(fs, "mkdir"); + await expect(write({ rootDir, filePath: path.join(rootDir, "token"), content: "synthetic", [option]: value })) + .rejects.toMatchObject({ code: "invalid-path" }); + expect(mkdir).not.toHaveBeenCalled(); + expect(await fs.readdir(sandbox)).toEqual([]); + }); + } + } + + itPosix.each(writers)("$operation rejects a created directory replaced while its descriptor is opened", async ({ write }) => { + const rootDir = await tempRoot("fs-safe-secret-mode-descriptor-swap-"); + const parent = path.join(rootDir, "parent"); + const moved = path.join(rootDir, "moved"); + const open = fs.open.bind(fs); + const chmod = fs.chmod.bind(fs); + let swapped = false; + vi.spyOn(fs, "open").mockImplementation(async (...args) => { + const handle = await open(...args); + if (String(args[0]) === parent && !swapped) { + swapped = true; + await fs.rename(parent, moved); + await fs.mkdir(parent, { mode: 0o750 }); + await chmod(parent, 0o750); + } + return handle; + }); + const chmodSpy = vi.spyOn(fs, "chmod"); + const previous = process.umask(0o400); + try { + await expect(write({ rootDir, filePath: path.join(parent, "token"), content: "synthetic" })) + .rejects.toMatchObject({ code: "path-mismatch" }); + expect(swapped).toBe(true); + expect(chmodSpy).not.toHaveBeenCalled(); + expect((await fs.lstat(parent)).mode & 0o7777).toBe(0o750); + expect((await fs.lstat(moved)).mode & 0o7777).toBe(0o300); + expect(await fs.readdir(parent)).toEqual([]); + } finally { + process.umask(previous); + await chmod(parent, 0o700).catch(() => undefined); + await chmod(moved, 0o700).catch(() => undefined); + } + }); + + itPosix.each(writers)("$operation rejects changed descriptor ownership before initialization", async ({ write }) => { + const rootDir = await tempRoot("fs-safe-secret-mode-owner-"); + const parent = path.join(rootDir, "parent"); + const open = fs.open.bind(fs); + const chmod = fs.chmod.bind(fs); + let changedOwner = false; + vi.spyOn(fs, "open").mockImplementation(async (...args) => { + const handle = await open(...args); + if (String(args[0]) === parent) { + const stat = handle.stat.bind(handle); + let inspections = 0; + vi.spyOn(handle, "stat").mockImplementation(async (options) => { + const value = await stat(options); + if (++inspections > 1) { + changedOwner = true; + Object.assign(value, { uid: typeof value.uid === "bigint" ? value.uid + 1n : value.uid + 1 }); + } + return value; + }); + } + return handle; + }); + const chmodSpy = vi.spyOn(fs, "chmod"); + const previous = process.umask(0o400); + try { + await expect(write({ rootDir, filePath: path.join(parent, "token"), content: "synthetic" })) + .rejects.toMatchObject({ code: "not-owned" }); + expect(changedOwner).toBe(true); + expect(chmodSpy).not.toHaveBeenCalled(); + expect((await fs.lstat(parent)).mode & 0o7777).toBe(0o300); + await expect(fs.lstat(path.join(parent, "token"))).rejects.toMatchObject({ code: "ENOENT" }); + } finally { + process.umask(previous); + await chmod(parent, 0o700).catch(() => undefined); + } + }); +}); diff --git a/test/secret-directory-receipt.test.ts b/test/secret-directory-receipt.test.ts new file mode 100644 index 00000000..743d25ad --- /dev/null +++ b/test/secret-directory-receipt.test.ts @@ -0,0 +1,56 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, vi } from "vitest"; +import { createSecretFileAtomic, writeSecretFileAtomic } from "../src/secret.js"; +import { itPosix, useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +const writers = [ + { operation: "write", write: writeSecretFileAtomic }, + { operation: "create", write: createSecretFileAtomic }, +] as const; +afterEach(() => vi.restoreAllMocks()); + +describe("secret directory admission receipts", () => { + for (const phase of ["final parent", "ancestor"] as const) { + itPosix.each(writers)(`$operation retains the admitted ${phase} instead of adopting a replacement`, async ({ write }) => { + const rootDir = await tempRoot("fs-safe-secret-directory-receipt-"); + const parent = path.join(rootDir, "parent"); + const moved = path.join(rootDir, "admitted"); + await fs.mkdir(parent, { mode: 0o700 }); + const filePath = path.join(parent, phase === "ancestor" ? "inner/token" : "token"); + const lstat = fs.lstat.bind(fs); + const realpath = fs.realpath.bind(fs); + let inspections = 0; + let admitted = false; + let swapped = false; + const swap = async () => { + swapped = true; + await fs.rename(parent, moved); + await fs.mkdir(parent, { mode: 0o750 }); + await fs.chmod(parent, 0o750); + }; + vi.spyOn(fs, "lstat").mockImplementation(async (target, options) => { + const isParent = String(target) === parent; + if (phase === "ancestor" && admitted && isParent && !swapped) await swap(); + const value = await lstat(target, options); + // Initial inspection, guard capture, then the permission-admission inspection. + if (isParent && options?.bigint && ++inspections >= 3) admitted = true; + return value; + }); + vi.spyOn(fs, "realpath").mockImplementation(async (target, options) => { + if (phase === "final parent" && admitted && String(target) === parent && !swapped) await swap(); + return await realpath(target, options); + }); + + const failure = await write({ rootDir, filePath, content: "synthetic" }).catch((error: unknown) => error); + + expect(swapped).toBe(true); + expect(failure).toMatchObject({ code: "path-mismatch" }); + expect((await fs.lstat(parent)).mode & 0o7777).toBe(0o750); + expect((await fs.lstat(moved)).mode & 0o7777).toBe(0o700); + expect(await fs.readdir(parent)).toEqual([]); + expect(await fs.readdir(moved)).toEqual([]); + }); + } +}); diff --git a/test/secret-directory-wide-identity.test.ts b/test/secret-directory-wide-identity.test.ts new file mode 100644 index 00000000..8671a2df --- /dev/null +++ b/test/secret-directory-wide-identity.test.ts @@ -0,0 +1,130 @@ +import fsSync, { type BigIntStats, type Stats } from "node:fs"; +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { createAsyncDirectoryGuard } from "../src/directory-guard.js"; +import { openPrivateStoreLockRoot } from "../src/file-store-boundary.js"; +import { configureFsSafeNative } from "../src/index.js"; +import { __loadBundledNativeForTest, __resetNativeLoaderForTest, __setNativeLoaderForTest } from "../src/native.js"; +import { runPinnedWriteHelper } from "../src/pinned-write.js"; +import { createSecretFileAtomic, prepareSecretFileWrite, writeSecretFileAtomic } from "../src/secret-file.js"; +import { useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +const platform = Object.getOwnPropertyDescriptor(process, "platform")!; +let nativeAvailable = false; +try { + __loadBundledNativeForTest(); + nativeAvailable = true; +} catch (error) { + if (process.env.FS_SAFE_NATIVE_MODE === "require") throw error; +} +afterEach(() => { + vi.restoreAllMocks(); + Object.defineProperty(process, "platform", platform); + configureFsSafeNative({ mode: "auto" }); + __resetNativeLoaderForTest(); +}); + +async function fixture() { + const rootDir = await tempRoot("fs-safe-secret-wide-identity-"); + const parent = path.join(rootDir, "parent"); + await fs.mkdir(parent, { mode: 0o700 }); + const identities = new Map([ + [rootDir, (1n << 56n) + 1n], + [parent, (1n << 56n) + 3n], + ]); + const directories = new Map(); + for (const directory of identities.keys()) { + const stat = await fs.lstat(directory, { bigint: true }); + directories.set(`${stat.dev}:${stat.ino}`, directory); + } + const project = (stat: T, directory: string): T => { + const ino = identities.get(directory); + return ino === undefined ? stat : Object.assign(Object.create(stat), { + ino: typeof stat.ino === "bigint" ? ino : Number(ino), + }); + }; + for (const method of ["lstat", "stat"] as const) { + const original = fs[method].bind(fs); + vi.spyOn(fs, method).mockImplementation((async (...args: Parameters) => + project(await original(...args), String(args[0]))) as typeof fs.stat); + } + const lstatSync = fsSync.lstatSync.bind(fsSync); + vi.spyOn(fsSync, "lstatSync").mockImplementation(((...args: Parameters) => + project(lstatSync(...args), String(args[0]))) as typeof fsSync.lstatSync); + const fstat = fsSync.fstatSync.bind(fsSync); + vi.spyOn(fsSync, "fstatSync").mockImplementation(((...args: Parameters) => { + const exact = fstat(args[0], { bigint: true }); + return project(fstat(...args), directories.get(`${exact.dev}:${exact.ino}`) ?? ""); + }) as typeof fsSync.fstatSync); + return { rootDir, parent, identities, filePath: path.join(parent, "state.json") }; +} + +describe("lossless secret directory identities", () => { + it("admits a stable identity without comparing it to rounded numeric stats", async () => { + const { rootDir, parent, filePath, identities } = await fixture(); + const prepared = await prepareSecretFileWrite({ rootDir, filePath }); + expect(prepared.rootGuard.stat.ino).toBe(identities.get(rootDir)); + expect(prepared.parentGuard.stat.ino).toBe(identities.get(parent)); + }); + + it("keeps default directory guards numeric", async () => { + const { rootDir } = await fixture(); + const guard = await createAsyncDirectoryGuard(rootDir); + expect(typeof guard.stat.ino).toBe("number"); + expect(typeof guard.stat.mode).toBe("number"); + }); + + it("binds private lock roots losslessly and rejects a different identity with the same numeric projection", async () => { + const { rootDir, parent, filePath, identities } = await fixture(); + const lockRoot = await openPrivateStoreLockRoot({ rootDir, filePath }); + await expect(lockRoot.stat("")).resolves.toMatchObject({ isDirectory: true }); + const original = identities.get(parent)!; + const replacement = original + 2n; + expect(Number(replacement)).toBe(Number(original)); + identities.set(parent, replacement); + await expect(lockRoot.stat("")).rejects.toMatchObject({ code: "path-mismatch" }); + }); +}); + +for (const route of ["fallback", "Windows fallback", "native", "Windows native"] as const) { + describe.skipIf((route.includes("native") && !nativeAvailable) || (route === "native" && process.platform === "win32"))( + `lossless secret directories through ${route}`, + () => { + function configureRoute() { + if (route.includes("native")) { + const binding = __loadBundledNativeForTest(); + __setNativeLoaderForTest(() => binding); + } + if (route.startsWith("Windows")) Object.defineProperty(process, "platform", { value: "win32" }); + configureFsSafeNative({ mode: route.includes("native") ? "require" : "off" }); + } + + it.each([ + { operation: "write", write: writeSecretFileAtomic }, + { operation: "create", write: createSecretFileAtomic }, + ])("$operation writes real bytes through exact directory guards", async ({ write }) => { + const { rootDir, filePath } = await fixture(); + configureRoute(); + await write({ rootDir, filePath, content: "synthetic wide-directory proof" }); + expect(await fs.readFile(filePath, "utf8")).toBe("synthetic wide-directory proof"); + }); + + it("rejects an exact root mismatch before creating a file", async () => { + const { rootDir, parent, filePath, identities } = await fixture(); + const { parentGuard } = await prepareSecretFileWrite({ rootDir, filePath }); + configureRoute(); + const original = identities.get(parent)!; + expect(Number(original + 2n)).toBe(Number(original)); + identities.set(parent, original + 2n); + await expect(runPinnedWriteHelper({ + rootPath: parent, relativeParentPath: "", basename: "state.json", mkdir: false, + mode: 0o600, input: { kind: "buffer", data: "must not be published" }, + rootIdentity: { dev: parentGuard.stat.dev, ino: parentGuard.stat.ino }, + })).rejects.toMatchObject({ code: "path-mismatch" }); + expect(await fs.readdir(parent)).toEqual([]); + }); + }, + ); +} diff --git a/test/secret-file-failure.test.ts b/test/secret-file-failure.test.ts index 6370470e..f6a1101e 100644 --- a/test/secret-file-failure.test.ts +++ b/test/secret-file-failure.test.ts @@ -50,7 +50,7 @@ describe("secret file refusal paths", () => { const root = await tempRoot("fs-safe-secret-write-link-"); const realRoot = path.join(root, "real"); const linkedRoot = path.join(root, "linked-root"); - await fs.mkdir(realRoot); + await fs.mkdir(realRoot, { mode: 0o700 }); await fs.symlink(realRoot, linkedRoot); await expect( writeSecretFileAtomic({ diff --git a/test/secret-write-publication.test.ts b/test/secret-write-publication.test.ts index 25c4f434..091d542a 100644 --- a/test/secret-write-publication.test.ts +++ b/test/secret-write-publication.test.ts @@ -113,7 +113,7 @@ for (const backend of ["off", "require"] as const) { const parent = path.join(rootDir, "parent"); const outside = path.join(sandbox, "outside"); const filePath = path.join(parent, "token"); - await fs.mkdir(parent, { recursive: true }); + await fs.mkdir(parent, { recursive: true, mode: 0o700 }); await fs.mkdir(outside); const outsideFile = path.join(outside, "untouched"); await fs.writeFile(outsideFile, "outside", { mode: 0o600 }); diff --git a/test/sidecar-lock-root-normalization.test.ts b/test/sidecar-lock-root-normalization.test.ts new file mode 100644 index 00000000..030fee7b --- /dev/null +++ b/test/sidecar-lock-root-normalization.test.ts @@ -0,0 +1,125 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { configureFsSafeNative } from "../src/native-config.js"; +import { root } from "../src/root.js"; +import { createSidecarLockManager } from "../src/sidecar-lock.js"; +import { itPosix, useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +afterEach(() => { + vi.restoreAllMocks(); + configureFsSafeNative({ mode: "auto" }); +}); + +describe.each(["off", "auto"] as const)("Root-backed lock normalization (%s)", (mode) => { + it("does not create an external target parent when an explicit in-root lock is supplied", async () => { + configureFsSafeNative({ mode }); + const directory = await tempRoot("fs-safe-lock-external-target-"); + const lockDirectory = path.join(directory, "locks"); + await fs.mkdir(lockDirectory); + const lockRoot = await root(lockDirectory); + const parent = path.join(directory, "external", "missing"); + const targetPath = path.join(parent, "state.json"); + const lockPath = path.join(lockDirectory, "state.lock"); + const manager = createSidecarLockManager(`external-target:${directory}`); + const create = vi.spyOn(lockRoot, "create"); + const lock = await manager.acquire({ targetPath, lockPath, lockRoot, payload: () => ({ owner: "synthetic" }) }); + try { + expect(lock.normalizedTargetPath).toBe(targetPath); + expect(create).toHaveBeenCalledWith("state.lock", expect.any(String), { mkdir: true, mode: 0o600 }); + await expect(fs.lstat(parent)).rejects.toMatchObject({ code: "ENOENT" }); + await expect(lock.verifyStillHeld()).resolves.toBe(true); + } finally { + await lock.release(); + } + expect(await fs.readdir(directory)).toEqual(["locks"]); + expect(await fs.readdir(lockDirectory)).toEqual([]); + }); + + it("canonicalizes missing parents below a Root alias and creates them only through Root", async () => { + configureFsSafeNative({ mode }); + const directory = await tempRoot("fs-safe-lock-parent-alias-"); + const actual = path.join(directory, "actual"); + const alias = path.join(directory, "alias"); + await fs.mkdir(actual); + await fs.symlink(actual, alias, process.platform === "win32" ? "junction" : "dir"); + const lockRoot = await root(alias); + const targetPath = path.join(alias, "missing", "state.json"); + const canonical = path.join(actual, "missing", "state.json"); + const manager = createSidecarLockManager(`aliased-target:${directory}`); + const create = vi.spyOn(lockRoot, "create"); + const payload = () => ({ owner: "synthetic" }); + const first = await manager.acquire({ targetPath, lockRoot, payload, reentrantOwner: "same" }); + try { + const second = await manager.acquire({ targetPath: canonical, lockRoot, payload, reentrantOwner: "same" }); + try { + expect(first.normalizedTargetPath).toBe(canonical); + expect(second.normalizedTargetPath).toBe(canonical); + expect(create).toHaveBeenCalledTimes(1); + expect(create).toHaveBeenCalledWith("missing/state.json.lock", expect.any(String), { mkdir: true, mode: 0o600 }); + } finally { + await second.release(); + } + await expect(first.verifyStillHeld()).resolves.toBe(true); + } finally { + await first.release(); + } + expect(await fs.readdir(path.join(actual, "missing"))).toEqual([]); + }); + + itPosix("does not follow the target leaf symlink when selecting its arbitration key", async () => { + configureFsSafeNative({ mode }); + const directory = await tempRoot("fs-safe-lock-leaf-alias-"); + const lockRoot = await root(directory); + const targetPath = path.join(directory, "alias.json"); + await fs.writeFile(path.join(directory, "original.json"), "unchanged"); + await fs.symlink("original.json", targetPath); + const manager = createSidecarLockManager(`leaf-target:${directory}`); + const lock = await manager.acquire({ targetPath, lockRoot, payload: () => ({ owner: "synthetic" }) }); + try { + expect(lock.normalizedTargetPath).toBe(targetPath); + expect(lock.lockPath).toBe(`${targetPath}.lock`); + expect(await fs.readFile(path.join(directory, "original.json"), "utf8")).toBe("unchanged"); + } finally { + await lock.release(); + } + }); + + it("rejects a replaced Root before payload execution or reentrant held-entry reuse", async () => { + configureFsSafeNative({ mode }); + const directory = await tempRoot("fs-safe-lock-stale-root-"); + const original = path.join(directory, "original"); + const moved = path.join(directory, "moved"); + await fs.mkdir(original); + const staleRoot = await root(original); + const before = await fs.stat(original, { bigint: true }); + // Replace before opening a sidecar: the policy must not depend on renaming an open subtree. + await fs.rename(original, moved); + await fs.mkdir(original); + expect((await fs.stat(original, { bigint: true })).ino).not.toBe(before.ino); + expect((await fs.stat(moved, { bigint: true })).ino).toBe(before.ino); + const freshRoot = await root(original); + const targetPath = path.join(original, "state.json"); + const manager = createSidecarLockManager(`stale-root:${directory}`); + const payload = vi.fn(() => ({ owner: "synthetic" })); + const options = { targetPath, payload, reentrantOwner: "same", timeoutMs: 1000, retry: { retries: 0 } }; + { + await using first = await manager.acquire({ ...options, lockRoot: freshRoot }); + payload.mockClear(); + let refusal: unknown; + // Retain an unexpected success too, so both handles close if the assertion fails. + await using unexpected = await manager.acquire({ ...options, lockRoot: staleRoot }).catch((error: unknown) => { + refusal = error; + return undefined; + }); + expect(refusal).toMatchObject({ code: "path-mismatch" }); + expect(unexpected).toBeUndefined(); + expect(payload).not.toHaveBeenCalled(); + await expect(first.verifyStillHeld()).resolves.toBe(true); + expect(await fs.readdir(original)).toEqual(["state.json.lock"]); + } + expect(await fs.readdir(original)).toEqual([]); + expect(await fs.readdir(moved)).toEqual([]); + }); +}); From 027f6435122b6f68185754815b7dd9b1eab97c70 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 3 Sep 2026 22:40:28 -0700 Subject: [PATCH 5/5] fix(write): preserve final modes and cleanup ownership (#223) Finalize explicit file modes after content, verify complete secret mode bits, and preserve exact ownership through native mode changes and failed-write cleanup. Keep append permission tightening before data while restoring special bits afterward and completing short synchronous writes. Add regressions, native CI selection, docs, and Unreleased notes. --- .github/workflows/ci.yml | 4 +- CHANGELOG.md | 2 + docs/regular-file.md | 10 +- docs/secret-file.md | 21 +++- src/native-operations.ts | 23 ++-- src/native-pinned-write-windows.ts | 16 +-- src/pinned-write.ts | 32 +++-- src/regular-file.ts | 11 +- src/replace-file-temp-owner.ts | 32 +++++ src/root-write-verification.ts | 2 +- test/fallback-write-mode-cleanup.test.ts | 45 ++++++++ test/file-mode-facades.test.ts | 77 ++++++++++++ test/native-created-cleanup.test.ts | 49 ++++++++ test/native-write-mode-ownership.test.ts | 122 ++++++++++++++++++++ test/pinned-write-cleanup-authority.test.ts | 48 ++++++++ test/pinned-write-file-mode.test.ts | 66 +++++++++++ test/regular-file-mode.test.ts | 122 ++++++++++++++++++++ test/root-write-exact-identity.test.ts | 5 +- test/secret-write-publication.test.ts | 39 +++++-- 19 files changed, 674 insertions(+), 52 deletions(-) create mode 100644 test/fallback-write-mode-cleanup.test.ts create mode 100644 test/file-mode-facades.test.ts create mode 100644 test/native-created-cleanup.test.ts create mode 100644 test/native-write-mode-ownership.test.ts create mode 100644 test/pinned-write-cleanup-authority.test.ts create mode 100644 test/pinned-write-file-mode.test.ts create mode 100644 test/regular-file-mode.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0329b04b..e61960b1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -140,7 +140,7 @@ jobs: - name: Test native Root publication verification env: FS_SAFE_NATIVE_MODE: require - run: pnpm test test/root-write-mode.test.ts test/root-write-verification.test.ts test/root-write-lifetime.test.ts test/root-write-exact-identity.test.ts + run: pnpm test test/root-write-mode.test.ts test/root-write-verification.test.ts test/root-write-lifetime.test.ts test/root-write-exact-identity.test.ts test/secret-write-publication.test.ts test/native-write-mode-ownership.test.ts test/native-created-cleanup.test.ts test/file-mode-facades.test.ts - name: Test Root sidecar admission with required native binding env: @@ -182,7 +182,7 @@ jobs: node scripts/sidecar-contention-proof.mjs require FS_SAFE_NATIVE_MODE=require pnpm test test/root-create-only-preflight.test.ts test/sidecar-lock-root-admission.test.ts test/sidecar-lock-root-ancestry.test.ts test/sidecar-lock-root-budget.test.ts test/sidecar-lock-root-resolver.test.ts test/sidecar-lock-root-unlink.test.ts test/sidecar-lock-unlink-siblings.test.ts test/file-lock-sync-stale.test.ts test/file-lock-sync-release.test.ts FS_SAFE_PAX_REQUIRE_NATIVE=1 pnpm test test/native-owned-tree.test.ts test/native-write-containment.test.ts test/native-staging-regression.test.ts test/staged-file.test.ts test/staged-file-failures.test.ts test/native-archive-equivalence.test.ts test/native-publish-equivalence.test.ts test/archive-pax.test.ts test/archive-pax-security.test.ts test/archive-pax-compressed.test.ts test/archive-tar-strip.test.ts test/archive-tar-framing.test.ts test/archive-tar-framing-compressed.test.ts test/archive-gzip-integrity.test.ts - FS_SAFE_NATIVE_MODE=require pnpm test test/root-write-mode.test.ts test/root-write-verification.test.ts test/root-write-lifetime.test.ts test/root-write-exact-identity.test.ts + FS_SAFE_NATIVE_MODE=require pnpm test test/root-write-mode.test.ts test/root-write-verification.test.ts test/root-write-lifetime.test.ts test/root-write-exact-identity.test.ts test/secret-write-publication.test.ts test/native-write-mode-ownership.test.ts test/native-created-cleanup.test.ts test/file-mode-facades.test.ts FS_SAFE_NATIVE_MODE=require pnpm test test/archive-zip-admission.test.ts test/archive-zip-metadata.test.ts test/archive-zip-integrity.test.ts FS_SAFE_PAX_REQUIRE_NATIVE=1 pnpm test test/archive-filter-paths.test.ts test/archive-filter-compressed.test.ts test/archive-tar-gnu.test.ts test/archive-tar-gnu-meter.test.ts test/archive-tar-ignored.test.ts test/archive-tar-ignored-meter.test.ts test/archive-tar-admission.test.ts test/archive-tar-manifest.test.ts ' diff --git a/CHANGELOG.md b/CHANGELOG.md index bf0e574f..da84920f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## 0.7.3 - Unreleased +- Finalize explicit file modes after content writes across pinned writers, verify all `0o7777` POSIX bits for secret publication, and use exact identities before native Windows mode changes and failed-write cleanup; JavaScript fallback cleanup preserves unverified replacements. +- Complete short synchronous regular-file appends and preserve explicitly requested special mode bits in both append helpers, retaining permission tightening before any data is written. - Preserve existing secret-directory permissions instead of repairing them; retain lossless directory identities through private locks and native writes, initialize new directories through guarded descriptor authority, honor full directory mode bits, and fail closed for unpinnable parents, including non-root macOS directories created under `umask(0o777)`. - Keep async Root-backed lock normalization read-only, rejecting deleted or replaced admitted parents without recreating them while preserving explicit in-root sidecars for external target keys. - Exercise secret-directory admission from isolated npm/pnpm consumer installs, retain real-identity and native-load proof, and honor explicit package-proof output paths. diff --git a/docs/regular-file.md b/docs/regular-file.md index b71cab2a..11d816cd 100644 --- a/docs/regular-file.md +++ b/docs/regular-file.md @@ -116,9 +116,17 @@ stalling admission. A confirmed non-regular target is refused before chmod or append; other open errors propagate unchanged. This safeguard does not change ordinary regular-file append semantics or require read permission. +The requested mode is applied through the admitted descriptor **before** any +content is appended, so an existing file is tightened first. On successful +completion, explicitly requested POSIX special bits are reapplied after the +content write, which can otherwise clear set-ID bits. An initial chmod failure +leaves the content untouched; a write or final chmod failure can leave appended +content and is not rolled back. Windows does not enforce POSIX mode semantics. + ### `appendRegularFileSync(options)` -Synchronous. Same options. +Synchronous. Same options and mode ordering. Writes the complete input through +the already-open descriptor, including when an individual write is short. ### `resolveRegularFileAppendFlags()` diff --git a/docs/secret-file.md b/docs/secret-file.md index 63919ab5..884a600a 100644 --- a/docs/secret-file.md +++ b/docs/secret-file.md @@ -123,7 +123,7 @@ startWebhookVerifier(signingKey); ### `writeSecretFileAtomic(params)` -Async. Creates the parent directory at `dirMode` (default `0o700`) if missing, writes content to a sibling temp file at `mode` (default `0o600`), atomically renames over the destination, and re-asserts the file mode after rename. +Async. Creates the parent directory at `dirMode` (default `0o700`) if missing, writes content to a sibling temp file, finalizes `mode` (default `0o600`) through an owned descriptor after content writes, and atomically renames over the destination. Publication verification checks the final file identity and mode. Concurrent writes to distinct leaves may share creation of a missing parent. After a parent-creation race, the helper re-inspects the entry and requires a @@ -132,10 +132,21 @@ the requested directory mode before writing either leaf. Publication verification borrows the writer's still-open descriptor to check the exact file identity, regular-file and link policy, requested POSIX mode, -and root/parent ancestry before the writer closes it. POSIX mode overrides such -as `0o000` and `0o200` do not require read permission or a readonly reopen, and -verification does not widen the final mode. Windows retains its existing -pathname-identity verification policy without enforcing POSIX mode bits. +and root/parent ancestry before the writer closes it. All `0o7777` mode bits +must match, including explicitly requested special bits; unexpected special +bits are rejected. POSIX mode overrides such as `0o000` and `0o200` do not +require read permission or a readonly reopen, and verification does not widen +the final mode. Windows retains pathname-identity verification without enforcing +POSIX mode bits; its native writer checks the reopened descriptor against the +original lossless file identity before changing the final mode. + +Failed JavaScript fallback writes attempt cleanup while retaining the original +descriptor and only after checking parent and file identities. Native cleanup +also compares lossless parent and file identities. Unverifiable paths are left +for caller-managed cleanup, and cleanup failures do not replace the original +write error. These are best-effort identity checks followed by name-based +removal, not atomic conditional unlink. A publication-verification failure +after a completed write does not authorize deleting the published file. ```ts import { writeSecretFileAtomic } from "@openclaw/fs-safe/secret"; diff --git a/src/native-operations.ts b/src/native-operations.ts index 1c38fcb4..1613671d 100644 --- a/src/native-operations.ts +++ b/src/native-operations.ts @@ -1,4 +1,4 @@ -import fsSync, { type Stats } from "node:fs"; +import fsSync, { type BigIntStats, type Stats } from "node:fs"; import fs from "node:fs/promises"; import path from "node:path"; import type { ContainmentGuarantee } from "./containment.js"; @@ -78,25 +78,23 @@ function wrapNativeFd(fd: number, containment: ContainmentGuarantee): NativeFile } export function removeNativeCreatedFileIfStillPinned(params: { - binding: NativeBinding; parentPath: string; parentFd: number; basename: string; - created?: Stats; + created?: BigIntStats; }): void { if (!params.created) { return; } try { - const parentPathStat = fsSync.lstatSync(params.parentPath); - const parentFdStat = params.binding.fstatIdentity(params.parentFd); + const parentPathStat = fsSync.lstatSync(params.parentPath, { bigint: true }); + const parentFdStat = fsSync.fstatSync(params.parentFd, { bigint: true }); const targetPath = path.join(params.parentPath, params.basename); - const target = fsSync.lstatSync(targetPath); + const target = fsSync.lstatSync(targetPath, { bigint: true }); if ( - !parentPathStat.isSymbolicLink() && - parentPathStat.dev === parentFdStat.dev && - parentPathStat.ino === parentFdStat.ino && - !target.isSymbolicLink() && + !parentPathStat.isSymbolicLink() && parentPathStat.isDirectory() && + sameFileIdentityForCleanup(parentPathStat, parentFdStat) && + !target.isSymbolicLink() && target.isFile() && sameFileIdentityForCleanup(target, params.created) ) { fsSync.rmSync(targetPath); @@ -123,7 +121,7 @@ export async function createNativeExclusiveFile( (typeof fsSync.constants.O_DIRECTORY === "number" ? fsSync.constants.O_DIRECTORY : 0), ); let fd: number | undefined; - let created: Stats | undefined; + let created: BigIntStats | undefined; try { let opened: ReturnType; try { @@ -146,7 +144,7 @@ export async function createNativeExclusiveFile( } fd = opened.fd; fsSync.fchmodSync(fd, mode); - created = fsSync.fstatSync(fd); + created = fsSync.fstatSync(fd, { bigint: true }); return wrapNativeFd(fd, opened.containment); } catch (error) { if (fd !== undefined) { @@ -156,7 +154,6 @@ export async function createNativeExclusiveFile( // Preserve the original error. } removeNativeCreatedFileIfStillPinned({ - binding, parentPath, parentFd: parent.fd, basename, diff --git a/src/native-pinned-write-windows.ts b/src/native-pinned-write-windows.ts index bb4ffab8..4ff9f59f 100644 --- a/src/native-pinned-write-windows.ts +++ b/src/native-pinned-write-windows.ts @@ -1,5 +1,5 @@ import { randomUUID } from "node:crypto"; -import fsSync, { type Stats } from "node:fs"; +import fsSync, { type BigIntStats } from "node:fs"; import type { FileHandle } from "node:fs/promises"; import type { AnyAsyncDirectoryGuard } from "./directory-guard.js"; import { FsSafeError } from "./errors.js"; @@ -12,6 +12,7 @@ import { } from "./native-operations.js"; import type { NativeBinding } from "./native.js"; import type { PinnedWriteParams } from "./pinned-write.js"; +import { inspectFileIdentitySync } from "./strict-file-identity.js"; export function sameNativeIdentity( left: Pick, @@ -39,7 +40,7 @@ export async function runPinnedWriteWindows( const parentPath = parentGuard.realPath; let tempFd: number | undefined; let targetFd: number | undefined; - let tempIdentity: Stats | undefined; + let tempIdentity: BigIntStats | undefined; let tempName = ""; let renamed = false; let completed = false; @@ -52,8 +53,8 @@ export async function runPinnedWriteWindows( fsSync.constants.O_WRONLY | fsSync.constants.O_CREAT | fsSync.constants.O_EXCL, ), ).fd; - tempIdentity = fsSync.fstatSync(tempFd); - const verificationIdentity = fsSync.fstatSync(tempFd, { bigint: true }); + const verificationIdentity = inspectFileIdentitySync(() => fsSync.fstatSync(tempFd!, { bigint: true })); + tempIdentity = verificationIdentity; // Creation is requested at 0600 in the binding, but a restrictive umask // can remove owner access. Keep the unpublished inode private and // reopenable until the published name has been identity-fenced. @@ -71,8 +72,11 @@ export async function runPinnedWriteWindows( params.basename, nativeOpenFlags(fsSync.constants.O_RDONLY), ).fd; + const targetStat = inspectFileIdentitySync( + () => fsSync.fstatSync(targetFd!, { bigint: true }), verificationIdentity, + ); const targetIdentity = binding.fstatIdentity(targetFd); - if (!targetIdentity.isFile || !sameNativeIdentity(tempIdentity, targetIdentity)) { + if (!targetStat.isFile()) { throw new FsSafeError("path-mismatch", "native write target changed after rename"); } // Native exclusive creation starts at 0600. Apply the requested mode only @@ -85,7 +89,6 @@ export async function runPinnedWriteWindows( closeWriteFd(targetFd); targetFd = undefined; removeNativeCreatedFileIfStillPinned({ - binding, parentPath, parentFd, basename: params.basename, @@ -103,7 +106,6 @@ export async function runPinnedWriteWindows( const tempCloseError = closeWriteFd(tempFd); if (!renamed) { removeNativeCreatedFileIfStillPinned({ - binding, parentPath, parentFd, basename: tempName, diff --git a/src/pinned-write.ts b/src/pinned-write.ts index 566e4237..321654c8 100644 --- a/src/pinned-write.ts +++ b/src/pinned-write.ts @@ -1,5 +1,5 @@ import { randomUUID } from "node:crypto"; -import fsSync from "node:fs"; +import fsSync, { type BigIntStats } from "node:fs"; import type { FileHandle } from "node:fs/promises"; import fs from "node:fs/promises"; import path from "node:path"; @@ -16,6 +16,7 @@ import { runPinnedWriteNative } from "./native-pinned-write.js"; import { getNativeBinding } from "./native.js"; import { validatePinnedOperationPayload } from "./pinned-operation.js"; import { resolveReadOpenFlags } from "./read-open-flags.js"; +import { cleanupPinnedFilePath } from "./replace-file-temp-owner.js"; import { withSidecarLock } from "./sidecar-lock.js"; import { getFsSafeTestHooks } from "./test-hooks.js"; @@ -204,9 +205,10 @@ async function runPinnedWriteFallback(params: PinnedWriteParams): Promise undefined); - if (created) { - await fs.rm(targetPath, { force: true }).catch(() => undefined); + try { + if (created) { + await cleanupPinnedFilePath({ pathname: targetPath, handle, identity: createdIdentity, parentGuard }); + } + } finally { + await handle.close().catch(() => undefined); } } } @@ -246,12 +253,13 @@ async function runPinnedWriteFallback(params: PinnedWriteParams): Promise> | undefined; let tempStat: Awaited["stat"]>> | undefined; + let tempIdentity: BigIntStats | undefined; let readHandle: FileHandle | undefined; let renamed = false; try { handle = await fs.open(tempPath, tempFlags, params.mode); let verificationIdentity = await handle.stat({ bigint: true }); - await handle.chmod(params.mode); + tempIdentity = verificationIdentity; if (params.input.kind === "buffer") { assertWithinMaxBytes( byteLength(params.input.data, params.input.encoding), @@ -271,6 +279,7 @@ async function runPinnedWriteFallback(params: PinnedWriteParams): Promise { @@ -309,10 +318,13 @@ async function runPinnedWriteFallback(params: PinnedWriteParams): Promise undefined); - await handle?.close().catch(() => undefined); - if (!renamed) { - await fs.rm(tempPath, { force: true }).catch(() => undefined); + try { + if (!renamed && handle) { + await cleanupPinnedFilePath({ pathname: tempPath, handle, identity: tempIdentity, parentGuard }); + } + } finally { + await readHandle?.close().catch(() => undefined); + await handle?.close().catch(() => undefined); } } } diff --git a/src/regular-file.ts b/src/regular-file.ts index dd77f760..fbcba73c 100644 --- a/src/regular-file.ts +++ b/src/regular-file.ts @@ -344,8 +344,11 @@ export async function appendRegularFile(options: AppendRegularFileOptions): Prom ) { return; } - await handle.chmod(options.mode ?? 0o600); + const mode = options.mode ?? 0o600; + // Tighten before writing; restore explicit special bits only after content is complete. + await handle.chmod(mode); await handle.appendFile(options.content, options.encoding ?? "utf8"); + if (mode & 0o7000) await handle.chmod(mode); } finally { await handle.close(); } @@ -424,8 +427,10 @@ export function appendRegularFileSync(options: AppendRegularFileOptions): void { ) { return; } - fsSync.fchmodSync(fd, options.mode ?? 0o600); - fsSync.writeSync(fd, contentBuffer, 0, contentBuffer.byteLength); + const mode = options.mode ?? 0o600; + fsSync.fchmodSync(fd, mode); + fsSync.appendFileSync(fd, contentBuffer); + if (mode & 0o7000) fsSync.fchmodSync(fd, mode); } finally { fsSync.closeSync(fd); } diff --git a/src/replace-file-temp-owner.ts b/src/replace-file-temp-owner.ts index db5dd636..5a70b7c6 100644 --- a/src/replace-file-temp-owner.ts +++ b/src/replace-file-temp-owner.ts @@ -1,5 +1,6 @@ import syncFs, { type BigIntStats } from "node:fs"; import fs, { type FileHandle } from "node:fs/promises"; +import { assertAsyncDirectoryGuard, type AnyAsyncDirectoryGuard } from "./directory-guard.js"; import { FsSafeError } from "./errors.js"; import { sameFileIdentityForCleanup, sha256Hex } from "./file-identity.js"; import { inspectFileIdentity, inspectFileIdentitySync } from "./strict-file-identity.js"; @@ -77,6 +78,37 @@ async function cleanupOwnedPath(params: { } } +// Borrowed handle: the caller retains it until this best-effort cleanup finishes. +export async function cleanupPinnedFilePath(params: { + pathname: string; + handle: FileHandle; + identity?: BigIntStats; + parentGuard: AnyAsyncDirectoryGuard; +}): Promise { + if (!params.identity) return; + try { + const guard = params.parentGuard; + if ([guard.stat.dev, guard.stat.ino].some( + (value) => typeof value === "number" && !Number.isSafeInteger(value), + )) return; + await assertAsyncDirectoryGuard(guard); + const parent = await fs.lstat(guard.dir, { bigint: true }); + if (parent.isSymbolicLink() || !parent.isDirectory() || + !sameFileIdentityForCleanup(parent, guard.stat)) return; + const opened = await params.handle.stat({ bigint: true }); + if (!opened.isFile() || opened.nlink !== 1n || + !sameFileIdentityForCleanup(opened, params.identity)) return; + await cleanupOwnedPath({ + fsModule: fs, + pathname: params.pathname, + identity: params.identity, + throwOnCleanupError: false, + }); + } catch { + // Unverifiable authority must preserve the path and the original write failure. + } +} + function cleanupOwnedPathSync(params: { fsModule: SyncOwnerFileSystem; pathname: string; diff --git a/src/root-write-verification.ts b/src/root-write-verification.ts index 4cd9d654..d854b9f4 100644 --- a/src/root-write-verification.ts +++ b/src/root-write-verification.ts @@ -45,7 +45,7 @@ export async function verifyAtomicWriteResult(params: { } assertFile(stat); if (process.platform !== "win32" && params.expectedMode !== undefined) { - const actualMode = Number(stat.mode & 0o777n); + const actualMode = Number(stat.mode & 0o7777n); if (actualMode !== params.expectedMode) { throw new Error( `Private secret file ${params.targetPath} has insecure permissions ${actualMode.toString(8)}.`, diff --git a/test/fallback-write-mode-cleanup.test.ts b/test/fallback-write-mode-cleanup.test.ts new file mode 100644 index 00000000..6718525e --- /dev/null +++ b/test/fallback-write-mode-cleanup.test.ts @@ -0,0 +1,45 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { configureFsSafeNative } from "../src/config.js"; +import { createSecretFileAtomic, writeSecretFileAtomic } from "../src/secret.js"; +import { useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +afterEach(() => { + vi.restoreAllMocks(); + configureFsSafeNative({ mode: "auto" }); +}); + +describe("fallback mode-failure cleanup ownership", () => { + it.each([ + { operation: "write", write: writeSecretFileAtomic }, + { operation: "create", write: createSecretFileAtomic }, + ])("$operation preserves a replacement after final chmod fails", async ({ write, operation }) => { + configureFsSafeNative({ mode: "off" }); + const rootDir = await tempRoot("fs-safe-fallback-mode-cleanup-"); + const filePath = path.join(rootDir, "target"); + const saved = path.join(rootDir, "original"); + const failure = Object.assign(new Error("synthetic mode failure"), { code: "EIO" }); + const open = fs.open.bind(fs); + let replacedPath: string | undefined; + vi.spyOn(fs, "open").mockImplementation(async (...args) => { + const handle = await open(...args); + if (!(await handle.stat()).isFile()) return handle; + vi.spyOn(handle, "chmod").mockImplementationOnce(async () => { + replacedPath = String(args[0]); + await fs.rename(replacedPath, saved); + await fs.writeFile(replacedPath, "replacement", { mode: 0o600 }); + throw failure; + }); + return handle; + }); + + await expect(write({ rootDir, filePath, content: "owned bytes", mode: 0o600 })).rejects.toBe(failure); + + expect(replacedPath).toBeDefined(); + expect(await fs.readFile(replacedPath!, "utf8")).toBe("replacement"); + expect(await fs.readFile(saved, "utf8")).toBe("owned bytes"); + if (operation === "write") await expect(fs.lstat(filePath)).rejects.toMatchObject({ code: "ENOENT" }); + }); +}); diff --git a/test/file-mode-facades.test.ts b/test/file-mode-facades.test.ts new file mode 100644 index 00000000..461b41a8 --- /dev/null +++ b/test/file-mode-facades.test.ts @@ -0,0 +1,77 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { Readable } from "node:stream"; +import { afterEach, describe, expect, it } from "vitest"; +import { configureFsSafeNative } from "../src/config.js"; +import { __loadBundledNativeForTest, __resetNativeLoaderForTest } from "../src/native.js"; +import { root } from "../src/root.js"; +import { fileStore, fileStoreSync } from "../src/store.js"; +import { useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +let nativeAvailable = false; +try { + __loadBundledNativeForTest(); + nativeAvailable = true; +} catch (error) { + if (process.env.FS_SAFE_NATIVE_MODE === "require") throw error; +} +afterEach(() => { + configureFsSafeNative({ mode: "auto" }); + __resetNativeLoaderForTest(); +}); + +async function fixture() { + const directory = await tempRoot("fs-safe-mode-facade-"); + const rootDir = path.join(directory, "root"); + const source = path.join(directory, "source"); + await fs.mkdir(rootDir, { mode: 0o700 }); + await fs.writeFile(source, "synthetic facade", { mode: 0o600 }); + return { rootDir, source, target: path.join(rootDir, "target") }; +} + +for (const backend of ["off", "require"] as const) { + describe.skipIf(process.platform === "win32" || (backend === "require" && !nativeAvailable))( + `explicit file modes through ${backend} facades`, + () => { + it.each(["write", "create", "copyIn"].flatMap((operation) => [0o600, 0o4600].map((mode) => ({ operation, mode }))))( + "Root.$operation preserves $mode", + async ({ operation, mode }) => { + configureFsSafeNative({ mode: backend }); + const { rootDir, source, target } = await fixture(); + const capability = await root(rootDir); + if (operation === "copyIn") await capability.copyIn("target", source, { mode }); + else if (operation === "create") await capability.create("target", "synthetic facade", { mode }); + else await capability.write("target", "synthetic facade", { mode }); + expect((await fs.stat(target)).mode & 0o7777).toBe(mode); + expect(await fs.readFile(target, "utf8")).toBe("synthetic facade"); + }, + ); + + it.each([false, true].flatMap((privateMode) => ["write", "copyIn", "writeStream"].flatMap((operation) => + [0o600, 0o4600].map((mode) => ({ privateMode, operation, mode })), + )))("FileStore.$operation preserves $mode (private=$privateMode)", async ({ privateMode, operation, mode }) => { + configureFsSafeNative({ mode: backend }); + const { rootDir, source, target } = await fixture(); + const store = fileStore({ rootDir, private: privateMode, mode }); + if (operation === "copyIn") await store.copyIn("target", source); + else if (operation === "writeStream") await store.writeStream("target", Readable.from(["synthetic ", "facade"])); + else await store.write("target", "synthetic facade"); + expect((await fs.stat(target)).mode & 0o7777).toBe(mode); + expect(await fs.readFile(target, "utf8")).toBe("synthetic facade"); + }); + }, + ); +} + +describe.skipIf(process.platform === "win32")("existing synchronous mode finalization", () => { + it.each([false, true].flatMap((privateMode) => [0o600, 0o4600].map((mode) => ({ privateMode, mode }))))( + "FileStoreSync preserves $mode (private=$privateMode)", + async ({ privateMode, mode }) => { + const { rootDir, target } = await fixture(); + fileStoreSync({ rootDir, private: privateMode, mode }).write("target", "synthetic facade"); + expect((await fs.stat(target)).mode & 0o7777).toBe(mode); + expect(await fs.readFile(target, "utf8")).toBe("synthetic facade"); + }, + ); +}); diff --git a/test/native-created-cleanup.test.ts b/test/native-created-cleanup.test.ts new file mode 100644 index 00000000..095c8d2e --- /dev/null +++ b/test/native-created-cleanup.test.ts @@ -0,0 +1,49 @@ +import fsSync from "node:fs"; +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { removeNativeCreatedFileIfStillPinned } from "../src/native-operations.js"; +import { useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +const platform = Object.getOwnPropertyDescriptor(process, "platform")!; +afterEach(() => { + vi.restoreAllMocks(); + Object.defineProperty(process, "platform", platform); +}); + +describe("native created-file cleanup authority", () => { + it.each(["matching", "parent-path", "parent-fd", "target", "created"])( + "handles Windows identity observation: %s", + async (scenario) => { + const parentPath = await tempRoot("fs-safe-native-cleanup-identity-"); + const targetPath = path.join(parentPath, "target"); + await fs.writeFile(targetPath, "owned", { mode: 0o600 }); + const created = await fs.lstat(targetPath, { bigint: true }); + const parent = await fs.open(parentPath, fsSync.constants.O_RDONLY | (fsSync.constants.O_DIRECTORY ?? 0)); + Object.defineProperty(process, "platform", { value: "win32" }); + const lstat = fsSync.lstatSync.bind(fsSync); + const fstat = fsSync.fstatSync.bind(fsSync); + vi.spyOn(fsSync, "lstatSync").mockImplementation(((...args: Parameters) => { + const stat = lstat(...args); + const selected = scenario === "parent-path" ? parentPath : scenario === "target" ? targetPath : undefined; + return String(args[0]) === selected ? Object.assign(Object.create(stat), { ino: 0n }) : stat; + }) as typeof fsSync.lstatSync); + vi.spyOn(fsSync, "fstatSync").mockImplementation(((...args: Parameters) => { + const stat = fstat(...args); + return scenario === "parent-fd" && args[0] === parent.fd + ? Object.assign(Object.create(stat), { ino: 0n }) : stat; + }) as typeof fsSync.fstatSync); + try { + removeNativeCreatedFileIfStillPinned({ + parentPath, parentFd: parent.fd, basename: "target", + created: scenario === "created" ? Object.assign(Object.create(created), { ino: 0n }) : created, + }); + expect(fsSync.existsSync(targetPath)).toBe(scenario !== "matching"); + if (scenario !== "matching") expect(await fs.readFile(targetPath, "utf8")).toBe("owned"); + } finally { + await parent.close(); + } + }, + ); +}); diff --git a/test/native-write-mode-ownership.test.ts b/test/native-write-mode-ownership.test.ts new file mode 100644 index 00000000..9e30ec29 --- /dev/null +++ b/test/native-write-mode-ownership.test.ts @@ -0,0 +1,122 @@ +import fsSync, { type BigIntStats, type Stats } from "node:fs"; +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { configureFsSafeNative } from "../src/config.js"; +import { __loadBundledNativeForTest, __resetNativeLoaderForTest, __setNativeLoaderForTest } from "../src/native.js"; +import { createSecretFileAtomic, writeSecretFileAtomic } from "../src/secret.js"; +import { useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +const platform = Object.getOwnPropertyDescriptor(process, "platform")!; +let nativeAvailable = false; +try { + __loadBundledNativeForTest(); + nativeAvailable = true; +} catch (error) { + if (process.env.FS_SAFE_NATIVE_MODE === "require") throw error; +} +afterEach(() => { + vi.restoreAllMocks(); + Object.defineProperty(process, "platform", platform); + configureFsSafeNative({ mode: "auto" }); + __resetNativeLoaderForTest(); +}); + +const writers = [ + { operation: "write", write: writeSecretFileAtomic }, + { operation: "create", write: createSecretFileAtomic }, +]; + +describe.skipIf(!nativeAvailable)("native Windows final-mode ownership", () => { + it.each(writers.flatMap((writer) => ["stable", "before-open", "mode-error"].map((scenario) => ({ ...writer, scenario }))))( + "$operation retains final-mode ownership through $scenario", + async ({ write, scenario }) => { + const rootDir = await tempRoot("fs-safe-native-mode-owner-"); + const target = path.join(rootDir, "token"); + const saved = path.join(rootDir, "published"); + const binding = __loadBundledNativeForTest(); + const fstat = fsSync.fstatSync.bind(fsSync); + const identities = new Map(); + const base = 1n << 53n; + expect(Number(base)).toBe(Number(base + 1n)); + const key = (stat: BigIntStats) => `${stat.dev}:${stat.ino}`; + const projectedInode = (stat: BigIntStats) => { + const id = key(stat); + if (!identities.has(id)) identities.set(id, base + BigInt(identities.size)); + return identities.get(id)!; + }; + // Only inode values are projected; descriptors, bytes, replacement and chmod are real. + const project = (stat: T, exact: BigIntStats): T => { + if (!stat.isFile()) return stat; + const ino = projectedInode(exact); + return Object.assign(Object.create(stat), { ino: typeof stat.ino === "bigint" ? ino : Number(ino) }); + }; + vi.spyOn(fsSync, "fstatSync").mockImplementation(((...args: Parameters) => + project(fstat(...args), fstat(args[0], { bigint: true }))) as typeof fsSync.fstatSync); + for (const method of ["lstat", "stat"] as const) { + const original = fs[method].bind(fs); + vi.spyOn(fs, method).mockImplementation((async (...args: Parameters) => { + const stat = await original(...args); + if (!stat.isFile()) return stat; + return project(stat, await original(args[0], { bigint: true })); + }) as typeof fs.stat); + } + const lstatSync = fsSync.lstatSync.bind(fsSync); + vi.spyOn(fsSync, "lstatSync").mockImplementation(((...args: Parameters) => + project(lstatSync(...args), lstatSync(args[0], { bigint: true }))) as typeof fsSync.lstatSync); + let published = false; + let replaced = false; + let replacementIdentity: string | undefined; + const swap = () => { + fsSync.renameSync(target, saved); + fsSync.writeFileSync(target, "replacement", { mode: 0o600 }); + fsSync.chmodSync(target, 0o600); + replacementIdentity = key(fsSync.statSync(target, { bigint: true })); + replaced = true; + }; + const modeFailure = Object.assign(new Error("synthetic final chmod failure"), { code: "EIO" }); + const chmod = fsSync.fchmodSync.bind(fsSync); + const changes: Array<{ identity: string; mode: number }> = []; + vi.spyOn(fsSync, "fchmodSync").mockImplementation((fd, mode) => { + if (scenario === "mode-error" && Number(mode) === 0o400) { + swap(); + throw modeFailure; + } + chmod(fd, mode); + changes.push({ identity: key(fstat(fd, { bigint: true })), mode: Number(mode) }); + }); + __setNativeLoaderForTest(() => ({ + ...binding, + renameReplace(...args) { binding.renameReplace(...args); published = true; }, + renameNoReplace(...args) { binding.renameNoReplace(...args); published = true; }, + openBeneath(...args) { + if (scenario === "before-open" && published && args[1] === "token" && !replaced) swap(); + return binding.openBeneath(...args); + }, + fstatIdentity(fd) { + const stat = binding.fstatIdentity(fd); + return stat.isFile ? { ...stat, ino: Number(projectedInode(fstat(fd, { bigint: true }))) } : stat; + }, + })); + // POSIX hosts exercise the Windows branch; Windows CI uses its actual native mechanism. + Object.defineProperty(process, "platform", { value: "win32" }); + configureFsSafeNative({ mode: "require" }); + const pending = write({ rootDir, filePath: target, content: "original", mode: 0o400 }); + if (scenario !== "stable") { + if (scenario === "mode-error") await expect(pending).rejects.toBe(modeFailure); + else await expect(pending).rejects.toMatchObject({ code: "path-mismatch" }); + expect(replaced).toBe(true); + expect(changes.filter((change) => change.identity === replacementIdentity)).toEqual([]); + expect(fsSync.existsSync(target)).toBe(true); + expect(fsSync.statSync(target).mode & 0o200).toBe(0o200); + expect(fsSync.readFileSync(target, "utf8")).toBe("replacement"); + expect(fsSync.readFileSync(saved, "utf8")).toBe("original"); + } else { + await expect(pending).resolves.toBeUndefined(); + expect(fsSync.statSync(target).mode & 0o200).toBe(0); + expect(fsSync.readFileSync(target, "utf8")).toBe("original"); + } + }, + ); +}); diff --git a/test/pinned-write-cleanup-authority.test.ts b/test/pinned-write-cleanup-authority.test.ts new file mode 100644 index 00000000..fdd33329 --- /dev/null +++ b/test/pinned-write-cleanup-authority.test.ts @@ -0,0 +1,48 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import { createAsyncDirectoryGuard } from "../src/directory-guard.js"; +import { cleanupPinnedFilePath } from "../src/replace-file-temp-owner.js"; +import { useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); + +describe("pinned failure cleanup authority", () => { + it.each(["owned", "replacement", "stale-parent", "missing-identity"])( + "borrows the retained descriptor and handles %s", + async (scenario) => { + const directory = await tempRoot("fs-safe-pinned-cleanup-authority-"); + const parent = path.join(directory, "parent"); + await fs.mkdir(parent, { mode: 0o700 }); + const parentGuard = await createAsyncDirectoryGuard(parent, { bigint: true }); + if (scenario === "stale-parent") { + await fs.rename(parent, path.join(directory, "old-parent")); + await fs.mkdir(parent, { mode: 0o700 }); + } + const pathname = path.join(parent, "target"); + const handle = await fs.open(pathname, "wx", 0o600); + try { + await handle.writeFile("owned"); + const identity = await handle.stat({ bigint: true }); + if (scenario === "replacement") { + await fs.rename(pathname, path.join(parent, "saved")); + await fs.writeFile(pathname, "replacement", { mode: 0o600 }); + } + const listeners = process.listenerCount("exit"); + await cleanupPinnedFilePath({ + pathname, handle, parentGuard, + identity: scenario === "missing-identity" ? undefined : identity, + }); + expect(process.listenerCount("exit")).toBe(listeners); + expect((await handle.stat({ bigint: true })).ino).toBe(identity.ino); + if (scenario === "owned") { + await expect(fs.lstat(pathname)).rejects.toMatchObject({ code: "ENOENT" }); + } else { + expect(await fs.readFile(pathname, "utf8")).toBe(scenario === "replacement" ? "replacement" : "owned"); + } + } finally { + await handle.close(); + } + }, + ); +}); diff --git a/test/pinned-write-file-mode.test.ts b/test/pinned-write-file-mode.test.ts new file mode 100644 index 00000000..c5c2574c --- /dev/null +++ b/test/pinned-write-file-mode.test.ts @@ -0,0 +1,66 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { Readable } from "node:stream"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { configureFsSafeNative } from "../src/config.js"; +import { runPinnedWriteHelper } from "../src/pinned-write.js"; +import { useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +afterEach(() => { + vi.restoreAllMocks(); + configureFsSafeNative({ mode: "auto" }); +}); + +describe.skipIf(process.platform === "win32")("fallback final file mode", () => { + it.each([false, true].flatMap((overwrite) => ["buffer", "stream"].map((kind) => ({ overwrite, kind }))))( + "applies mode after $kind content and before syncing (overwrite=$overwrite)", + async ({ overwrite, kind }) => { + configureFsSafeNative({ mode: "off" }); + const rootPath = await tempRoot("fs-safe-pinned-file-mode-"); + await fs.chown(rootPath, process.geteuid!(), process.getegid!()); + const target = path.join(rootPath, "target"); + const events: string[] = []; + const open = fs.open.bind(fs); + vi.spyOn(fs, "open").mockImplementation(async (...args) => { + const handle = await open(...args); + if (!(await handle.stat()).isFile()) return handle; + const chmod = handle.chmod.bind(handle); + const writeFile = handle.writeFile.bind(handle); + const write = handle.write.bind(handle); + const sync = handle.sync.bind(handle); + vi.spyOn(handle, "chmod").mockImplementation(async (mode) => { + events.push("chmod"); + await chmod(mode); + }); + vi.spyOn(handle, "writeFile").mockImplementation(async (...args) => { + events.push("write"); + await writeFile(...args); + }); + vi.spyOn(handle, "write").mockImplementation((async (...args: Parameters) => { + events.push("write"); + return await write(...args); + }) as typeof handle.write); + vi.spyOn(handle, "sync").mockImplementation(async () => { + events.push("sync"); + await sync(); + }); + return handle; + }); + + await runPinnedWriteHelper({ + rootPath, relativeParentPath: "", basename: "target", mkdir: false, overwrite, + mode: 0o4600, + input: kind === "buffer" ? { kind: "buffer", data: "synthetic mode" } + : { kind: "stream", stream: Readable.from(["synthetic ", "mode"]) }, + }); + + expect((await fs.stat(target)).mode & 0o7777).toBe(0o4600); + expect(events.filter((event) => event === "chmod")).toHaveLength(1); + expect(events.lastIndexOf("write")).toBeGreaterThanOrEqual(0); + expect(events.indexOf("chmod")).toBeGreaterThan(events.lastIndexOf("write")); + expect(events.indexOf("sync")).toBeGreaterThan(events.indexOf("chmod")); + expect(await fs.readFile(target, "utf8")).toBe("synthetic mode"); + }, + ); +}); diff --git a/test/regular-file-mode.test.ts b/test/regular-file-mode.test.ts new file mode 100644 index 00000000..7c5f7dcb --- /dev/null +++ b/test/regular-file-mode.test.ts @@ -0,0 +1,122 @@ +import fsSync from "node:fs"; +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { appendRegularFile, appendRegularFileSync } from "../src/regular-file.js"; +import { useRealTempDirs } from "./helpers/vitest.js"; + +const { tempRoot } = useRealTempDirs(); +afterEach(() => vi.restoreAllMocks()); +const append = async (kind: string, options: Parameters[0]) => { + if (kind === "async") await appendRegularFile(options); + else appendRegularFileSync(options); +}; + +describe.skipIf(process.platform === "win32")("regular append final file mode", () => { + it.each(["async", "sync"].flatMap((kind) => ["", "added"].flatMap((content) => + [0o600, 0o4600].map((mode) => ({ kind, content, mode })), + )))("$kind append '$content' preserves explicit mode $mode", async ({ kind, content, mode }) => { + const directory = await tempRoot("fs-safe-append-mode-"); + const filePath = path.join(directory, "target"); + await fs.writeFile(filePath, "initial", { mode: 0o600 }); + await fs.chown(filePath, process.geteuid!(), process.getegid!()); + await append(kind, { filePath, content, mode }); + expect((await fs.stat(filePath)).mode & 0o7777).toBe(mode); + expect(await fs.readFile(filePath, "utf8")).toBe(`initial${content}`); + }); + + it.each(["async", "sync"])("%s tightens existing permissions before appending", async (kind) => { + const directory = await tempRoot("fs-safe-append-private-first-"); + const filePath = path.join(directory, "target"); + await fs.writeFile(filePath, "initial", { mode: 0o644 }); + await fs.chmod(filePath, 0o644); + let observed = false; + if (kind === "async") { + const open = fs.open.bind(fs); + vi.spyOn(fs, "open").mockImplementation(async (...args) => { + const handle = await open(...args); + const appendFile = handle.appendFile.bind(handle); + vi.spyOn(handle, "appendFile").mockImplementation(async (...args) => { + expect((await handle.stat()).mode & 0o7777).toBe(0o600); + observed = true; + await appendFile(...args); + }); + return handle; + }); + } else { + const write = fsSync.writeSync.bind(fsSync); + vi.spyOn(fsSync, "writeSync").mockImplementation(((...args: Parameters) => { + expect(fsSync.fstatSync(args[0]).mode & 0o7777).toBe(0o600); + observed = true; + return write(...args); + }) as typeof fsSync.writeSync); + } + await append(kind, { filePath, content: "private", mode: 0o600 }); + expect(observed).toBe(true); + expect(await fs.readFile(filePath, "utf8")).toBe("initialprivate"); + }); + + it.each(["async", "sync"])("%s does not append when initial mode tightening fails", async (kind) => { + const directory = await tempRoot("fs-safe-append-mode-refusal-"); + const filePath = path.join(directory, "target"); + await fs.writeFile(filePath, "initial", { mode: 0o644 }); + const failure = Object.assign(new Error("mode denied"), { code: "EIO" }); + if (kind === "async") { + const open = fs.open.bind(fs); + vi.spyOn(fs, "open").mockImplementation(async (...args) => { + const handle = await open(...args); + vi.spyOn(handle, "chmod").mockRejectedValue(failure); + return handle; + }); + } else { + vi.spyOn(fsSync, "fchmodSync").mockImplementation(() => { throw failure; }); + } + await expect(append(kind, { filePath, content: "private", mode: 0o600 })).rejects.toBe(failure); + expect(await fs.readFile(filePath, "utf8")).toBe("initial"); + }); + it.each(["async", "sync"])("%s closes after final chmod failure without rolling back bytes", async (kind) => { + const directory = await tempRoot("fs-safe-append-final-mode-refusal-"); + const filePath = path.join(directory, "target"); + await fs.writeFile(filePath, "initial", { mode: 0o600 }); + const failure = Object.assign(new Error("final mode denied"), { code: "EIO" }); + let openedFd = -1; + let chmodCalls = 0; + if (kind === "async") { + const open = fs.open.bind(fs); + vi.spyOn(fs, "open").mockImplementation(async (...args) => { + const handle = await open(...args); + openedFd = handle.fd; + const chmod = handle.chmod.bind(handle); + vi.spyOn(handle, "chmod").mockImplementation(async (mode) => { + if (++chmodCalls === 2) throw failure; + await chmod(mode); + }); + return handle; + }); + } else { + const chmod = fsSync.fchmodSync.bind(fsSync); + vi.spyOn(fsSync, "fchmodSync").mockImplementation((fd, mode) => { + openedFd = fd; + if (++chmodCalls === 2) throw failure; + chmod(fd, mode); + }); + } + await expect(append(kind, { filePath, content: "added", mode: 0o4600 })).rejects.toBe(failure); + expect(chmodCalls).toBe(2); + expect(() => fsSync.fstatSync(openedFd)).toThrow(expect.objectContaining({ code: "EBADF" })); + expect(await fs.readFile(filePath, "utf8")).toBe("initialadded"); + }); +}); + +it("completes short synchronous writes before finalizing an append", async () => { + const directory = await tempRoot("fs-safe-append-short-write-"); + const filePath = path.join(directory, "target"); + await fs.writeFile(filePath, "initial", { mode: 0o600 }); + const write = fsSync.writeSync.bind(fsSync); + const calls = vi.spyOn(fsSync, "writeSync").mockImplementation((( + fd: number, buffer: Uint8Array, offset: number, length: number, position?: number | null, + ) => write(fd, buffer, offset, Math.min(2, length), position)) as typeof fsSync.writeSync); + appendRegularFileSync({ filePath, content: "abcdef", mode: 0o600 }); + expect(await fs.readFile(filePath, "utf8")).toBe("initialabcdef"); + expect(calls).toHaveBeenCalledTimes(3); +}); diff --git a/test/root-write-exact-identity.test.ts b/test/root-write-exact-identity.test.ts index dae7a5cd..b1b00fc2 100644 --- a/test/root-write-exact-identity.test.ts +++ b/test/root-write-exact-identity.test.ts @@ -155,7 +155,9 @@ for (const route of routes) { const binding = route.includes("native") ? __loadBundledNativeForTest() : undefined; if (route.startsWith("windows")) Object.defineProperty(process, "platform", { value: "win32" }); let published = false; - const currentInode = () => changed && published ? inode - 1n : inode; + let callbackStarted = false; + // Windows now fences exact fd identity before chmod; keep this fault at the callback boundary. + const currentInode = () => changed && published && (route !== "windows native" || callbackStarted) ? inode - 1n : inode; const fstat = fsSync.fstatSync.bind(fsSync); vi.spyOn(fsSync, "fstatSync").mockImplementation(((...args: Parameters) => project(fstat(...args), currentInode())) as typeof fsSync.fstatSync); @@ -196,6 +198,7 @@ for (const route of routes) { fd = params.fd; expect(params.expectedIdentity).toMatchObject({ dev: device, ino: inode }); // Direct-create paths have no rename; mutate only after their expected snapshot. + callbackStarted = true; published = true; await verify(params); }; diff --git a/test/secret-write-publication.test.ts b/test/secret-write-publication.test.ts index 091d542a..c7ccac2b 100644 --- a/test/secret-write-publication.test.ts +++ b/test/secret-write-publication.test.ts @@ -20,7 +20,7 @@ const writers = [ { operation: "write", write: writeSecretFileAtomic }, { operation: "create", write: createSecretFileAtomic }, ] as const; -const modes = [0o000, 0o200, 0o400, 0o600]; +const modes = [0o000, 0o200, 0o400, 0o600, 0o1600, 0o2600, 0o4600, 0o6600]; const verify = verification.verifyAtomicWriteResult; afterEach(() => { @@ -39,6 +39,7 @@ for (const backend of ["off", "require"] as const) { configureFsSafeNative({ mode: backend }); expect(process.getuid?.()).toBeGreaterThan(0); const rootDir = await tempRoot("fs-safe-secret-mode-"); + await fs.chown(rootDir, process.geteuid!(), process.getegid!()); const filePath = path.join(rootDir, "token"); const content = Uint8Array.from([0, 10, 127, 128, 255]); const open = vi.spyOn(fs, "open"); @@ -58,7 +59,7 @@ for (const backend of ["off", "require"] as const) { const stat = await fs.lstat(filePath); expect(stat.isFile()).toBe(true); expect(stat.nlink).toBe(1); - expect(stat.mode & 0o777).toBe(mode); + expect(stat.mode & 0o7777).toBe(mode); if ((mode & 0o400) === 0) { await expect(fs.readFile(filePath)).rejects.toMatchObject({ code: "EACCES" }); } @@ -73,13 +74,14 @@ for (const backend of ["off", "require"] as const) { const rootDir = await tempRoot("fs-safe-secret-default-"); const filePath = path.join(rootDir, "token"); await write({ rootDir, filePath, content: "synthetic default\n" }); - expect((await fs.stat(filePath)).mode & 0o777).toBe(0o600); + expect((await fs.stat(filePath)).mode & 0o7777).toBe(0o600); expect(await fs.readFile(filePath, "utf8")).toBe("synthetic default\n"); }); it.each(modes)("overwrites restrictive files and preserves create collisions at mode %i", async (mode) => { configureFsSafeNative({ mode: backend }); const rootDir = await tempRoot("fs-safe-secret-overwrite-"); + await fs.chown(rootDir, process.geteuid!(), process.getegid!()); const filePath = path.join(rootDir, "token"); await createSecretFileAtomic({ rootDir, filePath, content: "original", mode }); const original = await fs.stat(filePath, { bigint: true }); @@ -87,19 +89,38 @@ for (const backend of ["off", "require"] as const) { .rejects.toMatchObject({ code: "secret-exists" }); const collided = await fs.stat(filePath, { bigint: true }); expect(collided.ino).toBe(original.ino); - expect(collided.mode & 0o777n).toBe(BigInt(mode)); + expect(collided.mode & 0o7777n).toBe(BigInt(mode)); await fs.chmod(filePath, 0o600); expect(await fs.readFile(filePath, "utf8")).toBe("original"); await fs.chmod(filePath, mode); await writeSecretFileAtomic({ rootDir, filePath, content: "overwritten", mode }); const overwritten = await fs.stat(filePath, { bigint: true }); expect(overwritten.ino).not.toBe(original.ino); - expect(overwritten.mode & 0o777n).toBe(BigInt(mode)); + expect(overwritten.mode & 0o7777n).toBe(BigInt(mode)); await fs.chmod(filePath, 0o600); expect(await fs.readFile(filePath, "utf8")).toBe("overwritten"); expect(await fs.readdir(rootDir)).toEqual(["token"]); }); + it.each(writers.flatMap((writer) => [0o1000, 0o2000, 0o4000].map((extra) => ({ ...writer, extra }))))( + "$operation rejects unexpected special mode bits $extra after publication", + async ({ write, extra }) => { + configureFsSafeNative({ mode: backend }); + const rootDir = await tempRoot("fs-safe-secret-extra-mode-"); + await fs.chown(rootDir, process.geteuid!(), process.getegid!()); + const filePath = path.join(rootDir, "token"); + const checked = vi.spyOn(verification, "verifyAtomicWriteResult").mockImplementation(async (params) => { + fsSync.fchmodSync(params.fd, 0o600 | extra); + await verify(params); + }); + await expect(write({ rootDir, filePath, content: "synthetic", mode: 0o600 })) + .rejects.toThrow(/insecure permissions/); + expect(checked).toHaveBeenCalledOnce(); + expect((await fs.stat(filePath)).mode & 0o7777).toBe(0o600 | extra); + expect(await fs.readFile(filePath, "utf8")).toBe("synthetic"); + }, + ); + const attacks = [ "none", "same bytes", "symlink", "hardlink replacement", "late hardlink", "parent swap", "root swap", "parent rebind", "root rebind", "callback failure", "I/O failure", "late mode", @@ -134,7 +155,7 @@ for (const backend of ["off", "require"] as const) { borrowedFd = params.fd; const stat = fsSync.fstatSync(params.fd, { bigint: true }); expect(params.expectedIdentity).toMatchObject({ dev: stat.dev, ino: stat.ino }); - expect(stat.mode & 0o777n).toBe(0n); + expect(stat.mode & 0o7777n).toBe(0n); if (["same bytes", "symlink", "hardlink replacement"].includes(attack)) { publishedPath = path.join(parent, "published"); await fs.rename(filePath, publishedPath); @@ -200,13 +221,13 @@ for (const backend of ["off", "require"] as const) { + handles.filter(({ fd }) => fd === borrowedFd).reduce((sum, { close }) => sum + close.mock.calls.length, 0); expect(closes).toBe(1); vi.restoreAllMocks(); - expect((await fs.stat(publishedPath)).mode & 0o777).toBe(attack === "late mode" ? 0o644 : 0o000); + expect((await fs.stat(publishedPath)).mode & 0o7777).toBe(attack === "late mode" ? 0o644 : 0o000); await fs.chmod(publishedPath, 0o600); expect(await fs.readFile(publishedPath, "utf8")).toBe(payload); expect(await fs.readFile(outsideFile, "utf8")).toBe("outside"); - expect((await fs.stat(outsideFile)).mode & 0o777).toBe(0o600); + expect((await fs.stat(outsideFile)).mode & 0o7777).toBe(0o600); if (attack === "same bytes") { - expect((await fs.stat(filePath)).mode & 0o777).toBe(0o400); + expect((await fs.stat(filePath)).mode & 0o7777).toBe(0o400); expect(await fs.readFile(filePath, "utf8")).toBe(payload); } else if (attack === "symlink") expect((await fs.lstat(filePath)).isSymbolicLink()).toBe(true); else if (attack === "hardlink replacement") {