Skip to content

fix(runtime): enforce persistent permission denies (#3351) - #4849

Open
CxHsin wants to merge 5 commits into
apache:mainfrom
CxHsin:codex/permission-configuration
Open

fix(runtime): enforce persistent permission denies (#3351)#4849
CxHsin wants to merge 5 commits into
apache:mainfrom
CxHsin:codex/permission-configuration

Conversation

@CxHsin

@CxHsin CxHsin commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

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

  • Added Host-owned persistent deny rules for command glob patterns and filesystem paths.
  • Added maka permissions commands to list, add, and remove rules.
  • Added exact and subtree path matching with canonical path validation.
  • Enforced persistent denies before tool dispatch, including when --yolo is enabled.
  • Covered Bash, Read, Write, Edit, Glob, Grep, PTY input, and client-side ApplyPatch operations.
  • Prevented native OpenAI apply_patch from bypassing configured path denies.
  • Added Runtime Host persistence, protocol support, schema migration, Windows path handling, tests, and CLI documentation.
  • Compiled glob matchers are cached per immutable rule snapshot to avoid repeated compilation.
  • Existing backends read the current Host-owned rule snapshot before each dispatch, so a policy update applies to the next dispatch without rebuilding the backend.

Design scope and boundaries

This PR intentionally implements a small native deny-rule model for the basic use case described in #3351.

  • It supports deny rules only; it does not add general allow rules.
  • It does not attempt full compatibility with the OpenCode configuration format.
  • Operations that do not match a persistent rule continue to use the existing Session permission mode and sandbox behavior.
  • Command patterns use glob matching against the command string. The implementation does not provide a complete shell parser for aliases, command wrappers, substitutions, or every shell-specific execution form.
  • The path model uses canonical absolute paths with exact and subtree scopes. It is not a general policy language for network, browser, or arbitrary MCP capabilities.
  • Enforcement happens before dispatch. A tool already in progress is not cancelled or retroactively evaluated.
  • Rules are Host-owned and shared by sessions in the same Runtime Host.

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 run lint
  • npm run format:check
  • npm run build
  • npm run typecheck
  • npx knip --workspace apps/desktop
  • npx knip --workspace packages/ui
  • Core test suite: 827 passed
  • Runtime persistent-permission suite: 17 passed
  • Runtime Host policy coordinator suite: 18 passed, 1 skipped by the existing test condition
  • Runtime Host execution-host suite: 15 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

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex authored the implementation, tests, and this description. The affected commits include the required Generated-by: Codex trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — persistent deny rules now apply to the next dispatch made by an existing backend.
  • No

@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Sep 5, 2026
@CxHsin
CxHsin force-pushed the codex/permission-configuration branch from 00bc197 to 40c0297 Compare September 5, 2026 10:18

@Astro-Han Astro-Han left a comment

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.

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

  • globMatches recompiles 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 拦不拦,人类定。

@Astro-Han Astro-Han left a comment

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.

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 only a, -, z (verified: rm -rf /[a-z]* compiles to match rm -rf /-, not rm -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.
  • looksLikePath over-matches (any token containing . becomes a path candidate, each costing a real realpath per 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 拦不拦由人类定。

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

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add basic permission configuration methods

2 participants