Potential fix for code scanning alert no. 11: Uncontrolled command line#7
Merged
Potential fix for code scanning alert no. 11: Uncontrolled command line#7
Conversation
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Reviewer's GuideReplaces shell-based command execution in the Bash tool with a safer spawn-based implementation that parses the command string into argv tokens using shell-quote, while preserving the public BashTool API and adding shell-quote as a dependency. Sequence diagram for safe bash command execution via spawnsequenceDiagram
actor Caller
participant BashTool
participant ShellQuote
participant ChildProcess
Caller->>BashTool: execute(command, timeout?)
BashTool->>BashTool: checkConfirmation()
alt command startsWith cd
BashTool->>BashTool: update currentDirectory
BashTool-->>Caller: ToolResult (no external command)
else normal command
BashTool->>BashTool: runCommandSafely(command, options)
BashTool->>ShellQuote: parse(command)
ShellQuote-->>BashTool: argv tokens
BashTool->>ChildProcess: spawn(cmd, args, cwd, shell=false)
par capture stdout
ChildProcess-->>BashTool: stdout data events
BashTool->>BashTool: append to stdout buffer
and capture stderr
ChildProcess-->>BashTool: stderr data events
BashTool->>BashTool: append to stderr buffer
end
BashTool->>BashTool: start timeout timer
alt timeout reached
BashTool->>ChildProcess: kill()
ChildProcess-->>BashTool: close
BashTool-->>Caller: error Command timed out
else buffer exceeded
BashTool->>ChildProcess: kill()
ChildProcess-->>BashTool: close
BashTool-->>Caller: error Command output exceeded buffer limit
else normal completion
ChildProcess-->>BashTool: close with exit code
BashTool-->>Caller: ToolResult with stdout, stderr
end
end
Updated class diagram for BashTool with runCommandSafelyclassDiagram
class BashTool {
- currentDirectory : string
- confirmationService
- runCommandSafely(command : string, options : cwd? string, timeout? number, maxBuffer? number) Promise~stdout:string, stderr:string~
+ execute(command : string, timeout : number = 30000) Promise~ToolResult~
}
class ConfirmationService {
+ getInstance() ConfirmationService
}
class ToolResult
class ShellQuote {
+ parse(command : string) any[]
}
class ChildProcessSpawn {
+ spawn(cmd : string, args : string[], options : cwd? string, shell? boolean)
}
BashTool --> ConfirmationService : uses
BashTool --> ToolResult : returns
BashTool --> ShellQuote : uses parse
BashTool --> ChildProcessSpawn : uses spawn
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 2 other issues, and left some high level feedback:
Security issues:
- 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. (link)
General comments:
- The
runCommandSafelyhelper filtersshell-quote.parseoutput down to only string tokens, which will silently drop things like redirections, pipes, or env assignments; consider explicitly detecting non-string tokens and failing with a clear error rather than changing the behavior of such commands in a non-obvious way. - The buffer enforcement now tracks
stdoutandstderrseparately, whereasexec'smaxBufferapplies to combined output; if you need to preserve original semantics, you may want to track total output length across both streams instead of per-stream.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `runCommandSafely` helper filters `shell-quote.parse` output down to only string tokens, which will silently drop things like redirections, pipes, or env assignments; consider explicitly detecting non-string tokens and failing with a clear error rather than changing the behavior of such commands in a non-obvious way.
- The buffer enforcement now tracks `stdout` and `stderr` separately, whereas `exec`'s `maxBuffer` applies to combined output; if you need to preserve original semantics, you may want to track total output length across both streams instead of per-stream.
## Individual Comments
### Comment 1
<location> `src/tools/bash.ts:73-75` </location>
<code_context>
+ reject(error);
+ });
+
+ child.on("close", code => {
+ clearTimeout(timer);
+ if (killedForTimeout) {
+ return reject(new Error("Command timed out"));
+ }
</code_context>
<issue_to_address>
**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.
</issue_to_address>
### Comment 2
<location> `src/tools/bash.ts:13` </location>
<code_context>
private currentDirectory: string = process.cwd();
private confirmationService = ConfirmationService.getInstance();
+ private async runCommandSafely(
+ command: string,
+ options: { cwd?: string; timeout?: number; maxBuffer?: number },
</code_context>
<issue_to_address>
**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:
```ts
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.
</issue_to_address>
### Comment 3
<location> `src/tools/bash.ts:30` </location>
<code_context>
const child = spawn(cmd, args, {
</code_context>
<issue_to_address>
**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*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+73
to
+75
| child.on("close", code => { | ||
| clearTimeout(timer); | ||
| if (killedForTimeout) { |
There was a problem hiding this comment.
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.
| private currentDirectory: string = process.cwd(); | ||
| private confirmationService = ConfirmationService.getInstance(); | ||
|
|
||
| private async runCommandSafely( |
There was a problem hiding this comment.
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/timeoutmanagement killedForTimeout/killedForBufferflags and custom timer- the unused
promisifyFnalias
and keeps:
- no shell invocation
timeoutandmaxBufferbehaviorcommand: stringAPI withshell-quoteparsing
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.
| const args = argv.slice(1); | ||
|
|
||
| return await new Promise((resolve, reject) => { | ||
| const child = spawn(cmd, args, { |
There was a problem hiding this comment.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Potential fix for https://github.com/involvex/super-agent-cli/security/code-scanning/11
At a high level, we should stop passing a single, concatenated, untrusted string to
child_process.exec, which invokes a shell. Instead, we should parse the command string into an executable and an array of arguments and execute it viaspawnorexecFile, which do not invoke a shell by default. For robust parsing that respects quotes and escapes, we should use a well‑known library likeshell-quote.Best, minimal‑impact fix:
src/tools/bash.ts, replaceexecAsync(command, ...)with a safe wrapper that:cdlogic as today;shell-quote.parseto split the command string into tokens;cmd) and the rest asargs;child_process.spawnwithshell: false, capturingstdoutandstderrand preserving timeout/maxBuffer semantics as closely as practical.shell-quoteandspawnin this file, but leave all other files and behavior unchanged.BashTool.executeand helper methods (listFiles,findFiles,grep) intact so callers likeSuperAgent.executeBashCommandanduse-input-handler.tscontinue to work.Concretely, in
src/tools/bash.ts:exec/execAsyncusage with a new helperrunCommandSafely(command, options)that:shell-quote.parseto get tokens;stdout/stderrstrings;maxBufferby tracking accumulated output length and killing the child if exceeded.execute, callrunCommandSafely(command, { cwd: this.currentDirectory, timeout, maxBuffer: 1024*1024 })instead ofexecAsync.spawnfromchild_processandshellQuotefromshell-quote. Keep the existingexec/promisifyimport only if still needed; here we can safely removeexec/execAsyncsince they’re no longer used.This eliminates shell interpretation of the user‑controlled string while preserving the ability to run arbitrary commands with arguments.
Suggested fixes powered by Copilot Autofix. Review carefully before merging.
Summary by Sourcery
Harden bash command execution by replacing shell-based exec usage with a safer spawn-based implementation that parses commands into argv tokens while preserving existing BashTool behavior.
Bug Fixes:
Enhancements:
Build: