feat: add Tool Script Safety Guard#108
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #108 +/- ##
==========================================
Coverage ? 88.25751%
==========================================
Files ? 491
Lines ? 47128
Branches ? 0
==========================================
Hits ? 41594
Misses ? 5534
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…mands dead code, improve redaction, fix 9 false positives - Add _python_scanner.py: AST parsing with alias resolution, getattr detection, taint tracking - Add _bash_scanner.py: shlex tokenization with quote tracking, combined-flag rm -rf detection - Rewrite _scanner.py: three-layer scanning (AST + shlex + regex), early script-size check - Fix whitelist_commands dead code in ProcessAndSystemRule and NetworkEgressRule - Fix .env falsely matching os.environ, fix Python comments being flagged, fix re.compile false positive - Fix env taint for HOME/USER/PATH, fix /tmp write over-escalation, fix bash echo string false positives - Add threading.Lock to AuditLogger, add dual-layer evidence redaction - Add examples/tool_safety/ Chinese README.md and DESIGN.md Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
59cbb90 to
6a4495d
Compare
AI Code Review我已经掌握了足够的上下文，现在开始总结审查结论。 已确认的关键问题：
事实上，重读流程：黑名单覆盖仅在
让我再验证一下 但有一个实际问题：对于 Bash 脚本， 让我专注于最严重的、可验证的问题。让我验证 ENV 证据脱敏失败的假设，检查 正则表达式（详细模式）： 等等 —— 但 但是 —— 如果环境变量是 所以环境变量泄露取决于变量名。对于测试中的 另外 所以，对于像 现在让我敲定最终结论。我也要再次确认 让我再重新考虑一下 现在让我写下审查结果。我已经审查了 diff（新增的 发现的问题🚨 Critical
|
| # Determine decision | ||
| decision = self._policy.decision_for(max_risk) | ||
|
|
||
| # Apply blocklist override — blocklist patterns always → deny |
There was a problem hiding this comment.
allow_patterns 可绕过 blocklist 的 DENY，形成安全旁路
决策流程先在 _check_blocklist_override 将命中 blocklist 的脚本升级为 DENY，随后 _check_allow_patterns 又因 decision != ALLOW 触发，一旦脚本匹配任一 allow 正则就把 DENY 翻转为 ALLOW，导致 rm -rf / 等危险脚本被放行。建议 blocklist 命中时直接短路返回 DENY，不再进入 allow 覆盖分支。
AI Code Review确认： 让我再验证一下 512 KiB 的 现在我已经掌握了足够的信息。让我再检查一下在分词前针对整个源代码运行的正则表达式模式（ 让我完成审查结论的总结。 发现的问题🚨 Critical
|
| # EARLY RETURN: script too large → skip expensive scanning | ||
| # (moved BEFORE rules — was a bug: ran after all rules) | ||
| # ══════════════════════════════════════════════════════════════ | ||
| if script_lines > self._policy.max_script_lines: |
There was a problem hiding this comment.
超大脚本跳过全部扫描可被绕过拦截
当 script_lines 超过 max_script_lines（默认 500）时直接早返回，不运行任何 rule/AST/bash 扫描，决策为 NEEDS_HUMAN_REVIEW 仍放行执行。攻击者用空行/注释填充到 >500 行即可彻底绕过内容检测执行 rm -rf / 等载荷。建议对超大脚本直接 DENY，或至少在 blocklist_patterns 命中时强制 DENY，并先做整体 blocklist 扫描再判断体积。
…ew, run blocklist pre-check
AI Code ReviewI've completed a thorough review of the PR diff. Here are my findings. 发现的问题🚨 Critical未发现 Critical 级别问题。
|
AI Code ReviewBased on my analysis of the PR diff and surrounding repository context, here is my review. 发现的问题🚨 Critical
|
|
|
||
| # Apply blocklist override — blocklist patterns always → deny | ||
| if decision != Decision.DENY: | ||
| decision = self._check_blocklist_override(script, decision) |
There was a problem hiding this comment.
allow_patterns 可覆盖 blocklist 的 DENY
决策流水线先做 blocklist 升级为 DENY，随后 if decision != ALLOW and self._check_allow_patterns(script): decision = ALLOW 会把 DENY（含 blocklist 命中）降回 ALLOW，破坏 DESIGN.md 声明的安全不变量。建议 allow_patterns 仅在 decision == NEEDS_HUMAN_REVIEW 时生效。
| rule_id="ENV-001", | ||
| category=RiskCategory.SENSITIVE_INFO_LEAK, | ||
| risk_level=RiskLevel.HIGH, | ||
| evidence=f"env: {blocked_var}={scan_input.environment_variables[blocked_var][:50]}", |
There was a problem hiding this comment.
ENV-001 证据泄露敏感环境变量真实值
证据字段 f"env: {blocked_var}={...[:50]}" 会把 blocklist 命中的密钥（如 AWS_SECRET_ACCESS_KEY）前 50 字符明文写入审计日志和报告，且 mask_secrets_in_reports 不会处理该格式。修复：证据只记录变量名，不保留值。
1. Fix RESET NameError in _print_summary when --no-color (tool_safety_check.py)
- RESET was only defined in the color branch, causing crash with --no-color.
2. Fix allow_patterns overriding blocklist DENY (_scanner.py)
- allow_patterns now only upgrades NEEDS_HUMAN_REVIEW → ALLOW,
never overrides DENY from risk-level or blocklist decisions.
3. Fix ||/&& false positives in _check_operators (_bash_scanner.py)
- shlex splits || into two | tokens causing false pipe detection.
- Similarly && was falsely detected as background operator.
- Now skips adjacent |/| and &/& to correctly identify shell operators.
4. Update tests to reflect new allow_patterns behavior (test_tool_safety.py)
5. Update DESIGN.md and README.md to document fixed behaviors
- Decision pipeline: allow_patterns only upgrades NEEDS_HUMAN_REVIEW
- Oversized scripts: always DENY with blocklist pre-check
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AI Code Review已确认：如果找不到脚本，装饰器会静默跳过扫描（不记录日志，也不发出警告）。这是一个安全漏洞——受装饰的函数看起来受到了保护，但如果参数结构不匹配，可能会完全跳过检查。 另外请注意，即使找到了脚本，它也只调用 现在我已经有足够的信息了。让我再快速检查一下 让我检查最后一件事 —— 让我核实一下冗余的三元运算符以及 现在进行总结。我还要指出 让我核实一下对审计锁增长和仅使用线程锁（而非进程锁）的担忧是否值得作为次要说明。这是一个真实但较小的稳定性问题： 现在撰写审查报告。## 发现的问题 🚨 Critical
|
| if args_text.strip(): | ||
| script = script + "\n" + args_text | ||
|
|
||
| script_lines = script.count("\n") + (1 if script else 0) |
There was a problem hiding this comment.
字节大小限制 max_script_bytes 从未强制执行
scan() 仅按 script.count("\n") 统计行数，攻击者可用无换行的单行巨型脚本绕过 max_script_lines；而 max_script_bytes 在代码中完全未校验，与 README 声明不符。建议在 early-return 前对 len(script.encode()) > max_script_bytes 判断并 DENY，同时避免超大文本进入 regex/AST 扫描。
| if isinstance(arg, dict) and script_arg_name in arg: | ||
| script = arg[script_arg_name] | ||
| break | ||
| if script and isinstance(script, str): |
There was a problem hiding this comment.
装饰器找不到脚本参数时静默跳过扫描
当被装饰函数参数结构不匹配（脚本在嵌套对象或非 dict 实参中）时，if script and isinstance(script, str) 为假，扫描被完全跳过且无任何告警，被保护函数照常执行。建议在 script 为空时至少 logger.warning 或抛错，并支持自定义脚本提取回调。
1. ENV-001 evidence leak: evidence field now stores ***REDACTED*** instead of the raw environment variable value (fixes secrets leaking into JSON reports, CLI output, and SafetyWrapper.last_report). 2. ENV-001 deduplication collapse: _deduplicate_findings now includes matched_pattern in the dedup key for line_number==0 findings, so multiple blocklisted env vars each produce independent findings instead of being collapsed into one. 3. _audit.py docstring: corrected misleading 'thread/process-safe' claim to 'thread-safe' — threading.Lock is not cross-process. 4. _python_scanner._handle_for: range() loop detection now handles 2-arg range(start, stop) and 3-arg range(start, stop, step) forms in addition to the original 1-arg range(stop). 5. _bash_scanner._check_redirects: strip surrounding quotes from redirect targets to reduce false positives from bash test expression comparisons (e.g. [[ "a" > "/etc/passwd" ]]). 6. New tests: env value leak, env dedup collapse, allow_patterns vs blocklist conflict (both pattern and env-based). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sing, tee dead branch
1. Enforce max_script_bytes: single-line giant payloads with no newlines
are now caught by a byte-size check alongside the existing line-count
check. Both trigger DENY with a blocklist pre-scan.
2. Decorator silent skip: @safety_wrapper now logs a warning when the
script_arg_name is not found in kwargs / positional dict args, so
callers can detect missing protection.
3. _check_blocklist_override now receives script_type — Python
string-literal stripping is skipped for Bash to avoid false negatives
on cat '/etc/shadow' etc.
4. rm flag detection: parse short flags character-by-character instead of
substring matching (rm -i no longer falsely triggers rm -rf).
5. tee dead branch: tee writes to sensitive paths (tee /etc/shadow,
tee ~/.aws/credentials) are now detected.
6. Evidence truncation: <truncated:N> now reports len-300 consistently.
7. Redundant ternary: Decision.DENY if x else Decision.DENY → Decision.DENY.
AI Code Review我已经获取了所需的所有信息。以下是我的评审结果。 发现的问题🚨 Critical
|
| script = arg[script_arg_name] | ||
| break | ||
| if script and isinstance(script, str): | ||
| wrapper_inst.check(script) |
There was a problem hiding this comment.
装饰器路径漏扫 command_args/env/working_directory
装饰器路径只调用 wrapper_inst.check(script)，未透传 command_args / environment_variables / working_directory。导致经装饰器工具传入的命令行参数不被扫描、ENV-001 黑名单环境变量检测永不触发。建议从 args/kwargs 中提取并显式传给 check。
| script = arg[script_arg_name] | ||
| break | ||
| if script and isinstance(script, str): | ||
| wrapper_inst.check(script) |
There was a problem hiding this comment.
装饰器路径漏扫 command_args/env/working_directory
sync_wrapper 同样调用 wrapper_inst.check(script)，未透传 command_args / environment_variables / working_directory。建议从 args/kwargs 中提取并显式传给 check。
- _safety_wrapper: decorator now passes command_args/environment_variables/
working_directory to SafetyWrapper.check (was dead code path)
- _safety_filter: _before now extracts command_args/env_vars/work_dir from req
- _scanner: fix W504 line break after binary operator
AI Code Review我无法执行。让我从正则表达式进行静态推理： 让我再验证一下 现在让我检查一下 让我验证一下 让我检查一个更重要的正确性问题：在 让我检查 现在有一个真正的顾虑： 让我检查 现在让我总结一下。让我再检查一个真正有影响的 bug：扫描器中的 让我看看 现在，一个真正值得关注的问题： 但是这里有一个细微的顺序问题：第 261-262 行在允许升级后重新计算了 现在让我验证关于超大预检查无法捕获在允许的域名外进行 现在让我确认一个真正的问题：关于 让我最后检查一下 现在，我对发现的问题已经足够了。让我再检查一下 让我写下审查结论。让我再验证一下 再确认一下： 现在，关于 现在让我最终确定发现的问题。让我再通过检查 让我检查一下 bash 扫描器是否有独立检查。bash 扫描器的 对于没有端口的情况，例如 现在让我完成审查。 发现的问题🚨 Critical
|
| return list(seen.values()) | ||
|
|
||
|
|
||
| def _extract_url(text: str) -> Optional[str]: |
There was a problem hiding this comment.
URL 域名提取导致白名单绕过
_extract_url 使用 https?://([^\s/\"':]+) 提取域名，遇 : 即停止。对 http://localhost:8080@evil.com/x 提取出 localhost 命中白名单被降级为 INFO/ALLOW，但 curl 实际请求 evil.com。该逻辑在 AST、shlex、regex 三层共用（亦见 _rules.py:925-940），可用 localhost:<port>@evil.com 绕过网络出口管控。建议先剥离 user:pass@ 前缀再取 host，并显式校验端口/用户信息段。
- _extract_url: include :port and @userinfo in authority capture, then strip both to get bare hostname. Fixes localhost:8080@evil.com bypass. - scan(): use local effective_script_type instead of mutating scan_input. - Same fixes applied to duplicate _extract_url in _rules.py.
AI Code Review我已经有了足够的上下文。让我来撰写最终的审查报告。 发现的问题🚨 Critical
|
| }, | ||
| )) | ||
|
|
||
| def _check_redirects(self, line_no: int, raw_line: str, tokens: List[str]) -> None: |
There was a problem hiding this comment.
重定向检测将 2>/dev/null 误判为 CRITICAL 导致合法脚本被 DENY
_check_redirects 对 >/>> 后目标用 target.startswith("/dev/") 判定敏感，/dev/null 命中条件生成 BASH-FILE-003 CRITICAL 并强制 DENY。建议将 /dev/null、/dev/zero 等从重定向敏感判定中排除，仅对块设备 /dev/sd*、/dev/[sv]d*、/dev/mmc* 保留 CRITICAL。
- _bash_scanner: exclude /dev/null/zero/random/urandom/std*/tty from
redirect CRITICAL detection (2>/dev/null is harmless).
- _python_scanner: _extract_domain_from_url now strips userinfo:port
same as _extract_url (fixes localhost:8080@evil.com AST bypass).
- YAPF formatting applied across all changed Python files in the PR - Remaining flake8 E501/W391/E126/E127/E302/E999 issues resolved - All safety module tests passing (126/126)
AI Code Review已确认： 现在让我对 我现在有足够的信息来撰写审查意见了。让我再验证一下 让我写出最终的审查意见。 发现的问题
|
- blocklist_commands now line-by-line with echo/comment stripping - ResourceAbuseRule sleep regex parses s/m/h/d units - Redundant '>' + '>' simplified, duplicate reports removed - ~3200 new test lines covering scanner, rules, bash, wrapper, filter
AI Code Review发现的问题
|
…d pre-check - AuditLogger.log_event: fcntl.flock + os.fsync for cross-process safety - AuditLogger.read_events: tail-read with byte limit instead of full load - _FILE_LOCKS: LRU-capped at 128 entries, os.path.realpath normalisation - _scanner: oversized pre-check also scans blocklist_commands
AI Code Review现在我已经掌握了所需的所有信息。接下来，我将整理最终的审查报告，重点关注那些真正具有高价值且可验证的问题，并根据各层级之间的防御纵深对其进行适当评级。 发现的问题🚨 Critical
|
| # Taint tracking: assignments and sinks | ||
| # ------------------------------------------------------------------ | ||
|
|
||
| def _handle_assign(self, node: ast.Assign) -> None: |
There was a problem hiding this comment.
危险可调用赋值给变量后丢失跟踪
e = eval; e("...")、s = os.system; s("id")、m = __import__("os"); m.system("id") 均不被检测：_handle_assign 仅跟踪网络/污点来源，不对 bare-name 或动态导入结果做别名传播，拆成两语句即绕过内联链式检测。修复：当 node.value 为 ast.Name 且 canonical 名在危险集合，或为 __import__/importlib.import_module 调用时，向 _aliases 传播。
- _python_scanner: _resolve_canonical now checks _class_instances for
Session().get() bypass (s=requests.Session(); s.get(url) now detected)
- _python_scanner: _handle_assign propagates dangerous callable names
(e=eval; e('x'), m=__import__('os'); m.system('id') now caught)
- _scanner: rule exceptions now emit GLOBAL-003 sentinel finding instead
of silent fail-open
- _scanner: allow_patterns refuses upgrade when HIGH/CRITICAL findings exist
- _scanner: reload_policy() also refreshes _rules
- _bash_scanner: _PREFIX_CMDS extended with env/nohup/timeout/nice/xargs
- _rules: _all_commands_whitelisted splits on ;/&&/||/& in addition to |
AI Code Review现在我已经完全掌握了情况。我来写一下审查意见。 发现的问题🚨 Critical
|
| )) | ||
|
|
||
| # --- Eval / exec / dynamic execution --- | ||
| if canonical in _DYNAMIC_EXEC_CALLS: |
There was a problem hiding this comment.
compile/getattr 被无条件判为高危动态执行
_handle_call 对任何 compile(...)/getattr(...) 命中 _DYNAMIC_EXEC_CALLS 即产出 eval_exec，转为 HIGH→默认 policy 直接 DENY。这会误杀 re.compile(...)、regex.compile(...)、getattr(obj,"attr",default) 等合法脚本，且与 _rules.py 中 re.compile 的豁免冲突。建议豁免 re.compile/regex.compile，并对 getattr 仅在非常量/动态解析参数时告警。
- _python_scanner: exempt re.compile and regex.compile from dynamic
execution detection (pattern compilation is safe)
- _python_scanner: exempt getattr(obj, attr, default) with 3 args
- _scanner: remove unused has_critical variable (F841)
AI Code Review我已经审查了 发现的问题
|
- _audit: remove LRU lock eviction (could orphan in-use locks on Windows)
- _python_scanner: getattr(obj, attr, default) / getattr(...)() with 3+ args
no longer falsely triggers dyn-exec (HIGH→DENY)
- _python_scanner: re.compile / regex.compile exempted from dyn-exec detection
- _python_scanner: _check_dynamic_call also applies 3-arg getattr exemption
AI Code Review确认。现在让我来验证一下 让我检查一下在 我已经有足够的信息进行最终总结了。让我确认一下黑名单覆盖（blocklist override）调用顺序的问题—— 让我再验证一个关于 让我写下审查报告。 发现的问题🚨 Critical（无）
|
- _python_scanner: 3-arg getattr only exempted when NOT immediately called.
getattr(os,system,None)('id') now correctly flagged as dyn-exec.
x=getattr(cfg,key,def) as plain expression still safe.
- _policy: get_policy/reload_policy now use threading.Lock to prevent
concurrent double-load and inconsistent scanner cache.
- _python_scanner: remove W504 line break after binary operator.
AI Code Review两个辅助代理仍在生成中。我已经直接通过代码验证了我的核心发现，并从扫描器代理那里获得了确认。我已经有足够的内容来撰写一份稳健的审查报告。我现在进行最终定稿。 发现的问题🚨 Critical
|
| inner = self._resolve_canonical(node.func) | ||
| return inner | ||
|
|
||
| if isinstance(node, ast.Subscript): |
There was a problem hiding this comment.
builtins[...] exec/eval 绕过全部检测层
__builtins__["exec"](...) / globals()["__builtins__"]["eval"](...) 形式可绕过 AST 与 regex 双层检测：_resolve_canonical 对 Subscript 仅返回容器名，_check_dynamic_call 不处理 Subscript 调用，regex 层剥离字符串字面量还会清空 "exec"。应将 __builtins__/globals/vars/locals 上的 Subscript 调用视为 dynamic-exec sink，并在剥离注释/字符串前先匹配敏感关键字。
| receiver = self._resolve_canonical(node.func.value) if isinstance(node.func, ast.Attribute) else "" | ||
| if canonical == "compile" and receiver in ("re", "regex"): | ||
| pass # re.compile(pattern) — safe, pattern compilation only | ||
| # getattr(x, y, default) as plain expression (NOT immediately |
There was a problem hiding this comment.
3-arg getattr 豁免导致先存后调漏报
len(args)>=3 的 getattr 一律 pass，但 getattr(os, "system", None) 在属性存在时仍返回 os.system；配合 d=[...]; d[0]("id") 的 Subscript 调用漏报，整条混淆链逃过检测。策略 python_functions 未含 getattr，regex 层也不兜底。不应按参数个数豁免，对第二参数为危险属性名常量或非常量的 getattr 应视为 dynamic-exec。
…t crash
- _python_scanner: Subscript(__builtins__/globals/vars/locals[...]) now
detected as dynamic exec sink (was complete bypass)
- _python_scanner: remove 3-arg getattr exemption entirely
- _bash_scanner: normalize cmd with basename so /bin/rm matches rm check
- _safety_filter: safe extraction when req['args'] is list not dict
2 participants
实现 Tool 执行脚本的安全检查器，覆盖 6 种风险类型：
Resolves 构建 Tool 执行脚本安全扫描、Filter 拦截与监控机制 #90