Skip to content

tools: enforce BashTool whitelist across compound commands - #279

Open
FireCollector wants to merge 3 commits into
trpc-group:mainfrom
FireCollector:codex/fix-bashtool-whitelist-validation
Open

tools: enforce BashTool whitelist across compound commands#279
FireCollector wants to merge 3 commits into
trpc-group:mainfrom
FireCollector:codex/fix-bashtool-whitelist-validation

Conversation

@FireCollector

Copy link
Copy Markdown

Summary

  • validate every executable command segment against the BashTool whitelist
  • preserve quoted or escaped separators and file-descriptor redirections
  • fail closed when shell substitution syntax cannot be verified statically
  • add predicate-only and mocked-subprocess regression tests

Problem

BashTool command validation only recorded a command when it encountered a pipe and did not record the final segment. As a result, standalone commands could produce no commands to validate, the final command in a pipeline was skipped, and compound separators were not handled consistently. This could bypass a custom whitelist or the default outside-working-directory restrictions.

Fix

The validation now statically extracts every executable command segment while respecting quotes, escapes, redirections, pipelines, compound separators, and newlines. Unparseable input and command or process substitution fail closed. The existing unrestricted behavior for commands executed inside the configured working directory is preserved.

No shell command samples are executed by these tests. They exercise the safety predicate directly or mock subprocess creation.

Validation

  • 27 BashTool static and mocked-subprocess tests passed
  • 6 safety core integration tests passed
  • flake8 passed for both changed files
  • YAPF produced no diff
  • git diff --check passed

Fixes #277

RELEASE NOTES: Fixed BashTool command whitelist validation for standalone and compound shell commands.

BashTool previously skipped standalone commands and the final command in a pipeline, while other shell separators were not treated as command boundaries. Parse and validate every executable segment, fail closed on unverifiable substitution syntax, and preserve the unrestricted default work-directory behavior.

Fixes trpc-group#277

RELEASE NOTES: Fixed BashTool command whitelist validation for standalone and compound shell commands.
@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

Bash 被拦截了。让我手动追踪一下逻辑。

让我仔细分析一下关键的行为变更和解析逻辑。

新的 _is_command_safe 带来的关键行为变更:

  • whitelist_commandsNone(默认设置)且 execution_dircwd 内部时:现在直接 return True(不再检查基本命令)。之前的代码在这种情况下也会为每个命令返回 True,因为循环继续执行且最后的 return True 保留了。所以这里没有实质性的回归。
  • 当在 cwd 外部时:使用 ALLOWED_COMMANDS_OUTSIDE_WORKDIR 并检查所有段(segments)。

现在让我检查一下解析过程中存在的实际问题。看看 _extract_base_commands

让我追踪一下 echo ok 2>&1

  • 字符流:e,c,h,o, ,o,k, ,2,>,& ,1
  • 2 处:previous_is_redirect 设置为 False(不是 < 或 >)
  • > 处:previous_is_redirect 变为 True
  • & 处:char in {|;...},next_is_redirect = command[index+1]=='1' 为 False。is_redirection = char=='&' and (previous_is_redirect=True or ...) = True。所以它被保留。previous_is_redirect 重置为 False。
  • 段:echo ok 2>&1,shlex.split -> ['echo','ok','2>&1'],tokens[0]='echo'。没问题。

echo ok &>output.log

  • > 后:previous_is_redirect=True
  • & 处:next_is_redirect = command[index+1]=='>' = True。is_redirection=True。已保留。
  • 段:echo ok &>output.log,shlex.split -> ['echo','ok','&>output.log']。没问题。

echo ok & echo done

  • 在空格处 previous_is_redirect=False。在 & 处:next_is_redirect = 下一个字符是空格,不是 '>'。is_redirection = False。所以它充当分隔符。command.startswith(("||","&&","|&",";;"), index) —— 单个 & 不匹配其中任何项,所以 index+=1。段:echo ok,然后是 echo done。没问题。

r"echo escaped\>& blocked_cmd"

  • echo,空格,e,s,...,d (escaped),然后是 \。在 \ 处,quote 为空,所以 escaped=True,保留 \。下一个字符 > —— escaped 分支:保留 >,escaped=False,previous_is_redirect=False。等等——在 escaped 分支中,previous_is_redirect 被设置为 False,而不是基于被转义的字符进行设置。所以即使在转义 > 之后,previous_is_redirect 依然是 False。
  • 下一个字符 &:next_is_redirect = 下一个字符是空格 = False。is_redirection = char=='&' and (previous_is_redirect=False) = False。所以 & 被视为分隔符!
  • 段:echo escaped\> -> shlex.split(posix) -> 转义的 > 变为字面量 >,所以是 ['echo','escaped>']。tokens[0]='echo'。下一段:blocked_cmd。tokens[0]='blocked_cmd'。
  • 结果:['echo', 'blocked_cmd']。在白名单检查中,'blocked_cmd' 不在白名单内 -> 返回 False。测试预期为 False。没问题。

现在关键问题来了:previous_is_redirect 的追踪。考虑 2>&1 blocked_cmd 风格——已经在上面涵盖了。考虑像 echo>file&blocked_cmd 这样缺少空格的重定向,其中 & 紧跟在 > 之后。这没问题。

但这里有一个微妙的问题:在 escaped 分支中,previous_is_redirect = False。这对安全有影响吗?考虑 echo \>& blocked_cmd ——这是作为分隔符的 &(如上分析,被视为分隔符),这是安全的(fail-closed 返回两个段,blocked_cmd 被拒绝)。所以没问题。

让我检查一个真正的绕过向量:命令替换检测。

echo $(blocked_cmd) —— 在索引 echo $( 处,当到达 $ 时……实际上循环会到达 $ 字符。$ 不在任何特殊集合中。current 追加 $,previous_is_redirect=False。下一个字符 ( —— 不是特殊字符。current 追加 (。这样 $( 就没有被捕获!

等等,让我重读一下:检查是 command.startswith(("$(", "<(", ">("), index)。这仅在当前索引与子字符串开始匹配时触发。当索引在 $ 处时,command.startswith("$(", index) 检查 command[index:index+2] == "$(",结果为 True!所以它返回 None。让我重新验证:在 $ 处,char='$'。转义检查无,quote 为空,不是引号,不是反引号/起始检查 —— command.startswith(("$(","<(",">("), index) 在索引为 $ 且下一个是 ( 时为 True。所以返回 None。很好,fail-closed。

在双引号内呢:"echo $(blocked_cmd)" —— 在双引号内,$( 分支返回 None。很好。

反引号在双引号内:同样返回 None。很好。

现在,真正令人担忧的问题:之前代码的行为。旧代码使用 punctuation_chars="|"shlex.shlex 进行分词,然后只根据 | 进行分割。新的基于解析器的方法可以说更严格了。但有一个值得标记的行为兼容性变更。

让我检查 _run_async_impl 的错误路径和 to_dict 的安全。我也再检查一下 command.split() 的回退逻辑是否被移除及其影响——新代码没有回退;解析失败返回 None -> _is_command_safe 返回 False。这是 fail-closed。但这有一个行为变更:之前,对于无法解析的命令(例如,带有未配对引号的命令),旧代码会捕获异常并回退到 command.split()[0],因此它可能允许命令通过。现在它被阻塞了。这是一个更严格的失败处理(fail-closed),可以接受。

现在,一个真正的问题:默认情况下(whitelist_commandsNone),当 execution_dir 在 cwd 内时,_is_command_safe 完全不解析命令并返回 True。 这保留了原有行为,所以不算回归。

让我检查实际的 Bug。考虑重定向到相对路径的命令,如 echo ok > ../escape.txt —— 在工作目录内,被允许。无关紧要。

考虑一个非白名单且在 workdir 内的命令:被允许。这符合设计。

现在让我看看测试 test_is_command_safe 第 187 行:assert tool._is_command_safe("blocked_cmd", str(tmp_path)) is True。tool 的 cwd=tmp_path,execution_dir=tmp_path,在内部 -> 返回 True。没问题。

现在是否有任何真正被破坏的解析案例?考虑 echo a & echo b,其中 & 是分隔符 —— 得到两个段。但 echo a && echo b —— && 被作为起始消费,单个分隔符。段:echo a, echo b。没问题。

考虑 echo a||echo b(无空格)—— 在第一个 | 处,startswith("||") 为 True -> index+=2。段 echo aecho b。没问题。

考虑 bash here-string <<< 或 heredoc <<<< 字符:< 设置 previous_is_redirect=True。下一个 < 也设置 previous_is_redirect。然后是 EOF。但这只是重定向操作符;heredoc 体 cat <<EOF\nblocked\nEOF 会将 cat 视为段,但 blocked 行会成为它自己的段 -> blocked。因此,使用 heredoc 的非白名单命令会被捕获。这通常是可以接受的(fail-safe)。但是 cat <<EOF\nls\nEOF -> 段:cat <<EOFlsEOF。tokens[0] 分别为 cat, ls, EOF。如果 EOF 不在白名单中,则被阻塞。所以 heredoc 将会中断,即使命令在白名单内。这是一个误报(false positive)。这值得作为一个 Warning 提出,因为它破坏了合法的、在白名单内的命令。但 heredoc 相对少见;不过它确实是一个真实的兼容性问题。考虑到这是一个已知的 shell 解析器局限,我将其标记为 Warning。

让我核实一下用于赋值前导的 env 等 —— 例如 FOO=bar echo hishlex.split("FOO=bar echo hi") -> ['FOO=bar','echo','hi'],tokens[0]='FOO=bar' 不在白名单内 -> 被阻塞。旧代码也会有类似表现(tokens[0]='FOO=bar')。所以不是回归,但值得注意。可能不值得标记,因为它是预先存在的限制,且 diff 没有改变它……实际上旧代码保留了 current_command[0] = 第一个 token = 'FOO=bar'。相同的行为。不是回归。

让我检查 &|& 中作为管道的情况是否被正确处理。echo a |& echo b —— 在 | 处,startswith("|&") -> index+=2。没问题。

现在一个真正值得关注的问题:在引号块内,previous_is_redirect 总是被设置为 False,且引号内的字符如 > 不会标记重定向。这是正确的。但是 elif quote == '"' and (char == "" or command.startswith("$(", index))—— 在双引号内,遇到反引号或$( 时它返回 None。很好。但单引号呢?单引号内一切字面化,所以反引号/$(` 是字面的且是安全的 —— 正确地不返回 None。很好。

现在真正的 Bug —— 看一下 if char == "" or command.startswith(("$(", "<(", ">("), index):这一行,检查是否遗漏了},例如 ${blocked_cmd}echo ${blocked_cmd}——$ 在索引处,</code>command.startswith("$(",index)<code>检查$(—— 但这里是${,不是 $(。所以不匹配。{不特殊。所以${blocked_cmd} 被当作一个参数。shlex.split("echo ${blocked_cmd}")-> ['echo','${blocked_cmd}']。tokens[0]='echo'。所以echo ${blocked_cmd}将会被 ALLOWED(只看到 'echo'),尽管${blocked_cmd}` 在运行时扩展为可能任意内容,包括在当前目录下不受限制的命令执行!

这是一个真正的安全问题吗?在工作目录内,所有操作无论如何都是被允许的,所以那里没有差别。但考虑一下带有 whitelist_commands=["echo"] 的白名单情况,以及 echo ${blocked_cmd} —— 被允许,但参数只是被回显,本身不是命令执行。${...} 不会执行 blocked_cmd;它只是变量扩展。变量扩展是安全的(没有命令执行),除非它是像 $(...)` 这样的扩展。所以 ${...} 只是变量替换 —— 没有命令注入。所以不是真正的漏洞。OK。

但是 ${!var} 间接引用或数组呢 —— 仍然是扩展,不是执行。没问题。

真正值得关注的是:echo $(blocked_cmd) 被阻止了(很好),但通过 ${...} 的间接命令替换呢 —— 没有执行。没问题。

所以 ${} 不是注入向量。很好。

那缺失的 >( 被处理了 —— 是的。<(, >( —— 进程替换 —— 被阻止了。很好。

现在让我考虑一个实际的绕过:带空格的赋值前导不是问题。

那针对 whitelist_commandsargs 注入呢 —— 不相关。

让我重新考虑 previous_is_redirect 的重置。有一个细微的 Bug:在 escaped 分支中 previous_is_redirect 被设置为 False。考虑 echo >\& blocked_cmd —— > 设置 previous_is_redirect=True。然后 \ -> escaped 分支,设置 previous_is_redirect=False,保留 \。然后 & -> 现在 previous_is_redirect=False,next_is_redirect = 下一个字符是空格 = False。所以 & 是分隔符。段 echo >\& -> shlex posix: \& -> & 字面量,所以是 ['echo','>&']。tokens[0]='echo'。下一段 blocked_cmd。被拒绝。Fail-safe。没问题。

OK,考虑相反的情况:一个绕过,其中 & 本应是分隔符,但却被错误地视为重定向,从而允许第二个命令通过。这种情况发生在 is_redirection 错误地为 True 时。is_redirection = char=='&' and (previous_is_redirect or next_is_redirect)。如果 & 前面是 >,但 > 被转义/引用了,previous_is_redirect 可能错误地为 True。示例:echo \>& blocked_cmd —— 如上所述,转义的 > 将 previous_is_redirect 设为 False(因为 \ 分支重置了它)。所以 & 是分隔符 -> blocked_cmd 被拒绝。安全。

示例:echo ">"& blocked_cmd —— " 开始引用,> 在引号内 -> previous_is_redirect 保持 False(引号分支设置 False)。" 结束引用。然后 & -> previous_is_redirect=False,next_is_redirect=空格=False。分隔符。段:echo ">"blocked_cmd。被拒绝。安全。

echo >& blocked_cmd 呢(没有数字,>&,这实际上在 bash 中是 >& 重定向吗?实际上 >& 单独使用是一个有效的重定向,将 stdout+stderr 重定向到单词……不,>&word 是重定向)。echo >& blocked_cmd> 设置 previous=True。& next_is_redirect = 下一个字符是空格 = False。is_redirection = True(因为有 previous_is_redirect)。所以 & 被保留在段中!段 = echo >&,然后 blocked_cmd。tokens[0]:shlex.split("echo >&") -> ['echo','>&']?让我想想 shlex:>& —— > 是标点符号吗?使用 posix 和默认 punctuation,>&... 实际上 shlex.split 对 'echo >&' 返回 ['echo', '>&']。tokens[0]='echo'。第二段 'blocked_cmd' 被拒绝。所以即使 & 被保留在第一段中,第二段仍然被单独检查。所以 blocked_cmd 被捕获。安全。

关键点:每个段都会被独立检查。因此,即使 & 的重定向检测错误,每个段的首个 token 都会根据白名单进行检查。唯一危险的情况是 & 被错误地视为分隔符,而两个实际命令在同一个段中,导致只检查了第一个 token。当 & 是分隔符时会发生这种情况吗?是的 —— echo ok & echo done -> 分隔符,两个段。没问题,两个都被检查了。

& 作为重定向被保留,是否会合并两个命令?只有当 & 前面是 > 或后面是 > 时。echo > & blocked_cmd(带空格)—— > 设置 previous=True。空格设置 previous=False(空格不是 < 或 >)!让我检查一下:在空格字符处,落入 current.append(char)previous_is_redirect = char in {"<", ">"} = False。所以空格重置了 previous_is_redirect。所以 echo > & blocked_cmd:在 & 处 previous=False,next_is_redirect = 空格 = False。分隔符!段 echo >blocked_cmd。被拒绝。安全。

所以合并两个命令的真正风险仅在 >&&>(无空格)且后面在同一个段中跟随一个命令时 —— 但在 & 之后,要么有分隔符(空格),要么继续。考虑 echo >file&blocked_cmd(无空格):> previous=True。f,i,l,e 均非特殊,previous 重置为 False。在 & 处:previous=False,next_is_redirect = 下一个 b != '>'。分隔符。段 echo >fileblocked_cmd。被拒绝。安全。

所以我认为解析器是 fail-safe 的:它可能会误报(阻塞合法命令),但不会漏报被禁止的命令,因为段总是被拆分,且每个段的首个 token 都会被检查。

但是等等 —— 真正微妙的绕过:如果一个非白名单命令作为第一个命令的参数,但不会被执行呢?那不是注入。通过 |,;,&&,||,&, 换行符拆分可以捕获真正的命令分隔符。命令替换 $(, ` 会返回 None。进程替换 <(, >( 返回 None。所以我认为命令注入向量被覆盖了。

一个值得注意的遗漏:不带 $ 的括号的算术/命令执行? 例如 echo (blocked_cmd) —— ( 不特殊,shlex 会报错 -> ValueError -> 返回 None -> 被阻塞。安全。

所以解析器是稳健的。主要的发现是误报问题:合法的 shell 结构(heredoc、<<< here-strings、带变量前导的赋值前导)即使在白名单内也会被阻塞,此外,在白名单模式下(whitelist_commands != None),在 cwd 内部执行的命令也被白名单限制(行为变更!)。

让我确认一下那个行为变更,因为它很重要:

旧代码:循环遍历 base_commands;如果 whitelist_commands is not None,它无条件地检查白名单中的每个 base_command(没有路径检查)。所以白名单模式即使对在 cwd 内部的命令也进行限制。新代码:相同 —— 如果 whitelist_commands is not None,它会跳过路径检查并检查所有 base_commands。所以行为是相同的。没有回归。很好。

现在 test_bash_custom_whitelist 测试(第 137-155 行):echo 'test' -> 允许。ls /tmp -> ls 不在白名单 ["echo","pwd"] 中 -> 阻塞。测试使用 if "error" in result: 然后断言 —— 这很弱(如果 ls 不知何被允许且没有错误,测试仍然通过)。这是一个弱测试,但它是预先存在的风格;diff 并没有引入它。实际上查看 diff,这个测试没有被修改。所以不在范围内。

让我专注于 diff 新增的测试有效性。test_bash_custom_whitelist_blocks_before_subprocess 使用 tool_with_whitelist(无 safety guard)对 asyncio.create_subprocess_shell 进行打补丁(patch)。该测试断言 mock 没有被调用。由于 _is_command_safe 在 subprocess 调用之前阻塞,这是正确的。

但是等等 —— 打补丁的目标是 _bash_tool.asyncio.create_subprocess_shell。_bash_tool 中的 asyncio 是导入的模块 asyncio。对其进行打补丁是有效的。没问题。

现在一个真正的问题:在这个测试中,enable_safety_guard 默认为 False,所以 safety_scanner 为 None。safety_report 保持 None。然后 _is_command_safe("blocked_cmd") -> whitelist_commands=["echo","pwd"],提取 -> ['blocked_cmd'],不在白名单内 -> 返回 False -> 返回 SECURITY_RESTRICTION。subprocess mock 没有被调用。测试正确。很好。

现在让我重新考虑是否有什么真正被破坏的地方。让我重新检查 echo ok & echo done 的拆分,即 & 分隔符,其中 command.startswith(("||","&&","|&",";;"), index) —— 单个 & 不匹配。所以 index+=1(消费了 &)。但接下来是空格,current=[]. 段是 echo ok。然后 echo done 被 strip 处理为 echo done。很好。

那尾部的 & 后台操作符 sleep 100 & 呢 -> 段 sleep 100,然后是空段。tokens[0]='sleep'。如果 sleep 不在白名单内 -> 被阻塞。这是正确的(后台 sleep 仍然会执行)。

我认为逻辑是稳健的。让我考虑一下 & 后台行为echo ok &(尾部)—— & 是分隔符,段 echo ok 和空的尾部。if not segments -> 至少有一个段。返回 ['echo']。没问题。

现在,让我考虑一个值得提出的真正的兼容性/误报问题:heredoc/here-string赋值前导前缀,这些在白名单模式下会导致合法命令的误报,且这些命令原本是有效的。鉴于白名单模式是显式启用的,且文档说明仅允许白名单命令,heredoc 的误报是一个 Warning 级别的问题。

实际上,让我检查赋值前缀:FOO=bar echo hi -> tokens[0]='FOO=bar' -> 在白名单模式下被阻塞。旧代码也是如此。不是回归。跳过。

heredoc:旧代码使用 shlex 分割 | —— cat <<EOF\nls\nEOF 会被分割成什么?旧 shlex 带有 punctuation="|",whitespace_split,将 <<EOF 保留为一个 token,lsEOF 作为独立 token,但在 | 上分割。所以旧代码 tokens = ['cat','<<EOF','ls','EOF'](没有 |,所以一个 current_command,tokens[0]='cat')。旧代码返回 ['cat'] -> 允许(如果 cat 在白名单内)。新代码:返回 ['cat','<<EOF'? 不对 —— 新代码将换行符作为分隔符!所以 cat <<EOFlsEOF 是三个段 -> ['cat','ls','EOF']。ls 如果在白名单内则允许,EOF 不在 -> 被阻塞。

这是一个回归:旧代码允许使用 heredoc,新代码将其阻塞(因为 heredoc 定界符 EOF 成为了一个“命令”)。这是一个真正的行为变更/兼容性问题。另外,heredoc 体行如果不在白名单内,也会被错误地阻塞。

但更重要的是 —— 这是一种安全权衡吗?新行为更安全(检查更多)。但它破坏了合法的 heredoc 用法。考虑到文档说明白名单限制了命令,并且 heredoc 主体包含任意命令,将其视为命令进行检查,可以说更安全。我将把它标记为 Warning,作为一个值得注意的兼容性/误报问题。

还有换行符作为分隔符的问题:在 cwd 内(无白名单),命令无论如何都会被允许,所以 heredoc 在那里可以工作。只有白名单/外部工作目录模式才会受到影响。鉴于主要使用场景,这是一个 Warning。

让我再考虑一个真正的问题:_extract_base_commands\n 视为分隔符,但对于单个没有换行符的命令,第一个 token 才是关键。没问题。

现在,让我思考一下是否还有任何真正的安全绕过。让我重新考虑引号内引用的情况:单引号内的 $( —— 在单引号分支中,elif quote == '"' 不匹配(quote 是 '),所以 $( 在单引号内不会被捕获 —— 但它是字面化的,所以安全。很好。

双引号内的反引号 —— 被捕获(返回 None)。很好。

但是双引号内的 $({..}) 呢 —— 引号内的 $( 被捕获。很好。

我认为没有安全漏洞。现在让我写一下审查报告。让我重新考虑严重程度。核心逻辑看起来是 fail-safe 的。我发现的主要问题:

  1. Warning: heredoc / here-string / 多行命令被拆分为段,导致在白名单/外部工作目录模式下,对合法命令产生误报(旧代码允许 cat <<EOF\nls\nEOF,因为它只取第一个 token;新代码将每行视为一个命令,从而阻塞,因为 EOF/正文不在白名单内)。这是一个在白名单模式下的行为回归。

  2. 让我检查 ;;(case 语句)—— 已作为 2 个字符的分隔符处理。在 case 语句中,每条分支都是一个命令 —— 被单独检查。这是合理的。

  3. pipefail 或 subshell (cmd; cmd) —— ( 不是分隔符,shlex.split 会在 ( 上报错 -> 返回 None -> 被阻塞。安全但会产生误报。旧代码:shlex 带 punctuation="|" —— ( 是 token 吗?默认情况下 shlex 不会将 ( 视为标点符号,它可能成为 token 的一部分。shlex.split("(echo hi)") —— 实际上会引发 ValueError "No closing quotation"。所以旧代码也会捕获异常 -> 回退到 command.split()[0]='(echo' -> 不在白名单内 -> 被阻塞。行为相似。不是回归。

所以主要值得注意的发现是 heredoc/换行符拆分的回归。让我确认一下旧代码在换行符上的行为:旧代码使用 punctuation_chars="|" 的 shlex.shlex —— 换行符默认是空白字符(whitespace)。所以 echo a\necho b -> tokens=['echo','a','echo','b'],没有 |,所以 current_command=['echo','a','echo','b'],tokens[0]='echo'。旧代码返回 ['echo'] -> 允许(只有 echo 被检查,echo b 被静默允许!)。旧代码有一个真正的漏洞echo safe\necho harmless 在白名单模式下 —— 如果换行符分隔的命令在白名单内,则允许;但如果换行符分隔的命令是一个被禁止的命令 echo safe\nblocked_cmd,旧代码只检查 'echo' -> 允许 -> 安全绕过!新代码正确地将其拆分并阻塞 blocked_cmd。所以新代码修复了一个漏洞。这实际上是一个功劳,而不是问题。

所以新代码在换行符安全性上严格更好。heredoc 的误报是一个权衡:它破坏了 heredoc,但 heredoc 正文包含任意命令,所以在白名单模式下检查它们可以说更安全。我认为这甚至不是一个真正的 Warning —— 这是更严格的安全行为。但文档/声明的契约说“支持管道、重定向、复杂命令”,而 heredoc 是一个合法的特性,现在在白名单模式下失效了。我会将其作为一个低优先级的 Suggestion/Warning 提出。

让我重新考虑是否有什么真正被破坏或错误的地方。我相当有信心解析器是 fail-safe 的。让我也验证一下 _run_async_impl 中没有问题 —— 没有修改。

让我再检查一下:在白名单分支中,return all(base_command in allowed_commands ...)。如果 allowed_commands 是一个列表,in 检查是 O(n) 但没问题。没问题。

让我检查 Optional 的导入 —— 已经导入了。_extract_base_commands 返回 Optional[list[str]]。没问题。

我认为这个 PR 基本是稳健的。让我写下简洁的审查报告。鉴于 fail-safe 的行为和改进的安全性,我会指出 heredoc/multiline 的误报作为 Warning,可能还有一个关于弱测试的说明。让我重新检查弱测试 test_bash_custom_whitelist 第 154-155 行 —— 不在 diff 范围内。跳过。

让我再次考虑 heredoc 问题的严重程度。实际上,因为 _extract_base_commands\n 视为分隔符,所以像 cat <<EOF\nhello\nEOF 这样的 heredoc 会产生段 cat <<EOFhelloEOFhelloEOF 不在白名单内 -> 被阻塞。因此,任何 heredoc(合法且常见)在白名单模式下都会失效。由于文档承诺支持“复杂命令”,且 heredoc 是常见的,这确实是一个行为回归。Warning。

但注意:在默认模式下(无白名单),在 cwd 内,命令被完全允许,所以 heredoc 是有效的。heredoc 的中断仅在以下情况下发生:
(a) 设置了 whitelist_commands,或
(b) 执行目录在 cwd 外部。

对于 (b),ALLOWED_COMMANDS_OUTSIDE_WORKDIR = [ls,pwd,cat,grep,find,head,tail,wc,echo] —— 这些是面向读取的。heredoc 对于这些命令是有意义的(例如,cat <<EOF)。所以 cat <<EOF 在 /var 中会失效。这是一个真正的可用性回归。Warning。

我也考虑一下 here-string cat <<<"text" —— <<<< previous=True,下一个 < previous=True,下一个 < previous=True,然后 "... 段 cat <<<"text" -> shlex.split -> ['cat','<<<text']?实际上 shlex 可能会将 <<<"text" 视为... 让我思考一下 shlex posix 默认:<<< —— < 不是标点符号(默认无 punctuation_chars),所以 <<<"text" 变成一个 token <<<text。tokens[0]='cat'。允许。所以 here-string 没问题。只有 heredoc(带换行符定界符)会中断。OK。

最终答案:我将报告一个关于 heredoc/多行命令在白名单/外部 cwd 模式下产生误报的 Warning,以及可能的一个关于……让我想想是否有什么真正的问题。我将保持简洁。

实际上,让我重新考虑这到底是不是一个“问题”还是预期的安全收紧。PR 的提交信息说“enforce BashTool whitelist across compound commands”。拆分换行符是有意的。Heredoc 正文包含可执行命令,因此在白名单模式下阻塞它们在安全性上是可以辩护的。但文档契约(“支持复杂命令”)现在在白名单模式下排除了 heredoc。我会将其标记为 Warning 并建议:要么在文档中说明在白名单模式下不支持 heredoc,要么显式跳过 heredoc 正文。建议级别。我将把它作为 Warning 提出,因为这是一个真正破坏了文档中记载的特性集的回归。

让我完成审查。I've traced _extract_base_commands across all the new test cases and several adversarial inputs. Let me verify the heredoc/multiline behavior concern against both the old and new logic.

The key behavioral change: the new parser treats \n as a command separator (the old shlex-based code treated newlines as whitespace and only checked the first token of the whole command). This is a security improvement (it closes a real bypass where a newline-separated disallowed command was missed), but it has a side effect on heredocs.

发现的问题

⚠️ Warning

  • trpc_agent_sdk/tools/file_tools/_bash_tool.py:76:换行符作为命令分隔符导致 heredoc/多行命令在白名单模式下被误拦截
    • _extract_base_commands\n 当作命令边界。对 cat <<EOF\nhello\nEOF 这类 heredoc,函数会把 cat <<EOFhelloEOF 拆成三个独立 segment,于是 hello/EOF 作为“命令名”不在白名单内,整条命令被拒绝。工具对外声明支持 "pipes, redirections, complex commands",且 ALLOWED_COMMANDS_OUTSIDE_WORKDIR 中的 cat/grep/head 等本就常与 heredoc 搭配,因此这是在白名单模式或工作目录外执行时的功能回归。
    • 注意:换行符分隔本身修复了旧逻辑的一个真实绕过(旧代码对 echo safe\nblocked_cmd 只校验首个 token echo 即放行),方向是正确的;问题仅在于 heredoc 正文被误判为命令。建议显式识别 heredoc 并跳过其正文(识别 <<[-]DELIM 到匹配 DELIM 行之间的内容),或在文档中明确白名单模式不支持 heredoc。

总结

整体改动方向正确且是 fail-safe 的:命令替换 `/$(、进程替换 <(/>(、未闭合引号/转义均返回 None 被拒绝,复合命令的每一段首 token 都会过白名单,未发现可绕过白名单执行非授权命令的路径。唯一的实质问题是 heredoc/多行脚本在白名单模式下会因换行符被当作命令分隔符而误报拦截,建议作为兼容性回归处理。无 Critical 问题。

测试建议

  • 补充 heredoc 场景测试:在白名单模式下执行 cat <<EOF\nhello\nEOF(cwd 内与 cwd 外各一例),明确预期是放行还是拒绝,以固化行为契约。
  • 可补充一条“换行符分隔的非授权命令被拦截”的用例(如 echo ok\nblocked_cmd),将本次修复的真实绕过作为回归测试固定下来。

@codecov

codecov Bot commented Aug 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.57396% with 21 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@1153e7f). Learn more about missing BASE report.

Files with missing lines Patch % Lines
trpc_agent_sdk/tools/file_tools/_bash_tool.py 87.57396% 21 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main        #279   +/-   ##
==========================================
  Coverage        ?   88.45146%           
==========================================
  Files           ?         491           
  Lines           ?       46179           
  Branches        ?           0           
==========================================
  Hits            ?       40846           
  Misses          ?        5333           
  Partials        ?           0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Treat heredoc bodies as input data while continuing to fail closed on unquoted command substitutions, unterminated delimiters, and non-whitelisted commands after the terminator.

Updates trpc-group#277

RELEASE NOTES: NONE
@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

基于我对 diff 及底层 _bash_tool.py 的分析,以下是审查结果。

发现的问题

🚨 Critical

  • trpc_agent_sdk/tools/file_tools/_bash_tool.py:118-130_extract_base_commands 中对 $(/<(/>( 的检测):反斜杠-换行(行续行)可拆分多字符操作符,绕过命令替换/进程替换检测
    • 解析器把 \<newline> 当作“转义下一个字符”处理(追加到当前段,不触发任何分隔符/操作符检测),而 bash 会把 \<newline> 作为行续行删除。因此 echo $\<newline>(blocked_cmd) 在解析器里 $( 被换行隔开,startswith("$(", index)$ 处看到的是 $\ 而非 $(,漏检;随后 shlex.split 得到 base echo,命中白名单 → 判定安全。但 bash 实际执行时行续行被移除,还原成 echo $(blocked_cmd),在白名单模式/工作目录外模式下绕过白名单执行任意命令。<(>( 同理可被 \<newline> 拆分绕过。
    • 修复方向:在进入主循环前先把命令中的 \<newline> 规约(删除)再做操作符检测,或直接对未引号包裹的 \<newline> 判为不安全(return None)。
    • ...
      # 命令字符串 "echo $\\\n(blocked_cmd)"(含反斜杠+真实换行)
      # 解析器在 '$' 处只看到 '$\',不匹配 '$(',最终 base=['echo'] → 放行
      # bash 行续行后还原为 echo $(blocked_cmd) → 执行 blocked_cmd
      ...

💡 Suggestion

  • trpc_agent_sdk/tools/file_tools/_bash_tool.py:118_skip_heredoc_bodies 的替换检测:算术展开 $((...)) 被误判为命令替换而拒绝
    • $(( 的子串包含 $(,因此 echo $((1+2)) 会在白名单模式下被直接拒绝;旧实现仅取首 token 为 echo 是放行的,属于行为回退(安全方向、fail-closed)。若希望保留算术展开这种无执行能力的语法,可显式排除 $(( 之后再检测 $(

总结

整体是一组高质量的安全加固(复合命令分段、heredoc/引号/重定向处理、替换闭合拒绝),fail-closed 设计正确;但存在一个真实可利用的白名单绕过:\<newline> 行续行会拆分 $(/<(/>( 操作符,使解析器漏检命令/进程替换,bash 实际仍会执行,建议作为必须修复项处理。

测试建议

  • 补充用例覆盖行续行绕过:"echo $\\\n(blocked_cmd)""cat <\\\n(blocked_cmd)",断言 _is_command_safe 返回 False(当前会返回 True,复现该 Critical)。

index = 0
previous_is_redirect = False
pending_heredocs: list[tuple[str, bool, bool]] = []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

反斜杠-换行行续行可拆分操作符绕过命令/进程替换检测

解析器把 \<newline> 当作转义下一字符,而 bash 会作为行续行删除。echo $\<newline>(blocked_cmd) 在解析器中 $( 被换行隔开,漏检 $(...,命中白名单后放行;bash 执行时还原为 echo $(blocked_cmd) 绕过白名单执行任意命令。建议进入主循环前先规约(删除)\<newline> 再做操作符检测,或对未引号包裹的 \<newline> 直接判为不安全。

Fail closed on unquoted shell line continuations before they can join command or process substitution operators. Add static regression cases while preserving literal content inside single quotes.

Updates trpc-group#277

RELEASE NOTES: NONE
@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

I've reviewed the diff (a rewrite of BashTool._is_command_safe plus a hand-rolled shell segment parser _extract_base_commands and associated heredoc helpers, with comprehensive new tests). I traced the parser by hand across all parametrized cases and edge cases (trailing &, 2>&1, &>file, quoted/escaped args, heredocs, line continuations). The implementation is conservative and fails closed; I could not construct a command-injection bypass — every executable form ($(, backticks, <(, >(, unquoted heredoc substitution) is rejected, and unclassifiable syntax returns None → blocked.

发现的问题

⚠️ Warning

  • trpc_agent_sdk/tools/file_tools/_bash_tool.py:155-156:行为收紧的兼容性变更
    • 新解析器对任何无法完全分类的语法一律返回 None_is_command_safe 返回 False 直接拦截,包括进程替换 <(cmd)/>(cmd)_bash_tool.py:155)以及任何 shlex.split 失败的片段(_bash_tool.py:215-216)。旧实现异常时回退到 command.split()[0] 只检查首词,会放过 ;/&&/&/换行分隔的后续命令——本次修复正是针对该缺陷,方向正确。
    • 影响是对 whitelist_commands 或工作目录外的调用方存在破坏性变更:此前能放行的合法复合命令(如 diff <(sort a) <(sort b)、含进程替换的管道)现在会被 SECURITY_RESTRICTION 拦截,且无逃生口。建议在 release/文档中明确说明该收紧,或评估是否需要为进程替换解析内部命令而非整体拒绝。

💡 Suggestion

  • trpc_agent_sdk/tools/file_tools/_bash_tool.py:211-219:未处理 shell 复合关键字体
    • 解析器对每个片段只取 tokens[0] 作为基础命令,未对 case/if/for/while/until/select/[[/(( 等复合关键字做特殊处理,不会递归检查其体内的命令段。默认情况下这些关键字不在白名单内会整体被拦(fail-closed,安全);但若调用方将此类关键字加入白名单,体内命令不会被逐条校验。建议在 _extract_base_commands 文档注释中标注该作用域边界,便于长期维护。

总结

该 PR 是对 BashTool 命令白名单校验的安全加固重写,正确性良好、测试覆盖充分,未发现可绕过的命令注入或逻辑缺陷。唯一需关注的是对进程替换等语法由“放行”改为“整体拒绝”带来的兼容性收紧,建议在文档中明确告知调用方。

测试建议

暂无额外测试建议。现有参数化测试已覆盖分隔符、引号/转义、heredoc、进程替换、行续行与未终止语法等高风险路径;如需进一步加固,可补一条 case/for 复合关键字场景以固化预期行为(与上述 Suggestion 对应)。

@FireCollector

Copy link
Copy Markdown
Author

感谢审查。反斜杠续行导致的绕过问题已在 c9b78d4 中修复,并补充了命令替换以及输入、输出进程替换的静态回归测试。

对于进程替换及其他无法可靠分类的 shell 语法,本 PR 在白名单受限场景下会有意采用 fail-closed 策略直接阻止。这是一项明确的兼容性收紧;若要在保持安全边界的同时支持嵌套 shell 结构,需要引入更完整的 shell 解析能力,超出了本次修复范围。

目前所有自动检查均已通过。烦请维护者在方便时进行人工评审,谢谢!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BashTool whitelist validation skips standalone and compound commands

2 participants