feat(permissions): add tool permission policy system - #141
Conversation
fa8c168 to
0c74ade
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a structured, rule-based tool permission policy system across the Agentrail ecosystem. It adds a checkPermissions hook to the tool execution lifecycle, a new permission_request event for interactive approvals, and a comprehensive rule engine for evaluating access to file and shell operations. Feedback highlights several critical issues in the permission engine, including a wildcard implementation that breaks the Bash DSL for paths containing separators, a policy evaluation order that prevents strict allowlist configurations, and potential cross-platform inconsistencies in path resolution. Additionally, improvements were suggested for regex escaping and shell command normalization to handle various whitespace characters.
Introduces a structured rule-based permission layer between LLM tool calls and their execution. Resolves #67. Core changes: - New `PermissionDecision` type and `checkPermissions?` hook on `RuntimeTool` - New `permission_request` RuntimeEvent (persisted to trace log) - `ToolBuilder.checkPermissions()` fluent method - `tool-executor` calls `checkPermissions` after interceptor, before validate Capabilities: - New `packages/capabilities/src/permissions/` module with types, DSL parser, rule engine, path-safety, and shell-safety utilities - `createBashTool`, `createReadTool`, `createWriteTool`, `createEditTool` factories with optional `rootDir` and `policy` parameters (singletons kept for backward compat) - `createSandboxedBash` accepts optional `policy` - `CapabilityBuildContext.permissionPolicy` propagated through the stack App layer: - `AgentrailProfileContext.permissionPolicy` forwarded by `defineProfile` - `createAgentApp`, `createStreamRoute`, `createChatRoute` accept optional `permissionPolicy` - `AgentrailConfig` YAML gains optional `permissions` block - Docs updated: events, profile-contract, create-agent-app references Tests: permissions unit tests (rule-engine, rule-parser, path-safety), tool-executor checkPermissions paths, app config parsing Signed-off-by: yai-dev <sunzhenyucn@gmail.com>
1. Sandboxed Read/Write/Edit now accept a ToolPermissionPolicy and wire up
checkPermissions, matching the existing sandboxed-bash behavior. Both
filesystem() and buildDefaultCapabilityTools() forward ctx.permissionPolicy
to all four sandboxed file tools.
2. Bash DSL colon-convention was never matched because raw commands use spaces
("git status") while patterns use colons ("git:*"). Add normalizeBashCommand
which converts "git status" → "git:status" before evaluatePolicy is called.
Both createBashTool and createSandboxedBash now call normalizeBashCommand.
Export normalizeBashCommand from @agentrail/capabilities.
3. Config-to-runtime conversion was missing. Add configPermissionsToPolicy
(AgentrailPermissionsConfig → ToolPermissionPolicy) that calls parseRules
on each string array; export it from @agentrail/app together with the
AgentrailPermissionsConfig type. Update create-agent-app.md with the
correct DSL semantics, normalization note, and configPermissionsToPolicy
usage example.
Signed-off-by: yai-dev <sunzhenyucn@gmail.com>
- Update permissionPolicy JSDoc in AgentrailProfileContext (host/types.ts), CapabilityBuildContext (capabilities/src/types.ts), profile-contract.md, and shared-types.ts: all four now state that the policy covers both sandboxed (Bash, Read, Write, Edit) and non-sandboxed file/shell tools, removing the previous inaccurate "non-sandboxed + sandboxed Bash only" wording. - Add two tests to define-profile.test.ts that guard the profileCtx.permissionPolicy → CapabilityBuildContext.permissionPolicy hop: one asserts the policy reference is forwarded correctly, the other asserts absence propagates as undefined. Signed-off-by: yai-dev <sunzhenyucn@gmail.com>
The pattern /\brm\s+(-[a-z]*f[a-z]*\s+)*\//i used a quantified group with nested * quantifiers on [a-z]*, causing exponential backtracking on inputs like "rm -ff -ff -ff ..." (reported by CodeQL). Replace with three linear, non-repeating patterns that cover the same attack surface without ambiguity: - Combined flag block containing 'f': rm -rf /, rm -rrf /, rm -Rf / - Separate flags with force second: rm -r -f / - Separate flags with force first: rm -f -r / Add isDangerousCommand unit tests covering all variants including safe commands that must not be blocked (rm -rf ./build). Signed-off-by: yai-dev <sunzhenyucn@gmail.com>
1. contentMode for Bash DSL wildcard matching
Add ContentMatchMode ("path" | "command") param to matchPattern and
evaluatePolicy. In "command" mode * uses .* (matches across /) so
patterns like git:*/index.ts work for multi-segment paths. Bash tools
now explicitly pass "command" to evaluatePolicy. ContentMatchMode is
exported from @agentrail/capabilities. Path-mode semantics unchanged;
the change is backward-compatible.
2. "strict" PermissionMode (deny-by-default)
Add "strict" to PermissionMode, AgentrailPermissionsConfig.mode type
union, and VALID_MODES runtime validator. evaluatePolicy returns "deny"
as the final default when mode === "strict". Documented in
create-agent-app.md with a permission mode table and allowlist example.
3. Missing "?" in regex escape list
Add "?" to the character class in matchPattern's special-char escaper
so literal question marks in patterns are not treated as quantifiers.
4. Cross-platform ancestor resolution in path-safety.ts
Replace the POSIX-specific split/join loop with a path.dirname walk,
consistent with Node's cross-platform path semantics on Windows and
POSIX alike.
5. normalizeBashCommand whitespace handling
Replace indexOf(" ") with /^(\S+)\s+/ replace so tabs and other
whitespace between the verb and its arguments are handled correctly.
Tests: 17 new test cases across rule-engine, path-safety, and config
covering contentMode behavior, strict mode defaults and overrides,
ancestor-walk regression, tab normalization, and config parser.
Signed-off-by: yai-dev <sunzhenyucn@gmail.com>
1. matchPattern + evaluatePolicy JSDoc: the previous @PARAM contentMode examples claimed "pass command so git:* matches git:add src/main.ts", but git:* matches that content in path mode too because the regex is prefix-anchored (no trailing $). Replace with an accurate example: "git:*/index.ts" where path mode fails on multi-segment paths and command mode succeeds. Add explicit note that suffix-only wildcards like "git:*" are unaffected by contentMode. 2. create-agent-app.md strict mode example: the Bash allowlist code comments implied command confinement ("permit all git commands") which is misleading — prefix-anchored patterns cannot prevent chained shell commands like "git:status; curl evil.com". Replace with neutral comments ("prefix-match: any content starting with 'git:'") and add a Bash rule caveat block explaining prefix anchoring and its limits. Signed-off-by: yai-dev <sunzhenyucn@gmail.com>
…nd UI - Add permissionPolicy field to PlaygroundServerConfig and populate it from configPermissionsToPolicy in getPlaygroundServerConfig - Pass config.permissionPolicy to createChatRoute and createStreamRoute in playground-server examples - Add permission_request to StreamEvent union in playground-ui api.ts - Handle permission_request stream events in App.tsx: show a dismissible banner and clear it at all three reset points (finally block, startNewSession, switchSession) to match waitingQuestion lifecycle - Add permission_request to TRACE_EVENT_TYPES and waits filter so events are stored in the trace and visible in the DAG view - Add regression tests: getPlaygroundServerConfig permissionPolicy wiring in packages/app/test/config.test.ts; permissionBlocked state machine in examples/playground-ui/test/permissionBanner.test.ts Signed-off-by: yai-dev <sunzhenyucn@gmail.com>
…s into CI - Move setPermissionBlocked(null) into the existing session.end handler in App.tsx so the clear actually executes; remove the now-unnecessary session.end arm from the turn.complete branch - Add clarifying comment to permissionBannerReducer explaining why it covers both events while App.tsx handles them in separate branches - Add test script to playground-ui package.json (tsx --test) - Expand root pnpm test to include examples via --if-present so the permissionBanner.test.ts runs in CI without breaking examples that have no test script Signed-off-by: yai-dev <sunzhenyucn@gmail.com>
The test script added in the previous commit relies on tsx; declare it explicitly instead of depending on workspace hoisting from sibling examples. Mirrors the pattern in playground-server and deep-research. Signed-off-by: yai-dev <sunzhenyucn@gmail.com>
…-handle mechanism
Replace the immediate deny-on-ask behavior with a full suspend-and-resume
loop that mirrors the existing AskUserQuestion wait-handle pattern.
Core layer
- Add PermissionApprovalHandler interface to tool.types.ts and export
from packages/core
- Add permissionApprovalHandler? to AgentRunOptions and AgentLoopConfig
- Thread through agent-impl.ts stream() -> agentLoop -> runLoop ->
executeToolCalls
- In tool-executor: when decision === "ask" and a handler is present,
emit permission_request then await requestApproval(); emit
permission_resolved with decision and either continue or rejectToolCall
- Add permission_resolved to RuntimeEvent union in result.types.ts
- Without a handler, "ask" continues to behave as "deny" (backward compat)
App layer
- Add createPermissionApprovalHandler? factory to AgentrailStreamRouteOptions
- Add permissionApprovalHandler? to DrainAgentStreamOptions and forward
through agent.stream()
Playground server
- Upgrade WaitHandleRegistry with a permission kind (registerPermission /
respondPermission) alongside the existing question kind
- Update POST /api/sessions/:sessionId/respond to accept structured
{ kind: "question", answer } or { kind: "permission", decision } payloads
- Wire createPermissionApprovalHandler in stream.ts using the upgraded registry
Playground UI
- Add respondToPermission to api.ts; update StreamEvent union with
permission_resolved
- Replace permissionBlocked (display-only banner) with interactive
pendingPermission state; set on permission_request, clear on
permission_resolved / turn.complete / session.end / reset paths
- Add PermissionApprovalPrompt.tsx component with Approve / Reject buttons
- Update TRACE_EVENT_TYPES and TraceDAGView waits filter for permission_resolved
- Replace old CSS with .permission-approval-* styles
Signed-off-by: yai-dev <sunzhenyucn@gmail.com>
ba1588f to
8e903c0
Compare
- Wire createPermissionApprovalHandler unconditionally in playground-server so any "ask" decision (regardless of policy source) triggers the interactive suspend/resume flow instead of falling back to immediate deny. - Update JSDoc on PermissionDecision and the permission_request RuntimeEvent to reflect the actual suspend-and-resume semantics introduced with PermissionApprovalHandler; remove the stale "emit then deny" description. Signed-off-by: yai-dev <sunzhenyucn@gmail.com>
- Add useEffect keyed on toolCallId to reset submitting/error state when a new permission request arrives, preventing buttons from staying disabled and showing "…" across consecutive approval prompts. - Add pending/error state so onDismiss fires only after a successful POST; failed requests now surface an error message and restore the buttons. - Change Approve button text from white to black for better contrast on bright accent backgrounds. - Add .permission-approval-error CSS rule for the failure message. Signed-off-by: yai-dev <sunzhenyucn@gmail.com>
6a569de to
38ef32e
Compare
Summary
Implements Issue #67 — a rule-based tool permission policy system that intercepts tool calls before execution and supports
allow,deny, andaskdecisions.What's included
@agentrail/capabilities— newpermissions/module with:types.ts—ToolPermissionPolicy,PermissionRule,PermissionModerule-parser.ts— parses DSL strings like"Bash(git:*)"intoPermissionRule[]rule-engine.ts— evaluatesdeny → ask → allow → defaultpriority chainpath-safety.ts—workspaceAnchorvalidates paths againstrootDirusing nearest-ancestor traversalshell-safety.ts—isDangerousCommand,isReadOnlyCommand,normalizeBashCommand(converts"git status"→"git:status"sogit:*DSL patterns match correctly)checkPermissions@agentrail/core—PermissionDecisiontype +.checkPermissions()builder method onRuntimeTool;tool-executorenforces the hook after interceptors;RuntimeEvent.permission_requestemitted onask@agentrail/app—permissionPolicy?option oncreateAgentApp/createStreamRoute/createChatRoute;AgentrailPermissionsConfigparsed fromagentrail.yaml;configPermissionsToPolicy()exported for converting config → runtime policyBash DSL convention
Rules use a
verb:argsform —Bash(git:*)matches any command starting withgit. The tool normalises"git status"→"git:status"before pattern matching so glob patterns work as documented.Loading from
agentrail.yamlTest plan
pnpm --filter @agentrail/capabilities test -- permissions— rule-engine, path-safety, shell-safety (including Bash DSL colon-convention end-to-end)pnpm --filter @agentrail/core test -- tool-executor— deny/ask paths in executorpnpm --filter @agentrail/app test— config parsing,configPermissionsToPolicy, assembly-hop (profileCtx.permissionPolicy→CapabilityBuildContext.permissionPolicy)pnpm test— full suite green (240 tests across all packages)pnpm typecheck— no errorsCloses #67