mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-05 08:23:59 +01:00
fix: reply and issue templates never pass untrusted text in double quotes (#3046)
greptile-triage's reply APIs posted -f body="<reply text>" 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 <dgrant@users.noreply.github.com>
This commit is contained in:
1 parent
f0531a229f
commit
a853e413ad
8 files changed
+119
-39
No files matched your search
@@ -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'
|
||||
<reply text>
|
||||
GSTACK_REPLY
|
||||
)
|
||||
gh api repos/$REPO/pulls/$PR_NUMBER/comments/$COMMENT_ID/replies \
|
||||
-f body="<reply text>"
|
||||
-f body="$REPLY_BODY"
|
||||
```
|
||||
|
||||
**Top-level comments** (from `issues/$PR/comments`):
|
||||
```bash
|
||||
REPLY_BODY=$(cat <<'GSTACK_REPLY'
|
||||
<reply text>
|
||||
GSTACK_REPLY
|
||||
)
|
||||
gh api repos/$REPO/issues/$PR_NUMBER/comments \
|
||||
-f body="<reply text>"
|
||||
-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.
|
||||
|
||||
@@ -67,21 +67,31 @@ Ask with AskUserQuestion in the AskUserQuestion Format. List each failure the sa
|
||||
git log --format="%an (%ae)" -1 -- <source-file-under-test>
|
||||
\`\`\`
|
||||
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: <test-name>" \\
|
||||
--body "Found failing on branch <current-branch>. Failure is pre-existing.\\n\\n**Error:**\\n\`\`\`\\n<first 10 lines>\\n\`\`\`\\n\\n**Last modified by:** <author>\\n**Noticed by:** gstack /ship on <date>" \\
|
||||
--assignee "<github-username>"
|
||||
\`\`\`
|
||||
- **If GitLab:**
|
||||
\`\`\`bash
|
||||
glab issue create \\
|
||||
-t "Pre-existing test failure: <test-name>" \\
|
||||
-d "Found failing on branch <current-branch>. Failure is pre-existing.\\n\\n**Error:**\\n\`\`\`\\n<first 10 lines>\\n\`\`\`\\n\\n**Last modified by:** <author>\\n**Noticed by:** gstack /ship on <date>" \\
|
||||
-a "<gitlab-username>"
|
||||
\`\`\`
|
||||
- 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: <test-name>
|
||||
GSTACK_ISSUE
|
||||
)
|
||||
ISSUE_BODY=$(cat <<'GSTACK_ISSUE'
|
||||
Failing on <current-branch>; pre-existing.
|
||||
|
||||
**Error:**
|
||||
~~~
|
||||
<first 10 lines>
|
||||
~~~
|
||||
|
||||
**Last modified by:** <author>
|
||||
**Noticed by:** gstack /ship on <date>
|
||||
GSTACK_ISSUE
|
||||
)
|
||||
# GitHub:
|
||||
gh issue create --title "$ISSUE_TITLE" --body "$ISSUE_BODY" --assignee "<github-username>"
|
||||
# GitLab:
|
||||
glab issue create -t "$ISSUE_TITLE" -d "$ISSUE_BODY" -a "<gitlab-username>"
|
||||
\`\`\`
|
||||
|
||||
- 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.
|
||||
|
||||
|
||||
+25
-15
@@ -296,21 +296,31 @@ Ask with AskUserQuestion in the AskUserQuestion Format. List each failure the sa
|
||||
git log --format="%an (%ae)" -1 -- <source-file-under-test>
|
||||
```
|
||||
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: <test-name>" \
|
||||
--body "Found failing on branch <current-branch>. Failure is pre-existing.\n\n**Error:**\n```\n<first 10 lines>\n```\n\n**Last modified by:** <author>\n**Noticed by:** gstack /ship on <date>" \
|
||||
--assignee "<github-username>"
|
||||
```
|
||||
- **If GitLab:**
|
||||
```bash
|
||||
glab issue create \
|
||||
-t "Pre-existing test failure: <test-name>" \
|
||||
-d "Found failing on branch <current-branch>. Failure is pre-existing.\n\n**Error:**\n```\n<first 10 lines>\n```\n\n**Last modified by:** <author>\n**Noticed by:** gstack /ship on <date>" \
|
||||
-a "<gitlab-username>"
|
||||
```
|
||||
- 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: <test-name>
|
||||
GSTACK_ISSUE
|
||||
)
|
||||
ISSUE_BODY=$(cat <<'GSTACK_ISSUE'
|
||||
Failing on <current-branch>; pre-existing.
|
||||
|
||||
**Error:**
|
||||
~~~
|
||||
<first 10 lines>
|
||||
~~~
|
||||
|
||||
**Last modified by:** <author>
|
||||
**Noticed by:** gstack /ship on <date>
|
||||
GSTACK_ISSUE
|
||||
)
|
||||
# GitHub:
|
||||
gh issue create --title "$ISSUE_TITLE" --body "$ISSUE_BODY" --assignee "<github-username>"
|
||||
# GitLab:
|
||||
glab issue create -t "$ISSUE_TITLE" -d "$ISSUE_BODY" -a "<gitlab-username>"
|
||||
```
|
||||
|
||||
- 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.
|
||||
|
||||
|
||||
@@ -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 "<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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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');
|
||||
|
||||
Reference in new issue
Block a user