diff --git a/test/helpers/generated-bash-lint.ts b/test/helpers/generated-bash-lint.ts index f4c0c35fe..24227e959 100644 --- a/test/helpers/generated-bash-lint.ts +++ b/test/helpers/generated-bash-lint.ts @@ -11,40 +11,44 @@ * fence never assigned. In a fresh shell it is empty: `cd ""` stays put and * the next command runs in the user's project. Write `${VAR:?message}`, and * for `cd` also handle failure (`cd -- "${VAR:?...}" || exit 1`). - * - untrusted-in-quotes: a `` the model fills with reviewer, - * diff or error text, inside a double-quoted body/title/message argument - * of gh, glab, git or curl. In double quotes the shell runs every backtick - * span in that text (#3046). Pass it through a quoted heredoc into a - * variable and expand the variable instead. + * - free-text-placeholder: a `` the model fills in, anywhere + * in a command (any command, any quoting: double, single, unquoted, or a + * heredoc body, quoted or not). Free text (bodies, titles, messages, error + * output, reviewer or diff text) reaches a command only as the contents of + * a `mktemp` file the agent writes with its file-write tool (`--body-file`, + * `-F body=@file`). Only identifier placeholders on IDENTIFIER_PLACEHOLDERS + * may appear, each with the grammar the skill applies before use; their + * grammars exclude every shell metacharacter. * * The lexer understands single/double quotes, `$(...)`, backticks, comments * and heredoc bodies; it is not a full shell parser. */ -export interface LintFinding { rule: 'tilde-in-quotes' | 'mktemp-template' | 'unguarded-var' | 'untrusted-in-quotes'; line: number; detail: string } +import { IDENTIFIER_PLACEHOLDERS } from './placeholder-allowlist'; + +export { IDENTIFIER_PLACEHOLDERS }; +export interface LintFinding { rule: 'tilde-in-quotes' | 'mktemp-template' | 'unguarded-var' | 'free-text-placeholder'; line: number; detail: string } interface VarRef { name: string; op: string } -interface Word { raw: string; vars: VarRef[]; line: number } +interface Word { raw: string; own: string; vars: VarRef[]; line: number } +interface Heredoc { body: string; line: number; quoted: boolean } interface Command { words: Word[]; next: string; line: number } const NAME = /^[A-Za-z_][A-Za-z0-9_]*$/; const RESERVED = new Set(['if', 'then', 'elif', 'else', 'fi', 'do', 'done', 'while', 'until', '!', '{', '}', 'time', 'case', 'esac', 'in']); /** Always set by the environment the shell starts in. */ const AMBIENT = new Set(['HOME', 'PWD', 'OLDPWD', 'PATH', 'USER', 'SHELL']); -/** Commands that publish free text, and the flags that carry it. */ -const TEXT_SINKS = new Set(['gh', 'glab', 'git', 'curl']); -const TEXT_FLAGS = /^(?:-f|-F|--field|--raw-field|--body|-b|--title|-t|-m|--message|-d|--data|--description|--notes)$/; -const TEXT_FIELD = /^(?:body|title|message|description|notes)=/; -const QUOTED_PLACEHOLDER = /"[^"]*<[A-Za-z][^<>"\n]*>[^"]*"/; +/** A model-filled ``; `` is a literal mail address (commit trailers). */ +const MAIL = /^<[^<>\s@]+@[^<>\s@]+>$/; -function lex(src: string, findings: LintFinding[]): Command[] { +function lex(src: string, findings: LintFinding[], heredocBodies: Heredoc[] = []): Command[] { const commands: Command[] = []; let i = 0; let line = 1; let words: Word[] = []; let word: Word | null = null; let cmdLine = 1; - const heredocs: Array<{ delim: string; strip: boolean }> = []; + const heredocs: Array<{ delim: string; strip: boolean; quoted: boolean }> = []; const endWord = () => { if (word) { words.push(word); word = null; } }; const endCommand = (next: string) => { @@ -53,7 +57,7 @@ function lex(src: string, findings: LintFinding[]): Command[] { words = []; cmdLine = line; }; - const cur = () => (word ??= { raw: '', vars: [], line }); + const cur = () => (word ??= { raw: '', own: '', vars: [], line }); const readVar = (quoted: boolean) => { // at '$' const w = cur(); @@ -87,63 +91,72 @@ function lex(src: string, findings: LintFinding[]): Command[] { const startLine = line; i += 2; const inner = skipBalanced('(', ')'); - const nested = lex(inner, findings); + const nestedDocs: Heredoc[] = []; + const nested = lex(inner, findings, nestedDocs); for (const c of nested) commands.push({ ...c, line: c.line + startLine - 1 }); + for (const h of nestedDocs) heredocBodies.push({ ...h, line: h.line + startLine - 1 }); cur().raw += `$(${inner})`; + cur().own += '$()'; }; + const add = (w: Word, text: string) => { w.raw += text; w.own += text; }; while (i < src.length) { const c = src[i]; if (c === '\n') { endCommand('\n'); i++; line++; for (const h of heredocs.splice(0)) { + const bodyLine = line; + const body: string[] = []; while (i < src.length) { const eol = src.indexOf('\n', i); const text = src.slice(i, eol < 0 ? src.length : eol); i = eol < 0 ? src.length : eol + 1; line++; if ((h.strip ? text.replace(/^\t+/, '') : text).trim() === h.delim) break; + body.push(text); } + heredocBodies.push({ body: body.join('\n'), line: bodyLine, quoted: h.quoted }); } cmdLine = line; continue; } if (c === ' ' || c === '\t') { endWord(); i++; continue; } if (c === '#' && !word) { while (i < src.length && src[i] !== '\n') i++; continue; } - if (c === '\\') { cur().raw += src.slice(i, i + 2); if (src[i + 1] === '\n') line++; i += 2; continue; } + if (c === '\\' && src[i + 1] === '\n') { endWord(); line++; i += 2; continue; } + if (c === '\\') { add(cur(), src.slice(i, i + 2)); i += 2; continue; } if (c === "'") { const end = src.indexOf("'", i + 1); const text = src.slice(i, end < 0 ? src.length : end + 1); line += (text.match(/\n/g) ?? []).length; - cur().raw += text; i = end < 0 ? src.length : end + 1; continue; + add(cur(), text); i = end < 0 ? src.length : end + 1; continue; } if (c === '"') { const w = cur(); - w.raw += '"'; i++; + add(w, '"'); i++; if (src.startsWith('~/', i)) findings.push({ rule: 'tilde-in-quotes', line, detail: src.slice(i - 1, src.indexOf('"', i) + 1) }); while (i < src.length && src[i] !== '"') { - if (src[i] === '\\') { w.raw += src.slice(i, i + 2); i += 2; continue; } + if (src[i] === '\\') { add(w, src.slice(i, i + 2)); i += 2; continue; } if (src.startsWith('$(', i) && !src.startsWith('$((', i)) { substitution(); continue; } if (src[i] === '$') readVar(true); if (src[i] === '\n') line++; - w.raw += src[i]; i++; + add(w, src[i]); i++; } - w.raw += '"'; i++; + add(w, '"'); i++; continue; } - if (src.startsWith('$((', i)) { i += 3; cur().raw += `$((${skipBalanced('(', ')')})`; continue; } + if (src.startsWith('$((', i)) { i += 3; add(cur(), `$((${skipBalanced('(', ')')})`); continue; } if (src.startsWith('$(', i)) { substitution(); continue; } - if (c === '$') { readVar(false); cur().raw += c; i++; continue; } + if (c === '$') { readVar(false); add(cur(), c); i++; continue; } if (c === '`') { const end = src.indexOf('`', i + 1); - cur().raw += src.slice(i, end < 0 ? src.length : end + 1); + add(cur(), src.slice(i, end < 0 ? src.length : end + 1)); i = end < 0 ? src.length : end + 1; continue; } const hd = src.slice(i).match(/^<<(-?)\s*(['"]?)([A-Za-z_][A-Za-z0-9_]*)\2/); - if (hd) { heredocs.push({ delim: hd[3], strip: hd[1] === '-' }); endWord(); i += hd[0].length; continue; } + if (hd) { heredocs.push({ delim: hd[3], strip: hd[1] === '-', quoted: hd[2] !== '' }); endWord(); i += hd[0].length; continue; } const op = src.slice(i).match(/^(\|\||&&|;;|[;&|()])/); if (op) { endCommand(op[1]); i += op[1].length; continue; } - cur().raw += c; i++; + add(cur(), c); i++; } endCommand(''); return commands; @@ -178,9 +191,38 @@ function commandName(cmd: Command): number { return k; } +/** + * Placeholders in `text` that are not allowed there: free text anywhere, and + * `quoted` identifiers outside quotes. `inQuotes` is the context the text + * starts in (a heredoc body). + */ +function freeText(text: string, inQuotes = false): string[] { + const out: string[] = []; + let q: string | null = null; + for (let i = 0; i < text.length; i++) { + const c = text[i]; + if (q !== "'" && c === '\\') { i++; continue; } + if (q === null && (c === "'" || c === '"')) { q = c; continue; } + if (q !== null && c === q) { q = null; continue; } + if (c !== '<') continue; + const m = text.slice(i).match(/^<[A-Za-z][^<>\n]*>/); + if (!m) continue; + const p = m[0]; + i += p.length - 1; + if (MAIL.test(p)) continue; + const entry = Object.hasOwn(IDENTIFIER_PLACEHOLDERS, p) ? IDENTIFIER_PLACEHOLDERS[p] : undefined; + if (!entry || (entry.quoted && q === null && !inQuotes)) out.push(entry ? `${p} (unquoted)` : p); + } + return out; +} + export function lintFence(body: string): LintFinding[] { const findings: LintFinding[] = []; - const commands = lex(body, findings); + const heredocs: Heredoc[] = []; + const commands = lex(body, findings, heredocs); + for (const h of heredocs) { + for (const p of freeText(h.body, h.quoted)) findings.push({ rule: 'free-text-placeholder', line: h.line, detail: `${p} in a heredoc body` }); + } const assigned = new Set(); for (const cmd of commands) { const k = commandName(cmd); @@ -191,12 +233,8 @@ export function lintFence(body: string): LintFinding[] { const template = operands.at(-1)?.raw.replace(/^["']|["']$/g, ''); if (!template || template.startsWith('/tmp/')) findings.push({ rule: 'mktemp-template', line: cmd.line, detail: cmd.words.map(w => w.raw).join(' ') }); } - if (name && TEXT_SINKS.has(name)) { - args.forEach((w, n) => { - const carriesText = TEXT_FIELD.test(w.raw) || TEXT_FLAGS.test(args[n - 1]?.raw ?? ''); - const quotedHeredoc = /^"\$\(cat <<-?\s*'[A-Za-z_][A-Za-z0-9_]*'/.test(w.raw); - if (carriesText && !quotedHeredoc && QUOTED_PLACEHOLDER.test(w.raw.replace(/<[^<>\s"]+@[^<>\s"]+>/g, ''))) findings.push({ rule: 'untrusted-in-quotes', line: cmd.line, detail: `${name} ... ${args[n - 1]?.raw ?? ''} ${w.raw}`.replace(/\s+/g, ' ') }); - }); + for (const w of cmd.words) { + for (const p of freeText(w.own)) findings.push({ rule: 'free-text-placeholder', line: cmd.line, detail: `${p} in: ${cmd.words.map(x => x.raw).join(' ').replace(/\s+/g, ' ')}` }); } const recursiveRm = name === 'rm' && args.some(w => /^-[A-Za-z]*[rR]/.test(w.raw)); const gitC = name === 'git' && args[0]?.raw === '-C'; diff --git a/test/helpers/placeholder-allowlist.ts b/test/helpers/placeholder-allowlist.ts new file mode 100644 index 000000000..741d635eb --- /dev/null +++ b/test/helpers/placeholder-allowlist.ts @@ -0,0 +1,158 @@ +/** + * Reviewed identifier placeholders for generated bash (CEO-12, DX-5, ENG-8). + * + * A `` the model fills in may appear in a command only when it is + * listed here. Free text (bodies, titles, messages, briefs, prompts, error + * output, reviewer or diff text) is never listed: it reaches a command only as + * the contents of a `mktemp` file the agent writes with its file-write tool. + * + * Each entry names the grammar a value must match before the skill uses it; a + * value that fails is not used. `quoted` entries may appear only inside single + * or double quotes (or a quoted heredoc), and their grammar excludes every + * character that can end or expand those quotes: `'`, `"`, backtick, `$`, `\` + * and newline. Every other entry may appear unquoted, so its grammar also + * excludes whitespace and `;|&()<>*?[]{}!#`. test/generated-bash-placeholders + * .test.ts proves each grammar rejects those characters. + */ + +export interface IdentifierGrammar { grammar: RegExp; what: string; quoted?: true } + +const NUM = /^[0-9]+$/; +const ID = /^[A-Za-z0-9][A-Za-z0-9._-]*$/; +/** Shell-safe subset of git ref names; the posting skills also run `git check-ref-format --branch`. */ +const REF = /^[\p{L}\p{N}_][\p{L}\p{N}._/@+-]*$/u; +const PATH = /^[\p{L}\p{N}._/@+:,%=~-]+$/u; +/** Space-separated path lists (word splitting intended); `*` may glob. */ +const PATHS = /^[\p{L}\p{N}._/@+:,%=~* -]+$/u; +/** Inside quotes only: anything that cannot end or expand the quotes. */ +const QUOTED = /^[^'"`$\\\n]+$/; +const DATE = /^[0-9]{4}-[0-9]{2}-[0-9]{2}$/; +const NUMBER = /^-?[0-9]+(?:\.[0-9]+)?$/; +const URL = /^https?:\/\/[^'"`$\\\s<>]+$/; +const oneOf = (...alts: string[]) => new RegExp(`^(?:${alts.map(a => a.replace(/[.*+?^${}()|[\]\\/-]/g, '\\$&')).join('|')})$`); + +export const IDENTIFIER_PLACEHOLDERS: Record = { + // Enumerations: the value is one of the listed literals. + '': { grammar: oneOf('approval', 'clarification', 'routing', 'cherry-pick', 'feedback-loop'), what: 'question category' }, + '': { grammar: oneOf('one-way', 'two-way'), what: 'door type' }, + '': { grammar: oneOf('never-ask', 'always-ask', 'ask-only-for-one-way'), what: 'question preference' }, + '': { grammar: oneOf('never-ask', 'always-ask', 'ask-only-for-one-way'), what: 'question preference' }, + '': { grammar: oneOf('clean', 'flagged'), what: 'semantic review outcome' }, + '': { grammar: /^(?:[0-9]+(?:\.[0-9]+)?|null)$/, what: 'health score or null' }, + '': { grammar: oneOf('github', 'gitlab', 'unknown'), what: 'detected git platform' }, + + // Ids, keys and numbers. + '': { grammar: ID, what: 'question or record id (kebab-case)' }, + '': { grammar: ID, what: 'option key' }, + '': { grammar: ID, what: 'question summary as a kebab-case slug' }, + '': { grammar: NUM, what: 'GitHub comment id from the fetched JSON' }, + '': { grammar: /^[0-9]*$/, what: 'filed issue number, or empty when none was filed' }, + '': { grammar: NUM, what: 'canary check number' }, + '': { grammar: NUM, what: 'GitHub Actions run id' }, + '': { grammar: NUM, what: 'process id printed by an earlier block' }, + '': { grammar: ID, what: 'Codex session id printed by an earlier call' }, + '': { grammar: ID, what: 'freeze owner token printed by acquire' }, + '': { grammar: ID, what: 'run id printed by Setup' }, + '': { grammar: ID, what: 'run id printed by Setup' }, + '': { grammar: oneOf('ceo', 'design', 'dx', 'eng'), what: 'autoplan phase' }, + '': { grammar: /^[A-Z]$/, what: 'variant letter' }, + '': { grammar: ID, what: 'screen name slug' }, + '': { grammar: ID, what: 'screen name slug' }, + '': { grammar: ID, what: 'page name slug' }, + '': { grammar: ID, what: 'slug' }, + '': { grammar: ID, what: 'slug' }, + '': { grammar: ID, what: 'design dimension slug' }, + '': { grammar: ID, what: 'profile dimension name' }, + '': { grammar: NUMBER, what: 'profile dimension value' }, + '': { grammar: NUMBER, what: 'profile answer value' }, + '': { grammar: NUMBER, what: 'profile answer value' }, + '': { grammar: NUMBER, what: 'profile answer value' }, + '': { grammar: NUMBER, what: 'profile answer value' }, + '': { grammar: NUMBER, what: 'profile answer value' }, + '': { grammar: oneOf('claude', 'codex'), what: 'harness name' }, + '': { grammar: ID, what: 'eval suite name' }, + '': { grammar: /^[A-Za-z0-9._:,-]+$/, what: 'comma-separated model ids' }, + '': { grammar: /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})$/, what: 'GitHub login' }, + '': { grammar: /^[A-Za-z0-9_][A-Za-z0-9_.-]*$/, what: 'GitLab username' }, + + // Dates. + '': { grammar: DATE, what: 'YYYY-MM-DD' }, + '': { grammar: DATE, what: 'YYYY-MM-DD' }, + '': { grammar: /^[0-9]+[dhwm]$|^[0-9]{4}-[0-9]{2}-[0-9]{2}$/, what: 'retro window (7d, 24h, 2w) or date' }, + '': { grammar: /^[0-9]+[dhwm]$/, what: 'retro window (7d, 24h, 2w)' }, + + // Git refs. Branch names from remote data travel as data (ENG-8), never through this list. + '': { grammar: REF, what: 'base branch detected by the skill' }, + '': { grammar: REF, what: 'default branch detected by the skill' }, + '': { grammar: REF, what: 'diff base ref detected by the skill' }, + '': { grammar: REF, what: 'base branch detected by the skill' }, + '': { grammar: REF, what: 'current branch (git branch --show-current)' }, + '': { grammar: /^[0-9a-f]{7,40}$/, what: 'commit sha' }, + + // Paths printed by earlier gstack blocks or chosen by the skill. + '': { grammar: ID, what: 'basename of the mktemp body file printed by the previous block' }, + '': { grammar: ID, what: 'basename of the mktemp title file printed by the previous block' }, + '': { grammar: ID, what: 'basename of the mktemp reply file printed by the previous block' }, + '': { grammar: ID, what: 'basename of the mktemp draft file printed by the free-text block' }, + '': { grammar: ID, what: 'basename of the mktemp approach file printed by the free-text block' }, + '': { grammar: ID, what: 'basename of the mktemp text file printed by the previous block' }, + '': { grammar: PATH, what: 'mktemp directory printed by the Greptile fetch block' }, + '': { grammar: QUOTED, what: 'snapshot tool path printed by autoplan', quoted: true }, + '': { grammar: QUOTED, what: 'plan path', quoted: true }, + '': { grammar: QUOTED, what: 'plan path', quoted: true }, + '': { grammar: QUOTED, what: 'restore path printed by autoplan', quoted: true }, + '': { grammar: QUOTED, what: 'methodology path printed by autoplan', quoted: true }, + '': { grammar: QUOTED, what: 'checkpoint path printed by autoplan', quoted: true }, + '': { grammar: QUOTED, what: 'checkpoint path printed by autoplan', quoted: true }, + '': { grammar: QUOTED, what: 'prompt file path printed by the prepare step', quoted: true }, + '': { grammar: QUOTED, what: 'run directory printed by an earlier block', quoted: true }, + '': { grammar: QUOTED, what: 'Aside output directory printed by an earlier block', quoted: true }, + '': { grammar: QUOTED, what: 'Aside output directory printed by an earlier block', quoted: true }, + '': { grammar: QUOTED, what: 'report directory printed by Setup', quoted: true }, + '': { grammar: QUOTED, what: 'file path printed by the previous block', quoted: true }, + '': { grammar: QUOTED, what: 'install directory printed by Step 2', quoted: true }, + '': { grammar: QUOTED, what: 'approved design path', quoted: true }, + '': { grammar: QUOTED, what: 'image path printed by the design tool', quoted: true }, + '': { grammar: QUOTED, what: 'design doc path', quoted: true }, + '': { grammar: QUOTED, what: 'review directory printed by mktemp', quoted: true }, + '': { grammar: QUOTED, what: 'round file path', quoted: true }, + '': { grammar: QUOTED, what: 'round file path', quoted: true }, + '': { grammar: QUOTED, what: 'round file path', quoted: true }, + '': { grammar: QUOTED, what: 'round file path', quoted: true }, + '': { grammar: QUOTED, what: 'app source directory', quoted: true }, + '': { grammar: QUOTED, what: 'directory the user named', quoted: true }, + '': { grammar: QUOTED, what: 'directory detected by the skill', quoted: true }, + '': { grammar: QUOTED, what: 'staged HTML path', quoted: true }, + '': { grammar: QUOTED, what: 'staged directory printed by an earlier block', quoted: true }, + '': { grammar: PATH, what: 'output directory' }, + '': { grammar: PATH, what: 'repository path from discovery' }, + '': { grammar: PATH, what: 'sketch directory printed by an earlier block' }, + '': { grammar: PATH, what: 'finalized HTML path' }, + '': { grammar: PATH, what: 'finalized HTML path' }, + '': { grammar: PATH, what: 'approved variant image path' }, + '': { grammar: PATH, what: 'test file path' }, + '': { grammar: PATH, what: 'source file path' }, + '': { grammar: PATHS, what: 'space-separated file paths' }, + '': { grammar: PATHS, what: 'space-separated file paths' }, + '': { grammar: PATHS, what: 'space-separated file paths' }, + '': { grammar: PATHS, what: 'path or glob' }, + + // Quoted-only values: URLs, selectors, search words and commands. + '': { grammar: URL, what: 'http(s) URL', quoted: true }, + '': { grammar: URL, what: 'http(s) URL', quoted: true }, + '': { grammar: URL, what: 'http(s) URL', quoted: true }, + '': { grammar: URL, what: 'http(s) URL', quoted: true }, + '': { grammar: URL, what: 'http(s) URL', quoted: true }, + '': { grammar: URL, what: 'http(s) URL', quoted: true }, + '': { grammar: URL, what: 'http(s) URL', quoted: true }, + '': { grammar: QUOTED, what: 'CSS selector', quoted: true }, + '': { grammar: QUOTED, what: 'CSS selector', quoted: true }, + '': { grammar: QUOTED, what: 'CSS selector', quoted: true }, + '': { grammar: QUOTED, what: 'search words', quoted: true }, + '': { grammar: QUOTED, what: 'search words', quoted: true }, + '': { grammar: QUOTED, what: 'search word', quoted: true }, + '': { grammar: QUOTED, what: 'search word', quoted: true }, + '': { grammar: QUOTED, what: 'search word', quoted: true }, + '': { grammar: QUOTED, what: 'test command', quoted: true }, + '': { grammar: QUOTED, what: 'test command', quoted: true }, +};