mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-10 15:09:00 +02:00
fix(memory-helpers): a slow gitleaks probe no longer disables secret scanning
`gitleaksAvailable()` cached every failure the same way, so a 2s timeout on `gitleaks version` was recorded as "the binary is absent" for the rest of the process. One busy moment and the whole ingest ran unscanned behind a single stderr line — a fail-open outcome decided by machine load rather than by anything about the machine's setup. The caller only acts on `scanner === "gitleaks"`, so every later file was written with no scan and no second warning. The probe now classifies three outcomes. ENOENT (and a present-but-unusable binary: bad exit, EACCES) stays cached — that is a fact about the box, and re-probing it per file would be waste. A timeout gets one retry on a 10s budget, and if that also expires nothing is cached: the file is reported unscanned, the warning says so in those words, and the next file probes again. Observed under the 7-way sharded free-test runner, where spawning a shell script inside a temp bin dir took longer than the 2s budget. Tests: the retry path, the no-cache-on-timeout path (the second call must re-probe), and the cached-absent path. The fake gitleaks hangs for 30s rather than racing a short sleep against a short budget, and the budgets are chosen so load cannot flip an outcome: 30s where the retry MUST answer, 800ms where the probe MUST expire. An earlier draft used 1s/5s and flaked under the same shard runner this commit is about. The existing probe test pinned `detect` to calls[1], which a retry breaks; it now asserts the order instead of the index. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0111Mq3JGwZDcstn5wYcbhSw
This commit is contained in:
committed by
Garry Tan
co-authored by
Claude Opus 5
parent
5424ac5fe0
commit
8c7ff15fc4
@@ -23,6 +23,8 @@ import {
|
||||
withErrorContext,
|
||||
detectEngineTier,
|
||||
_resetGitleaksAvailabilityCache,
|
||||
_setGitleaksProbeTimeouts,
|
||||
_gitleaksCacheState,
|
||||
} from "../lib/gstack-memory-helpers";
|
||||
|
||||
// ── canonicalizeRemote ─────────────────────────────────────────────────────
|
||||
@@ -153,8 +155,140 @@ exit 2
|
||||
expect(result.scanner).toBe("gitleaks");
|
||||
expect(result.findings).toEqual([]);
|
||||
const calls = readFileSync(log, "utf-8").trim().split("\n");
|
||||
// Under load the first probe can expire and retry, so assert the shape:
|
||||
// one or more `version` probes, then the scan. Pinning calls[1] made a
|
||||
// busy machine look like a broken scanner.
|
||||
expect(calls[0]).toBe("version");
|
||||
expect(calls[1]).toContain("detect --no-git --source");
|
||||
expect(calls.at(-1)).toContain("detect --no-git --source");
|
||||
expect(calls.slice(0, -1).every((c) => c === "version")).toBe(true);
|
||||
} finally {
|
||||
if (oldPath === undefined) delete process.env.PATH;
|
||||
else process.env.PATH = oldPath;
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
// ── probe timeout vs missing binary ──────────────────────────────────────
|
||||
//
|
||||
// A timeout used to be cached as "gitleaks is absent", which turned one busy
|
||||
// moment into an entire run of unscanned files behind a single stderr line.
|
||||
// These pin the two outcomes apart. Budgets are shrunk via the test-only
|
||||
// hook so a sleeping fake costs milliseconds, not seconds.
|
||||
|
||||
/**
|
||||
* Fake gitleaks. With a `marker` path, the FIRST `version` call hangs far
|
||||
* past any budget and later calls answer instantly; with an empty marker it
|
||||
* hangs every time. Timing is expressed as "hangs forever" vs "immediate"
|
||||
* rather than as a race between a short sleep and a short budget — a race is
|
||||
* exactly the flake being fixed here.
|
||||
*/
|
||||
function fakeGitleaks(binDir: string, log: string, marker: string): void {
|
||||
mkdirSync(binDir, { recursive: true });
|
||||
writeFileSync(
|
||||
join(binDir, "gitleaks"),
|
||||
`#!/bin/sh
|
||||
printf '%s\\n' "$*" >> "${log}"
|
||||
if [ "$1" = "version" ]; then
|
||||
if [ -n "${marker}" ] && [ -f "${marker}" ]; then
|
||||
exit 0
|
||||
fi
|
||||
if [ -n "${marker}" ]; then
|
||||
touch "${marker}"
|
||||
fi
|
||||
sleep 30
|
||||
exit 0
|
||||
fi
|
||||
if [ "$1" = "detect" ]; then
|
||||
echo '[]'
|
||||
exit 0
|
||||
fi
|
||||
exit 2
|
||||
`,
|
||||
"utf-8",
|
||||
);
|
||||
chmodSync(join(binDir, "gitleaks"), 0o755);
|
||||
}
|
||||
|
||||
function withFakeOnPath<T>(binDir: string, fn: () => T): T {
|
||||
const oldPath = process.env.PATH;
|
||||
process.env.PATH = `${binDir}:${oldPath || ""}`;
|
||||
try {
|
||||
return fn();
|
||||
} finally {
|
||||
if (oldPath === undefined) delete process.env.PATH;
|
||||
else process.env.PATH = oldPath;
|
||||
}
|
||||
}
|
||||
|
||||
const versionProbes = (log: string): number =>
|
||||
existsSync(log)
|
||||
? readFileSync(log, "utf-8").trim().split("\n").filter((c) => c === "version").length
|
||||
: 0;
|
||||
|
||||
it("retries a slow probe instead of declaring gitleaks missing", () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), "gstack-test-"));
|
||||
const binDir = join(dir, "bin");
|
||||
const log = join(dir, "calls.log");
|
||||
const file = join(dir, "clean.txt");
|
||||
writeFileSync(file, "no secrets here\n");
|
||||
fakeGitleaks(binDir, log, join(dir, "hung-once"));
|
||||
try {
|
||||
// Budgets are picked so neither outcome can hinge on machine speed: 3s is
|
||||
// ample for a shell to start and log even on a loaded box (yet the hung
|
||||
// `sleep 30` still cannot answer within it), and the 30s retry cannot
|
||||
// expire against a fake that exits immediately. The first draft used
|
||||
// 1s/5s and flaked under the 7-way shard runner — the very failure mode
|
||||
// this file is about.
|
||||
_setGitleaksProbeTimeouts(3_000, 30_000);
|
||||
const result = withFakeOnPath(binDir, () => secretScanFile(file));
|
||||
expect(result.scanner).toBe("gitleaks");
|
||||
expect(versionProbes(log)).toBe(2);
|
||||
expect(_gitleaksCacheState()).toBe(true);
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("does not cache a timed-out probe, so the next file tries again", () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), "gstack-test-"));
|
||||
const binDir = join(dir, "bin");
|
||||
const log = join(dir, "calls.log");
|
||||
const file = join(dir, "clean.txt");
|
||||
writeFileSync(file, "no secrets here\n");
|
||||
// Empty marker: EVERY call hangs, so both budgets expire.
|
||||
fakeGitleaks(binDir, log, "");
|
||||
try {
|
||||
// Short on purpose, and safe to be short: the fake hangs for 30s, so the
|
||||
// probe times out at ANY budget — load cannot flip this outcome the way
|
||||
// it can in the retry case above. 800ms only has to cover writing one
|
||||
// line to the log.
|
||||
_setGitleaksProbeTimeouts(800, 800);
|
||||
const first = withFakeOnPath(binDir, () => secretScanFile(file));
|
||||
expect(first.scanner).toBe("missing");
|
||||
// The question stays open: nothing was learned about the binary.
|
||||
expect(_gitleaksCacheState()).toBeNull();
|
||||
|
||||
const before = versionProbes(log);
|
||||
withFakeOnPath(binDir, () => secretScanFile(file));
|
||||
expect(versionProbes(log)).toBeGreaterThan(before);
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("caches an absent binary, so it is probed once per process", () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), "gstack-test-"));
|
||||
const binDir = join(dir, "empty-bin");
|
||||
mkdirSync(binDir, { recursive: true });
|
||||
const file = join(dir, "clean.txt");
|
||||
writeFileSync(file, "no secrets here\n");
|
||||
const oldPath = process.env.PATH;
|
||||
try {
|
||||
// Nothing named gitleaks anywhere on PATH -> ENOENT, a permanent fact.
|
||||
process.env.PATH = binDir;
|
||||
const result = secretScanFile(file);
|
||||
expect(result.scanner).toBe("missing");
|
||||
expect(_gitleaksCacheState()).toBe(false);
|
||||
} finally {
|
||||
if (oldPath === undefined) delete process.env.PATH;
|
||||
else process.env.PATH = oldPath;
|
||||
|
||||
Reference in New Issue
Block a user