mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-09 10:18:32 +01:00
fix(redact-prepush): stop flagging your own email and already-public authors (#3060)
The hook never passed the engine's selfEmail / repoPublicEmails, so every push that added the pusher's address, or a commit author's, raised a MEDIUM pii.email warning for an address the destination already shows. The hook now reads `git config user.email` and one bounded `git log` of author and committer addresses, raw and mailmapped (%ae %aE %ce %cE): the history of every pushed tip (those commits become public with the push), each remote tip being replaced, and, for a new branch or an unknown remote tip pushed to a configured remote, that remote's own tracking refs. A URL or unknown destination never borrows tracking refs, and another remote's history never counts. The read stops at 50,000 commits (the newest authors are used) and 5 seconds. An error or the timeout passes no history and, when an email finding is listed, prints one line saying suppression was limited for this push. The two inputs fail independently. HIGH findings and every other rule are untouched. Controls: a stranger's address, an address only another remote knows, a URL push next to a matching remote, and a first push of a repo with no history all still report. Plan items: A3, CEO-7, CEO-23 (A3 half), DX-9 (partial history), ENG-8 (history scope, mailmap). Co-authored-by: akmandhania <143462686+akmandhania@users.noreply.github.com> Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
8f29f1cf58
commit
e7a481a28d
2 files changed
+230
-4
No files matched your search
@@ -21,6 +21,8 @@
|
||||
* - HIGH finding in added lines → print + exit 1 (block), for public AND private.
|
||||
* - MEDIUM → warn (non-blocking), one line per finding naming its rule,
|
||||
* file line and fix, never the matched value. LOW/WARN → silent.
|
||||
* - `pii.email` skips the pusher's own address and addresses already in
|
||||
* the destination's commit metadata (see emailScanOptions).
|
||||
* - GSTACK_REDACT_PREPUSH=skip → log + exit 0 (escape valve).
|
||||
*
|
||||
* Installed/uninstalled via `gstack-redact install-prepush-hook` (see the
|
||||
@@ -29,7 +31,7 @@
|
||||
import { spawnSync } from "child_process";
|
||||
import * as fs from "fs";
|
||||
import * as path from "path";
|
||||
import { normalizeWithMap, scan, type Finding } from "../lib/redact-engine";
|
||||
import { normalizeWithMap, scan, type Finding, type ScanOptions } from "../lib/redact-engine";
|
||||
import { mkdirpSync } from "../lib/fs-utils";
|
||||
import { resolveStateRoot } from "../lib/state-root";
|
||||
|
||||
@@ -500,6 +502,50 @@ function logSkip(reason: string): void {
|
||||
}
|
||||
}
|
||||
|
||||
const HISTORY_MAX_COMMITS = 50_000;
|
||||
const HISTORY_TIMEOUT_MS = 5_000;
|
||||
|
||||
function configValues(args: string[]): string[] {
|
||||
return git(["config", ...args]).split("\n").map((v) => v.trim()).filter(Boolean);
|
||||
}
|
||||
|
||||
/**
|
||||
* Addresses `pii.email` treats as not a new leak (#3060): the pusher's own
|
||||
* (`git config user.email`) and the author and committer addresses (raw and
|
||||
* mailmapped) the destination will hold after this push. That last set is the history of every pushed tip
|
||||
* (pushed commits become public with the push, third-party authors included),
|
||||
* of each remote tip being replaced, and, for a ref whose remote tip is new or
|
||||
* unknown, the push remote's own tracking refs. A URL or unknown destination
|
||||
* never borrows a remote's tracking refs; another remote's history never counts.
|
||||
*
|
||||
* Each input fails on its own and a failure omits only that input, which is
|
||||
* the behavior before these exemptions existed. The history read is bounded:
|
||||
* past the commit cap it uses the newest authors it read; an error or the
|
||||
* timeout passes no history, and the reason is returned so the report can say
|
||||
* suppression was limited.
|
||||
*/
|
||||
function emailScanOptions(historyRevs: string[], trackingRemote: string | null): {
|
||||
options: Pick<ScanOptions, "selfEmail" | "repoPublicEmails">;
|
||||
limited?: string;
|
||||
} {
|
||||
const selfEmail = configValues(["--get", "user.email"])[0];
|
||||
const r = spawnSync("git", [
|
||||
"log", "--no-show-signature", "--no-color", `--max-count=${HISTORY_MAX_COMMITS}`, "--format=%ae%n%aE%n%ce%n%cE",
|
||||
...historyRevs, ...(trackingRemote ? [`--remotes=${trackingRemote}/*`] : []), "--",
|
||||
], { encoding: "utf8", maxBuffer: 64 * 1024 * 1024, timeout: HISTORY_TIMEOUT_MS, killSignal: "SIGKILL" });
|
||||
const timedOut = (r.error as NodeJS.ErrnoException | undefined)?.code === "ETIMEDOUT";
|
||||
const history = r.status === 0 && !r.error
|
||||
? (r.stdout ?? "").split("\n").map((e) => e.trim()).filter((e) => e.includes("@"))
|
||||
: null;
|
||||
return {
|
||||
options: {
|
||||
...(selfEmail ? { selfEmail } : {}),
|
||||
...(history ? { repoPublicEmails: [...new Set(history)] } : {}),
|
||||
},
|
||||
...(history ? {} : { limited: timedOut ? `reading commit history took over ${HISTORY_TIMEOUT_MS / 1000} s` : "reading commit history failed" }),
|
||||
};
|
||||
}
|
||||
|
||||
/** One MEDIUM report line: rule id, file line (and commit), and the fix. Never the matched value. */
|
||||
function mediumLine(f: Finding, part: AddedFile): string {
|
||||
const line = part.lines[f.line - 1];
|
||||
@@ -523,6 +569,8 @@ function main() {
|
||||
.map((l) => l.split(/\s+/));
|
||||
|
||||
const parts: AddedFile[] = [];
|
||||
const historyRevs = new Set<string>();
|
||||
let trackingRemote: string | null = null;
|
||||
|
||||
for (const fields of refs) {
|
||||
// Fail CLOSED on a ref line we cannot parse (#2498): git hands pre-push
|
||||
@@ -557,20 +605,31 @@ function main() {
|
||||
process.exit(1);
|
||||
}
|
||||
parts.push(...added.filter((part) => part.text.trim()));
|
||||
historyRevs.add(localSha);
|
||||
if (!ZERO.test(remoteSha) && objectExists(remoteSha)) historyRevs.add(remoteSha);
|
||||
else {
|
||||
const target = pushTarget();
|
||||
if (target.kind === "remote") trackingRemote = target.name;
|
||||
}
|
||||
}
|
||||
|
||||
const allHigh: Finding[] = [];
|
||||
const mediums: string[] = [];
|
||||
let emailMedium = false;
|
||||
const emails = parts.length ? emailScanOptions([...historyRevs], trackingRemote) : { options: {} };
|
||||
// Visibility doesn't change HIGH behavior; pass private so nothing is treated
|
||||
// as public-strict (HIGH blocks regardless either way).
|
||||
// Sliced (see scanAddedLines) so a large-but-legitimate diff is actually
|
||||
// scanned rather than blocked unscanned on the engine's size cap.
|
||||
// Scanned per file so a pattern can use the file's type (#3048).
|
||||
for (const part of parts) {
|
||||
const opts = { repoVisibility: "private" as const, ...(part.file ? { sourcePath: part.file } : {}) };
|
||||
const opts = { repoVisibility: "private" as const, ...emails.options, ...(part.file ? { sourcePath: part.file } : {}) };
|
||||
for (const f of scanAddedLines(part.text, opts)) {
|
||||
if (f.severity === "HIGH") allHigh.push(f);
|
||||
else if (f.severity === "MEDIUM") mediums.push(mediumLine(f, part));
|
||||
else if (f.severity === "MEDIUM") {
|
||||
mediums.push(mediumLine(f, part));
|
||||
if (f.id === "pii.email") emailMedium = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -580,6 +639,12 @@ function main() {
|
||||
"Not blocking. Review before this becomes public.\n",
|
||||
);
|
||||
for (const line of mediums) process.stderr.write(line);
|
||||
if (emailMedium && "limited" in emails && emails.limited) {
|
||||
process.stderr.write(
|
||||
`gstack-redact-prepush: existing-email suppression was limited for this push (${emails.limited}), ` +
|
||||
"so an address already in the history may be listed above.\n",
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
if (allHigh.length > 0) {
|
||||
|
||||
@@ -116,7 +116,43 @@ function publishForeignBranch(fx: Fixture, remote: string, branch: string, email
|
||||
git(fx, producer, ["push", "-q", "origin", `HEAD:refs/heads/${branch}`]);
|
||||
}
|
||||
|
||||
describe("MEDIUM findings are listed one per line", () => {
|
||||
describe("own and already-public addresses are not reported (#3060)", () => {
|
||||
test("the pusher's own address (git config user.email) is not reported", () => {
|
||||
const fx = fixture({ selfEmail: "me@corp.io" });
|
||||
commitFile(fx, fx.repo, "AUTHORS.md", "Maintainer: me@corp.io\n", "colleague@corp.io");
|
||||
const { code, stderr } = push(fx, ["origin", "main"]);
|
||||
expect(code).toBe(0);
|
||||
expect(stderr).not.toContain("MEDIUM");
|
||||
});
|
||||
|
||||
test("an address that authored a commit the remote already has is not reported", () => {
|
||||
const fx = fixture();
|
||||
commitFile(fx, fx.repo, "lib.txt", "code\n", "colleague@corp.io");
|
||||
expect(push(fx, ["origin", "main"]).code).toBe(0);
|
||||
commitFile(fx, fx.repo, "CODEOWNERS", "* colleague@corp.io\n");
|
||||
const { code, stderr } = push(fx, ["origin", "main"]);
|
||||
expect(code).toBe(0);
|
||||
expect(stderr).not.toContain("MEDIUM");
|
||||
});
|
||||
|
||||
test("an address that authors one of the pushed commits is not reported", () => {
|
||||
const fx = fixture();
|
||||
commitFile(fx, fx.repo, "package.json", '{ "author": "newcomer@corp.io" }\n', "newcomer@corp.io");
|
||||
const { code, stderr } = push(fx, ["origin", "main"]);
|
||||
expect(code).toBe(0);
|
||||
expect(stderr).not.toContain("MEDIUM");
|
||||
});
|
||||
|
||||
test("mailmapped author addresses count as already public", () => {
|
||||
const fx = fixture();
|
||||
commitFile(fx, fx.repo, "lib.txt", "code\n", "old-name@corp.io");
|
||||
expect(push(fx, ["origin", "main"]).code).toBe(0);
|
||||
fs.writeFileSync(path.join(fx.repo, ".mailmap"), "Someone <current@corp.io> <old-name@corp.io>\n");
|
||||
commitFile(fx, fx.repo, "docs/contact.md", "Ask current@corp.io\n");
|
||||
const { stderr } = push(fx, ["origin", "main"]);
|
||||
expect(stderr).not.toContain("MEDIUM");
|
||||
});
|
||||
|
||||
test("control: a stranger's address is reported with rule, file, line and fix, never the address", () => {
|
||||
const fx = fixture();
|
||||
commitFile(fx, fx.repo, "notes.md", "line one\nline two\ncontact stranger@corp.io\n");
|
||||
@@ -141,6 +177,131 @@ describe("MEDIUM findings are listed one per line", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("history scope follows the push destination (CEO-7, ENG-8)", () => {
|
||||
test("a new branch to a configured remote counts authors on that remote's tracking refs", () => {
|
||||
const fx = fixture();
|
||||
publishForeignBranch(fx, fx.origin, "other", "colleague@corp.io");
|
||||
git(fx, fx.repo, ["fetch", "-q", "origin"]);
|
||||
git(fx, fx.repo, ["checkout", "-q", "-b", "feature"]);
|
||||
commitFile(fx, fx.repo, "CODEOWNERS", "* colleague@corp.io\n");
|
||||
const { code, stderr } = push(fx, ["origin", "feature"]);
|
||||
expect(code).toBe(0);
|
||||
expect(stderr).not.toContain("MEDIUM");
|
||||
});
|
||||
|
||||
test("control: a URL push does not count the tracking refs of a remote that happens to match", () => {
|
||||
const fx = fixture();
|
||||
publishForeignBranch(fx, fx.origin, "other", "colleague@corp.io");
|
||||
git(fx, fx.repo, ["fetch", "-q", "origin"]);
|
||||
git(fx, fx.repo, ["checkout", "-q", "-b", "feature"]);
|
||||
commitFile(fx, fx.repo, "CODEOWNERS", "* colleague@corp.io\n");
|
||||
const { code, stderr } = push(fx, [fx.origin, "feature"]);
|
||||
expect(code).toBe(0);
|
||||
expect(mediumLines(stderr)).toHaveLength(1);
|
||||
});
|
||||
|
||||
test("control: an address only another remote's history holds is still reported", () => {
|
||||
const fx = fixture();
|
||||
const upstream = path.join(fx.root, "upstream.git");
|
||||
git(fx, fx.root, ["init", "--bare", "-q", "-b", "main", upstream]);
|
||||
git(fx, fx.repo, ["push", "-q", upstream, "main"]);
|
||||
publishForeignBranch(fx, upstream, "other", "private-colleague@corp.io");
|
||||
git(fx, fx.repo, ["remote", "add", "upstream", upstream]);
|
||||
git(fx, fx.repo, ["fetch", "-q", "upstream"]);
|
||||
git(fx, fx.repo, ["checkout", "-q", "-b", "feature"]);
|
||||
commitFile(fx, fx.repo, "CODEOWNERS", "* private-colleague@corp.io\n");
|
||||
const { code, stderr } = push(fx, ["origin", "feature"]);
|
||||
expect(code).toBe(0);
|
||||
expect(mediumLines(stderr)).toHaveLength(1);
|
||||
});
|
||||
|
||||
test("a force-push counts authors in the history of the remote tip it replaces", () => {
|
||||
const fx = fixture();
|
||||
commitFile(fx, fx.repo, "draft.txt", "draft\n", "reviewer@corp.io");
|
||||
expect(push(fx, ["origin", "main"]).code).toBe(0);
|
||||
git(fx, fx.repo, ["reset", "-q", "--hard", "HEAD~1"]);
|
||||
commitFile(fx, fx.repo, "CODEOWNERS", "* reviewer@corp.io\n");
|
||||
const { code, stderr } = push(fx, ["--force", "origin", "main"]);
|
||||
expect(code).toBe(0);
|
||||
expect(stderr).not.toContain("MEDIUM");
|
||||
});
|
||||
|
||||
test("hitting the 50,000-commit cap uses the newest authors and stays quiet about it", () => {
|
||||
const fx = fixture({ seed: false });
|
||||
const count = 50_001;
|
||||
const lines: string[] = [];
|
||||
for (let i = 1; i <= count; i++) {
|
||||
const email = i === 1 ? "oldest@corp.io" : "filler@example.com";
|
||||
lines.push("commit refs/heads/main", `mark :${i}`, `committer Someone <${email}> ${1_700_000_000 + i} +0000`, "data 1", "c");
|
||||
if (i > 1) lines.push(`from :${i - 1}`);
|
||||
lines.push("");
|
||||
}
|
||||
const imported = spawnSync("git", ["fast-import", "--quiet"], {
|
||||
cwd: fx.repo, input: lines.join("\n"), encoding: "utf8", env: fx.env, timeout: 120_000,
|
||||
});
|
||||
expect(imported.status).toBe(0);
|
||||
git(fx, fx.repo, ["reset", "-q", "--hard", "main"]);
|
||||
const head = commitFile(fx, fx.repo, "CODEOWNERS", "* oldest@corp.io\n* newest@corp.io\n", "newest@corp.io");
|
||||
const { code, stderr } = runHook(fx, `refs/heads/main ${head} refs/heads/main ${ZERO}\n`, []);
|
||||
expect(code).toBe(0);
|
||||
const found = mediumLines(stderr);
|
||||
expect(found).toHaveLength(1);
|
||||
expect(found[0]).toContain("CODEOWNERS:1");
|
||||
expect(stderr).not.toMatch(limitedLine);
|
||||
}, 120_000);
|
||||
});
|
||||
|
||||
describe("each input fails on its own (CEO-23, DX-9)", () => {
|
||||
/** A `git` that fails or hangs only for `git log`, so every other call is real. */
|
||||
function stubGitLog(fx: Fixture, behavior: "fail" | "hang"): Record<string, string> {
|
||||
const realGit = Bun.which("git");
|
||||
if (!realGit) throw new Error("git not found");
|
||||
const bin = path.join(fx.root, "stub-bin");
|
||||
fs.mkdirSync(bin, { recursive: true });
|
||||
const action = behavior === "fail" ? 'echo "fatal: stub" >&2; exit 128' : "exec sleep 30";
|
||||
fs.writeFileSync(path.join(bin, "git"), `#!/bin/sh\nif [ "$1" = "log" ]; then ${action}; fi\nexec "${realGit}" "$@"\n`, { mode: 0o755 });
|
||||
const pathKey = Object.keys(fx.env).find((k) => k.toLowerCase() === "path") || "PATH";
|
||||
return { [pathKey]: `${bin}${path.delimiter}${fx.env[pathKey] ?? ""}` };
|
||||
}
|
||||
|
||||
function pushedRange(fx: Fixture): string {
|
||||
const base = git(fx, fx.repo, ["rev-parse", "origin/main"]);
|
||||
const head = git(fx, fx.repo, ["rev-parse", "HEAD"]);
|
||||
return `refs/heads/main ${head} refs/heads/main ${base}\n`;
|
||||
}
|
||||
|
||||
test("without user.email, already-public authors are still not reported", () => {
|
||||
const fx = fixture({ selfEmail: null });
|
||||
commitFile(fx, fx.repo, "lib.txt", "code\n", "colleague@corp.io");
|
||||
expect(push(fx, ["origin", "main"]).code).toBe(0);
|
||||
commitFile(fx, fx.repo, "CODEOWNERS", "* colleague@corp.io\n* stranger@corp.io\n", "colleague@corp.io");
|
||||
const { code, stderr } = push(fx, ["origin", "main"]);
|
||||
expect(code).toBe(0);
|
||||
const found = mediumLines(stderr);
|
||||
expect(found).toHaveLength(1);
|
||||
expect(found[0]).toContain("CODEOWNERS:2");
|
||||
});
|
||||
|
||||
for (const behavior of ["fail", "hang"] as const) {
|
||||
test(`a history read that ${behavior === "fail" ? "errors" : "times out"} still honors the pusher's own address and says suppression was limited`, () => {
|
||||
const fx = fixture({ selfEmail: "me@corp.io" });
|
||||
commitFile(fx, fx.repo, "lib.txt", "code\n", "colleague@corp.io");
|
||||
expect(push(fx, ["origin", "main"]).code).toBe(0);
|
||||
git(fx, fx.repo, ["fetch", "-q", "origin"]);
|
||||
commitFile(fx, fx.repo, "CODEOWNERS", "* me@corp.io\n* colleague@corp.io\n", "colleague@corp.io");
|
||||
const started = Date.now();
|
||||
const { code, stderr } = runHook(fx, pushedRange(fx), ["origin", fx.origin], stubGitLog(fx, behavior));
|
||||
expect(Date.now() - started).toBeLessThan(25_000);
|
||||
expect(code).toBe(0);
|
||||
const found = mediumLines(stderr);
|
||||
expect(found).toHaveLength(1);
|
||||
expect(found[0]).toContain("CODEOWNERS:2");
|
||||
expect(stderr.split("\n").filter((l) => limitedLine.test(l))).toHaveLength(1);
|
||||
expect(stderr).toContain(behavior === "fail" ? "reading commit history failed" : "reading commit history took over 5 s");
|
||||
}, 40_000);
|
||||
}
|
||||
});
|
||||
|
||||
describe("MEDIUM lines point at the real file line (ENG-8)", () => {
|
||||
test("separated hunks in one file report each hunk's own line", () => {
|
||||
const fx = fixture();
|
||||
|
||||
Reference in new issue
Block a user