Skip to content

Commit 9f3bbda

Browse files
SK-3156-addressed-review-comments
1 parent 096a98a commit 9f3bbda

2 files changed

Lines changed: 221 additions & 41 deletions

File tree

‎.githooks/pre-commit‎

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@
22
# Two-tier secret-leak guard, run on every local commit.
33
# Tier 1: auto-redact gitleaks findings inside src/_generated_ (driven live
44
# by Rule/gitleaks.toml via scripts/patch-generated-secrets.js, not
5-
# a hand-maintained list) and re-stage that directory. Never blocks
6-
# the commit by itself - it either fixes generated code or leaves
7-
# it untouched for tier 2 to catch.
5+
# a hand-maintained list) and re-stage the files it touched. Never
6+
# blocks the commit by itself - it either fixes generated code or
7+
# leaves it untouched for tier 2 to catch.
88
# Tier 2: run the real gitleaks scan against the staged diff and block on
99
# any finding tier 1 didn't (or couldn't) resolve.
1010
# Installed via the repo's own "prepare" npm script - see package.json.
@@ -13,6 +13,22 @@ set -uo pipefail
1313
repo_root="$(git rev-parse --show-toplevel)"
1414
cd "$repo_root"
1515

16+
generated_dir="src/ _generated_"
17+
18+
# Snapshot what's already staged in the generated dir before tier 1 runs, so
19+
# an intentionally-unstaged, in-progress change there (e.g. mid-way through
20+
# testing a Fern regen) isn't unconditionally swept into this commit. Tier 1
21+
# only edits working-tree files, never the index, so this snapshot stays
22+
# accurate regardless of what it does next.
23+
already_staged_in_generated="$(git diff --cached --name-only -- "$generated_dir" || true)"
24+
25+
git_dir="$(git rev-parse --git-dir)"
26+
touched_file_list="$git_dir/leak-guard-touched-files.txt"
27+
# A list from a previous commit's tier 1 run must never be reused here - if
28+
# tier 1 doesn't run this time (node missing, below), stale entries from
29+
# that earlier run would otherwise get staged again.
30+
rm -f "$touched_file_list"
31+
1632
# git invokes hooks with a leaner PATH than your interactive shell, so a
1733
# Node install managed by nvm/volta/fnm or bundled with an IDE (rather than
1834
# a system package) is often invisible here even though `node` works fine
@@ -38,12 +54,25 @@ else
3854
fi
3955
fi
4056

41-
# Stages the whole generated-code directory rather than just the files tier 1
42-
# touched, since we don't get that list back from the script. Safe in
43-
# practice: src/_generated_ is machine-owned (Fern-generated, never hand
44-
# edited), so there's no legitimate "leave part of it unstaged" case to worry
45-
# about disturbing.
46-
git add -- "src/ _generated_"
57+
# Stages only what tier 1 actually touched this run (recorded to
58+
# $touched_file_list - see recordTouchedFiles in the script) plus whatever
59+
# was already staged above. A file sitting in src/_generated_ that tier 1
60+
# didn't touch and the caller hadn't staged is left alone rather than
61+
# force-added - see the med-severity review finding on the old
62+
# unconditional `git add` of the whole directory.
63+
to_stage=()
64+
if [ -f "$touched_file_list" ]; then
65+
while IFS= read -r f; do
66+
[ -n "$f" ] && to_stage+=("$f")
67+
done < "$touched_file_list"
68+
fi
69+
while IFS= read -r f; do
70+
[ -n "$f" ] && to_stage+=("$f")
71+
done <<< "$already_staged_in_generated"
72+
73+
if [ "${#to_stage[@]}" -gt 0 ]; then
74+
git add -- "${to_stage[@]}"
75+
fi
4776

4877
if ! command -v gitleaks >/dev/null 2>&1; then
4978
echo "[leak-guard] tier 2 skipped: 'gitleaks' binary not found locally."

‎scripts/patch-generated-secrets.js‎

Lines changed: 183 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,16 @@
2525
* (e.g. a plain `import { A, B } from '...'` line) as a leak, which
2626
* this script would then corrupt by "redacting" it. If the self-test
2727
* fails, nothing is touched.
28+
* - Redaction itself never reads a finding's reported "Secret" text. It
29+
* uses gitleaks' (StartLine, StartColumn) only as an approximate
30+
* anchor to locate the surrounding quoted string literal in the
31+
* actual file content, then redacts strictly between its quotes - see
32+
* findQuotedValueSpan. This sidesteps two problems in one move: some
33+
* locally observed gitleaks builds report a "Secret" that swallows a
34+
* trailing quote (would corrupt the surrounding code) or an
35+
* off-by-one column for certain rules, and reading that field at all
36+
* is a source CodeQL's clear-text-logging/storage-sensitive-data
37+
* queries key off of.
2838
* - After redacting, `tsc --noEmit` verifies the generated code still
2939
* compiles. If it doesn't, every change made in this run is rolled
3040
* back and the run fails - a broken build is never left in place
@@ -118,11 +128,87 @@ function placeholderFor(ruleID) {
118128
return `<REDACTED_${ruleID.toUpperCase().replace(/[^A-Z0-9]+/g, "_")}>`;
119129
}
120130

131+
const QUOTE_CHARS = new Set(['"', "'", "`"]);
132+
// How far findQuotedValueSpan will search outward from gitleaks' reported
133+
// (line, column) for an actual quote character in the file. Only needs to
134+
// cover small reporting inaccuracies (a couple of characters), not
135+
// arbitrary distances - see its comment below.
136+
const QUOTE_SEARCH_WINDOW = 8;
137+
138+
// Absolute char offset where each 1-indexed line starts in `text`.
139+
// offsets[line - 1] is the offset of `line`. Used to turn gitleaks'
140+
// 1-indexed (line, column) position into a plain absolute offset into
141+
// `text`.
142+
function lineStartOffsets(text) {
143+
const offsets = [0];
144+
for (const lineText of text.split("\n")) {
145+
offsets.push(offsets[offsets.length - 1] + lineText.length + 1);
146+
}
147+
return offsets;
148+
}
149+
150+
// Finds [valueStart, valueEnd) strictly inside the quoted string literal
151+
// nearest to `approxPos`, or null if there isn't one.
152+
//
153+
// Every flagged value in these generated files is a quoted example (e.g.
154+
// `assertion: "..."`), and gitleaks' reported column can be off by a
155+
// character (observed on the "jwt" rule with at least one locally
156+
// installed gitleaks build) or include a trailing delimiter it shouldn't
157+
// (also observed on "jwt" - the reported Secret text itself included the
158+
// closing quote), so this never trusts the exact position or reads the
159+
// flagged value's own text - it searches a small window in `content`
160+
// around the approximate position for an actual quote character, then
161+
// finds its matching closing quote. Because this only ever looks at
162+
// `content` and plain integer offsets, the redaction below has no
163+
// dependency on gitleaks' "Secret" field at all - not just a fix for the
164+
// trailing-quote bug, but also structurally free of the taint path
165+
// CodeQL's clear-text-logging/storage-sensitive-data queries would
166+
// otherwise follow from that field into console.log()/writeFileSync().
167+
function findQuotedValueSpan(content, approxPos) {
168+
const n = content.length;
169+
for (let delta = 0; delta <= QUOTE_SEARCH_WINDOW; delta++) {
170+
// Backward first: the one observed real-world case (an off-by-one
171+
// StartColumn on the "jwt" rule) landed one character INSIDE the
172+
// value, so the nearest quote is behind it, not ahead of it.
173+
const candidates = delta === 0 ? [approxPos] : [approxPos - delta, approxPos + delta];
174+
for (const pos of candidates) {
175+
if (pos >= 0 && pos < n && QUOTE_CHARS.has(content[pos])) {
176+
const valueStart = pos + 1;
177+
const valueEnd = content.indexOf(content[pos], valueStart);
178+
return valueEnd === -1 ? null : [valueStart, valueEnd];
179+
}
180+
}
181+
}
182+
return null;
183+
}
184+
185+
// Records exactly which files this run modified (an empty list if none),
186+
// so the pre-commit hook can stage only those - plus whatever the caller
187+
// already had staged - instead of the entire generated-code directory,
188+
// which would otherwise sweep in unrelated or intentionally-unstaged
189+
// in-progress changes sitting in that same directory. Called at every exit
190+
// point so the hook never reads a stale list left over from a prior run.
191+
// Best-effort: if this can't be written, the hook simply finds no list and
192+
// stages nothing beyond what was already staged.
193+
function recordTouchedFiles(relativeFiles) {
194+
try {
195+
const gitDir = execFileSync("git", ["rev-parse", "--git-dir"], {
196+
cwd: repoRoot,
197+
encoding: "utf8",
198+
}).trim();
199+
const listPath = path.resolve(repoRoot, gitDir, "leak-guard-touched-files.txt");
200+
fs.writeFileSync(listPath, relativeFiles.map((f) => `${f}\n`).join(""));
201+
} catch {
202+
// ignored - see comment above
203+
}
204+
}
205+
121206
if (!gitleaksAvailable()) {
122207
console.warn(
123208
"gitleaks isn't installed locally - skipping auto-redaction of generated code. " +
124209
"CI will still scan for this.",
125210
);
211+
recordTouchedFiles([]);
126212
process.exit(0);
127213
}
128214

@@ -134,6 +220,7 @@ if (!selfTestAllowlistSupport()) {
134220
"secret. Refusing to auto-redact - please upgrade gitleaks (run `gitleaks version`; " +
135221
"CI uses zricethezav/gitleaks:latest) and try again.",
136222
);
223+
recordTouchedFiles([]);
137224
process.exit(1);
138225
}
139226

@@ -147,6 +234,7 @@ const findings = runGitleaksDetect(
147234

148235
if (findings.length === 0) {
149236
console.log("No gitleaks findings in generated code.");
237+
recordTouchedFiles([]);
150238
process.exit(0);
151239
}
152240

@@ -160,6 +248,7 @@ if (outOfScope.length > 0) {
160248
"Refusing to continue: gitleaks reported findings outside the generated code directory:\n" +
161249
outOfScope.map((f) => ` - ${f.File}`).join("\n"),
162250
);
251+
recordTouchedFiles([]);
163252
process.exit(1);
164253
}
165254

@@ -170,29 +259,80 @@ for (const finding of findings) {
170259
}
171260

172261
const originalContents = new Map();
262+
const touchedFiles = new Set();
173263
let redactedCount = 0;
174264

265+
// Restores every file this run has written so far back to what it read at
266+
// the start. Called from every failure path after files start getting
267+
// written, so a build break, a final-scan tool failure, or leftover
268+
// findings after redaction never leaves a partially-redacted, unverified
269+
// file sitting in the working tree.
270+
function rollback() {
271+
for (const [filePath, content] of originalContents) {
272+
fs.writeFileSync(filePath, content);
273+
}
274+
recordTouchedFiles([]);
275+
}
276+
277+
// Redacts by position - gitleaks' own (StartLine, StartColumn), an
278+
// approximate anchor used only to locate the surrounding quoted string
279+
// literal in each file's content (see findQuotedValueSpan) - and never
280+
// reads finding.Secret at all.
281+
//
282+
// Resolved in a first pass, across ALL files, before anything is written:
283+
// an unresolved finding in a later file must not leave an earlier file's
284+
// already-computed redaction written to disk with no way back.
285+
const fileContents = new Map();
286+
const uniqueSpansByFile = new Map();
287+
const unresolved = [];
175288
for (const [relativeFile, fileFindings] of byFile) {
176289
const filePath = path.join(repoRoot, relativeFile);
177-
let content = fs.readFileSync(filePath, "utf8");
178-
originalContents.set(filePath, content);
290+
const content = fs.readFileSync(filePath, "utf8");
291+
fileContents.set(filePath, content);
292+
293+
const lineOffsets = lineStartOffsets(content);
294+
const spans = [];
295+
for (const finding of fileFindings) {
296+
const approxPos = lineOffsets[finding.StartLine - 1] + (finding.StartColumn - 1);
297+
const span = findQuotedValueSpan(content, approxPos);
298+
if (span === null) {
299+
unresolved.push([relativeFile, finding]);
300+
continue;
301+
}
302+
spans.push([span[0], span[1], finding.RuleID]);
303+
}
304+
305+
// Deduplicate by [start, end]: if two rules flag the exact same span, it
306+
// must only be redacted once. Applied back-to-front (highest offset
307+
// first) so replacing a later span never shifts the offsets of an
308+
// earlier one still waiting to be processed.
309+
const seenSpans = new Map();
310+
for (const [start, end, ruleID] of spans) seenSpans.set(`${start}:${end}`, [start, end, ruleID]);
311+
const uniqueSpans = [...seenSpans.values()].sort((a, b) => b[0] - a[0] || b[1] - a[1]);
312+
uniqueSpansByFile.set(filePath, uniqueSpans);
313+
}
179314

180-
// Assign a distinct placeholder per unique secret value, numbering only
181-
// when the same rule fires more than once in the same file (so two
182-
// different example tokens don't collapse into one identical placeholder).
183-
// Longest-first ordering avoids a rare but real hazard: if one finding's
184-
// secret text happened to be a substring of another's, redacting the
185-
// shorter one first would consume part of the longer one, and the later
186-
// `content.includes(secret)` check for it would then (correctly) come up
187-
// empty. That finding would just be silently skipped here - which is fine,
188-
// because the post-redaction gitleaks re-scan below still catches any
189-
// secret that didn't actually get replaced and fails the run for review.
190-
const uniqueSecrets = [...new Set(fileFindings.map((f) => f.Secret))].sort(
191-
(a, b) => b.length - a.length,
315+
if (unresolved.length > 0) {
316+
console.error(
317+
"Refusing to continue: couldn't locate a quoted value near " +
318+
`${unresolved.length} finding(s) - this script only knows how to redact ` +
319+
'`key: "value"`-shaped examples. Manual review needed:\n' +
320+
unresolved.map(([rel, f]) => ` - [${f.RuleID}] ${rel}:${f.StartLine}`).join("\n"),
192321
);
322+
recordTouchedFiles([]);
323+
process.exit(1);
324+
}
325+
326+
for (const [filePath, spans] of uniqueSpansByFile) {
327+
const relativeFile = path.relative(repoRoot, filePath);
328+
let content = fileContents.get(filePath);
329+
originalContents.set(filePath, content);
330+
331+
// Assign a distinct placeholder per span, numbering only when the same
332+
// rule fires more than once in the same file (so two different example
333+
// values don't collapse into one identical placeholder).
193334
const byRule = new Map();
194-
for (const secret of uniqueSecrets) {
195-
const ruleID = fileFindings.find((f) => f.Secret === secret).RuleID;
335+
for (const [start, end, ruleID] of spans) {
196336
const base = placeholderFor(ruleID);
197337
const seen = byRule.get(base) || 0;
198338
byRule.set(base, seen + 1);
@@ -203,21 +343,20 @@ for (const [relativeFile, fileFindings] of byFile) {
203343
// even though there's only ever one to begin with here.
204344
const placeholder = seen === 0 ? base : `${base.slice(0, -1)}_${seen + 1}>`;
205345

206-
if (content.includes(secret)) {
207-
content = content.split(secret).join(placeholder);
208-
redactedCount += 1;
209-
console.log(`[${ruleID}] redacted in ${relativeFile} -> ${placeholder}`);
210-
}
346+
content = content.slice(0, start) + placeholder + content.slice(end);
347+
redactedCount += 1;
348+
console.log(`[${ruleID}] redacted in ${relativeFile} -> ${placeholder}`);
211349
}
212350

213351
fs.writeFileSync(filePath, content);
352+
if (content !== originalContents.get(filePath)) {
353+
touchedFiles.add(relativeFile);
354+
}
214355
}
215356

216357
const localTsc = path.join(repoRoot, "node_modules", ".bin", "tsc");
217358
if (!fs.existsSync(localTsc)) {
218-
for (const [filePath, content] of originalContents) {
219-
fs.writeFileSync(filePath, content);
220-
}
359+
rollback();
221360
console.error(
222361
`${localTsc} not found (run \`npm install\`) - can't verify redaction is safe, so rolling back and refusing to redact.`,
223362
);
@@ -236,29 +375,41 @@ try {
236375
}
237376

238377
if (!buildOk) {
239-
for (const [filePath, content] of originalContents) {
240-
fs.writeFileSync(filePath, content);
241-
}
378+
rollback();
242379
console.error(
243380
"Rolled back. Redaction needs manual review - see the tsc output above.",
244381
);
245382
process.exit(1);
246383
}
247384

248-
const remaining = runGitleaksDetect(
249-
GENERATED_CODE_PREFIX.replace(/\/$/, ""),
250-
repoRoot,
251-
);
385+
// The redaction itself, and this verification re-scan, can each fail for
386+
// reasons other than "still leaks" (e.g. the gitleaks binary crashing
387+
// mid-run) - runGitleaksDetect throws in that case rather than returning a
388+
// findings list. Either way, an unverified redaction must never be left on
389+
// disk: roll back exactly as the tsc-failure path above does.
390+
let remaining;
391+
try {
392+
remaining = runGitleaksDetect(GENERATED_CODE_PREFIX.replace(/\/$/, ""), repoRoot);
393+
} catch (err) {
394+
rollback();
395+
console.error(
396+
`Final gitleaks re-scan failed to run (${err.message}) - rolling back all changes from this run. Redaction needs manual review.`,
397+
);
398+
process.exit(1);
399+
}
252400
if (remaining.length > 0) {
401+
rollback();
253402
console.error(
254-
`Redacted ${redactedCount} secret(s), but ${remaining.length} finding(s) remain after re-scanning. Manual review needed:\n` +
403+
`Redacted ${redactedCount} secret(s), but ${remaining.length} finding(s) remain after re-scanning. ` +
404+
"Rolled back all changes from this run. Manual review needed:\n" +
255405
remaining
256406
.map((f) => ` - [${f.RuleID}] ${f.File}:${f.StartLine}`)
257407
.join("\n"),
258408
);
259409
process.exit(1);
260410
}
261411

412+
recordTouchedFiles([...touchedFiles]);
262413
console.log(
263414
`\nDone. Redacted ${redactedCount} secret(s) across ${byFile.size} file(s). Build and gitleaks re-scan both clean.`,
264415
);

0 commit comments

Comments
 (0)