revert #386 due to unsafe shell command parsing (#478)

Reverts https://github.com/openai/codex/pull/386 because:

* The parsing logic for shell commands was unsafe (`split(/\s+/)`
instead of something like `shell-quote`)
* We have a different plan for supporting auto-approved commands.
This commit is contained in:
Michael Bolin
2025-04-21 09:52:11 -07:00
committed by GitHub
parent 797eba4930
commit d36d295a1a
4 changed files with 1 additions and 68 deletions

View File

@@ -289,9 +289,6 @@ model: o4-mini # Default model
approvalMode: suggest # or auto-edit, full-auto
fullAutoErrorMode: ask-user # or ignore-and-continue
notify: true # Enable desktop notifications for responses
safeCommands:
- npm test # Automatically approve npm test
- yarn lint # Automatically approve yarn lint
```
```json

View File

@@ -4,7 +4,6 @@ import {
identify_files_added,
identify_files_needed,
} from "./utils/agent/apply-patch";
import { loadConfig } from "./utils/config";
import * as path from "path";
import { parse } from "shell-quote";
@@ -297,24 +296,6 @@ export function isSafeCommand(
): SafeCommandReason | null {
const [cmd0, cmd1, cmd2, cmd3] = command;
const config = loadConfig();
if (config.safeCommands && Array.isArray(config.safeCommands)) {
for (const safe of config.safeCommands) {
// safe: "npm test" → ["npm", "test"]
const safeArr = typeof safe === "string" ? safe.trim().split(/\s+/) : [];
if (
safeArr.length > 0 &&
safeArr.length <= command.length &&
safeArr.every((v, i) => v === command[i])
) {
return {
reason: "User-defined safe command",
group: "User config",
};
}
}
}
switch (cmd0) {
case "cd":
return {

View File

@@ -78,8 +78,6 @@ export type StoredConfig = {
saveHistory?: boolean;
sensitivePatterns?: Array<string>;
};
/** User-defined safe commands */
safeCommands?: Array<string>;
};
// Minimal config written on first run. An *empty* model string ensures that
@@ -113,8 +111,6 @@ export type AppConfig = {
saveHistory: boolean;
sensitivePatterns: Array<string>;
};
/** User-defined safe commands */
safeCommands?: Array<string>;
};
// ---------------------------------------------------------------------------
@@ -297,7 +293,6 @@ export const loadConfig = (
instructions: combinedInstructions,
notify: storedConfig.notify === true,
approvalMode: storedConfig.approvalMode,
safeCommands: storedConfig.safeCommands ?? [],
};
// -----------------------------------------------------------------------
@@ -375,13 +370,6 @@ export const loadConfig = (
};
}
// Load user-defined safe commands
if (Array.isArray(storedConfig.safeCommands)) {
config.safeCommands = storedConfig.safeCommands.map(String);
} else {
config.safeCommands = [];
}
return config;
};
@@ -425,10 +413,6 @@ export const saveConfig = (
sensitivePatterns: config.history.sensitivePatterns,
};
}
// Save: User-defined safe commands
if (config.safeCommands && config.safeCommands.length > 0) {
configToSave.safeCommands = config.safeCommands;
}
if (ext === ".yaml" || ext === ".yml") {
writeFileSync(targetPath, dumpYaml(configToSave), "utf-8");

View File

@@ -1,13 +1,7 @@
import type { SafetyAssessment } from "../src/approvals";
import { canAutoApprove } from "../src/approvals";
import { describe, test, expect, vi } from "vitest";
vi.mock("../src/utils/config", () => ({
loadConfig: () => ({
safeCommands: ["npm test", "sl"],
}),
}));
import { describe, test, expect } from "vitest";
describe("canAutoApprove()", () => {
const env = {
@@ -95,27 +89,4 @@ describe("canAutoApprove()", () => {
expect(check(["cargo", "build"])).toEqual({ type: "ask-user" });
});
test("commands in safeCommands config should be safe", async () => {
expect(check(["npm", "test"])).toEqual({
type: "auto-approve",
reason: "User-defined safe command",
group: "User config",
runInSandbox: false,
});
expect(check(["sl"])).toEqual({
type: "auto-approve",
reason: "User-defined safe command",
group: "User config",
runInSandbox: false,
});
expect(check(["npm", "test", "--watch"])).toEqual({
type: "auto-approve",
reason: "User-defined safe command",
group: "User config",
runInSandbox: false,
});
});
});