feat: add Tool Script Safety Guard - #232
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
AI Code Review好的， 现在我已经有了全面的理解。让我再验证一下 让我检查一下是否存在这样一个问题： 现在有一个关键点： 现在让我完成最后的工作。让我确认一下 BashTool 的默认超时时间是否确实为 300。 BashTool 描述显示默认值为 300。代码中为 让我再检查一下是否没有测试捕获到 BashTool 默认超时时间与安全防护之间的交互。集成测试都显式传递了 timeout。因此这个过度阻塞的问题未被测试覆盖。已确认。 现在撰写审查意见。 让我再确认一个潜在的真正问题： 让我确认一下 实际上，更大的问题是：每个过滤器都会打开/关闭文件（每次调用 让我再验证一下 让我对 现在让我检查 pydantic 的 我已经完成了审查，让我来写最终的审查结论。 发现的问题🚨 Critical
|
|
|
||
| Each tool gets its own filter instance to avoid state leakage. | ||
| """ | ||
| for tool in tools: |
There was a problem hiding this comment.
安全 filter 随请求无界累积
add_tool_safety_filter 每次调用都对工具 filters 列表 append，而 get_tools 每次请求都会调用它，导致同一工具被重复挂载 ToolSafetyFilter，请求被重复扫描 N 次、审计事件重复写 N 份，且 filters 随请求数无界增长。建议注入前剔除已有实例或用去重标记位，保证每个工具只挂一个实例。
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #232 +/- ##
==========================================
Coverage ? 88.00906%
==========================================
Files ? 494
Lines ? 46360
Branches ? 0
==========================================
Hits ? 40801
Misses ? 5559
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
AI Code Review我已获取所有必要信息，正在编写审查报告。 发现的问题🚨 Critical（无）
|
AI Code ReviewI have enough context now. Let me write the final review. 发现的问题🚨 Critical
（经核对，安全守卫整体为 fail-closed 设计，未发现会导致核心功能失败或权限绕过的 Critical 问题。）
|
AI Code Review沙盒环境正在反复拦截 基于我的静态分析，以下是我的代码审查结论： 发现的问题🚨 Critical
|
| policy: Optional[PolicyConfig] = None, | ||
| audit_path: Optional[str] = None, | ||
| block_on_review: bool = False) -> None: | ||
| """Attach a fresh ToolSafetyFilter instance to each tool. |
There was a problem hiding this comment.
ToolSafetyFilter 阻塞返回结构缺少统一 success/error
阻塞执行时仅设置 rsp.rsp={...} 和 rsp.is_continue=False，未设置 rsp.error，且返回 dict 与 BashTool 的 {success,error,return_code} schema 不一致，下游 FunctionTool 解析可能无法识别 blocked 结果。建议复用 BashTool 返回结构或在 rsp.error 上明确设置错误。
| blocked: bool | ||
| sanitized: bool | ||
| target: ScanTarget | ||
| language: ScriptLanguage |
There was a problem hiding this comment.
AuditLogger 锁缓存竞态与 resolve() 异常风险
_path_locks 为类级共享 dict，if key not in _path_locks 与赋值非原子，并发首次写同一路径可能生成多把锁导致行交错；Path(path).resolve() 在路径不存在时可能抛异常，使整条工具执行链抛错。建议用 threading.Lock 保护 dict 访问并对 resolve() 做异常兜底。
AI Code Review所有行号已确认。现在开始撰写审查意见。 发现的问题🚨 Critical
|
| )) | ||
|
|
||
| # Check cwd against denied paths | ||
| if request.cwd and self._policy.is_path_denied(request.cwd): |
There was a problem hiding this comment.
基于 cwd 的路径拒绝判定使用 startswith 导致误阻断
is_path_denied 对 denied_paths 用 startswith 匹配，当进程在 /root、/etc 下运行时 execution_dir 会命中 R001_SYSTEM_PATH_OVERWRITE（CRITICAL→DENY），使开启安全守卫后所有 bash 命令被误阻断。建议对 cwd 改用路径等值或直接子目录的精确匹配。
AI Code Review我已经掌握了所需的所有信息，现在让我来写审查报告。 发现的问题🚨 Critical
|
|
|
||
| # Check for sensitive path access (e.g. cat ~/.ssh/id_rsa) | ||
| for sensitive in SENSITIVE_PATHS: | ||
| if sensitive in line and not sensitive.startswith("*"): |
There was a problem hiding this comment.
敏感路径 glob 规则 (.pem/.key) 永远无法命中，造成 R001 漏报
SENSITIVE_PATHS 中 .pem、.key 是 glob 形式，但匹配用的是子串匹配且 not sensitive.startswith("") 直接跳过它们；Python 侧 _check_sensitive_path 同样是子串匹配。建议改为按后缀匹配（如 text.endswith(".pem")）或用 .pem/.key 子串形式，并移除 startswith("") 跳过逻辑。
AI Code Review现在我已经获取了所有信息。让我来编写审查报告。 发现的问题🚨 Critical
|
| from trpc_agent_sdk.skills import SkillToolSet | ||
| return SkillToolSet( | ||
| paths=[str(SKILL_ROOT)], | ||
| filters=[safety_filter], |
There was a problem hiding this comment.
SkillToolSet 用错误参数挂载安全过滤器
SkillToolSet 构造签名只接受 tool_filter 和 **run_tool_kwargs，没有 filters/allowed_cmds 形参，传入的 filters=[safety_filter] 被静默吞入 run_tool_kwargs，ToolSafetyFilter 永远不会挂到 skill 工具上，skill 执行路径实际无安全防护。应改为通过 add_tool_safety_filter 或 toolset 实际暴露的过滤器接入方式注入。
| connection_params=StdioConnectionParams( | ||
| server_params={"command": sys.executable, "args": [str(MCP_SERVER)]}, | ||
| ), | ||
| tool_filter=safety_filter, |
There was a problem hiding this comment.
MCPToolset 把 ToolSafetyFilter 错误地当作 tool_filter 传入
MCPToolset.tool_filter 期望 ToolPredicate 或工具名列表，而非 BaseFilter；且 is_include_all_tools 默认为 True 时 _is_tool_selected 直接返回 True，过滤器被完全忽略，安全过滤器从未挂到 MCP 工具上。应使用 filters=[safety_filter]。
AI Code Review我已经了解了。让我最后确认一件事：关于 在 其实 让我整理一下审查结果。最重要的真正问题是：
让我再次验证 #1 的严重性，并通过检查在 查看 让我撰写审查报告。Based on my review of the safety guard implementation, the integration points, and the tests, here are my findings. 发现的问题
|
AI Code Review我现在已经有了完整且深入的了解。让我来总结一下我的发现。 发现的问题🚨 Critical
|
| block_on_review: If True, NEEDS_HUMAN_REVIEW decisions also block | ||
| execution. Default False (only DENY blocks). | ||
| """ | ||
|
|
There was a problem hiding this comment.
扫描异常路径构造 SafetyReport 缺少必填字段且未真正阻断
scan 抛异常时构造的 DENY SafetyReport 缺少必填的 language/target，未设置 sanitized，会在构造时抛 TypeError 被外层 except 捕获，路径与设计不符且丢失安全上下文。应从 scan_req 补齐 language/target/sanitized=False 并显式 set_blocked(True)。
| if should_block: | ||
| rsp.rsp = { | ||
| "success": False, | ||
| "error": f"TOOL_SAFETY_BLOCKED: {report.summary}", |
There was a problem hiding this comment.
filter 阻断路径未调用 set_blocked(True) 导致审计字段不一致
阻断时只设置 rsp 与 is_continue=False，未调用 report.set_blocked(True)，使审计日志 blocked 记为 False，与 BashTool 路径不一致。block_on_review 触发 NEEDS_HUMAN_REVIEW 阻断时同样遗漏，应在阻断前补 report.set_blocked(should_block)。
b85b7d7 to
a304488
Compare
AI Code Review现在我已经掌握了准确的行号。让我再重新思考一下，关于 BashTool 的稳定性问题是否真的值得提出警告——因为它周围的外部代码中确实有一个宽泛的 让我再重新考虑一下 SafeCodeExecutor 是否暴露出了一个更严重的问题——它没有宽泛的 except，所以扫描器的异常会向上抛出给调用链，这可能导致 agent 在执行代码时崩溃。这比 BashTool 的问题更严重。值得作为警告提出。 我还需要验证一下 让我完成最终总结。 发现的问题
|
AI Code Review我已经完整审查了 发现的问题🚨 Critical
|
|
|
||
| # Block? | ||
| if should_block: | ||
| rsp.rsp = { |
There was a problem hiding this comment.
ToolSafetyFilter._before 通过 mutation 阻断但未 return FilterResult
_before 直接修改传入的 rsp（rsp.rsp=...、rsp.is_continue=False）来阻断执行，却没有 return 阻断结果。当前依赖框架对同一对象的副作用传递，属脆弱契约；一旦上游改为以 _before 返回值作为结果，阻断会静默失效并放行危险命令。建议显式 return 阻断结果或与框架确认契约，并补一条阻断后 handle 不被调用的端到端测试。
AI Code Review我将停止运行命令（它们被持续拦截），并基于我对代码的深入阅读来完成我的审查。我已经有足够的上下文来提供审查结果。 让我记录下从阅读
此外，
让我专注于验证过的、高置信度的问题。最清晰的是：
让我再重新考虑一下去重问题，这是最能从代码中验证的“正确性” Bug：
但是多行上的 真正的一个：在 让我重新考虑 现在是一个具体的 Bug： 让我敲定那些高置信度的问题： 警告 1: 警告 2: 警告 3: 鉴于我已经深入阅读且命令执行受阻，让我写一份包含最可辩护发现的审查。我会保持简洁，并使用 diff 文件行号。 让我验证 对于执行器扫描不传递元数据的问题： 我还要指出 让我再检查一下： 一个是警告级别的真正安全问题： 一个是明确的 Bug：在 让我写出最终的审查报告。我将包含：
我还将指出 我还要重新考虑测试有效性问题： 现在开始撰写。基于对 发现的问题
|
AI Code Review正确的 API 应该是 现在我已经有了所需的一切。让我再验证一下 让我快速确认一下 Python 版本的行为不会产生干扰 —— 即使属性没有 setter，在 Python 3.11 及以上版本中，普通实例的属性赋值仍可能成功吗？不会：一个没有 setter 的 让我来写评审报告。我还想指出一个值得一提的稳定性问题： 再检查一个潜在问题： 再看一下： 现在开始撰写评审报告。 发现的问题🚨 Critical
|
| Each tool gets its own filter instance to avoid state leakage. | ||
| """ | ||
| for tool in tools: | ||
| tool.filters = [f for f in tool.filters if not isinstance(f, ToolSafetyFilter)] |
There was a problem hiding this comment.
向只读 filters 属性赋值导致安全注入崩溃
add_tool_safety_filter 直接对 tool.filters 赋值，而 FilterRunner 上的 filters 是只读 @property 无 setter，真实 BaseTool/MCPTool/SkillTool 实例调用会抛 AttributeError，使 SafetyWrappedToolSet.get_tools() 及示例注入路径运行即崩溃、安全防护失效。应改用 tool.add_one_filter(ToolSafetyFilter(...))。
AI Code Review我已经准备好了。让我来写最终的审查报告。 发现的问题🚨 Critical（无）
|
…atching to the is_path_denied function
…ignment and sample issues
…zation of README.md
da92bc3 to
b6356ae
Compare
AI Code Review现在我已经有了全面的理解，让我来整理一下审查意见。 关键观察点：
但有一个隐蔽的问题：在默认配置下，对“安全” Bash 的
但
现在让我来撰写审查意见。需要强调的最具可操作性的问题：
让我检查一下 一个真正的 Bug：在 让我再检查一个潜在的实际 Bug：在 现在让我检查 还有一个： 让我定稿。我需要核实关于过度阻断的声明是否属实，通过检查 现在让我开始撰写审查意见。## 发现的问题 🚨 Critical
|
| # (being IN the credential dir is already dangerous). | ||
| _CREDENTIAL_DIRS = frozenset({"~/.ssh", "~/.aws", "~/.kube"}) | ||
|
|
||
| def is_path_denied(self, path_text: str) -> bool: |
There was a problem hiding this comment.
is_path_denied 对 cwd 精确等于敏感目录放行，可绕过路径保护
is_path_denied 仅在路径为 denied 目录的子路径或扩展名/凭据目录时才拒绝，当 cwd 恰好等于 /root、/etc 等时返回 False，导致 _scan_context_safety 不报 R001。攻击者可将工作目录直接设为敏感目录后在内部读写，绕过路径保护。建议对 denied 路径的精确匹配也判为拒绝，至少覆盖 /root、/etc、~/.ssh 等危险前缀。
53bc472 to
91a499b
Compare
AI Code Review注意：现有的 现在，让我完成最终审查。让我重新考虑一下关于 dead-limit 的 Warning 是否值得提及——它确实是一个从策略到扫描器上下文检查的全新代码路径，但从未被调用者填充过。这就是“测试有效性/正确性”方面的差距。值得给出一个 Warning。 让我快速验证一下执行器是否没有传递 max_output。在 让我写出审查意见。 发现的问题🚨 Critical
|
| risk_type=RiskType.NETWORK_EGRESS, | ||
| risk_level=RiskLevel.HIGH, | ||
| evidence=sanitize_text(url, self._policy.secret_patterns), | ||
| line=line_num, |
There was a problem hiding this comment.
递归删除检测可被 -fr/-Rf 参数顺序与大小写绕过
删除规则正则只匹配 -rf 顺序且小写，rm -fr /、rm -Rf /、rm -rF / 均不命中任何规则，根目录递归删除被判为 ALLOW，构成关键绕过。建议分别匹配 -r/-R 与 -f/--force 的组合，或归一化 flags 后再判断。
AI Code Review我现在已经全面了解了。我来整理一下审查意见。重点突出问题如下：
我来核实一下关于 ReDoS 注释准确性的疑虑 —— 我来重新考虑一下哪些问题是真正值得标记的严重问题。最实质性的正确性问题：
我来确认一下：yaml 中 这是一个合理的警告（匹配逻辑对于常见的 我来核实一下 我再将 实际上 这是最有力的发现。我来定稿。 我还要注意： 我来写审查意见。## 发现的问题 🚨 Critical
|
| found_denied = False | ||
| for denied in self._policy.denied_commands: | ||
| try: | ||
| denied_tokens = shlex.split(denied) |
There was a problem hiding this comment.
denied/review 命令令牌前缀匹配失效
行用 shlex.shlex(punctuation_chars="|;&") 切分，而 denied/review 条目用 shlex.split 切分，两侧 tokenize 不一致导致前缀永不相等。带值的 denied 条目（如 dd if=）对 dd if=/dev/zero of=/dev/sda 永远漏判，dd 无正则兜底形成真实绕过；&&/;/| 链内命令同样无法命中自定义 denied/review 条目。建议对剥离 shell 操作符后的每个子命令分别匹配，并对带 = 的条目改用子串/正则匹配。
ddacff1 to
0cc5336
Compare
AI Code Review我已经完全理解了。让我来整理一下评审意见。 发现的问题🚨 Critical
|
| reports.append(report) | ||
| all_findings.extend(report.findings) | ||
| except Exception: # pylint: disable=broad-except | ||
| # fail-closed: scanner error blocks execution |
There was a problem hiding this comment.
传入空 work_dir/timeout 致上下文安全检查失效
_scan_code_block 把 cwd=self.work_dir(通常为空字符串)与 tool_metadata 中 self.timeout(默认0)传入 ScanRequest，导致 _scan_context_safety 的 denied-path 检查对 cwd 永不生效、超限检查形同虚设。应从 input_data/inner executor 解析真实 work_dir 与 timeout 后再构造 ScanRequest。
| script=command, | ||
| language=ScriptLanguage.BASH, | ||
| tool_name=self.name, | ||
| target=ScanTarget.TOOL, |
There was a problem hiding this comment.
完整 os.environ 注入扫描链路存凭证泄露风险
BashTool 将 os.environ.copy() 整份进程环境(含 TOKEN/PASSWORD 等真实值)复制进 ScanRequest.env 并随 report/audit 流转，一旦 report 被序列化或异常打印即可能泄露凭证。建议仅传键集合用于敏感键检测，或先对 env 值脱敏。
3d7820a to
936a9f8
Compare
AI Code Review我现在已经有了完整的了解，让我来写最终的审查报告。 发现的问题🚨 Critical
|
AI Code Review发现的问题🚨 Critical（未发现必须修复的 Critical 问题。）
|
2 participants
Description
实现 Tool Script Safety Guard，用于在 Tool、Skill、MCP Tool 和 CodeExecutor 执行前进行静态安全扫描和风险控制。
Resolves #90
新增
trpc_agent_sdk/tools/safety/模块，支持 Python 和 Bash 脚本安全检测，覆盖以下风险类型：Key Features
Python AST 静态分析：
Bash 静态分析：
shlex解析命令结构策略控制：
tool_safety_policy.yaml风险决策：
allowdenyneeds_human_review提供多种接入方式：
Integration
接入已有执行链路：
trpc_agent_sdk/tools/file_tools/_bash_tool.pyenable_safety_guardsafety_scannerblock_on_reviewtrpc_agent_sdk/code_executors/local/_unsafe_local_code_executor.py_scan_code_block()默认保持关闭：
不影响已有用户行为，保持向后兼容。
Wrapper Supporting
新增：
支持对已有 CodeExecutor、ToolSet 和 MCP Tool 进行安全包装。
Examples and Tests
新增示例：
新增测试覆盖：
Validation
测试结果：
✅ 188 tests passed
✅ 23/23 安全扫描样例通过
✅ 高危样本检出率 100%（32 条）
✅ 安全样本误报率 0%（10 条）
✅ 密钥、删除、网络风险检测覆盖率 100%
✅ 500 行代码扫描：
✅ 策略配置修改可实时生效