diff --git a/bin/gstack-approvals b/bin/gstack-approvals index be13d01f2..3db93bf07 100755 --- a/bin/gstack-approvals +++ b/bin/gstack-approvals @@ -6,7 +6,7 @@ * [--pending ] [--outcome approved|rejected|pending|standing] * [--date YYYY-MM-DD] [--source ] [--branch ] [--json] * gstack-approvals import --autoplan [--by ] [--branch | --stack ] [--json] - * gstack-approvals render [--no-heading] [--full] [--branch ] [--plan ] + * gstack-approvals render [--no-heading] [--full | --redacted] [--branch ] [--plan ] * gstack-approvals plan-check --plan * gstack-approvals rule --finding --head --reason --words --by [--json] * gstack-approvals rulings [--branch ] @@ -48,9 +48,11 @@ const HELP = `gstack-approvals — the per-thread approval record (capy/skill/gs the owner's grant that a CI-carried lane counts although exactly those files are in no run's tree.json (docs/ship-policy.md#test-lane-exceptions); gstack-evidence record-ci binds it only at that head, for that exact set, after a local preflight of those files passes; refused inside this checkout's open /ship run - render [--no-heading] [--full] the ## Approvals section for a PR body or report; a public repository, or one whose - visibility gh cannot read, gets only "Approved: yes|no (record )", because a PR description keeps + render [--no-heading] [--full | --redacted] the ## Approvals section for a PR body or report; a public repository, or one + whose visibility gh cannot read, gets only "Approved: yes|no (record )", because a PR description keeps its edit history; --full prints the rows when gh cannot read the visibility and you know the repo is private; + --redacted prints the public form on any repository (a private repo that may go public); the two conflict; + the rows show each local path in Asked, Covers and Still pending as …/ (list keeps the path); --plan "" adds that line and its check, which gstack-ship-queue ready reads (lib/plan-record.ts); redacted, an approval prints as "Plan: approved (record r-)" with no run or row id, and one that does not resolve refuses with no output @@ -78,7 +80,7 @@ function parse(args: string[]): { flags: Record; bools: Set | exempt: | required"; ${JSON.stringify(flags['--plan'])} is none of them\n`); return EXIT.usage; } const visibility = repoVisibility(process.cwd()); - const mode = renderMode(visibility, bools.has('--full')); + const mode = renderMode(visibility, bools.has('--full'), bools.has('--redacted')); const res = decl ? resolvePlan(decl, { cwd: process.cwd(), env: process.env }) : null; if (decl && res && !res.ok && mode === 'redacted' && decl.kind === 'approved') { return fail(cmd, { code: res.failure.code, message: `${res.failure.message}; on a public or unknown-visibility repository render prints no Plan line until it resolves; fix: ${res.failure.fix}` }); } - if (mode === 'redacted') process.stderr.write(`APPROVALS_RENDER: redacted (repository visibility ${visibility}${bools.has('--full') ? '; --full applies only to a private repository' : ''}); gstack-approvals list shows the full record\n`); + if (mode === 'redacted') process.stderr.write(`APPROVALS_RENDER: redacted (${bools.has('--redacted') ? '--redacted; ' : ''}repository visibility ${visibility}${bools.has('--full') ? '; --full applies only to a private repository' : ''}); gstack-approvals list shows the full record\n`); process.stdout.write(renderRecord(read, loc, { heading: !bools.has('--no-heading'), mode })); if (decl && res) { const line = mode === 'redacted' ? publicPlanLine(decl, res) : decl.line; diff --git a/changelog.d/keep-local-paths-out-of-approvals-render.md b/changelog.d/keep-local-paths-out-of-approvals-render.md new file mode 100644 index 000000000..d8ecc4417 --- /dev/null +++ b/changelog.d/keep-local-paths-out-of-approvals-render.md @@ -0,0 +1,4 @@ +--- +important: security +--- +- **`gstack-approvals render` keeps local paths out of the approvals table in a PR body, and `--redacted` prints the public form on any repository.** On a private repository the full table printed each row's Covers text as recorded, so a spend-cap row carried its ledger path (`/home//.capy/drive/…`) into the PR body, where gstack-post flagged it as `internal.user_path` and the PR's edit history kept it after the repository went public (gstack-evals#289). The table now shows each local path in the Asked, Covers and Still pending columns as `…/`, and a bare home directory as `~`: absolute and home paths (`~/`, `~user/`, `$HOME/`, `/home/…`, `/Users/…`), Windows-drive, UNC and `file://` paths, and Drive-relative ones (`.capy/drive/…`). Exact words stays as the owner wrote it, and `gstack-approvals list` still shows the full record. `render --redacted` prints the same bytes a public repository gets, for a private repository that may go public, and `--redacted` with `--full` refuses with a usage error instead of letting one win. The redacted line now ends "The full approval record stays private." without naming a reason, so it stays true when `--redacted` is used on a private repository. diff --git a/docs/skills.md b/docs/skills.md index bc796c564..cd72a8fb4 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -705,7 +705,7 @@ A lot of branches die when the interesting work is done and only the boring rele 1. **`## What changed`**: one to three captioned before/after images from `gstack-pr-shots` or a UI screenshot, highest-signal first, with a collapsed text version under each. 2. **`## Why`**: two or three sentences. The first states the problem in the person's words (the issue, the message, the failing run), then the cause and the user-visible effect. 3. **`## Blast radius`**: what else the change touches and the one fact that makes that safe, with the evidence-ladder rung that fact reached (a test that exercises the shared path, a grep over every caller, a measured run). A change to a shared helper, a generated file, a selection list or a policy default always has one; a leaf fix says so in one line. -`## Approvals`, the output of `gstack-approvals render` verbatim, goes inside Agent notes. On a private repository that is the table; on a public repository, or one whose visibility gh cannot read, it is only `Approved: yes|no (record )`, because GitHub keeps every earlier version of a PR description. +`## Approvals`, the output of `gstack-approvals render` verbatim, goes inside Agent notes. On a private repository that is the table; on a public repository, or one whose visibility gh cannot read, it is only `Approved: yes|no (record )`, because GitHub keeps every earlier version of a PR description. The table shows each local path in Asked, Covers and Still pending as `…/`, so the home directory, user name and Drive folder in those paths stay out of the body; Exact words stays verbatim, and `gstack-approvals list` keeps the full path. For a private repository that may go public, `render --redacted` prints the public form, byte for byte; `--redacted` with `--full` is refused. Before the body is composed, `gstack-prose-lint` reads the Why, Summary and Blast radius drafts (`--sections 'Why,Summary,Blast radius' --gate`) and the CHANGELOG entry (`--base origin/ --gate`), and a `PROSE_LINT_FOUND` line names the sentence to rewrite; the rest of the body is advisory. The Captain's merge-ask template (`captain/templates/merge-ask.md`) carries the same sections, so a READY report and its PR say the same thing. diff --git a/lib/approval-record.ts b/lib/approval-record.ts index 3cd300e9f..e0649dc9d 100644 --- a/lib/approval-record.ts +++ b/lib/approval-record.ts @@ -246,27 +246,49 @@ export function repoVisibility(cwd: string, env: Env = process.env): RepoVisibil } /** - * Full rows only where the repository is known private, or --full asks for them and GitHub does not report it public. + * Full rows only where the repository is known private, or --full asks for them and GitHub does not report it public; + * --redacted forces the redacted block anywhere, for a private repository that may go public. * A PR description keeps every earlier version in its edit history, so a public or unknown repository gets the * redacted block: whether anything was approved and an opaque id of the record (a hash of its row ids), nothing else. */ -export function renderMode(visibility: RepoVisibility, full: boolean): 'full' | 'redacted' { +export function renderMode(visibility: RepoVisibility, full: boolean, redacted = false): 'full' | 'redacted' { + if (redacted) return 'redacted'; return visibility === 'private' || (full && visibility === 'unknown') ? 'full' : 'redacted'; } +const LOCAL_PATH = /(?]+|(?:~[\w.-]*|\$HOME|\$\{HOME\}|%USERPROFILE%|[A-Za-z]:)[\\/][^\s|,;'"()<>]+|\\\\[^\s\\|,;'"()<>]+\\[^\s|,;'"()<>]+|(?:[\w.-]+[\\/])*\.capy[\\/]drive[\\/][^\s|,;'"()<>]+|\/[^\s/\\|,;'"()<>]+\/[^\s|,;'"()<>]*)/gi; +const HOME_DIR = /^(?:(?:~[\w.-]*|\$HOME|\$\{HOME\}|%USERPROFILE%)[\\/]?|\/(?:home|Users)\/[^/]+\/?|[A-Za-z]:[\\/]Users[\\/][^\\/]+[\\/]?)$/i; + +/** + * A table cell with each local path replaced by `…/` (a bare home directory by `~`): absolute, home + * (`~`, `~user`, `$HOME`, `%USERPROFILE%`), Windows-drive, UNC and `file:` paths, also glued onto a `:` or `-` (a `://` scheme prefix is never a path), and Drive-relative ones + * (`.capy/drive/…`). The file name tells a reader what the approval counted; the directories above it name the + * machine, the user and the Drive scope (`project-user--`). A path cut by a space keeps the words + * after the space. `gstack-approvals list` keeps the full path. + */ +export function pathLabels(text: string): string { + return text.replace(LOCAL_PATH, m => { + const tail = /[.:]*$/.exec(m)![0]; + const p = m.slice(0, m.length - tail.length).replace(/^file:\/*(?:localhost)?(?=\/)/i, ''); + if (HOME_DIR.test(p)) return '~' + tail; + const name = p.split(/[\\/]/).filter(Boolean).pop(); + return name ? `…/${name}${tail}` : m; + }); +} + export function renderRecord(read: ReadResult, loc: StoreLocation, o: { heading?: boolean; mode?: 'full' | 'redacted' } = {}): string { const head = o.heading === false ? [] : ['## Approvals', '']; if (o.mode === 'redacted' && read.rows.length) { const approved = read.rows.some(r => r.outcome === 'approved' || r.outcome === 'standing'); const id = 'r-' + createHash('sha256').update(read.rows.map(r => r.id).sort().join('\n')).digest('hex').slice(0, 12); - return [...head, `Approved: ${approved ? 'yes' : 'no'} (record ${id}). The full approval record stays private: this repository is public or its visibility is unknown.`, ''].join('\n'); + return [...head, `Approved: ${approved ? 'yes' : 'no'} (record ${id}). The full approval record stays private.`, ''].join('\n'); } if (read.state === 'unavailable') return [...head, `Record unavailable on this machine: no approval record for branch ${cell(loc.branch)} here, and the state root is ephemeral with no Drive mirror, so this is not proof that nothing was approved.`, ''].join('\n'); if (!read.rows.length) return [...head, `No approvals recorded for branch ${cell(loc.branch)}.`, ''].join('\n'); const lines = [APPROVALS_TABLE_HEADER, '|---|---|---|---|---|---|']; for (const r of read.rows) { const outcome = r.outcome === 'standing' ? 'standing approval' : r.outcome; - lines.push(`| ${r.date} | ${cell(r.ask)} | “${cell(r.words, 280)}” (${cell(r.by)}) | ${outcome} | ${cell(r.scope)} | ${cell(r.pending)} |`); + lines.push(`| ${r.date} | ${cell(pathLabels(r.ask))} | “${cell(r.words, 280)}” (${cell(r.by)}) | ${outcome} | ${cell(pathLabels(r.scope))} | ${cell(pathLabels(r.pending))} |`); } return [...head, ...lines, '', `Source: gstack-approvals record for ${cell(loc.branch)} ${APPROVALS_FOOTER_END}`, ''].join('\n'); } diff --git a/test/approval-record.test.ts b/test/approval-record.test.ts index 47978fec0..cadc65631 100644 --- a/test/approval-record.test.ts +++ b/test/approval-record.test.ts @@ -10,7 +10,7 @@ import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import { parseCitations } from '../lib/approval-citations'; -import { APPROVALS_TABLE_HEADER, branchKey, buildRow, cell, importAutoplan, readRecord, renderRecord, type StoreLocation } from '../lib/approval-record'; +import { APPROVALS_TABLE_HEADER, branchKey, buildRow, cell, importAutoplan, pathLabels, readRecord, renderRecord, type StoreLocation } from '../lib/approval-record'; const ROOT = path.resolve(import.meta.dir, '..'); const dirs: string[] = []; @@ -174,6 +174,121 @@ describe('public repositories get no identifying approval text (GST-1160)', () = bin(ADD, { cwd, env: { GSTACK_STATE_ROOT: state } }); expect(id()).not.toBe(first); }); + + describe('full rows never carry a local path in Covers (GST-2048)', () => { + const LEDGER = '/home/user/.capy/drive/project-user-gstack-garry-tan/ship-budget/ledger.jsonl'; + const SCOPES = [ + `spend cap 25000 credits, counted in ${LEDGER}`, + 'the plan at ~/.gstack/projects/acme-widgets/plans/fix.md, and /Users/dana/work/widgets/notes.txt', + 'the checkout /workspace/gstack/lib/approval-record.ts; C:\\Users\\dana\\gstack\\plan.md', + ]; + const pathLike = /(?:^|[\s(])(?:~[\\/]|\/[^\s/|]+\/|[A-Za-z]:[\\/])/; + const addScoped = (cwd: string, state: string) => SCOPES.forEach((scope, i) => + expect(bin(['add', '--ask', `Spend cap ${i}`, '--words', 'do it', '--by', 'Garry (via GStack Minder)', '--scope', scope], { cwd, env: { GSTACK_STATE_ROOT: state } }).status).toBe(0)); + const post = (body: string) => { + const d = tmp('appr-post-'); const stubs = tmp('appr-post-gh-'); + fs.writeFileSync(path.join(stubs, 'gitconfig'), ''); + const gitEnv = { ...process.env, GIT_CONFIG_GLOBAL: path.join(stubs, 'gitconfig'), GIT_CONFIG_NOSYSTEM: '1' }; + spawnSync('git', ['init', '-q'], { cwd: d, timeout: 30_000, env: gitEnv }); + spawnSync('git', ['remote', 'add', 'origin', 'git@github.com:acme/widget.git'], { cwd: d, timeout: 30_000, env: gitEnv }); + fs.writeFileSync(path.join(stubs, 'gh'), '#!/bin/sh\ncat >/dev/null\nexit 0\n', { mode: 0o755 }); + const file = path.join(d, 'body.md'); fs.writeFileSync(file, body); + const r = spawnSync('bun', [path.join(ROOT, 'bin', 'gstack-post'), 'pr-body', '7', '--replace', '--repo-visibility', 'private', '--body-file', file], { + cwd: d, encoding: 'utf8', timeout: 60_000, env: { ...gitEnv, PATH: `${stubs}:${process.env.PATH}`, GSTACK_STATE_ROOT: tmp('appr-post-state-') }, + }); + return { status: r.status ?? -1, err: r.stderr ?? '' }; + }; + + test('a private render labels each path by its file name, and gstack-post finds no internal.user_path in it', () => { + const cwd = repo(); const state = tmp('appr-state-'); + addScoped(cwd, state); + const r = bin(['render'], { cwd, env: { GSTACK_STATE_ROOT: state, ...fakeGh('PRIVATE') } }); + expect(r.status, r.err).toBe(0); + expect(r.out).toContain(APPROVALS_TABLE_HEADER); + const control = post(`## Approvals\n\nCovers ${LEDGER}\n`); + expect(control.status, control.err).toBe(0); + expect(control.err).toContain('internal.user_path'); + const posted = post(r.out); + expect(posted.status, posted.err).toBe(0); + expect(posted.err).not.toContain('internal.user_path'); + const covers = r.out.split('\n').filter(l => /^\| \d{4}-/.test(l)).map(l => l.split(' | ')[4]!); + expect(covers).toHaveLength(3); + for (const c of covers) expect(c).not.toMatch(pathLike); + expect(r.out).toContain('ledger.jsonl'); + expect(r.out).toContain('fix.md'); + expect(r.out).not.toContain('garry-tan'); + expect(r.out).not.toContain('dana'); + const list = bin(['list', '--json'], { cwd, env: { GSTACK_STATE_ROOT: state } }); + expect(JSON.parse(list.out).rows.map((row: { scope: string }) => row.scope).sort()).toEqual([...SCOPES].sort()); + }); + + test('--redacted on a private repository prints the same bytes as the public render', () => { + const cwd = repo(); const state = tmp('appr-state-'); + addScoped(cwd, state); + const pub = bin(['render'], { cwd, env: { GSTACK_STATE_ROOT: state, ...fakeGh('PUBLIC') } }); + const forced = bin(['render', '--redacted'], { cwd, env: { GSTACK_STATE_ROOT: state, ...fakeGh('PRIVATE') } }); + expect(forced.status, forced.err).toBe(0); + expect(forced.out).toBe(pub.out); + expect(forced.out).not.toContain(APPROVALS_TABLE_HEADER); + expect(forced.err).toContain('APPROVALS_RENDER: redacted (--redacted'); + }); + + test('--redacted with --full refuses and prints nothing', () => { + const cwd = repo(); const state = tmp('appr-state-'); + addScoped(cwd, state); + for (const args of [['render', '--redacted', '--full'], ['render', '--full', '--redacted']]) { + const r = bin(args, { cwd, env: { GSTACK_STATE_ROOT: state, ...fakeGh('PRIVATE') } }); + expect(r.status).toBe(2); + expect(r.out).toBe(''); + expect(r.err).toContain('--redacted and --full conflict'); + } + }); + + test('Asked and Still pending get the same labels as Covers; Exact words stays verbatim', () => { + const cwd = repo(); const state = tmp('appr-state-'); + expect(bin(['add', '--ask', `Raise the cap in ${LEDGER}?`, '--words', 'do it', '--by', 'Garry', '--scope', 'spend cap', '--pending', `settle file:///home/dana/ledger.json`], { cwd, env: { GSTACK_STATE_ROOT: state } }).status).toBe(0); + const r = bin(['render'], { cwd, env: { GSTACK_STATE_ROOT: state, ...fakeGh('PRIVATE') } }); + expect(r.status, r.err).toBe(0); + expect(r.out).toContain('Raise the cap in …/ledger.jsonl?'); + expect(r.out).toContain('settle …/ledger.json'); + expect(r.out).not.toContain('garry-tan'); + expect(r.out).not.toContain('dana'); + }); + + test('paths glued onto a colon or dash and file: in any case are labelled, and gstack-post finds no internal.user_path', () => { + const GLUED = ['ledger:/home/u/.capy/drive/project-user-x-y/x.md', 'garry@host:/home/u/x.md', 'file:/home/u/x', 'FILE:///home/u/a/b', 'run-/home/u/a.md']; + const cwd = repo(); const state = tmp('appr-state-'); + for (const [i, scope] of [...GLUED, 'see https://github.com/o/r'].entries()) expect(bin(['add', '--ask', `Glued path ${i}`, '--words', 'do it', '--by', 'Garry', '--scope', scope], { cwd, env: { GSTACK_STATE_ROOT: state } }).status).toBe(0); + const r = bin(['render'], { cwd, env: { GSTACK_STATE_ROOT: state, ...fakeGh('PRIVATE') } }); + expect(r.status, r.err).toBe(0); + for (const label of ['ledger:…/x.md', 'host:…/x.md', '…/x', '…/b', 'run-…/a.md', 'see https://github.com/o/r']) expect(r.out).toContain(label); + expect(r.out).not.toContain('/home/u'); + const posted = post(r.out); + expect(posted.status, posted.err).toBe(0); + expect(posted.err).not.toContain('internal.user_path'); + }); + + test.each([ + ['see https://github.com/acme/widget/pull/12', 'see https://github.com/acme/widget/pull/12'], + ['https://github.com/o/r', 'https://github.com/o/r'], + ['ledger:/home/u/.capy/drive/project-user-x-y/x.md', 'ledger:…/x.md'], + ['garry@host:/home/u/x.md', 'garry@host:…/x.md'], + ['file:/home/u/x', '…/x'], + ['FILE:///home/u/a/b', '…/b'], + ['run-/home/u/a.md', 'run-…/a.md'], + ['lib/approval-record.ts and 3/4 done', 'lib/approval-record.ts and 3/4 done'], + ['/home/dana', '~'], + ['~/.gstack/plans/fix.md.', '…/fix.md.'], + ['~dana/notes/x.md', '…/x.md'], + ['$HOME/.capy/drive/project-user-acme-dana/l.json', '…/l.json'], + ['.capy/drive/project-user-acme-dana/l.json', '…/l.json'], + ['file:///Users/dana/a/b.json', '…/b.json'], + ['\\\\server\\share\\dana\\x.md', '…/x.md'], + ['%USERPROFILE%\\dana\\a.md', '…/a.md'], + ])('pathLabels(%p) is %p', (input, label) => { + expect(pathLabels(input)).toBe(label); + }); + }); }); describe('the rendered table is display only', () => {