From d36d295a1a8d047fa5d64cc4d67f92f113660f24 Mon Sep 17 00:00:00 2001
From: Michael Bolin <mbolin@openai.com>
Date: Mon, 21 Apr 2025 09:52:11 -0700
Subject: [PATCH] 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.
---
 README.md                         |  3 ---
 codex-cli/src/approvals.ts        | 19 -------------------
 codex-cli/src/utils/config.ts     | 16 ----------------
 codex-cli/tests/approvals.test.ts | 31 +------------------------------
 4 files changed, 1 insertion(+), 68 deletions(-)

diff --git a/README.md b/README.md
index 2094b939..e5690d7d 100644
--- a/README.md
+++ b/README.md
@@ -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
diff --git a/codex-cli/src/approvals.ts b/codex-cli/src/approvals.ts
index b9eb50d1..f4a35402 100644
--- a/codex-cli/src/approvals.ts
+++ b/codex-cli/src/approvals.ts
@@ -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 {
diff --git a/codex-cli/src/utils/config.ts b/codex-cli/src/utils/config.ts
index a4a9c0cb..190d1117 100644
--- a/codex-cli/src/utils/config.ts
+++ b/codex-cli/src/utils/config.ts
@@ -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");
diff --git a/codex-cli/tests/approvals.test.ts b/codex-cli/tests/approvals.test.ts
index 43490cf8..a39adff4 100644
--- a/codex-cli/tests/approvals.test.ts
+++ b/codex-cli/tests/approvals.test.ts
@@ -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,
-    });
-  });
 });