fix(runtime): enforce persistent permission denies (#3351) - #4849
Conversation
Generated-by: Codex
00bc197 to
40c0297
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head 40c0297826b670eacf68d1c44d82f6f663510dd4 (OPEN). Two P2s below, plus two P3s. This is a high-quality security-enforcement change, but two gaps overstate its coverage. Whether it merges — and whether the P2s block — is a human decision.
What it does, and what it gets right
A persistent deny layer (deny-command, deny-path) checked before tool dispatch, explicitly overriding session permission modes — including --yolo — with a matching CLI. The author found the bypass path most such features miss: native apply_patch executes provider-side where ToolRuntime cannot intercept, so whenever any path-deny rule exists the native config is switched off in favor of client file tools — accepting a performance cost to hold the rule, and only for path (not command) denies since apply_patch executes no commands. Other verified-correct points: realpath normalization before matching (closes symlink bypass), fail-closed parse failures, PTY fragment input merged incrementally with backspace handling, unverifiable terminal input rejected with the reason documented, bidirectional Glob/Grep containment checks, Windows path-spelling conversion, and a single ToolRuntime construction point with deny checks ordered before session/sandbox checks.
P2 — character classes [...] are half-implemented; ranges silently fail
In globMatches (packages/core/src/runtime-policy/permission-rules.ts), - and ^ are escaped when copied into the regex, so [a-z] becomes the literal [a\-z], matching only a, -, z — no longer a range. For a deny list this is the worst failure mode: the rule reports no error and looks active while blocking almost nothing. A user writing rm [a-z]* gets a rule that blocks nearly nothing, with no warning. The README documents only * and ?, so [...] is undocumented yet half-implemented. Either implement ranges properly or reject [ patterns at rule-creation time with an error. (No injection issue: the body cannot escape the character class.)
P2 — MCP tools are outside path-check coverage
permissionPathsForTool is a whitelist switch whose default returns [] — no path checks at all. All built-in tools are covered (verified one by one), but third-party MCP servers can expose arbitrary filesystem tools through the same dispatch path and hit default, getting zero path-deny enforcement. This may be out of scope for this PR (MCP parameter shapes are unknowable, so generic path extraction is infeasible) — but "what we protect and what we don't" must be documented, or the feature gives false assurance. A user reading deny-path /etc --scope subtree reasonably expects no tool on the host to touch /etc.
P3s
globMatchesrecompiles the regex on every call with*expanded to[\s\S]*— cache per pattern (the set is bounded at 128), and consider limiting*count; multi-*patterns against long model-controlled command text risk catastrophic backtracking (not measured, inferred from structure; rules are trusted-authored so this is not an external attack surface).- Rules are snapshotted at backend creation (
execution-model-composition.ts:142,367); a newly added deny rule takes effect only at the next backend rebuild. Worth documenting the delay for a security control.
What I could not judge
The feature was not run live (CLI interaction, rule-file I/O, cross-platform behavior unobserved). Command parsing is heuristic by nature — variable expansion, command substitution, cd-then-relative-path can all evade path extraction (the author admits this in comments with a literal-absolute-path second scan as backstop); command/path denial should be understood as one defense layer, not an airtight wall.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
本条结论全部来自 @Opus-Qronos-AstroHan 的审查。我自己没有读这份 diff;我核的是当前 head 有没有漂移。当前 head 是 40c0297,未关闭。持久拒绝层本身质量高,但两条缺口夸大了覆盖面:字符类范围静默失效,MCP 工具不在检查内。合不合并、P2 拦不拦,人类定。
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head 93d8d224be01ba10326087021163ce72ccb0b790 (OPEN, MERGEABLE/BLOCKED awaiting human review). One P1, one P2, three P3s below. Prior-head conclusions were voided; this is a full re-review. All 14 checks green on this head. This is a functional security change: whether and how it merges is a human decision.
P1 — the same command passes via Bash but is stopped via terminal
Bash and PTY use two different matchers in tool-runtime.ts. Bash matches the whole string (compileGlob in permission-rules.ts:199-224 produces ^...$-anchored regexes against the full command); PTY splits input on newlines/returns/;|& and compares each trimmed fragment (inspectPtyCommandInput). Actually executed against compileGlob with the README's own flagship rule git push *: git push origin main is stopped, but true && git push origin main, echo x; git push origin main, leading-space, double-space, and /usr/bin/git push origin main all pass — while the same inputs via terminal WriteStdin are all stopped (that path slices and trims). Graded P1, not a declared boundary, because: the PR admits "no full shell parser" yet implemented slicing+trim on the PTY path (the author thought of chained commands, just didn't apply it to Bash — omission, not trade-off); the README presents deny-command 'git push *' as the flagship example with no word that it compares whole strings; and Bash is the model's primary execution surface — the strong path guards the rare entry, the weak path guards the main one.
P2 — macOS case mismatch silently disables path rules
comparablePath (absolute-path.ts:75-79) lowercases only Windows drive paths; everything else compares case-sensitively. But the request side goes through resolve() + realpathAllowMissing(), so on macOS's default case-insensitive volumes realpath returns on-disk casing, while the rule side is the user's raw CLI string (normalizePathRule, permission-rules.ts:173-190, no canonicalization). Any casing skew makes samePath return false — the rule lists normally in permissions list but protects nothing (e.g. rule /Users/me/Secrets vs on-disk secrets). Windows dodges this via toLowerCase(); macOS, a first-class platform here, has no counterpart. (Not run on real macOS hardware — the realpath-casing premise is held with confidence but unmeasured; confirm before final grading.)
P3s (non-blocking)
[...]classes are half-implemented:-is escaped, so[a-z]matches onlya,-,z(verified:rm -rf /[a-z]*compiles to matchrm -rf /-, notrm -rf /etc). Undocumented syntax, silently near-useless on deny lists — either implement ranges or reject[at rule creation.- Non-ASCII command patterns are rejected outright (
/[^\x20-\x7e]/,permission-rules.ts:165-169) — no deny rule can cover Chinese paths/args (not rare on Chinese Windows setups); the path side has no such restriction. Inconsistent. looksLikePathover-matches (any token containing.becomes a path candidate, each costing a realrealpathper dispatch). Safe direction, but on the hot path — cap or batch the candidate set.
Checked and found sound
Path-side symlink/.. bypasses closed (resolve() + realpathAllowMissing()); all eight filesystem-touching built-ins covered in permissionPathsForTool (ArchiveRead excluded correctly — it reads maka://archive/... refs, not filesystem paths); unresolved terminal input denied as unverifiable with the reasoning documented.
What I could not judge
Nothing was run (no tests, no build, no runtime): CLI interaction, rule-file I/O, cross-platform behavior unobserved. The macOS realpath-casing premise needs hardware confirmation. --yolo behavior verified against code (denial check precedes mode checks, matching the claim) but not executed.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
评审结论来自自动化审查流程;发布者没有读这份 diff,核的是当前 head 有没有漂移、以及 exact-head 的门禁状态。当前 head 是 93d8d22,未关闭。一条 P1:同一条命令走 Bash 放行走终端拦住,主入口反而弱;一条 P2:macOS 大小写不一致致规则失效;另三条小的提醒。安全类功能改动,合不合、P2 拦不拦由人类定。
Related issue
This PR implements apache/maka#3351, Add basic permission configuration methods.
The issue asks for a basic way to configure command and filesystem permission rules, including examples such as denying git commit *, git push *, /etc/wsl.conf, and /mnt/**.
Fixes #3351
What changed
Design scope and boundaries
This PR intentionally implements a small native deny-rule model for the basic use case described in #3351.
These boundaries keep the change focused on the basic command and filesystem permission configuration requested by the issue. Broader policy categories or OpenCode-compatible configuration can be designed separately if needed.
Validation
Passed:
npm test was also attempted, but did not complete cleanly in this Windows environment. The observed failures were in existing platform/resource-sensitive tests involving symlink privileges, SQLite file locking, PTY/Shell behavior, and MCP/Eval external resources; the affected permission suites above pass independently.
AI use
Tool(s) and scope: Codex authored the implementation, tests, and this description. The affected commits include the required Generated-by: Codex trailer.
Checklist
Does this PR entail a change in behavior?