From a5e5d3b82e68f62fd7da6e6cdea64e0191cd9a93 Mon Sep 17 00:00:00 2001 From: alexglynn Date: Sat, 13 Jun 2026 22:45:14 -0700 Subject: [PATCH] fix(parse-sdk-options): prevent shell-quote from collapsing unquoted Bash(X:*) rules to bare Bash (#1350) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(parse-sdk-options): prevent shell-quote from collapsing unquoted Bash(X:*) rules to bare Bash shell-quote's parse() tokenizes unquoted `(`, `)` as control operators and barewords containing `*` as glob ops, all returned as non-string objects. parseClaudeArgsToExtraArgs filtered those out, so an unquoted `--allowedTools View,Bash(gh:*),Bash(cat:*)` collapsed to bare `Bash` — silently widening scoped permission rules to unrestricted Bash(*). Escape shell control metachars to Unicode private-use placeholders before parse() and restore after; extract .pattern from glob ops. Preserves existing quote/whitespace handling. Co-Authored-By: Claude Opus 4.7 * ci: retrigger Co-Authored-By: Claude Opus 4.7 --------- Co-authored-by: Claude Opus 4.7 --- base-action/src/parse-sdk-options.ts | 45 ++++++++- base-action/test/parse-sdk-options.test.ts | 104 +++++++++++++++++++++ 2 files changed, 146 insertions(+), 3 deletions(-) diff --git a/base-action/src/parse-sdk-options.ts b/base-action/src/parse-sdk-options.ts index ec65b8fb..260dd384 100644 --- a/base-action/src/parse-sdk-options.ts +++ b/base-action/src/parse-sdk-options.ts @@ -24,6 +24,35 @@ const ACCUMULATING_FLAGS = new Set([ // Delimiter used to join accumulated flag values const ACCUMULATE_DELIMITER = "\x00"; +// shell-quote treats ()|&;<> as control operators and splits adjacent text +// around them into separate tokens (returned as `{op}` objects, which we then +// dropped). For CLI args these must be literal characters — e.g. unquoted +// `--allowedTools Bash(gh:*)` was being mangled into bare `Bash`, silently +// widening a scoped permission rule to Bash(*). We escape each metachar to a +// Unicode private-use codepoint before parsing and restore it afterward, +// keeping shell-quote's quote/whitespace handling intact. +const SHELL_META_PAIRS: [string, string][] = [ + ["(", ""], + [")", ""], + ["|", ""], + ["&", ""], + [";", ""], + ["<", ""], + [">", ""], +]; +const SHELL_META_ESCAPE = new Map(SHELL_META_PAIRS); +const SHELL_META_UNESCAPE = new Map(SHELL_META_PAIRS.map(([k, v]) => [v, k])); +const SHELL_META_ESCAPE_RE = /[()|&;<>]/g; +const SHELL_META_UNESCAPE_RE = /[-]/g; + +function escapeShellMeta(s: string): string { + return s.replace(SHELL_META_ESCAPE_RE, (c) => SHELL_META_ESCAPE.get(c)!); +} + +function unescapeShellMeta(s: string): string { + return s.replace(SHELL_META_UNESCAPE_RE, (c) => SHELL_META_UNESCAPE.get(c)!); +} + type McpConfig = { mcpServers?: Record; }; @@ -106,9 +135,19 @@ function parseClaudeArgsToExtraArgs( if (!claudeArgs?.trim()) return {}; const result: Record = {}; - const args = parseShellArgs(stripShellComments(claudeArgs)).filter( - (arg): arg is string => typeof arg === "string", - ); + const args = parseShellArgs(escapeShellMeta(stripShellComments(claudeArgs))) + .map((arg) => { + if (typeof arg === "string") return unescapeShellMeta(arg); + // With control metachars escaped above, the only non-string shell-quote + // can still emit is a glob op (bareword containing *, ?, or [). Its + // `pattern` field is the verbatim token text — use it as-is so values + // like `Bash(cmd:*)` and `Read(path/**)` round-trip intact. + if (typeof arg === "object" && arg !== null && "pattern" in arg) { + return unescapeShellMeta((arg as { pattern: string }).pattern); + } + return undefined; + }) + .filter((arg): arg is string => typeof arg === "string"); for (let i = 0; i < args.length; i++) { const arg = args[i]; diff --git a/base-action/test/parse-sdk-options.test.ts b/base-action/test/parse-sdk-options.test.ts index c74d98e9..751d7d76 100644 --- a/base-action/test/parse-sdk-options.test.ts +++ b/base-action/test/parse-sdk-options.test.ts @@ -137,6 +137,110 @@ describe("parseSdkOptions", () => { ]); }); + test("should preserve unquoted Bash(cmd:*) rules instead of collapsing to bare Bash", () => { + // Regression: shell-quote tokenizes unquoted `(`/`)` as control ops and + // `*` as a glob, which were filtered out — collapsing scoped rules like + // `Bash(gh:*)` into bare `Bash` (= Bash(*), unrestricted shell). + const options: ClaudeOptions = { + claudeArgs: "--allowedTools View,Bash(gh:*),Bash(cat:*)", + }; + + const result = parseSdkOptions(options); + + expect(result.sdkOptions.allowedTools).toEqual([ + "View", + "Bash(gh:*)", + "Bash(cat:*)", + ]); + expect(result.sdkOptions.allowedTools).not.toContain("Bash"); + }); + + test("should preserve unquoted space-separated Bash(cmd:*) rules", () => { + const options: ClaudeOptions = { + claudeArgs: "--allowed-tools Bash(gh:*) Bash(cat:*) Read(//tmp/**)", + }; + + const result = parseSdkOptions(options); + + expect(result.sdkOptions.allowedTools).toEqual([ + "Bash(gh:*)", + "Bash(cat:*)", + "Read(//tmp/**)", + ]); + expect(result.sdkOptions.allowedTools).not.toContain("Bash"); + }); + + test("should preserve unquoted Tool(content) rules without glob chars", () => { + const options: ClaudeOptions = { + claudeArgs: + "--allowedTools Read(~/file),WebFetch(domain:example.com),Edit", + }; + + const result = parseSdkOptions(options); + + expect(result.sdkOptions.allowedTools).toEqual([ + "Read(~/file)", + "WebFetch(domain:example.com)", + "Edit", + ]); + }); + + test("should still preserve quoted Bash(cmd:*) rules (no regression)", () => { + const options: ClaudeOptions = { + claudeArgs: '--allowedTools "Bash(gh:*),Bash(cat:*)"', + }; + + const result = parseSdkOptions(options); + + expect(result.sdkOptions.allowedTools).toEqual([ + "Bash(gh:*)", + "Bash(cat:*)", + ]); + }); + + test("should merge quoted tag-mode tools with unquoted user tools without widening", () => { + // Real-world shape: the action's tag mode wraps its own --allowedTools in + // double quotes, then appends the user's claude_args (typically unquoted + // in workflow YAML). Both halves must round-trip. + const options: ClaudeOptions = { + claudeArgs: + '--permission-mode acceptEdits --allowedTools "Glob,Grep,Read,Bash(git add:*),Bash(git commit:*)" ' + + "--model claude-opus-4-7\n" + + "--allowedTools View,Bash(gh:*),Bash(printf:*),Bash(cat:*)", + }; + + const result = parseSdkOptions(options); + + expect(result.sdkOptions.allowedTools).toEqual([ + "Glob", + "Grep", + "Read", + "Bash(git add:*)", + "Bash(git commit:*)", + "View", + "Bash(gh:*)", + "Bash(printf:*)", + "Bash(cat:*)", + ]); + expect(result.sdkOptions.allowedTools).not.toContain("Bash"); + }); + + test("should preserve unquoted disallowedTools rules without widening", () => { + // Same bug class on the deny side: a scoped deny collapsing to bare + // `Bash` would block all shell instead of the intended prefix. + const options: ClaudeOptions = { + claudeArgs: "--disallowedTools Bash(rm:*),Bash(sudo:*)", + }; + + const result = parseSdkOptions(options); + + expect(result.sdkOptions.disallowedTools).toEqual([ + "Bash(rm:*)", + "Bash(sudo:*)", + ]); + expect(result.sdkOptions.disallowedTools).not.toContain("Bash"); + }); + test("should handle mixed camelCase and hyphenated allowedTools flags", () => { const options: ClaudeOptions = { claudeArgs: '--allowedTools "Edit,Read" --allowed-tools "Write,Glob"',