From ec1f2aa6a4774ab83a7941e3da259ef12873d837 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 4 Nov 2026 16:01:33 -0800 Subject: [PATCH] fix(redact): a short sha is not a phone number or a bc1 wallet; plan-reality receipt and pregate fixture flakes (#3998) * fix(redact): a short sha is not a phone number or a bc1 wallet; plan-reality receipt and pregate fixture flakes (GSTA-279) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs: the review_loop_grant sha exemption and the fixture origin's maintenance setting (GSTA-279) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(changelog): name the fragment for the branch and narrow its claim to the review_loop_grant position (GSTA-279) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/capy-a-short-sha-is.md | 5 +++ docs/TESTING_INTERNALS.md | 5 ++- docs/troubleshooting.md | 6 +++ lib/redact-patterns.ts | 54 +++++++++++++++++++++++--- test/approval-record.test.ts | 12 ++++++ test/helpers/pregate-fixture.ts | 2 + test/plan-reality-receipts.test.ts | 5 ++- test/pregate-user-policy.test.ts | 17 ++++++++ test/redact-context-exemptions.test.ts | 45 +++++++++++++++++++++ 9 files changed, 144 insertions(+), 7 deletions(-) create mode 100644 changelog.d/capy-a-short-sha-is.md diff --git a/changelog.d/capy-a-short-sha-is.md b/changelog.d/capy-a-short-sha-is.md new file mode 100644 index 000000000..0b2475987 --- /dev/null +++ b/changelog.d/capy-a-short-sha-is.md @@ -0,0 +1,5 @@ +### Fixed + +- **The secret scanner no longer reads the sha in an owner's `review_loop_grant` as a phone number, or a git sha that starts with `bc1` as a Bitcoin address. Both made free tests flaky.** About one 12-character sha in 300 is all digits, and `pii.phone.e164` flagged it, so `gstack-approvals add` refused the owner's `review_loop_grant ` grant with APPROVAL_RECORD_SECRET (exit 3). A digits-only span right after `review_loop_grant` or `review_loop_exception` is now a sha. A phone number with a `+`, spaces or dashes, or the same digits anywhere else (an all-digit sha in other wording included), still reports (Free Tests 41667825418, 41766865679, 41680498938, 41843093520). A sha that starts with `bc1` was redacted as `` in CSO's history range. A `bc1` address now has to use the bech32 alphabet and carry a valid bech32 or bech32m checksum, which every real address has. An address with a mistyped character no longer reports (Free Tests 41715139988). Both rules have true-positive controls in `test/redact-context-exemptions.test.ts`. +- **`test/plan-reality-receipts.test.ts`**: the commit-sha case writes its receipt up to the sha's first hex letter. A bare all-digit prefix is not a receipt by design, and about one 10-character prefix in 110 is all digits (Free Tests 41669379416, 41790613651). +- **`test/helpers/pregate-fixture.ts`**: the fixture's bare origin turns off auto-maintenance, as `test/helpers/restamp-fixture.ts` already does. Git's local transport drops the free runner's maintenance-off setting, so each push into the origin started a detached `git maintenance run --auto` that could change `origin.git/objects` while the template was being copied (ENOENT, Free Tests 41760556132). A new test traces a push and checks that no maintenance starts. diff --git a/docs/TESTING_INTERNALS.md b/docs/TESTING_INTERNALS.md index ff5939113..7752e239d 100644 --- a/docs/TESTING_INTERNALS.md +++ b/docs/TESTING_INTERNALS.md @@ -1556,7 +1556,10 @@ Free-suite flake rules: replacement env never sees the backstop. Neither does the receiving side of a push to a local path: git's local transport drops `GIT_CONFIG_*` from the `receive-pack` it starts, so every push into a fixture's bare origin still - leaves a detached maintenance run in that origin. A fixture that clones such + leaves a detached maintenance run in that origin unless the origin's own + config turns it off (`maintenance.auto=false` and `gc.auto=0` on the bare + origin, as `test/helpers/pregate-fixture.ts` and + `test/helpers/restamp-fixture.ts` do). A fixture that clones such an origin passes `--no-local` (`coordinatorClone` in `test/helpers/ship-queue-fixture.ts`), so the clone reads it through git's transport instead of copying `objects/` entry by entry. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 46246e90f..e675e7f7b 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -2807,6 +2807,12 @@ exact tokens it finds in these positions: - a 12-digit value right after `tree`, `gated_tree`, `head` or `predecessor` with `=` or a space (`tree=704992822996`, `tree 704992822996`); +- an all-digit sha right after `review_loop_grant` or `review_loop_exception` + with a space, `=` or `:` (`review_loop_grant 367120488513`), so the owner's + grant records through `gstack-approvals add`. The scanner itself exempts it + (`isReviewLoopSha` in `lib/redact-patterns.ts`), and only that position + counts: the same digits elsewhere, or a number with a `+`, spaces or dashes + after the token, still report; - an 11-digit id inside an `/actions/runs/` URL or as that link's text; - any id inside an `actions/runs/` path, with or without a host, when `actions/runs/` starts the line or follows whitespace, `/`, `(`, `[`, diff --git a/lib/redact-patterns.ts b/lib/redact-patterns.ts index 9497c1c15..e4e29a5df 100644 --- a/lib/redact-patterns.ts +++ b/lib/redact-patterns.ts @@ -167,10 +167,36 @@ function looksLikeWallet(span: string): boolean { const body = span.slice(2).toLowerCase(); return !/^(.)\1{39}$/.test(body); } - // bech32 / base58 — length sanity only + if (span.startsWith("bc1")) return bech32ChecksumValid(span); + // base58 — length sanity only return span.length >= 26 && span.length <= 62; } +/** + * A bc1 address is bech32 (BIP 173) or bech32m (BIP 350): `bc`, the separator `1`, then data from a 32-letter alphabet + * with no `1`, `b`, `i` or `o`, ending in a six-letter checksum. A git sha that starts with `bc1` read as a wallet and + * was redacted out of `..` (Free Tests 41715139988, test/cso-cli.test.ts); the alphabet in the regex refuses + * most such shas, and the checksum, which a random string passes about once in 500 million tries, refuses the rest. Every real + * address carries a valid checksum, so none is lost. + */ +const BECH32_ALPHABET = "qpzry9x8gf2tvdw0s3jn54khce6mua7l"; +const BECH32_GENERATOR = [0x3b6a57b2, 0x26508e6d, 0x1ea119fa, 0x3d4233dd, 0x2a1462b3]; +function bech32ChecksumValid(address: string): boolean { + const sep = address.lastIndexOf("1"); + const hrp = [...address.slice(0, sep)].map((c) => c.charCodeAt(0)); + const data = [...address.slice(sep + 1)].map((c) => BECH32_ALPHABET.indexOf(c)); + if (sep < 1 || data.length < 6 || data.some((v) => v < 0)) return false; + let chk = 1; + for (const v of [...hrp.map((c) => c >> 5), 0, ...hrp.map((c) => c & 31), ...data]) { + const top = chk >>> 25; + chk = ((chk & 0x1ffffff) << 5) ^ v; + BECH32_GENERATOR.forEach((g, i) => { + if ((top >>> i) & 1) chk ^= g; + }); + } + return chk === 1 || chk === 0x2bc830a3; +} + // Compact log/backup stamps (`20260727202423` = YYYYMMDDHHMMSS) are bare digit // runs that the phone regex happily eats. Only a SEPARATOR-FREE 14-digit span // qualifies: E.164 tops out at 15 digits and real numbers carry a + or spacing, @@ -392,6 +418,22 @@ export function isCiRunIdentifier(span: string, match: RegExpExecArray): boolean return [...line.matchAll(CI_RUN_LIST)].some(m => m.index! < at && at + span.length <= m.index! + m[0].length); } +/** + * The owner's grant of one scoped fix-check is recorded as `review_loop_grant `, and an accepted stop as + * `review_loop_exception ` (docs/review-policy.md). About one 12-character sha in 300 is all digits + * (`367120488513`), and pii.phone.e164 read it as a phone number, so gstack-approvals refused the owner's grant + * (APPROVAL_RECORD_SECRET; Free Tests 41667825418, 41766865679). Only the position decides: a digits-only span right + * after one of those two tokens and a space, `=` or `:`. The same digits anywhere else, or a number with a `+`, + * spaces or dashes after the token, still report. + */ +const REVIEW_LOOP_SHA_BEFORE = /\breview_loop_(?:grant|exception)(?:[ \t]+|[ \t]*[:=][ \t]*)$/; +function isReviewLoopSha(span: string, match: RegExpExecArray): boolean { + if (!/^\d+$/.test(span)) return false; + const input = match.input ?? ""; + const { start } = spanBounds(match); + return REVIEW_LOOP_SHA_BEFORE.test(input.slice(Math.max(0, start - 40), start)); +} + /** * A four-part version (MAJOR.MINOR.PATCH.BUILD: .NET assembly versions, * gstack's own VERSION) is byte-for-byte a dotted quad, and `1.128.1.0` is a @@ -1276,8 +1318,9 @@ export const PATTERNS: RedactPattern[] = [ // so do a county tax-map parcel ID (see looksLikeParcelId), vector // coordinates (looksLikeDecimalCoordinates) and seed/nonce/timestamp JSON // values (isNumericMetadataValue), GitHub Actions run and job ids in - // their id positions (isCiRunIdentifier), and bare powers of two and - // hash/PRNG constants (isNumericConstant). + // their id positions (isCiRunIdentifier), a short sha after + // review_loop_grant or review_loop_exception (isReviewLoopSha), and bare + // powers of two and hash/PRNG constants (isNumericConstant). validate: (span, match) => !insideUuid(match) && span.replace(/\D/g, "").length >= 10 && @@ -1286,7 +1329,8 @@ export const PATTERNS: RedactPattern[] = [ !looksLikeParcelId(span, match) && !looksLikeDecimalCoordinates(span) && !isNumericMetadataValue(span, match) && - !isCiRunIdentifier(span, match), + !isCiRunIdentifier(span, match) && + !isReviewLoopSha(span, match), }, { id: "pii.ssn", @@ -1330,7 +1374,7 @@ export const PATTERNS: RedactPattern[] = [ tier: "MEDIUM", category: "pii", description: "Crypto wallet address (ETH/BTC)", - regex: /\b(0x[a-fA-F0-9]{40}|bc1[a-z0-9]{25,39}|[13][a-km-zA-HJ-NP-Z1-9]{25,34})\b/, + regex: /\b(0x[a-fA-F0-9]{40}|bc1[qpzry9x8gf2tvdw0s3jn54khce6mua7l]{25,39}|[13][a-km-zA-HJ-NP-Z1-9]{25,34})\b/, validate: (span) => looksLikeWallet(span), }, diff --git a/test/approval-record.test.ts b/test/approval-record.test.ts index e318d766c..448c0588e 100644 --- a/test/approval-record.test.ts +++ b/test/approval-record.test.ts @@ -88,6 +88,18 @@ describe('gstack-approvals add and render', () => { expect(control.err).toContain('pii.phone.e164'); }); + test("the owner's grant for a stop head whose short sha is all digits records (Free Tests 41667825418, 41766865679)", () => { + const cwd = repo(); const state = tmp('appr-state-'); + const ask = 'Owner grant: one scoped fix-check after round 2 (review_loop_grant 367120488513)'; + const r = bin(['add', '--ask', ask, '--words', 'do it', '--by', 'GStack Minder', '--scope', 'this /ship invocation'], { cwd, env: { GSTACK_STATE_ROOT: state } }); + expect(r.status, r.err).toBe(0); + const file = /^APPROVAL_RECORDED: a-[0-9a-f]{12} (.*\.jsonl)$/m.exec(r.out)![1]!; + expect(fs.readFileSync(file, 'utf8')).toContain('review_loop_grant 367120488513'); + const control = bin(['add', '--ask', 'Owner grant (review_loop_grant 367120488513), call 367120488513 first', '--words', 'do it', '--by', 'g', '--scope', 's'], { cwd, env: { GSTACK_STATE_ROOT: state } }); + expect(control.status).toBe(3); + expect(control.err).toContain('field ask: pii.phone.e164'); + }); + test('missing fields, bad outcomes and impossible dates are usage errors', () => { const cwd = repo(); const state = tmp('appr-state-'); expect(bin(['add', '--ask', 'x'], { cwd, env: { GSTACK_STATE_ROOT: state } }).err).toContain('APPROVAL_RECORD_INVALID'); diff --git a/test/helpers/pregate-fixture.ts b/test/helpers/pregate-fixture.ts index e7f0462fc..2737194af 100644 --- a/test/helpers/pregate-fixture.ts +++ b/test/helpers/pregate-fixture.ts @@ -122,6 +122,8 @@ function seedFixtureRepo(root: string, opts: FixtureOptions): void { fs.mkdirSync(work, { recursive: true }); fs.writeFileSync(env.GIT_CONFIG_GLOBAL, '[user]\n\tname = t\n\temail = t@test\n[commit]\n\tgpgsign = false\n[init]\n\tdefaultBranch = main\n'); spawnSync('git', ['init', '-q', '--bare', origin], { timeout: 30_000, env: { ...process.env, ...env } }); + git('--git-dir', origin, 'config', 'maintenance.auto', 'false'); + git('--git-dir', origin, 'config', 'gc.auto', '0'); git('init', '-q', '-b', 'main'); for (const dir of ['src', 'lib', 'bin', 'test', 'test/golden', 'test/nightly', 'generated', 'docs', 'scripts', '.gstack', '.github/workflows']) fs.mkdirSync(path.join(work, dir), { recursive: true }); write(work, 'src/version.js', "module.exports = { VERSION: '1.0.0' };\n"); diff --git a/test/plan-reality-receipts.test.ts b/test/plan-reality-receipts.test.ts index 0d1c2239c..e0c488a10 100644 --- a/test/plan-reality-receipts.test.ts +++ b/test/plan-reality-receipts.test.ts @@ -108,7 +108,10 @@ describe('resolveReceipts', () => { gitOk(dir, 'add', '.'); gitOk(dir, '-c', 'user.name=t', '-c', 'user.email=t@t', 'commit', '-qm', 'seed'); const sha = gitOk(dir, 'rev-parse', 'HEAD').stdout.trim(); - const out = resolveReceipts(parseRealityLines(`REALITY: numbers pass ok ${sha.slice(0, 10)} abcdef1234`), { roots: [dir] }); + // A bare sha is a receipt only with a hex letter in it (RECEIPT_RE reads an all-digit run as a number unless `commit ` or + // backticks mark it), and about one 10-character prefix in 110 is all digits (Free Tests 41669379416, 41790613651). + const receipt = sha.slice(0, Math.max(10, sha.search(/[a-f]/) + 1)); + const out = resolveReceipts(parseRealityLines(`REALITY: numbers pass ok ${receipt} abcdef1234`), { roots: [dir] }); expect(out.map(r => r.outcome)).toEqual(['resolved', 'unverifiable']); }); }); diff --git a/test/pregate-user-policy.test.ts b/test/pregate-user-policy.test.ts index 6cb2b2667..c6fefe52c 100644 --- a/test/pregate-user-policy.test.ts +++ b/test/pregate-user-policy.test.ts @@ -91,6 +91,23 @@ describe('loadConfigFromUser', () => { }); }); +describe('fixture origin', () => { + // Value: protects=a push into the fixture's bare origin starts no detached auto-maintenance there, so copying the template never races + // a repack (ENOENT lstat origin.git/objects/1d, Free Tests 41760556132); fails_when=the origin loses maintenance.auto=false; + // why_new=local transport drops the runner's GIT_CONFIG_COUNT backstop, so only the origin's own config reaches receive-pack; + // seam=none; no_claim=which maintenance task removed the object directory on the CI runner's git + test('a push into the origin starts no auto-maintenance even when the runner backstop is dropped', () => { + const { repo } = setup(); + const trace = path.join(repo.root, 'trace2.txt'); + const backstop = { GIT_CONFIG_COUNT: '2', GIT_CONFIG_KEY_0: 'maintenance.auto', GIT_CONFIG_VALUE_0: 'false', GIT_CONFIG_KEY_1: 'gc.auto', GIT_CONFIG_VALUE_1: '0' }; + const push = spawnSync('git', ['push', '-q', 'origin', 'HEAD:refs/heads/probe'], { cwd: repo.work, encoding: 'utf8', timeout: 30_000, env: { ...process.env, ...repo.env, ...backstop, GIT_TRACE2: trace } }); + expect(push.status, push.stderr).toBe(0); + const events = fs.readFileSync(trace, 'utf8'); + expect(events).toContain('receive-pack'); + expect(events.split('\n').filter(l => l.includes('child_start') && l.includes('maintenance'))).toEqual([]); + }); +}); + describe('gstack-pregate with a user-owned policy', () => { // Value: protects=with no base copy the user file is the policy and its `local` lanes run here, while an unnamed platform lane stays // requires-remote; fails_when=the user fallback or the `local` branch in selectLanes is dropped; why_new=both are new; diff --git a/test/redact-context-exemptions.test.ts b/test/redact-context-exemptions.test.ts index 86d813991..b7f51d1af 100644 --- a/test/redact-context-exemptions.test.ts +++ b/test/redact-context-exemptions.test.ts @@ -352,3 +352,48 @@ describe("pii.phone.e164: run ids in a runs= list or an actions/runs/ path (GST- ], ); }); + +describe("pii.phone.e164: a short sha after review_loop_grant or review_loop_exception (Free Tests 41667825418, 41766865679)", () => { + table( + "pii.phone.e164", + "MEDIUM", + [ + // The ask gstack-approvals records for the owner's grant, here with a stop head whose 12-character sha is all digits. + "Owner grant: one scoped fix-check after round 2 (review_loop_grant 367120488513)", + "Owner accepts the stop after the fix-check (review_loop_exception 367120488513)", + "review_loop_grant=367120488513", + "review_loop_grant: 3671204885", + ], + [ + "review_loop_grant +14155552671", + "review_loop_grant 415-555-2671", + "review_loop_grant (415) 555-2671", + "call 367120488513 about the review_loop_grant", + "grant 367120488513", + "my_review_loop_grant 367120488513", + "review_loop_grant for 367120488513", + ], + ); +}); + +describe("pii.wallet: a bc1 address needs the bech32 alphabet and checksum (Free Tests 41715139988)", () => { + table( + "pii.wallet", + "MEDIUM", + [ + // The sha the CSO history range printed as ..; it holds 1 and b, outside bech32's alphabet. + "bc1f7576f1eaf9d548ddb6f742b85ec11b4a3e79..bc1f7576f1eaf9d548ddb6f742b85ec11b4a3e79", + // A sha inside the alphabet (no 1 or b after the prefix) fails the checksum. + "commit bc1f7576f2eaf9d548ddf6f742c85ec22d4a3e79", + // A real address with its last character changed fails the checksum too. + "bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t5", + ], + [ + "bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4", + "send it to bc1qar0srrr7xfkvy5l643lydnw9re59gtzzwf5mdq today", + "wallet: bc1qxy2kgdygjrsqtzq2n0yrf2493p83kkfjhx0wlh", + "0x52908400098527886E0F7030069857D2E4169EE7", + "1BvBMSEYstWetqTFn5Au4m4GFg7xJaNVN2", + ], + ); +});