mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-10 10:40:32 +01:00
fix(autoplan): keep compact renames and leave out both sides of a secret file renamed or copied on the branch (GST-1164)
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
f1e40af14b
commit
f8571f33d5
4 files changed
+23
-14
No files matched your search
@@ -1,9 +1,9 @@
|
||||
**Security: the lean /autoplan reviewer no longer gets `.envrc`, `.env-production` or a key pasted into an ordinary file (GST-1164, #3914).** Since v1.93.36.0 (#3897), the lean run puts the branch's draft code into a prompt that goes to an outside model provider. It left out only some secret-named untracked files: `.env` and `.env.*`, but not `.envrc` or `.env-production`. It applied no name rule to tracked files and no content check at all. So a direnv `.envrc`, a `.env-production`, a committed or edited secret file, or a key sitting in any source file could reach the provider.
|
||||
|
||||
### Fixed
|
||||
- **`lib/autoplan-lean.ts` `captureDraft`:** the secret-name rule now covers the whole `.env*` family, `.envrc` included, every `*credentials*` name, `_netrc`, `.pgpass`, `*.jks` and `id_*_sk` keys. Matching files are never diffed, tracked or untracked, and the draft names each one it left out. Renames are listed as a delete and an add, so a secret file renamed on the branch stays out under both names. Every other file's patch goes through gstack's redact engine (`redactFindingSpans`), so a key in an ordinary file reaches the reviewer as `<REDACTED-…>`. A patch the engine can't redact safely, including one over 1 MiB, is left out with a note.
|
||||
- **`lib/autoplan-lean.ts` `captureDraft`:** the secret-name rule now covers the whole `.env*` family, `.envrc` included, every `*credentials*` name, `_netrc`, `.pgpass`, `*.jks` and `id_*_sk` keys. Matching files are never diffed, tracked or untracked, and the draft names each one it left out. A secret-named file renamed or copied on the branch stays out under both names, even when the new name looks ordinary. Every other file's patch goes through gstack's redact engine (`redactFindingSpans`), so a key in an ordinary file reaches the reviewer as `<REDACTED-…>`. A patch the engine can't redact safely, including one over 1 MiB, is left out with a note.
|
||||
|
||||
### Tests
|
||||
- `test/autoplan-lean.test.ts`: an untracked `.envrc`, `.env-production` and six other secret files stay out of `draft.diff` and the prompt; a committed and edited `.envrc` stays out of the tracked diff, and so does one renamed to `.envrc.local`; a fake key in an untracked file and in a working-tree edit to a tracked file comes out redacted. All four fail on main at b42ad1e.
|
||||
- `test/autoplan-lean.test.ts`: an untracked `.envrc`, `.env-production` and six other secret files stay out of `draft.diff` and the prompt; a committed and edited `.envrc` stays out of the tracked diff, and so do an `.envrc` renamed to `.envrc.local` and a `.pgpass` renamed to `pg.txt`; a fake key in an untracked file and in a working-tree edit to a tracked file comes out redacted. All four fail on main at b42ad1e.
|
||||
|
||||
Thanks to the /ship pre-landing review of #3901, which filed the finding.
|
||||
+2
-1
@@ -389,7 +389,8 @@ The example above is the full run. The first `next` also takes `--kind`:
|
||||
looks like a secret (the whole `.env*` family, `.envrc` included, `*.pem`,
|
||||
`*.key`, `*.p12`, `id_rsa` and the other SSH private keys, `.netrc`,
|
||||
`.npmrc`, `.pgpass`, `*credentials*`, `*secret*`) is never read, tracked or
|
||||
untracked, and is left out with a note. Every other file's patch goes
|
||||
untracked, and is left out with a note; so is a file renamed or copied from
|
||||
one on the branch. Every other file's patch goes
|
||||
through the redact engine (`lib/redact-engine.ts`), so a key in an ordinary
|
||||
file reaches the reviewer as `<REDACTED-…>`; a patch it can't redact safely
|
||||
is left out with a note. The `DRAFT:` line names the checkout it diffed (`root=`). The prompt inlines the diff in a fence longer than any tilde
|
||||
|
||||
+12
-6
@@ -75,21 +75,27 @@ export function captureDraft(cwd: string, dir: string, out: string, base?: strin
|
||||
const inRun = (f: string) => !runDir.startsWith('..') && !path.isAbsolute(runDir) && (runDir === '' || f === runDir || f.startsWith(`${runDir}/`));
|
||||
const failed = (r: ReturnType<typeof git>, what: string, ok = [0]) => r.error || r.status === null || !ok.includes(r.status)
|
||||
? `draft capture failed: ${what} ${r.error ? `errored (${r.error.message})` : `exited ${r.status ?? r.signal}`}${r.stderr?.trim() ? `: ${r.stderr.trim().split('\n')[0]}` : ''}` : undefined;
|
||||
const raw = ['diff', '--no-color', '--no-ext-diff', '--no-textconv', '--no-renames'];
|
||||
const names = git(['diff', '--name-only', '-z', '--no-renames', mergeBase, '--'], root);
|
||||
const raw = ['diff', '--no-color', '--no-ext-diff', '--no-textconv'];
|
||||
const names = git(['diff', '--name-status', '-z', '-M', '-C', mergeBase, '--'], root);
|
||||
const others = git(['ls-files', '--others', '--exclude-standard', '-z'], root);
|
||||
const listed = failed(names, 'git diff') ?? failed(others, 'git ls-files');
|
||||
if (listed) return { base: ref, none: listed };
|
||||
const changed = names.stdout.split('\0').filter(Boolean);
|
||||
const secretTracked = changed.filter(f => SECRET_NAME.test(f));
|
||||
const tracked = git([...raw, mergeBase, '--', '.', ...secretTracked.map(f => `:(exclude,literal)${f}`)], root);
|
||||
const fields = names.stdout.split('\0');
|
||||
const changed: string[] = [];
|
||||
const secretTracked: string[][] = [];
|
||||
for (let i = 0; fields[i];) {
|
||||
const entry = /^[RC]/.test(fields[i++]!) ? [fields[i++]!, fields[i++]!] : [fields[i++]!];
|
||||
changed.push(...entry);
|
||||
if (entry.some(f => SECRET_NAME.test(f))) secretTracked.push(entry);
|
||||
}
|
||||
const tracked = git([...raw, '-M', mergeBase, '--', '.', ...secretTracked.flat().map(f => `:(exclude,literal)${f}`)], root);
|
||||
const broken = failed(tracked, 'git diff');
|
||||
if (broken) return { base: ref, none: broken };
|
||||
const redacted = (patch: string, what: string) => redactFindingSpans(patch) ?? `# draft: ${what} left out (it could not be redacted safely)\n`;
|
||||
const untracked = others.stdout.split('\0').filter(f => f && !inRun(f)).sort();
|
||||
const parts = [
|
||||
...tracked.stdout.split(/^(?=diff --git )/m).map(p => redacted(p, p.slice(0, p.indexOf('\n')))),
|
||||
...secretTracked.map(f => `# draft: ${f} left out (its name looks like a secret)\n`),
|
||||
...secretTracked.map(e => `# draft: ${e.join(' -> ')} left out (${e.length > 1 ? 'a rename or copy of a file whose name looks like a secret' : 'its name looks like a secret'})\n`),
|
||||
];
|
||||
for (const f of untracked.slice(0, UNTRACKED_MAX)) {
|
||||
if (SECRET_NAME.test(f)) { parts.push(`# draft: untracked ${f} left out (its name looks like a secret)\n`); continue; }
|
||||
|
||||
@@ -430,21 +430,23 @@ describe('draft capture edges', () => {
|
||||
const cwd = repo('rename-envrc', false);
|
||||
git(cwd, 'checkout', '-q', 'main');
|
||||
fs.writeFileSync(path.join(cwd, '.envrc'), 'dotenv_if_exists marker-zz9-old\n');
|
||||
git(cwd, 'add', '.envrc');
|
||||
fs.writeFileSync(path.join(cwd, '.pgpass'), 'db.example.com:5432:app:bot:marker-zz9-pg\n');
|
||||
git(cwd, 'add', '.envrc', '.pgpass');
|
||||
git(cwd, 'commit', '-q', '-m', 'direnv');
|
||||
git(cwd, 'checkout', '-q', 'feature');
|
||||
git(cwd, 'merge', '-q', 'main');
|
||||
git(cwd, 'mv', '.envrc', '.envrc.local');
|
||||
fs.appendFileSync(path.join(cwd, '.envrc.local'), 'dotenv_if_exists marker-zz9-new\n');
|
||||
fs.appendFileSync(path.join(cwd, 'lib', 'worker.ts'), 'export const JITTER = 0.1;\n');
|
||||
git(cwd, 'mv', '.pgpass', 'pg.txt');
|
||||
git(cwd, 'mv', 'lib/worker.ts', 'lib/retry.ts');
|
||||
const { ps, prompt } = lean('rename-envrc', cwd);
|
||||
const diff = fs.readFileSync(path.join(ps.snapshot_dir, 'draft.diff'), 'utf8');
|
||||
for (const text of [diff, prompt]) {
|
||||
expect(text).not.toContain('marker-zz9');
|
||||
expect(text).toContain('# draft: .envrc left out (its name looks like a secret)');
|
||||
expect(text).toContain('# draft: .envrc.local left out (its name looks like a secret)');
|
||||
expect(text).toContain('# draft: .envrc -> .envrc.local left out (a rename or copy of a file whose name looks like a secret)');
|
||||
expect(text).toContain('# draft: .pgpass -> pg.txt left out (a rename or copy of a file whose name looks like a secret)');
|
||||
}
|
||||
expect(diff).toContain('+export const JITTER = 0.1;');
|
||||
expect(diff).toContain('rename from lib/worker.ts\nrename to lib/retry.ts');
|
||||
});
|
||||
|
||||
test('a key inside an ordinary file is redacted, untracked or edited in the working tree', () => {
|
||||
|
||||
Reference in new issue
Block a user