mirror of
https://github.com/garrytan/gstack.git
synced 2026-06-17 07:10:12 +02:00
harden(sync): close staging-guard TOCTOU + fail hard on marker write (#1802 C5)
checkOwnedStagingDir() now returns the realpath-resolved canonicalPath on a pass, and cleanupStagingDir() rmSync's that instead of the raw input — closing the gap where the input is a symlink swapped between the ownership check and the delete. makeStagingDir() tears down the partial dir and rethrows if the marker write fails, so a marker-less dir (which the guard would refuse forever) can never leak. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1211,7 +1211,15 @@ function makeStagingDir(): string {
|
||||
mkdirSync(dir, { recursive: true });
|
||||
// Mint the ownership marker (#1802) so cleanupStagingDir() and decideResume()
|
||||
// can prove this dir was created by us before any recursive delete or resume.
|
||||
writeFileSync(join(dir, STAGING_MARKER), `${process.pid}\n${Date.now()}\n`, "utf-8");
|
||||
// #1802 C5: fail hard if the marker can't be written — a marker-less dir would
|
||||
// be refused by the guard forever (leaked, never cleaned). Tear down the
|
||||
// partial dir and rethrow so the caller fails loudly instead of leaking.
|
||||
try {
|
||||
writeFileSync(join(dir, STAGING_MARKER), `${process.pid}\n${Date.now()}\n`, "utf-8");
|
||||
} catch (err) {
|
||||
try { rmSync(dir, { recursive: true, force: true }); } catch { /* best-effort */ }
|
||||
throw err;
|
||||
}
|
||||
return dir;
|
||||
}
|
||||
|
||||
@@ -1284,7 +1292,10 @@ function cleanupStagingDir(dir: string): void {
|
||||
return;
|
||||
}
|
||||
try {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
// #1802 C5: delete the realpath-resolved dir the guard validated, not the
|
||||
// raw input — closes the TOCTOU gap where `dir` is a symlink swapped between
|
||||
// the check above and this rmSync. canonicalPath is always set when ok.
|
||||
rmSync(verdict.canonicalPath ?? dir, { recursive: true, force: true });
|
||||
} catch {
|
||||
// best-effort
|
||||
}
|
||||
|
||||
@@ -50,6 +50,13 @@ export interface StagingVerdict {
|
||||
ok: boolean;
|
||||
/** Precise rejection reason, for actionable logging. Undefined when ok. */
|
||||
reason?: string;
|
||||
/**
|
||||
* The realpath-resolved directory the verdict actually validated. Present only
|
||||
* when ok. Callers that delete MUST `rmSync` this path, not the raw input —
|
||||
* deleting the canonical path closes the TOCTOU gap where the input is a
|
||||
* symlink swapped between this check and the delete (#1802 C5).
|
||||
*/
|
||||
canonicalPath?: string;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -100,7 +107,7 @@ export function checkOwnedStagingDir(dir: string, gstackHome: string): StagingVe
|
||||
} catch {
|
||||
return { ok: false, reason: `missing "${STAGING_MARKER}" marker — not minted by makeStagingDir` };
|
||||
}
|
||||
return { ok: true };
|
||||
return { ok: true, canonicalPath: canon };
|
||||
}
|
||||
|
||||
/** Boolean convenience wrapper around {@link checkOwnedStagingDir}. */
|
||||
|
||||
@@ -287,6 +287,14 @@ describe("#1802 checkOwnedStagingDir — ownership matrix", () => {
|
||||
expect(checkOwnedStagingDir(mintStaging(), home).ok).toBe(true);
|
||||
});
|
||||
|
||||
test("#1802 C5: ok verdict carries the realpath-resolved canonicalPath", () => {
|
||||
const d = mintStaging();
|
||||
const v = checkOwnedStagingDir(d, home);
|
||||
expect(v.ok).toBe(true);
|
||||
// Callers must delete this (not the raw input) to close the symlink TOCTOU.
|
||||
expect(v.canonicalPath).toBe(fs.realpathSync(d));
|
||||
});
|
||||
|
||||
test("repo root (direct child, has .git, no marker) → refused", () => {
|
||||
const repo = path.join(home, "my-repo");
|
||||
fs.mkdirSync(path.join(repo, ".git"), { recursive: true });
|
||||
@@ -409,6 +417,27 @@ describe("#1802 C3 — import-timeout preserve (static invariant)", () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ── #1802 C5: hardening (static invariant) ─────────────────────────────────
|
||||
describe("#1802 C5 — hardening (static invariant)", () => {
|
||||
const ingest = fs.readFileSync(
|
||||
path.join(ROOT, "bin", "gstack-memory-ingest.ts"),
|
||||
"utf-8",
|
||||
);
|
||||
|
||||
test("cleanupStagingDir deletes the canonical path, not the raw input", () => {
|
||||
expect(ingest).toMatch(/rmSync\(verdict\.canonicalPath \?\? dir/);
|
||||
});
|
||||
|
||||
test("makeStagingDir tears down + rethrows if the marker write fails", () => {
|
||||
const at = ingest.indexOf("function makeStagingDir");
|
||||
expect(at).toBeGreaterThan(-1);
|
||||
const slice = ingest.slice(at, at + 800);
|
||||
expect(slice).toMatch(/catch \(err\)/);
|
||||
expect(slice).toMatch(/rmSync\(dir, \{ recursive: true, force: true \}\)/);
|
||||
expect(slice).toMatch(/throw err/);
|
||||
});
|
||||
});
|
||||
|
||||
// ── #1802 C4: resume must not mark failed files as ingested ─────────────────
|
||||
// readNewFailures() maps gbrain's per-file failures (keyed by staging-relative
|
||||
// path) back to source paths so the caller can EXCLUDE them from state
|
||||
|
||||
Reference in New Issue
Block a user