From 6cd982572a0fe9f7cfba89f86db295fa4b2e75d3 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 20:21:08 -0700 Subject: [PATCH] fix(cli): read-only git refuses the modifier forms of the gpg placeholders, and checks abbreviated or detached format options (TASK-296) checkFormat matched only a bare %G. Git also reads a %G placeholder behind a +, - or space modifier (%+G?, %-GG, % GS) as the same placeholder, so a workspace seat's log or show could still run gpg on a signed commit. The check now matches an optional modifier. Probed on git 2.54.0: of all 95 printable characters, only those three expand %G? to the signature status. The new control shows bare git running the stub gpg for each of them. Sable's non-blocking note on #2746: the guard should not depend on git rejecting `--form=` and `--pret=`. An abbreviation is now checked like the full option name. A --format with no `=` is refused, so the next argument can never be taken as its format. A bare --pretty still means medium. git 2.54.0 rejects every abbreviation and a bare --format, so those tests pin the validator alone. Co-Authored-By: Claude Opus 5.5 Co-authored-by: wren Agent-Model: claude/claude-opus-5-5/max --- cli/__tests__/git-readonly.test.mjs | 30 +++++++++++++++++++++++++++++ cli/src/lib/git-readonly.js | 28 +++++++++++++++++++-------- 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/cli/__tests__/git-readonly.test.mjs b/cli/__tests__/git-readonly.test.mjs index fe62bd76c4..9c464c56c6 100644 --- a/cli/__tests__/git-readonly.test.mjs +++ b/cli/__tests__/git-readonly.test.mjs @@ -429,6 +429,36 @@ describe('read-only git runs no gpg and no textconv', () => { expect(ran('gpg')).toBe(false); }); + test('a %G placeholder behind a +, - or space modifier runs gpg (control), and is refused', async () => { + for (const format of ['%+G?', '%-GG', '% GS']) { + await cfgGit(['log', `--format=${format}`, '-1', signed]); + expect(ran('gpg')).toBe(true); + } + for (const arg of ['--format=%+G?', '--pretty=format:%-GK', '--pretty=tformat:%h% GF', '--format=%<(8)%+GP', '--format=%-GT']) { + expect((await run('log', [arg, '-1', signed])).text).toMatch(/%G placeholder runs gpg/); + } + expect(ran('gpg')).toBe(false); + const modified = await run('log', ['--format=%h%+s', '-1', signed]); + expect(modified.ok).toBe(true); + expect(modified.text).toContain('\nunsigned'); + }); + + // git 2.54 rejects every abbreviation of --pretty and --format, and a bare + // --format, so none of these has a control: the check holds without that. + test('an abbreviation is checked like the full name, and --format never takes the next argument', () => { + expect(refusal({ subcommand: 'log', args: ['--form=%G?'] })).toMatch(/%G placeholder runs gpg/); + expect(refusal({ subcommand: 'show', args: ['--pret=format:%+GS'] })).toMatch(/%G placeholder runs gpg/); + expect(refusal({ subcommand: 'log', args: ['--forma=sig'] })).toMatch(/name a built-in format/); + for (const option of ['--format', '--form', '--for']) { + expect(refusal({ subcommand: 'log', args: [option, '%G?'] })).toMatch(/give the format in the same argument/); + } + for (const args of [['--form=%h'], ['--pret=oneline'], ['--pretty'], ['--pretty', '-n1']]) { + expect(readonlyGitArgv({ subcommand: 'log', args, root: repo }).ok).toBe(true); + } + // blame reads --pr as its own --progress. + expect(readonlyGitArgv({ subcommand: 'blame', args: ['--pr', 'tracked.txt'], root: repo }).ok).toBe(true); + }); + test('log.showSignature in the repository config runs gpg (control); the guarded log and show do not', async () => { await cfgGit(['config', 'log.showSignature', 'true']); try { diff --git a/cli/src/lib/git-readonly.js b/cli/src/lib/git-readonly.js index 6cdc9825e4..509dd19808 100644 --- a/cli/src/lib/git-readonly.js +++ b/cli/src/lib/git-readonly.js @@ -127,16 +127,28 @@ const TOP_LEVEL = Object.freeze([ ]); // A %G placeholder (%G?, %GG, %GS, %GK, %GF, %GP, %GT) runs gpg on each signed -// commit. A value with no %, no format: and no tformat: is looked up by -// prefix among the built-in formats and the repository's `pretty.` -// aliases, and an alias can hold one, so only a built-in name is taken. -const FORMAT_ARGUMENT = /^--(?:pretty|format)=([\s\S]*)$/; +// commit, and git reads one behind a `+`, `-` or space modifier (`%+G?`, +// `%-GG`, `% GS`) as the same placeholder. A value with no %, no format: and +// no tformat: is looked up by prefix among the built-in formats and the +// repository's `pretty.` aliases, and an alias can hold one, so only a +// built-in name is taken. +const SIGNATURE_PLACEHOLDER = /%[-+ ]?G/; const BUILTIN_FORMATS = Object.freeze(['oneline', 'short', 'medium', 'full', 'fuller', 'reference', 'email', 'mboxrd', 'raw']); +// Git's revision parser reads --pretty and --format only by their exact names, +// and their format only after `=`: a bare --pretty means medium, and a bare +// --format is an unrecognized argument. The check does not depend on that. An +// abbreviation (`--form=`, `--pret=`) is checked like the full name, and a +// --format that would take its format from the next argument is refused. +const FORMAT_OPTIONS = Object.freeze(['--pretty', '--format']); const checkFormat = (arg) => { - const match = FORMAT_ARGUMENT.exec(arg); - if (!match) return null; - const value = match[1]; - if (value.includes('%G')) return `${arg} is refused: a %G placeholder runs gpg to check a signature`; + const [name] = arg.split('=', 1); + const option = name.length >= 3 && FORMAT_OPTIONS.find((full) => full.startsWith(name)); + if (!option) return null; + if (name === arg) { + return option === '--format' ? `${arg} is refused: give the format in the same argument, as --format=` : null; + } + const value = arg.slice(name.length + 1); + if (SIGNATURE_PLACEHOLDER.test(value)) return `${arg} is refused: a %G placeholder runs gpg to check a signature`; const userFormat = value === '' || value.startsWith('format:') || value.startsWith('tformat:') || value.includes('%'); if (userFormat || BUILTIN_FORMATS.includes(value.toLowerCase())) return null; return `${arg} is refused: name a built-in format (${BUILTIN_FORMATS.join(', ')}) or give format:`;