mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-18 19:02:18 +02:00
test: pin the security-property regression guards from pre-landing review
The pre-landing review found the fixes were correct but three regression guards were missing — each pins a property whose silent revert would keep behavior identical while reopening the hole: - validateAuth: a static tripwire asserting crypto.timingSafeEqual + the got.length===want.length gate + the null-header guard (a revert to `===` keeps accept/reject green but restores the timing side-channel). - redact: a table-driven loop over the exported URL_PASSWORD_PLACEHOLDER_WORDS so a typo or dropped entry can't silently start blocking a doc placeholder; plus a substring-can't-rescue-a-real-secret assertion. - config: assert the self-contained .gitignore is written even when git already ignores .gstack/, proving the write precedes the isIgnoredByGit early return. - bun-polyfill: cover the 128+signal exit branch (POSIX only). URL_PASSWORD_PLACEHOLDER_WORDS is exported so the table test can't drift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
341d7be27c
commit
5d74ed7231
@@ -123,6 +123,22 @@ describe('bun-polyfill', () => {
|
|||||||
expect(out).toMatch(/^exit:\d+$/);
|
expect(out).toMatch(/^exit:\d+$/);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Signal-exit branch: Bun reports 128 + signal number when a child is killed
|
||||||
|
// by a signal (code === null). Skipped on Windows, whose kill() semantics
|
||||||
|
// don't produce the POSIX 128+n mapping.
|
||||||
|
test.skipIf(process.platform === 'win32')('Bun.spawn proc.exited maps a killing signal to 128+signal', async () => {
|
||||||
|
const result = Bun.spawnSync(['node', '-e', `
|
||||||
|
require(${JSON.stringify(polyfillPath)});
|
||||||
|
(async () => {
|
||||||
|
const p = Bun.spawn(['node', '-e', 'setInterval(() => {}, 1000)'], { stdio: ['ignore', 'ignore', 'ignore'] });
|
||||||
|
setTimeout(() => p.kill('SIGTERM'), 150);
|
||||||
|
console.log('exit:' + await p.exited);
|
||||||
|
})();
|
||||||
|
`], { stdout: 'pipe', stderr: 'pipe' });
|
||||||
|
// SIGTERM = 15 → 128 + 15 = 143.
|
||||||
|
expect(result.stdout.toString().trim()).toBe('exit:143');
|
||||||
|
});
|
||||||
|
|
||||||
// GSTACK_SPAWN_MAX_BUFFER caps the drain so a runaway child can't OOM the
|
// GSTACK_SPAWN_MAX_BUFFER caps the drain so a runaway child can't OOM the
|
||||||
// server. Past the cap, the pipe keeps flowing (child doesn't block) but
|
// server. Past the cap, the pipe keeps flowing (child doesn't block) but
|
||||||
// further bytes are dropped. Set a small cap, write more than that, assert
|
// further bytes are dropped. Set a small cap, write more than that, assert
|
||||||
|
|||||||
@@ -76,6 +76,24 @@ describe('config', () => {
|
|||||||
fs.rmSync(tmpDir, { recursive: true, force: true });
|
fs.rmSync(tmpDir, { recursive: true, force: true });
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('writes the self-contained .gitignore even when git already ignores .gstack/ (before the early return)', () => {
|
||||||
|
// Pins the load-bearing property: the state-dir ignore is written
|
||||||
|
// UNCONDITIONALLY, before the `if (isIgnoredByGit(...)) return` early exit.
|
||||||
|
// A git repo whose root .gitignore already lists .gstack/ makes
|
||||||
|
// isIgnoredByGit true, so the early return fires — moving the write below
|
||||||
|
// it (the exact bug the fix removed) would skip the guard here.
|
||||||
|
const tmpDir = path.join(os.tmpdir(), `browse-gitignored-repo-test-${Date.now()}`);
|
||||||
|
fs.mkdirSync(tmpDir, { recursive: true });
|
||||||
|
Bun.spawnSync(['git', 'init'], { cwd: tmpDir, stdout: 'ignore', stderr: 'ignore' });
|
||||||
|
fs.writeFileSync(path.join(tmpDir, '.gitignore'), '.gstack/\n');
|
||||||
|
const config = resolveConfig({ BROWSE_STATE_FILE: path.join(tmpDir, '.gstack', 'browse.json') });
|
||||||
|
ensureStateDir(config);
|
||||||
|
const selfIgnore = path.join(config.stateDir, '.gitignore');
|
||||||
|
expect(fs.existsSync(selfIgnore)).toBe(true);
|
||||||
|
expect(fs.readFileSync(selfIgnore, 'utf-8')).toBe('*\n');
|
||||||
|
fs.rmSync(tmpDir, { recursive: true, force: true });
|
||||||
|
});
|
||||||
|
|
||||||
test('adds .gstack/ to .gitignore if not present', () => {
|
test('adds .gstack/ to .gitignore if not present', () => {
|
||||||
const tmpDir = path.join(os.tmpdir(), `browse-gitignore-test-${Date.now()}`);
|
const tmpDir = path.join(os.tmpdir(), `browse-gitignore-test-${Date.now()}`);
|
||||||
fs.mkdirSync(tmpDir, { recursive: true });
|
fs.mkdirSync(tmpDir, { recursive: true });
|
||||||
|
|||||||
@@ -64,6 +64,22 @@ describe('Server auth security', () => {
|
|||||||
expect(scopeBlock).toContain('Domain not allowed');
|
expect(scopeBlock).toContain('Domain not allowed');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Test 1d: validateAuth compares the bearer token in CONSTANT TIME with a
|
||||||
|
// length gate. A revert to `header === \`Bearer ${authToken}\`` keeps
|
||||||
|
// accept/reject behavior identical (functional tests still pass) but silently
|
||||||
|
// reintroduces the byte-by-byte timing side-channel; dropping the length gate
|
||||||
|
// makes timingSafeEqual throw RangeError (500 instead of 401) on a wrong-length
|
||||||
|
// token. Pin both properties, mirroring the token-registry sibling guard.
|
||||||
|
test('validateAuth uses constant-time comparison with a length gate', () => {
|
||||||
|
const authBlock = sliceBetween(SERVER_SRC, 'function validateAuth(req: Request): boolean {', '// Factory-scoped shutdown');
|
||||||
|
expect(authBlock).toContain('crypto.timingSafeEqual');
|
||||||
|
expect(authBlock).toContain('got.length === want.length');
|
||||||
|
// The null-header guard must remain (Buffer.from(null) would otherwise throw).
|
||||||
|
expect(authBlock).toContain('header === null');
|
||||||
|
// The raw === comparison of the header against the bearer string must be gone.
|
||||||
|
expect(authBlock).not.toContain('header === `Bearer ${authToken}`');
|
||||||
|
});
|
||||||
|
|
||||||
// Test 2: /refs endpoint requires auth via validateAuth
|
// Test 2: /refs endpoint requires auth via validateAuth
|
||||||
test('/refs endpoint requires authentication', () => {
|
test('/refs endpoint requires authentication', () => {
|
||||||
const refsBlock = sliceBetween(SERVER_SRC, "url.pathname === '/refs'", "url.pathname === '/activity/stream'");
|
const refsBlock = sliceBetween(SERVER_SRC, "url.pathname === '/refs'", "url.pathname === '/activity/stream'");
|
||||||
|
|||||||
@@ -276,7 +276,7 @@ const INTERPOLATED_PASSWORD_RE = /^(\$\{.+\}|\$[A-Z_][A-Z0-9_]*)$/;
|
|||||||
// case-sensitively against the raw span: the convention is ALL CAPS, and a
|
// case-sensitively against the raw span: the convention is ALL CAPS, and a
|
||||||
// lowercase `password`/`pass` at this position is a real (terrible) credential
|
// lowercase `password`/`pass` at this position is a real (terrible) credential
|
||||||
// that must still block.
|
// that must still block.
|
||||||
const URL_PASSWORD_PLACEHOLDER_WORDS = new Set([
|
export const URL_PASSWORD_PLACEHOLDER_WORDS = new Set([
|
||||||
"PASSWORD",
|
"PASSWORD",
|
||||||
"PASS",
|
"PASS",
|
||||||
"PASSWD",
|
"PASSWD",
|
||||||
|
|||||||
@@ -21,6 +21,7 @@ import {
|
|||||||
shannonEntropy,
|
shannonEntropy,
|
||||||
isPublicIPv4,
|
isPublicIPv4,
|
||||||
isPlaceholderSpan,
|
isPlaceholderSpan,
|
||||||
|
URL_PASSWORD_PLACEHOLDER_WORDS,
|
||||||
} from "../lib/redact-patterns";
|
} from "../lib/redact-patterns";
|
||||||
|
|
||||||
function ids(text: string, vis: RepoVisibility = "private"): string[] {
|
function ids(text: string, vis: RepoVisibility = "private"): string[] {
|
||||||
@@ -143,6 +144,21 @@ describe("HIGH credential patterns", () => {
|
|||||||
expect(ids("postgres://admin:" + "ADMIN" + "123@host/db")).toContain("db.url_with_password");
|
expect(ids("postgres://admin:" + "ADMIN" + "123@host/db")).toContain("db.url_with_password");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Every curated placeholder word must suppress at the URL-password position.
|
||||||
|
// The fix replaced a shape rule with a hand-curated EXACT set, so a typo or a
|
||||||
|
// dropped entry (CHANGEME -> CHANGME) would silently start blocking a legit
|
||||||
|
// doc placeholder with zero failure elsewhere. Loop the real exported set so
|
||||||
|
// the test can't drift from the source list.
|
||||||
|
test("db.url_with_password suppresses every curated placeholder word", () => {
|
||||||
|
for (const word of URL_PASSWORD_PLACEHOLDER_WORDS) {
|
||||||
|
expect(ids(`postgres://user:${word}@host/db`)).not.toContain("db.url_with_password");
|
||||||
|
}
|
||||||
|
// Guard the set stays a non-trivial curated list (catches an accidental clear).
|
||||||
|
expect(URL_PASSWORD_PLACEHOLDER_WORDS.size).toBeGreaterThanOrEqual(8);
|
||||||
|
// And a real secret that merely CONTAINS a placeholder word still blocks.
|
||||||
|
expect(ids("postgres://user:" + "MY" + "SECRETPASS@host/db")).toContain("db.url_with_password");
|
||||||
|
});
|
||||||
|
|
||||||
test("all HIGH patterns block (exit 3)", () => {
|
test("all HIGH patterns block (exit 3)", () => {
|
||||||
const r = scan("AKIA1234567890ABCDEF", { repoVisibility: "private" });
|
const r = scan("AKIA1234567890ABCDEF", { repoVisibility: "private" });
|
||||||
expect(exitCodeFor(r)).toBe(3);
|
expect(exitCodeFor(r)).toBe(3);
|
||||||
|
|||||||
Reference in New Issue
Block a user