fix(fs): close fallback mkdir and archive cleanup races
This commit is contained in:
parent
500243f398
commit
85f5b55050
@ -11,6 +11,7 @@
|
||||
- Close guarded fallback write handles without following path names if post-write directory verification fails, avoiding descriptor leaks and unsafe cleanup in symlink-swap races.
|
||||
- Preserve empty-directory pruning and broken-symlink trash moves across guarded fallback paths.
|
||||
- Preserve sync file-store read policy errors for directory and hardlink validation failures.
|
||||
- Guard fallback mkdir component creation and skip archive destination cleanup after pre-commit races.
|
||||
|
||||
### Tests
|
||||
|
||||
|
||||
@ -31,7 +31,6 @@ import {
|
||||
type TarEntryInfo,
|
||||
} from "./archive-tar.js";
|
||||
import { loadZipArchiveWithPreflight } from "./archive-zip-preflight.js";
|
||||
import { isNotFoundPathError } from "./path.js";
|
||||
import { writeSiblingTempFile } from "./sibling-temp.js";
|
||||
import { withTimeout } from "./timing.js";
|
||||
|
||||
@ -80,21 +79,6 @@ const OPEN_WRITE_CREATE_FLAGS =
|
||||
fsConstants.O_EXCL |
|
||||
(SUPPORTS_NOFOLLOW ? fsConstants.O_NOFOLLOW : 0);
|
||||
|
||||
async function cleanupPartialRegularFile(filePath: string): Promise<void> {
|
||||
let stat: Awaited<ReturnType<typeof fs.lstat>>;
|
||||
try {
|
||||
stat = await fs.lstat(filePath);
|
||||
} catch (err) {
|
||||
if (isNotFoundPathError(err)) {
|
||||
return;
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
if (stat.isFile()) {
|
||||
await fs.unlink(filePath).catch(() => undefined);
|
||||
}
|
||||
}
|
||||
|
||||
type ZipEntry = {
|
||||
name: string;
|
||||
dir: boolean;
|
||||
@ -214,7 +198,8 @@ async function writeZipFileEntry(params: {
|
||||
}
|
||||
}
|
||||
} catch (err) {
|
||||
await cleanupPartialRegularFile(destinationPath).catch(() => undefined);
|
||||
// Failures here happen before the temp has been committed. The destination
|
||||
// parent may already be untrusted, so cleanup must stay limited to temp state.
|
||||
throw err;
|
||||
} finally {
|
||||
const openTempHandle = tempHandle as FileHandle | null;
|
||||
|
||||
51
src/guarded-mkdir.ts
Normal file
51
src/guarded-mkdir.ts
Normal file
@ -0,0 +1,51 @@
|
||||
import fs from "node:fs/promises";
|
||||
import path from "node:path";
|
||||
import { assertAsyncDirectoryGuard, createAsyncDirectoryGuard } from "./directory-guard.js";
|
||||
import { FsSafeError } from "./errors.js";
|
||||
|
||||
function isSameOrChildPath(candidate: string, parent: string): boolean {
|
||||
return candidate === parent || candidate.startsWith(`${parent}${path.sep}`);
|
||||
}
|
||||
|
||||
function isPathEscape(relativePath: string): boolean {
|
||||
return relativePath === ".." || relativePath.startsWith(`..${path.sep}`) || path.isAbsolute(relativePath);
|
||||
}
|
||||
|
||||
export async function mkdirPathComponentsWithGuards(params: {
|
||||
rootReal: string;
|
||||
targetPath: string;
|
||||
beforeComponent?: (componentPath: string) => Promise<void> | void;
|
||||
}): Promise<void> {
|
||||
const root = path.resolve(params.rootReal);
|
||||
const target = path.resolve(params.targetPath);
|
||||
const relative = path.relative(root, target);
|
||||
if (isPathEscape(relative)) {
|
||||
throw new FsSafeError("outside-workspace", "directory is outside workspace root");
|
||||
}
|
||||
let current = root;
|
||||
for (const part of relative.split(path.sep).filter(Boolean)) {
|
||||
const next = path.join(current, part);
|
||||
const parentGuard = await createAsyncDirectoryGuard(current);
|
||||
await params.beforeComponent?.(next);
|
||||
await assertAsyncDirectoryGuard(parentGuard);
|
||||
try {
|
||||
await fs.mkdir(next);
|
||||
} catch (error) {
|
||||
if (!error || typeof error !== "object" || !("code" in error) || error.code !== "EEXIST") {
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
const stat = await fs.lstat(next);
|
||||
if (stat.isSymbolicLink() || !stat.isDirectory()) {
|
||||
throw new FsSafeError("not-file", "directory component must be a directory");
|
||||
}
|
||||
// Node's recursive mkdir follows symlinks in missing components. Build one
|
||||
// segment at a time and realpath-check each segment before descending.
|
||||
if (!isSameOrChildPath(path.resolve(await fs.realpath(next)), root)) {
|
||||
throw new FsSafeError("outside-workspace", "directory escaped workspace root");
|
||||
}
|
||||
await createAsyncDirectoryGuard(next);
|
||||
await assertAsyncDirectoryGuard(parentGuard);
|
||||
current = next;
|
||||
}
|
||||
}
|
||||
@ -9,6 +9,7 @@ import { createBoundedReadStream } from "./bounded-read-stream.js";
|
||||
import { assertAsyncDirectoryGuard, createAsyncDirectoryGuard, createNearestExistingDirectoryGuard } from "./directory-guard.js";
|
||||
import { FsSafeError } from "./errors.js";
|
||||
import { sameFileIdentity } from "./file-identity.js";
|
||||
import { mkdirPathComponentsWithGuards } from "./guarded-mkdir.js";
|
||||
import { withAsyncDirectoryGuards } from "./guarded-mutation.js";
|
||||
import { isPinnedPathHelperSpawnError, runPinnedPathHelper } from "./pinned-path.js";
|
||||
import { runPinnedCopyHelper, runPinnedWriteHelper } from "./pinned-write.js";
|
||||
@ -1384,11 +1385,10 @@ async function removePathFallback(resolved: { resolved: string }): Promise<void>
|
||||
}
|
||||
|
||||
async function mkdirPathFallback(resolved: { rootReal: string; resolved: string }): Promise<void> {
|
||||
const guard = await createNearestExistingDirectoryGuard(resolved.rootReal, resolved.resolved);
|
||||
await getFsSafeTestHooks()?.beforeRootFallbackMutation?.("mkdir", resolved.resolved);
|
||||
await assertAsyncDirectoryGuard(guard);
|
||||
await fs.mkdir(resolved.resolved, { recursive: true });
|
||||
await assertAsyncDirectoryGuard(guard);
|
||||
await mkdirPathComponentsWithGuards({
|
||||
rootReal: resolved.rootReal, targetPath: resolved.resolved,
|
||||
beforeComponent: async (componentPath) => await getFsSafeTestHooks()?.beforeRootFallbackMutation?.("mkdir", componentPath),
|
||||
});
|
||||
}
|
||||
|
||||
async function statPathFallback(root: RootContext, relativePath: string): Promise<PathStat> {
|
||||
|
||||
@ -169,6 +169,38 @@ describe("archive extraction", () => {
|
||||
},
|
||||
);
|
||||
|
||||
it.runIf(process.platform !== "win32")(
|
||||
"does not cleanup through a swapped zip entry parent before commit",
|
||||
async () => {
|
||||
const root = await tempRoot("fs-safe-archive-cleanup-race-");
|
||||
const archivePath = path.join(root, "pkg.zip");
|
||||
const destDir = path.join(root, "dest");
|
||||
const outsideDir = path.join(root, "outside");
|
||||
const outsideFile = path.join(outsideDir, "payload.txt");
|
||||
await fs.mkdir(destDir);
|
||||
await fs.mkdir(outsideDir);
|
||||
await fs.writeFile(outsideFile, "outside");
|
||||
const zip = new JSZip();
|
||||
zip.file("nested/payload.txt", "inside");
|
||||
await fs.writeFile(archivePath, await zip.generateAsync({ type: "nodebuffer" }));
|
||||
const realMkdir = fs.mkdir.bind(fs);
|
||||
let swapped = false;
|
||||
vi.spyOn(fs, "mkdir").mockImplementation(async (...args: Parameters<typeof fs.mkdir>) => {
|
||||
const candidate = String(args[0]);
|
||||
if (!swapped && path.basename(candidate) === "nested" && await fs.lstat(candidate).then(() => true, () => false)) {
|
||||
swapped = true;
|
||||
await fs.rename(candidate, path.join(path.dirname(candidate), "nested-real"));
|
||||
await fs.symlink(outsideDir, candidate, "dir");
|
||||
}
|
||||
return await realMkdir(...args);
|
||||
});
|
||||
|
||||
await expect(extractArchive({ archivePath, destDir, kind: "zip", timeoutMs: 15_000 }))
|
||||
.rejects.toBeTruthy();
|
||||
await expect(fs.readFile(outsideFile, "utf8")).resolves.toBe("outside");
|
||||
},
|
||||
);
|
||||
|
||||
it.runIf(process.platform !== "win32")(
|
||||
"rejects zip extraction when a hardlink appears after write",
|
||||
async () => {
|
||||
|
||||
@ -89,4 +89,22 @@ describe("platform fallback coverage", () => {
|
||||
// without recursive rejects dirs and would silently leave pruneEmptyDirs work.
|
||||
await expect(fs.stat(path.join(rootDir, "old"))).rejects.toMatchObject({ code: "ENOENT" });
|
||||
});
|
||||
|
||||
it.runIf(process.platform !== "win32")("rejects symlinked missing mkdir components in fallback", async () => {
|
||||
const { root: openRoot } = await importRootForPlatform("win32");
|
||||
const { __setFsSafeTestHooksForTest } = await import("../src/test-hooks.js");
|
||||
const rootDir = await tempRoot("fs-safe-win-mkdir-race-");
|
||||
const outsideDir = await tempRoot("fs-safe-win-mkdir-outside-");
|
||||
const scoped = await openRoot(rootDir);
|
||||
const racedComponent = path.join(rootDir, "link");
|
||||
__setFsSafeTestHooksForTest({
|
||||
async beforeRootFallbackMutation(operation, targetPath) {
|
||||
if (operation !== "mkdir" || path.basename(targetPath) !== "link") return;
|
||||
await fs.symlink(outsideDir, targetPath, "dir");
|
||||
},
|
||||
});
|
||||
|
||||
await expect(scoped.mkdir("link/created")).rejects.toBeTruthy();
|
||||
await expect(fs.stat(path.join(outsideDir, "created"))).rejects.toMatchObject({ code: "ENOENT" });
|
||||
});
|
||||
});
|
||||
|
||||
Loading…
Reference in New Issue
Block a user