拒绝AI代码里的Shell字符串：Agent工具PR的安全审查实战

一次周五的线上事故让一个小型结账服务停摆。导致任务失败的，是一个由 AI Agent 提交的 Pull Request。那个补丁新增了一个名为 run_check 的辅助函数，用于冒烟测试。

整个 diff 只有四十行，看起来相当整洁。但有一行模板字符串打破了这份整洁——这个辅助函数从分支名拼接出了一条 shell 命令。随着越来越多团队把编码工作交给 AI Agent，这类看似无害、实则危险的代码正在悄悄进入代码库。

先读代码，再读摘要

Agent 的摘要承诺了一个更快的审查循环，但处理函数本身讲述了一个更严格、更小的故事：一个来自工具 JSON 的分支字段，被直接拼进了命令字符串。

// Untrusted sketch from the PR. Do not merge it.
function runCheck(branch) {
  const cmd = `git fetch origin ${branch} && npm test`;
  return exec(cmd, { shell: true });
}

问题的核心在于信任边界。在这个处理函数里，工具 JSON 属于调用方可控的输入。一个 shell 字符串等于在执行路径上引入了第二个解析器。如果测试只传入 main 这样的"干净"分支名，就永远不会触发那个解析器——漏洞因此潜伏下来，直到某个恶意或畸形的分支名（如 feature;echo）出现。

这揭示了审查 AI 生成代码的第一条原则：不要被摘要带节奏，先看真实的调用路径。Agent 善于把危险操作包装得"整洁"，而安全问题往往藏在那一行不起眼的字符串拼接里。

**命令注入（Command Injection）**是指攻击者通过在输入数据中嵌入 shell 元字符（如 ;、&&、|、$() 等），使原本只传递数据的字符串被 shell 解析器当作额外命令执行的漏洞。当 Node.js 的 exec() 或 spawn() 以 shell: true 调用时，操作系统会先把整条命令字符串交给 /bin/sh 解析，再执行结果。这意味着一个包含 feature;rm -rf / 的分支名会让 shell 依次执行两条命令——而调用方对此毫不知情。之所以称之为"第二个解析器"，正是因为程序代码本身是第一个解析器，而 shell 是第二个：当两个解析器串联处理同一段字符串时，数据与指令的边界就会消失。

保留目标，去掉机制

修复的思路不是推倒重来，而是精确区分"要保留什么"和"要丢弃什么"。

应当保留：如果现有任务已经调用了这个工具名，就保留它；保留先 fetch 再 test 的执行顺序；保留任何要求加超时的注释。这些是业务目标，没有问题。

应当丢弃：下一次提交就删掉模板字符串；在这条调用路径上去掉 shell: true 标志；移除那种唯一作用就是包裹 shell 的依赖包。这些是危险的机制，而非真正的需求。

这种"保留目标、替换机制"的思路，比笼统地说"重写一遍"更可执行，也更容易让 PR 作者接受——因为它没有否定需求，只否定了实现方式。

强制使用固定参数列表

正确的做法是用 execFile 配合独立的参数数组，用严格的正则拒绝不合规的分支文本，并设置超时让卡住的子进程能够结束。

import { execFile } from "node:child_process";
import { promisify } from "node:util";

const execFileAsync = promisify(execFile);
const BRANCH = /^[A-Za-z0-9._/-]{1,80}$/;

export async function runCheck(branch) {
  if (!BRANCH.test(branch)) {
    throw new Error("branch_rejected");
  }
  await execFileAsync("git", ["fetch", "origin", branch], {
    timeout: 20_000,
    shell: false,
  });
  return execFileAsync("npm", ["test"], {
    timeout: 120_000,
    shell: false,
  });
}

这段代码的关键改动有三点：参数以数组形式传入，分支名永远作为纯数据而非可执行脚本；shell: false 彻底关闭了二次解析；两处 timeout 确保卡死的进程不会一直占用任务。需要说明的是，这块代码应被视为一份"提议补丁"，它定义的是审查应当要求的形态，而非经过生产硬件压测的最终方案。

execFile 与 exec 的根本区别在于参数传递方式。exec 接受一整条字符串并默认将其交给 shell 解析；execFile 则直接用 execve 系统调用启动指定的可执行文件，参数数组中的每个元素被操作系统作为独立的字节序列传递，完全绕开 shell 层。这意味着即使 branch 参数包含 ;、空格或 $VAR，它也只会被 git 当作一个字面量参数处理，而非触发任何 shell 展开。promisify 则是将基于回调的 Node.js 核心 API 包装成返回 Promise 的形式，使其可以与 async/await 配合使用，不影响安全语义，只改善代码的可读性与错误处理流程。

把规则固化进测试文件

仅修好代码还不够，必须用测试把规则锁死，防止下一个 PR 又把 shell 字符串加回来。做法是把测试文件和其他工具测试放在一起，在测试时从磁盘读取处理函数的源码，一旦发现 shell 模式就让任务失败。

import assert from "node:assert/strict";
import { readFileSync } from "node:fs";
import test from "node:test";
import { runCheck } from "../tools/runCheck.js";

const src = readFileSync(
  new URL("../tools/runCheck.js", import.meta.url),
  "utf8"
);

test("handler does not build a shell string", () => {
  assert.equal(src.includes("shell: true"), false);
  assert.equal(src.includes("exec(`"), false);
  assert.match(src, /execFile/);
  assert.match(src, /timeout:/);
});

test("bad branch names fail before a process starts", async () => {
  await assert.rejects(() => runCheck("feature;echo"), /branch_rejected/);
  await assert.rejects(() => runCheck(""), /branch_rejected/);
});

这里有两类断言：一类直接扫描源码文本，确保 shell: true 和反引号 exec 不再出现；另一类是行为测试，验证畸形分支名（feature;echo 和空字符串）会在任何进程启动之前就被拒绝。这两个拒绝用例必须"fail closed"（默认拒绝），用 node --test 在干净的检出树上运行。本地通过只是起点信号——同一个文件还应在一次性的 runner 上跑一遍，并把退出码记录到 PR 里。

文中提到的**"fail closed"（默认拒绝）是安全设计中的一项基本原则，与之相对的是"fail open"（默认放行）。在验证逻辑中，fail closed 意味着：当输入不满足白名单规则时，系统必须明确拒绝，而不是"试着允许"或静默忽略。对于分支名验证，这意味着空字符串、包含特殊字符的名称、超长字符串都应在任何子进程启动之前抛出错误。测试中同时对源码文本进行静态断言（检查字面字符串是否存在），是一种轻量的"防回退测试"（regression guard）**——它不测试运行时行为，而是直接锁定代码结构，让后续 PR 无法在不破坏测试的情况下悄悄将危险模式带回来。

用八步流程审查 AI 提交的 PR

原文给出了一套可直接套用的审查流程，核心是"先隔离、再验证、最后由人决策"：

1. 冻结 diff，第一遍审查时隐藏 Agent 的摘要
2. 列出所有来自工具 JSON、环境文件或审查文本的值
3. 标记每一个流向 exec、spawn 或反引号命令的值
4. 用参数数组和显式拒绝路径替换 shell 插值
5. 加上源码断言和两个拒绝用例
6. 在不持有生产密钥的空闲服务器上运行测试文件
7. 只把冻结的 diff 和测试日志发给免费模型
8. 由人类负责人在读完两份输出后批准合并

第六步需要一个没有共享状态的"草稿工作区"：把 PR 分支克隆进去，让生产 token 远离这台审查机器。把命令输出复制回审查线程，非零退出码意味着回退，而非争论——不要带着生产凭据重跑任务。

让免费模型起草笔记，而非批准合并

（披露：本文作为 MonkeyCode 的产品推广内容准备，运营方将 MonkeyCode 描述为一个开源项目。）

免费模型可以用来起草第一份审查笔记，免费服务器也能在笔记本之外跑这套测试。但使用时要守住边界：只粘贴 diff 和测试日志，绝不包含环境文件、客户数据行或 cookie。可以让模型返回 trust、revert、test 三列的结论，并删掉任何编造出来的"安全公告编号"。

Review this frozen diff for shell interpolation.
Return trust, revert, and test rows.
Do not suggest new network calls.

如果笔记要求读取线上密钥，立即停止；如果笔记改写了处理函数，重跑测试。模型文本只能作为注释保留，它本身不构成合并批准。绿色的模型笔记永远替代不了人类负责人的决策。

当 diff 变大时，用一张审查矩阵

原文提供了一张实用的信号-动作对照表，可作为审查辅助：

diff 中的信号	审查动作	原因
工具字段被拼进 shell 字符串	回退	不可信文本遇到第二个解析器
固定命令旁出现 shell: true	回退	execFile 不需要这个标志
测试只覆盖 main 这一个名字	补充	畸形分支永远不会被拒绝
git 或 npm 缺少超时	添加	卡住的子进程不该占用任务
仅为包裹 sh 而新增的依赖	移除	供应链膨胀却毫无实际收益
没有文件引用的模型笔记	忽略	无出处的建议不算审查证据

这张矩阵只是审查辅助，不是评分或基准。对于获批的"白名单包装器"，某一行可能不适用——这时要在审查评论里引用该包装器，但仍然禁止在新工具里出现裸命令字符串。

诚实说明局限与适用边界

好的审查会在同一处说明自己的局限：分支正则只是一道闸门，不是完整的 ref 语法，合法的 Unicode 名称会被"安全地"拒绝掉；生产 CI 应当用绝对路径调用 git 和 npm；源码扫描会漏掉散落在其他文件里的调用，因此应在整个 PR 范围内运行扫描。此外，execFile 仍会启动本地二进制文件，可写的 PATH 依然要留在审查清单上。

有三种场景不应使用这条路径：runner 持有生产凭据时（共享算力不是放密钥的地方，应改用内部锁定的 runner）；策略要求签名私有镜像时；维护者必须运行任意脚本时（这需要独立的白名单设计，而非一个"更薄的 shell 字符串"）。纯样式审查则可以安全跳过这套测试——只有当工具代码能够启动进程时才需要它。这条边界让额外的工作量保持最小。

最后用一条具体的评论收尾审查：点名被回退的那段模板字符串，链接测试文件和退出码，并说明免费服务器的运行只覆盖了这个文件。结账服务需要的是一个"无聊"的处理函数——分支名必须始终是纯数据，而不该变成一段 shell 脚本。

文中提到的 PATH 可写风险是指：execFile 虽然避免了 shell 注入，但它调用的 git 和 npm 仍然是通过环境变量 PATH 查找的。如果攻击者能够修改进程的 PATH（例如通过写入 .env 文件或污染父进程环境），就可以将一个同名的恶意可执行文件放在搜索路径前列，使 execFile('git', ...) 实际运行的是攻击者控制的程序。使用绝对路径（如 /usr/bin/git）可以消除这一向量，代价是与具体系统环境耦合。这也说明安全加固是分层的——消除 shell 注入并不等于消除所有执行路径上的风险，每一层防御都有其边界。