fix: pre-landing review fixes (env.kv takes a leading - before indentation or export)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
garrytanandcapy-ai[bot] committed 2026-10-31 18:15:53 +00:00
1 parent 26416342fa
commit e90988d0a1
3 files changed
+20 -13

No files matched your search

+3 -3
View File
@@ -1,5 +1,5 @@
**`gstack-redact` now catches a credential on an added diff line (GST-543).** The env.kv rule matched from the start of the line. On a unified-diff added line, the leading `+` blocked the match, so `git diff | gstack-redact` returned no finding and exit 0 for `+API_TOKEN=<random value>`. A `diff` code block in a PR body had the same blind spot. Pregate's `secrets` check and the pre-push hook were not affected, because both strip the `+` before scanning.
**`gstack-redact` now catches a credential on a diff line (GST-543).** The env.kv rule matched from the start of the line, so on a unified diff the leading `+` or `-` stopped the match. `git diff | gstack-redact` returned no finding and exit 0 for `+API_TOKEN=<random value>`, and for a removed line that was indented or used `export`. A `diff` code block in a PR body had the same blind spot. Pregate's `secrets` check and the pre-push hook were not affected, because both strip the `+` before scanning.
### Fixed
- **`lib/redact-patterns.ts`:** env.kv accepts one leading `+`, so the line in the example above now reports MEDIUM env.kv and exits 2. The name-shape check and the schema-field exemption accept the same prefix, so `API_TOKEN=changeme`, `cacheKey: …` and `input_key: …` stay clean on an added line, as they are without the prefix.
- **`test/redact-diff-prefix.test.ts`:** each pattern in `PATTERNS` has a sample. The test requires it to report bare, behind `+`, `-` and a space, and on an added line inside a real hunk. A new pattern without a sample fails the test. The audit found env.kv was the only rule with the leading-`+` blind spot. Every other rule starts at a word boundary or has no anchor.
- **`lib/redact-patterns.ts`:** env.kv accepts one leading `+` or `-`, so the line in the example above now reports MEDIUM env.kv and exits 2. The name-shape check and the schema-field exemption accept the same marker. As a result, `API_TOKEN=changeme`, `cacheKey: …` and `input_key: …` stay clean on a diff line, as they are without the marker. An unindented YAML list item (`- api_token: <random value>`) now reports too, because it has the same shape as a removed line.
- **`test/redact-diff-prefix.test.ts`:** each pattern in `PATTERNS` has a sample. The test requires it to report bare, behind `+`, `-` and a space, and on an added or removed line inside a real hunk, indented or not. A new pattern without a sample fails the test. The audit found env.kv was the only rule with this blind spot. Every other rule starts at a word boundary or has no anchor.
+9 -8
View File
@@ -121,7 +121,7 @@ export function shannonEntropy(s: string): number {
// (apiKey, authToken, clientSecret, stripeApiKey) — cacheKey/sortKey/
// monkey have no credential prefix and are rejected.
const ENV_KV_NAME =
/^\+?[ \t]*(?:export[ \t]+)?["']?([A-Za-z0-9_.-]*?(?:KEY|TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIALS?|DSN|AUTH|COOKIE|SESSION|PRIVATE))["']?[ \t]*[:=]/i;
/^[+-]?[ \t]*(?:export[ \t]+)?["']?([A-Za-z0-9_.-]*?(?:KEY|TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIALS?|DSN|AUTH|COOKIE|SESSION|PRIVATE))["']?[ \t]*[:=]/i;
const ENV_KV_SUFFIX =
/(KEY|TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIALS?|DSN|AUTH|COOKIE|SESSION|PRIVATE)$/i;
const ENV_KV_CRED_PREFIX =
@@ -656,7 +656,7 @@ const CALL_SHAPED_VALUE = /^[A-Za-z_$][\w$]*(?:\.[A-Za-z_$][\w$]*)*\(/;
* A random key is mixed-case or one unbroken run, so it never takes this shape.
*/
const SCHEMA_FIELD_NAME =
/^\+?[ \t]*["']?(?:input|output|field|column|lookup|cache|sort|partition|range|index|row|group|join|property|metric|catalog|schema|translation|idempotency|dedup)[_.-]key["']?[ \t]*[:=]/;
/^[+-]?[ \t]*["']?(?:input|output|field|column|lookup|cache|sort|partition|range|index|row|group|join|property|metric|catalog|schema|translation|idempotency|dedup)[_.-]key["']?[ \t]*[:=]/;
const SCHEMA_FIELD_VALUE = /^[a-z][a-z0-9]*(?:[_.-][a-z][a-z0-9]*)+$/;
function isSchemaFieldIdentifier(span: string, match: RegExpExecArray): boolean {
return SCHEMA_FIELD_NAME.test(match[0]) && SCHEMA_FIELD_VALUE.test(span);
@@ -962,13 +962,14 @@ export const PATTERNS: RedactPattern[] = [
// (cacheKey:, sortKey:, monkey:) fires a MEDIUM confirm on entropic
// values. The value must stay capture group 1 (the engine masks group 1),
// so name-shape checking lives in validate, not in a second group.
// GST-543: an optional leading `+` is a unified-diff added line
// GST-543: an optional leading `+` or `-` is a unified-diff line marker
// (`git diff | gstack-redact`, a diff fence in a PR body); without it the
// line anchor stopped at the prefix and an added credential went
// unreported. ENV_KV_NAME and SCHEMA_FIELD_NAME take the same prefix so
// the name-shape and schema exemptions hold on an added line. `-` and a
// space already match (`-` as a name character, a space as indentation).
regex: /^\+?[ \t]*(?:export[ \t]+)?["']?[A-Za-z0-9_.-]*(?:KEY|TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIALS?|DSN|AUTH|COOKIE|SESSION|PRIVATE)["']?[ \t]*[:=][ \t]*["']?([^\s'"]{8,})["']?/i,
// line anchor stopped at the marker, so an added or removed credential
// went unreported (`-` alone matched only as a name character, never
// before indentation or `export`). ENV_KV_NAME and SCHEMA_FIELD_NAME take
// the same marker so the name-shape and schema exemptions hold on a diff
// line.
regex: /^[+-]?[ \t]*(?:export[ \t]+)?["']?[A-Za-z0-9_.-]*(?:KEY|TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIALS?|DSN|AUTH|COOKIE|SESSION|PRIVATE)["']?[ \t]*[:=][ \t]*["']?([^\s'"]{8,})["']?/i,
// Only fire on credential-shaped names with high-entropy values — kills
// `FOO_KEY=changeme` and `cacheKey: <entropic-id>` FPs. #2912: a value
// that is exactly an environment read (`os.environ["X"]`,
+8 -2
View File
@@ -73,12 +73,18 @@ describe("GST-543: a diff line prefix never hides a finding", () => {
const missed: string[] = [];
for (const p of PATTERNS) {
const s = SAMPLES[p.id]!;
for (const [label, text] of [["+", "+" + s], ["-", "-" + s], ["space", " " + s], ["hunk", hunk("+" + s)], ["indented hunk", hunk("+ " + s)]] as const)
for (const [label, text] of [["+", "+" + s], ["-", "-" + s], ["space", " " + s], ["hunk", hunk("+" + s)], ["indented hunk", hunk("+ " + s)], ["indented removed", hunk("- " + s)], ["tab removed", hunk("-\t" + s)]] as const)
if (!ids(text).includes(p.id)) missed.push(`${p.id} (${label})`);
}
expect(missed).toEqual([]);
});
test("env.kv reports an exported or indented credential on an added or removed line", () => {
const v = SAMPLES["env.kv"]!.slice("API_TOKEN=".length);
for (const line of [`-export API_TOKEN=${v}`, `+export API_TOKEN=${v}`, `- api_token: ${v}`, `- api_token: ${v}`])
expect({ line, kv: ids(hunk(line)).includes("env.kv") }).toEqual({ line, kv: true });
});
test("git diff | gstack-redact reports an added env.kv credential and exits 2; a placeholder on the same line stays clean", () => {
const run = (line: string) => spawnSync("bun", [path.join(import.meta.dir, "..", "bin", "gstack-redact"), "--json"], { input: hunk("+" + line), encoding: "utf8", timeout: 30_000 });
const real = run(SAMPLES["env.kv"]!);
@@ -89,7 +95,7 @@ describe("GST-543: a diff line prefix never hides a finding", () => {
test("the prefix changes nothing else: env.kv's exemptions hold on an added line", () => {
for (const line of ["API_TOKEN=changeme", "cacheKey: 8Fk2pQ9vXz4wL7mN3rT6yB1cD5eG0hJ", "input_key: expected_media_rate_bits_per_second", 'api_key=os.environ["API_KEY"]', "TOKEN=${API_TOKEN}"]) {
expect({ line, bare: ids(line).includes("env.kv") }).toEqual({ line, bare: false });
expect({ line, added: ids(hunk("+" + line)).includes("env.kv") }).toEqual({ line, added: false });
for (const marker of ["+", "-", "+ ", "- "]) expect({ line: marker + line, kv: ids(hunk(marker + line)).includes("env.kv") }).toEqual({ line: marker + line, kv: false });
}
});
});