Skip to content

Potential fix for code scanning alert no. 11: Uncontrolled command line #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
involvex merged 1 commit into from
Jan 31, 2026
Merged

Potential fix for code scanning alert no. 11: Uncontrolled command line #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@
"react": "^19.2.4",
"ripgrep-node": "^1.0.0",
"tiktoken": "^1.0.22",
"ws": "^8.19.0"
"ws": "^8.19.0",
"shell-quote": "^1.8.3"
},
"devDependencies": {
"@types/fs-extra": "^11.0.4",
Expand Down
80 changes: 77 additions & 3 deletions src/tools/bash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,88 @@
import { ConfirmationService } from "../utils/confirmation-service";
import { ToolResult } from "../types/index";
import { exec } from "child_process";
import { spawn } from "child_process";
import { promisify } from "util";
import * as shellQuote from "shell-quote";

const execAsync = promisify(exec);
const promisifyFn = promisify; // kept in case other parts of this file evolve to use promisify

export class BashTool {
private currentDirectory: string = process.cwd();
private confirmationService = ConfirmationService.getInstance();

private async runCommandSafely(
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (complexity): Consider replacing the custom spawn-based implementation of runCommandSafely with promisified execFile to keep safety guarantees while simplifying the control flow and resource handling logic.

You can keep the “safer command handling” (no shell, timeout, maxBuffer) while dropping most of the custom exec-like implementation by switching to execFile instead of manual spawn wiring.

This removes:

  • manual stdout/stderr buffering
  • explicit maxBuffer / timeout management
  • killedForTimeout / killedForBuffer flags and custom timer
  • the unused promisifyFn alias

and keeps:

  • no shell invocation
  • timeout and maxBuffer behavior
  • command: string API with shell-quote parsing

Example refactor:

import { spawn, execFile } from "child_process"; // or drop `spawn` if unused
import { promisify } from "util";
import * as shellQuote from "shell-quote";

const execFileAsync = promisify(execFile);

export class BashTool {
  // ...

  private async runCommandSafely(
    command: string,
    options: { cwd?: string; timeout?: number; maxBuffer?: number },
  ): Promise<{ stdout: string; stderr: string }> {
    const argv = shellQuote
      .parse(command)
      .filter(token => typeof token === "string") as string[];

    if (argv.length === 0) {
      return { stdout: "", stderr: "" };
    }

    const [file, ...args] = argv;

    // execFile does no shell, and honors timeout/maxBuffer
    const { stdout, stderr } = await execFileAsync(file, args, {
      cwd: options.cwd,
      timeout: options.timeout ?? 30000,
      maxBuffer: options.maxBuffer ?? 1024 * 1024,
    });

    return { stdout, stderr };
  }
}

This preserves the new safety behavior and resource limits but substantially reduces control-flow and implementation complexity.

command: string,
options: { cwd?: string; timeout?: number; maxBuffer?: number },
): Promise<{ stdout: string; stderr: string }> {
const parsed = shellQuote.parse(command);
const argv = parsed
.filter(token => typeof token === "string")
.map(token => token as string);

if (argv.length === 0) {
return { stdout: "", stderr: "" };
}

const cmd = argv[0];
const args = argv.slice(1);

return await new Promise((resolve, reject) => {
const child = spawn(cmd, args, {
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security (javascript.lang.security.detect-child-process): Detected calls to child_process from a function argument command. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.

Source: opengrep

cwd: options.cwd,
shell: false,
});

let stdout = "";
let stderr = "";
const maxBuffer = options.maxBuffer ?? 1024 * 1024;
const timeout = options.timeout ?? 30000;

let killedForTimeout = false;
let killedForBuffer = false;

const timer = setTimeout(() => {
killedForTimeout = true;
child.kill();
}, timeout);

child.stdout?.on("data", (data: Buffer | string) => {
const chunk = data.toString();
if (stdout.length + chunk.length > maxBuffer) {
killedForBuffer = true;
child.kill();
return;
}
stdout += chunk;
});

child.stderr?.on("data", (data: Buffer | string) => {
const chunk = data.toString();
if (stderr.length + chunk.length > maxBuffer) {
killedForBuffer = true;
child.kill();
return;
}
stderr += chunk;
});

child.on("error", error => {
clearTimeout(timer);
reject(error);
});

child.on("close", code => {
clearTimeout(timer);
if (killedForTimeout) {
Comment on lines +73 to +75
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Non-zero exit codes no longer cause a rejected promise, changing error handling semantics vs. exec.

Previously, execAsync rejected on non-zero exit codes, allowing callers to use try/catch for failures. Here the promise resolves regardless of code (unless killed/errored), so failing commands can appear successful. Please add an explicit code !== 0 check in the close handler and reject with an Error that includes the exit code (and maybe truncated stderr) to preserve the prior behavior and avoid regressions.

return reject(new Error("Command timed out"));
}
if (killedForBuffer) {
return reject(new Error("Command output exceeded buffer limit"));
}
resolve({ stdout, stderr });
});
});
}

async execute(command: string, timeout: number = 30000): Promise<ToolResult> {
try {
// Check if user has already accepted bash commands for this session
Expand Down Expand Up @@ -53,7 +127,7 @@ export class BashTool {
}
}

const { stdout, stderr } = await execAsync(command, {
const { stdout, stderr } = await this.runCommandSafely(command, {
cwd: this.currentDirectory,
timeout,
maxBuffer: 1024 * 1024,
Expand Down
Loading