From a853e413ad5f56035a3f200eae238bb096734783 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 6 Oct 2026 18:06:59 +0000 Subject: [PATCH] fix: reply and issue templates never pass untrusted text in double quotes (#3046) greptile-triage's reply APIs posted -f body="" while the templates quote SHAs and diff code in backticks; in double quotes bash ran each span (replies posted as 'Fixed in .', and diff or reviewer text ran as a command). The pre-existing-failure issue template and /spec's issue title had the same shape. All three now go through quoted heredocs. The generated-bash lint gains untrusted-in-quotes and now also covers runtime reference docs in skill directories (review/greptile-triage.md). Co-authored-by: dgrant --- review/greptile-triage.md | 16 ++++++-- .../preamble/generate-test-failure-triage.ts | 40 ++++++++++++------- ship/sections/tests.md | 40 ++++++++++++------- spec/sections/gate-and-file.md | 8 +++- spec/sections/gate-and-file.md.tmpl | 8 +++- test/env-host-fence-conformance.test.ts | 23 +++++++++++ test/helpers/generated-bash-lint.ts | 19 ++++++++- test/spec-template-invariants.test.ts | 4 +- 8 files changed, 119 insertions(+), 39 deletions(-) diff --git a/review/greptile-triage.md b/review/greptile-triage.md index 2f0072eed..862ceb242 100644 --- a/review/greptile-triage.md +++ b/review/greptile-triage.md @@ -94,18 +94,28 @@ For each non-suppressed comment: ## Reply APIs -When replying to Greptile comments, use the correct endpoint based on comment source: +When replying to Greptile comments, use the correct endpoint based on comment source. + +**Pass the reply body through a quoted heredoc, never inside double quotes.** Replies quote commit SHAs, diff lines and reviewer text in backticks. Inside `"..."` the shell runs every backtick span as a command: the reply posts with the text missing ("Fixed in ."), and untrusted text from the diff or the comment runs on this machine. The quoted delimiter (`'GSTACK_REPLY'`) makes the shell expand nothing, and expanding `"$REPLY_BODY"` afterwards never re-runs backticks inside its value. **Line-level comments** (from `pulls/$PR/comments`): ```bash +REPLY_BODY=$(cat <<'GSTACK_REPLY' + +GSTACK_REPLY +) gh api repos/$REPO/pulls/$PR_NUMBER/comments/$COMMENT_ID/replies \ - -f body="" + -f body="$REPLY_BODY" ``` **Top-level comments** (from `issues/$PR/comments`): ```bash +REPLY_BODY=$(cat <<'GSTACK_REPLY' + +GSTACK_REPLY +) gh api repos/$REPO/issues/$PR_NUMBER/comments \ - -f body="" + -f body="$REPLY_BODY" ``` **If a reply POST fails** (e.g., PR was closed, no write permission): warn and continue. Do not stop the workflow for a failed reply. diff --git a/scripts/resolvers/preamble/generate-test-failure-triage.ts b/scripts/resolvers/preamble/generate-test-failure-triage.ts index 1a1262b2c..ab3602d99 100644 --- a/scripts/resolvers/preamble/generate-test-failure-triage.ts +++ b/scripts/resolvers/preamble/generate-test-failure-triage.ts @@ -67,21 +67,31 @@ Ask with AskUserQuestion in the AskUserQuestion Format. List each failure the sa git log --format="%an (%ae)" -1 -- \`\`\` If these are different people, prefer the production code author — they likely introduced the regression. -- Create an issue assigned to that person (use the platform detected in Step 0): - - **If GitHub:** - \`\`\`bash - gh issue create \\ - --title "Pre-existing test failure: " \\ - --body "Found failing on branch . Failure is pre-existing.\\n\\n**Error:**\\n\`\`\`\\n\\n\`\`\`\\n\\n**Last modified by:** \\n**Noticed by:** gstack /ship on " \\ - --assignee "" - \`\`\` - - **If GitLab:** - \`\`\`bash - glab issue create \\ - -t "Pre-existing test failure: " \\ - -d "Found failing on branch . Failure is pre-existing.\\n\\n**Error:**\\n\`\`\`\\n\\n\`\`\`\\n\\n**Last modified by:** \\n**Noticed by:** gstack /ship on " \\ - -a "" - \`\`\` +- Create an issue assigned to that person; run only your platform's create line. Test names and errors stay inside the quoted heredocs (column 0): in a double-quoted argument their backticks run as commands. + +\`\`\`bash +ISSUE_TITLE=$(cat <<'GSTACK_ISSUE' +Pre-existing test failure: +GSTACK_ISSUE +) +ISSUE_BODY=$(cat <<'GSTACK_ISSUE' +Failing on ; pre-existing. + +**Error:** +~~~ + +~~~ + +**Last modified by:** +**Noticed by:** gstack /ship on +GSTACK_ISSUE +) +# GitHub: +gh issue create --title "$ISSUE_TITLE" --body "$ISSUE_BODY" --assignee "" +# GitLab: +glab issue create -t "$ISSUE_TITLE" -d "$ISSUE_BODY" -a "" +\`\`\` + - If neither CLI is available or \`--assignee\`/\`-a\` fails (user not in org, etc.), create the issue without assignee and note who should look at it in the body. - Continue with the workflow. diff --git a/ship/sections/tests.md b/ship/sections/tests.md index 6e18b5a0c..44b7efb2f 100644 --- a/ship/sections/tests.md +++ b/ship/sections/tests.md @@ -296,21 +296,31 @@ Ask with AskUserQuestion in the AskUserQuestion Format. List each failure the sa git log --format="%an (%ae)" -1 -- ``` If these are different people, prefer the production code author — they likely introduced the regression. -- Create an issue assigned to that person (use the platform detected in Step 0): - - **If GitHub:** - ```bash - gh issue create \ - --title "Pre-existing test failure: " \ - --body "Found failing on branch . Failure is pre-existing.\n\n**Error:**\n```\n\n```\n\n**Last modified by:** \n**Noticed by:** gstack /ship on " \ - --assignee "" - ``` - - **If GitLab:** - ```bash - glab issue create \ - -t "Pre-existing test failure: " \ - -d "Found failing on branch . Failure is pre-existing.\n\n**Error:**\n```\n\n```\n\n**Last modified by:** \n**Noticed by:** gstack /ship on " \ - -a "" - ``` +- Create an issue assigned to that person; run only your platform's create line. Test names and errors stay inside the quoted heredocs (column 0): in a double-quoted argument their backticks run as commands. + +```bash +ISSUE_TITLE=$(cat <<'GSTACK_ISSUE' +Pre-existing test failure: +GSTACK_ISSUE +) +ISSUE_BODY=$(cat <<'GSTACK_ISSUE' +Failing on ; pre-existing. + +**Error:** +~~~ + +~~~ + +**Last modified by:** +**Noticed by:** gstack /ship on +GSTACK_ISSUE +) +# GitHub: +gh issue create --title "$ISSUE_TITLE" --body "$ISSUE_BODY" --assignee "" +# GitLab: +glab issue create -t "$ISSUE_TITLE" -d "$ISSUE_BODY" -a "" +``` + - If neither CLI is available or `--assignee`/`-a` fails (user not in org, etc.), create the issue without assignee and note who should look at it in the body. - Continue with the workflow. diff --git a/spec/sections/gate-and-file.md b/spec/sections/gate-and-file.md index 66b5f3ce2..78d7730ad 100644 --- a/spec/sections/gate-and-file.md +++ b/spec/sections/gate-and-file.md @@ -244,10 +244,14 @@ reuse it; write the exact bytes to `$REDACT_FILE`; `~/.claude/skills/gstack/bin/ exit-3/2/0 handling. On exit 3, do NOT file the issue; HIGH has no skip. Pass the same `$REDACT_FILE` downstream so the bytes scanned are the bytes sent. -If `gh` is available and authenticated, file from the scanned temp file: +If `gh` is available and authenticated, file from the scanned temp file. Keep the title inside the quoted heredoc (backticks in a double-quoted title would run as shell commands): ```bash -ISSUE_URL=$(gh issue create --title "" --body-file "$REDACT_FILE") +ISSUE_TITLE=$(cat <<'GSTACK_TITLE' +<title> +GSTACK_TITLE +) +ISSUE_URL=$(gh issue create --title "$ISSUE_TITLE" --body-file "$REDACT_FILE") ISSUE_NUMBER=$(echo "$ISSUE_URL" | sed -E 's|.*/issues/([0-9]+)$|\1|') echo "Filed: $ISSUE_URL" ~/.claude/skills/gstack/bin/gstack-decision-log '{"decision":"Spec filed #ISSUE_NUMBER: TITLE","rationale":"APPROACH","scope":"issue","issue":"ISSUE_NUMBER","source":"skill","confidence":7}' 2>/dev/null || true diff --git a/spec/sections/gate-and-file.md.tmpl b/spec/sections/gate-and-file.md.tmpl index 5cec22a2c..e989a1050 100644 --- a/spec/sections/gate-and-file.md.tmpl +++ b/spec/sections/gate-and-file.md.tmpl @@ -118,10 +118,14 @@ never saw, and the issue is world-readable): {{REDACT_INVOCATION_BLOCK:pre-issue:brief}} -If `gh` is available and authenticated, file from the scanned temp file: +If `gh` is available and authenticated, file from the scanned temp file. Keep the title inside the quoted heredoc (backticks in a double-quoted title would run as shell commands): ```bash -ISSUE_URL=$(gh issue create --title "<title>" --body-file "$REDACT_FILE") +ISSUE_TITLE=$(cat <<'GSTACK_TITLE' +<title> +GSTACK_TITLE +) +ISSUE_URL=$(gh issue create --title "$ISSUE_TITLE" --body-file "$REDACT_FILE") ISSUE_NUMBER=$(echo "$ISSUE_URL" | sed -E 's|.*/issues/([0-9]+)$|\1|') echo "Filed: $ISSUE_URL" ~/.claude/skills/gstack/bin/gstack-decision-log '{"decision":"Spec filed #ISSUE_NUMBER: TITLE","rationale":"APPROACH","scope":"issue","issue":"ISSUE_NUMBER","source":"skill","confidence":7}' 2>/dev/null || true diff --git a/test/env-host-fence-conformance.test.ts b/test/env-host-fence-conformance.test.ts index 872f3d304..d54b9dad2 100644 --- a/test/env-host-fence-conformance.test.ts +++ b/test/env-host-fence-conformance.test.ts @@ -20,6 +20,7 @@ import { binaryAssignment, fencePrelude, insertRuntimePreludes, PRELUDE_BYTE_BUD import { HOST_PATHS, type TemplateContext } from '../scripts/resolvers/types'; import { lintFence, normalizePlaceholders } from './helpers/generated-bash-lint'; +const ROOT = path.resolve(import.meta.dir, '..'); const ENV_HOSTS = ALL_HOST_CONFIGS.filter(h => h.usesEnvVars); const FORBIDDEN = /^(?:\/bin\/|\/browse|\/design|\/gstack-)/; const PRELUDE_LINE = /^\[ -d "\$\{GSTACK_ROOT:-\/-\}\/bin" \]|^(?:GSTACK_(?:BIN|BROWSE|DESIGN|MAKE_PDF)=\$GSTACK_ROOT\/\S+ ?)+$|^[BD]=\$GSTACK_ROOT\//; @@ -268,6 +269,21 @@ describe('INV-3: every rendered fence parses, runs its prelude and passes the li }); }); +describe('#3046: runtime reference docs that skills execute pass the lint', () => { + test('every non-generated markdown file in a skill directory (review/greptile-triage.md and friends)', () => { + const skillDirs = fs.readdirSync(ROOT, { withFileTypes: true }) + .filter(e => e.isDirectory() && fs.existsSync(path.join(ROOT, e.name, 'SKILL.md.tmpl'))) + .map(e => path.join(ROOT, e.name)); + const docs = skillDirs.flatMap(dir => fs.readdirSync(dir) + .filter(f => f.endsWith('.md') && f !== 'SKILL.md') + .map(f => path.join(dir, f))); + expect(docs.map(d => path.relative(ROOT, d))).toContain('review/greptile-triage.md'); + const findings = docs.flatMap(file => bashFences(fs.readFileSync(file, 'utf8'), path.relative(ROOT, file)) + .flatMap(f => lintFence(f.body).map(x => `${f.file}:${f.line + x.line} ${x.rule}: ${x.detail}`))); + expect(findings).toEqual([]); + }); +}); + describe('INV-3: negative controls (planted bad fences fail each check)', () => { test('lint rules', () => { const rules = (body: string) => lintFence(body).map(f => f.rule); @@ -286,6 +302,13 @@ describe('INV-3: negative controls (planted bad fences fail each check)', () => expect(rules('ls | while IFS= read -r d; do rm -rf "$d"; done')).toEqual([]); expect(rules('cat <<EOF\ncd "$NOT_CODE"\nEOF\necho ok')).toEqual([]); expect(rules("echo 'cd \"$QUOTED\"'")).toEqual([]); + // #3046: reviewer, diff and error text inside a double-quoted body runs its backticks. + expect(rules('gh api repos/o/r/issues/1/comments -f body="<reply text>"')).toEqual(['untrusted-in-quotes']); + expect(rules('gh issue create --title "Failure: <test-name>" --body-file "$F"')).toEqual(['untrusted-in-quotes']); + expect(rules('glab issue create -t "$T" -d "Error: <first 10 lines>"')).toEqual(['untrusted-in-quotes']); + expect(rules("B=$(cat <<'GSTACK_REPLY'\n**Fixed** in `<sha>`.\nGSTACK_REPLY\n)\ngh api x -f body=\"$B\"")).toEqual([]); + expect(rules("git commit -m \"$(cat <<'EOF'\nfix: <summary>\n\nCo-Authored-By: Claude <noreply@anthropic.com>\nEOF\n)\"")).toEqual([]); + expect(rules('gh pr view <number> --json title')).toEqual([]); }); test('placeholder normalization keeps real syntax errors', () => { diff --git a/test/helpers/generated-bash-lint.ts b/test/helpers/generated-bash-lint.ts index 1064a27a5..f4c0c35fe 100644 --- a/test/helpers/generated-bash-lint.ts +++ b/test/helpers/generated-bash-lint.ts @@ -11,12 +11,17 @@ * 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 `<placeholder>` 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. * * 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'; line: number; detail: string } +export interface LintFinding { rule: 'tilde-in-quotes' | 'mktemp-template' | 'unguarded-var' | 'untrusted-in-quotes'; line: number; detail: string } interface VarRef { name: string; op: string } interface Word { raw: string; vars: VarRef[]; line: number } @@ -26,6 +31,11 @@ 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]*>[^"]*"/; function lex(src: string, findings: LintFinding[]): Command[] { const commands: Command[] = []; @@ -181,6 +191,13 @@ 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, ' ') }); + }); + } const recursiveRm = name === 'rm' && args.some(w => /^-[A-Za-z]*[rR]/.test(w.raw)); const gitC = name === 'git' && args[0]?.raw === '-C'; if (name === 'cd' || name === 'mv' || recursiveRm || gitC) { diff --git a/test/spec-template-invariants.test.ts b/test/spec-template-invariants.test.ts index 4b43b8627..a82638f8f 100644 --- a/test/spec-template-invariants.test.ts +++ b/test/spec-template-invariants.test.ts @@ -184,7 +184,9 @@ describe('/spec redaction at every sink (scan-at-sink, carved: gate-and-file sec expect(fileIdx).toBeGreaterThan(scanIdx); }); test('files from the scanned temp file (exact bytes, not a re-render)', () => { - expect(SEC_GEN).toMatch(/gh issue create --title "<title>" --body-file "\$REDACT_FILE"/); + expect(SEC_GEN).toMatch(/gh issue create --title "\$ISSUE_TITLE" --body-file "\$REDACT_FILE"/); + // #3046: the title reaches the shell only through a quoted heredoc. + expect(SEC_GEN).toMatch(/ISSUE_TITLE=\$\(cat <<'GSTACK_TITLE'\n<title>\nGSTACK_TITLE\n\)/); }); test('scan precedes the archive write (pre-archive)', () => { const scanIdx = SEC_GEN.indexOf('Re-scan before archiving');