read/grep/find/glob bypass the permission filter and can't be path-confined #189

New issue

Closed

opened 2026-11-02 13:53:33 +01:00 by omp-deck · 0 comments

omp-deck commented

2026-11-02 13:53:33 +01:00

Collaborator

Problem

The SDK sends only bash, edit, delete and move through ClientBridge.requestPermission. PermissionFilter.makeClientBridge hooks into that path, and deck custom tools get their own execute-wrapper. All other built-ins (read, grep, find, glob, write, eval, ast_edit, task, browser, lsp, …) never reach PermissionFilter. As a result, user permission rules cannot gate them in any session, and trigger policies cannot confine them to the checkout. This is why the read access added in #49 was accepted as unconfined. A second problem: the rule matcher's default parameter map (PARAM_FIELD_MAP) uses stale tool names (search, find: paths), so path rules on these tools don't match reliably.

The SDK already has a general hook for this. Extension tool_call handlers run for every wrapped tool, including nested xd:// dispatches (node_modules/@oh-my-pi/pi-coding-agent/src/extensibility/extensions/wrapper.ts:230-304). A { block: true, reason } result fails the call. idea-mode-guard.ts already relies on this. Two constraints apply:

Sessions with restrictToolNames: true (all trigger sessions) ignore options.extensions (sdk.ts:2593-2625) and bind only preloadedPreparedExtensions (sdk.ts:2655-2666).
tool_call handlers run under a fail-closed timeout (extensionHandlers.toolCallTimeoutMs, default 30 000 ms). It pauses only during ctx.ui.* dialogs, so a deck permission prompt that waits longer than 30 s would be auto-denied.

Decided scope:

Gate every tool that is not already gated.
Make rules match each path of a multi-path call.
Confine read/grep/find/glob to the checkout in conversation, pull-request and CI-watchdog trigger sessions. The implementer stays unconfined, since it has bash.
In confined sessions, allow http(s)://, local:// and artifact://, and deny every other URI scheme.
Raise the handler timeout.
Leave deck custom tools on their existing wrapper.

Plan

Context

Send every tool call the SDK does not already route to PermissionFilter through it, using a deck tool_call extension that is active in both normal and trigger sessions. Fix path matching for path-taking tools. Use the new reach to confine read tools to the checkout in the no-bash trigger session kinds.

Approach

Expose the filter check and fix path matching (apps/server/src/bridge/permission-filter.ts).
- Remove private from evaluatePermission (signature unchanged: (toolCallId, toolName, params, signal?) => Promise<"allow" | { deny: string }>).
- Replace PARAM_FIELD_MAP with: bash: "command", read: "path", write: "path", edit: "path", grep: "path", find: "path", glob: "path", lsp: "file". Drop search.
- Add const MULTI_PATH_TOOLS = new Set(["grep", "find", "glob"]). Change resolveParamValue to return string[]. When paramField === null, the tool is in MULTI_PATH_TOOLS, and the value is a string, return value.split(";").map(s => s.trim()).filter(Boolean); if that list is empty, return ["."]. If the default field is missing from params for one of these three tools, also return ["."], because the tool searches cwd. Otherwise return a one-element array, keeping the current array-first / stringify behaviour.
- In matchRule, evaluate paramPattern per value. A rule with action === "allow" matches only when every value matches. deny/ask rules match when any value matches. This prevents src;/etc from passing an allow rule on ^src.
- Update the header comment (lines 15-19) to say that the remaining built-ins are gated through the permissionGate extension (step 2).
Add the gate extension: new file apps/server/src/bridge/permission-gate.ts. It follows the shape of idea-mode-guard.ts; no equivalent exists.
```
/** Tools PermissionFilter already sees via makeClientBridge(). */
export const CLIENT_BRIDGE_TOOLS: ReadonlySet<string> = new Set(["bash", "edit", "delete", "move"]);
export function permissionGate(
  filterRef: { current: PermissionFilter | null },
  skip: ReadonlySet<string>,
): ExtensionFactory
```
- The handler is pi.on("tool_call", async (event) => …). Return undefined when CLIENT_BRIDGE_TOOLS.has(event.toolName) or skip.has(event.toolName), because those tools are already gated and checking them again would double-prompt. Also return undefined when filterRef.current is null; this is the zero-prompt window before attach(), the same reasoning as wrapToolWithFilterRef.
- Otherwise call await filterRef.current.evaluatePermission(event.toolCallId, event.toolName, event.input) and map { deny } to { block: true, reason: deny }. Pass no signal. The runner already settles the gate on abort (raceHandlerWithTimeout), and turn abort/dispose call cancelAll(), which clears the pending prompt.
- Write a doc comment explaining why the extension exists: the SDK routes only the ClientBridge tools to requestPermission.
Install the gate in every session (apps/server/src/bridge/in-process.ts, createSession ~L285-318 and resumeSession ~L416-436).
- Build the custom-tool array as a local customTools (the same [...buildDeckTools, ...buildGitTools, ...forgejoTools, ...portainerTools]) and compute const gate = permissionGate(filterRef, new Set(customTools.map(t => t.name))). Map customTools through wrapToolWithFilterRef(wrapRebaseGate(…)) as today.
- Normal sessions: prepend gate to extensions ([gate, rebaseStopGuard(cwd), ideaModeGuard(ideaBridgeRef)]).
- Trigger sessions: give restrictedToolOptions a third parameter gate: ExtensionFactory. In the trigger branch, add preloadedPreparedExtensions: [{ path: "deck:permission-gate", resolvedPath: "deck:permission-gate", factory: gate, error: null }]. Update both callers. Update the restrictedToolOptions doc comment: the tool list is the surface, and the gate adds per-call checks.
- Update the comments at L933-935: deck custom tools are wrapped, ClientBridge tools go through the bridge, and everything else goes through permissionGate.
Raise the handler timeout (apps/server/src/model-config.ts L530). Add "extensionHandlers.toolCallTimeoutMs": 86_400_000 to DECK_SESSION_OVERRIDES, with a comment: a tool_call handler that awaits a deck permission prompt must not fail closed after the SDK's 30 s default; turn abort still cancels it.
Confine read tools in no-bash trigger sessions.
- New file apps/server/src/bridge/path-confinement.ts:
```
export const READ_TOOLS: ReadonlySet<string> = new Set(["read", "grep", "find", "glob"]);
/** Why `toolName(params)` reaches outside `root`, or null when every target is inside it or an allowed URI. */
export function confinementDenial(toolName: string, params: unknown, root: string): string | null
```
  - Targets: read → [params.path]. grep/find/glob → params.path split on ;, trimmed, empties dropped. A missing or empty path becomes ["."]. A non-string path is denied with "path must be a string".
  - For each target:
    - /^https?:\/\//i → allowed.
    - Starts with local:// or artifact:// → allowed.
    - Matches /^[a-z][a-z0-9+.-]*:\/\//i → denied.
    - Otherwise it is a filesystem path. A leading ~ expands to os.homedir(); then abs = path.resolve(root, target). Build candidates from abs plus abs.slice(0, i) for every index i of : in abs past the root portion; these cover file:50-100 selectors and archive.zip:member. Each candidate that exists (fs.existsSync) must have a fs.realpathSync inside realRoot. If no candidate exists, walk path.dirname up from abs to the nearest existing ancestor; its realpath must be inside realRoot. This covers glob patterns and not-yet-existing files.
    - realRoot = fs.realpathSync(root). "Inside" means p === realRoot || p.startsWith(realRoot + path.sep). Realpath is required because checkout content is untrusted, and a committed symlink could point outside.
  - Deny message (exact): `${target} is outside this session's checkout; only files in the checkout, http(s) URLs, local:// and artifact:// can be read`.
- apps/server/src/bridge/session-policy.ts:
  - Add a cwd: string parameter to SessionPolicy.decide (after sessionId) and to decidePolicy, and forward it in all four decidePolicy switch arms.
  - In forgejoConversationPolicy, forgejoPullRequestPolicy and forgejoCiWatchdogPolicy, start decide with: if (READ_TOOLS.has(toolName)) { const d = confinementDenial(toolName, params, cwd); if (d) return { deny: d }; }. Do not change forgejoImplementerPolicy beyond the new unused parameter (_cwd).
  - Rewrite the header comment (L10-20) and the comment above CONVERSATION_TOOL_NAMES (L134-138): all tools now reach PermissionFilter through the bridge, the custom-tool wrapper, or permissionGate; toolNames remains the surface, and read tools are confined to the checkout in the no-bash kinds.
- permission-filter.ts: store this.cwd = opts.cwd in the constructor and pass it as the new last argument to decidePolicy in evaluatePermission. Trigger sessions are constructed with cwd = the checkout (in-process.ts L932).
- Fix every other decidePolicy( / .decide( caller found with grep -rn "decidePolicy(\|\.decide(" apps/server/src, including tests, by passing a cwd.

Critical files & anchors

apps/server/src/bridge/idea-mode-guard.ts — the pattern for the tool_call extension.
apps/server/src/bridge/in-process.ts — restrictedToolOptions (L150), createSession/resumeSession createAgentSession calls, attach L931-939.
apps/server/src/bridge/permission-filter.ts — PARAM_FIELD_MAP/resolveParamValue/matchRule (L99-151), evaluatePermission (L213).
apps/server/src/bridge/session-policy.ts — SessionPolicy.decide, the three no-bash policies, decidePolicy (L566).
node_modules/@oh-my-pi/pi-coding-agent/src/sdk.ts:2593-2666 — why trigger sessions need preloadedPreparedExtensions.

Verification

Run from apps/server: bun test src/bridge and bun run typecheck. Add tests following permission-filter.test.ts conventions (bootDb(), createPermission):

permission-gate.test.ts: call permissionGate(ref, new Set(["deck_x"])) with a fake pi whose on captures the handler.
- With a global deny rule toolPattern: "^read$", paramPattern: "secret", the handler returns { block: true, reason: "Tool call denied for read: …" } for { path: "secret.txt" } and undefined for { path: "ok.txt" }.
- It returns undefined for bash and deck_x even under a deny-all rule toolPattern: ".*".
- An ask rule on glob emits a tool_permission_prompt frame, and respond(id, "approve") resolves the handler to undefined.
permission-filter.test.ts: a deny rule with paramPattern: "^/etc" on grep blocks { pattern: "x", path: "src;/etc" }. An allow rule ^src on grep combined with an ask-by-default setup does not allow src;/etc.
path-confinement.test.ts with a tmp checkout:
- Allowed: ".", missing path, "src/a.ts:10-20", "src/**/*.ts", "local://x.md", "artifact://3", "https://example.com".
- Denied: "../x", "/etc/passwd", "/etc/passwd:1", "skill://grill-me", "src;/etc" (on grep).
- Symlink cases: link -> /etc committed in the checkout is denied for both "link/passwd" and "link:1".
Manual check: in a normal deck session, create a deny rule (Settings → permissions) toolPattern ^read$, paramPattern \.env. Ask the agent to read .env; the call fails with Tool call denied for read: …. Add an ask rule on glob and confirm the prompt appears and still works after waiting more than 30 s.

Assumptions & contingencies

bindPreparedExtensions calls a non-null factory without importing path (loader.ts:491-512 → bindExtension; unverified — confirm first). If it tries to import the path, use path: "<inline-0>", resolvedPath: "<inline-0>" instead.
If Settings.isolated rejects or ignores extensionHandlers.toolCallTimeoutMs (check by reading cfgExtensionHandlersToolCallTimeoutMs.get(settings) in a throwaway script), set the key with settings.set(...) after reloadForCwd in buildDeckSessionSettings, in both the main and the fallback path.

## Problem The SDK sends only `bash`, `edit`, `delete` and `move` through `ClientBridge.requestPermission`. `PermissionFilter.makeClientBridge` hooks into that path, and deck custom tools get their own execute-wrapper. All other built-ins (`read`, `grep`, `find`, `glob`, `write`, `eval`, `ast_edit`, `task`, `browser`, `lsp`, …) never reach `PermissionFilter`. As a result, user permission rules cannot gate them in any session, and trigger policies cannot confine them to the checkout. This is why the read access added in #49 was accepted as unconfined. A second problem: the rule matcher's default parameter map (`PARAM_FIELD_MAP`) uses stale tool names (`search`, `find: paths`), so path rules on these tools don't match reliably. The SDK already has a general hook for this. Extension `tool_call` handlers run for every wrapped tool, including nested `xd://` dispatches (`node_modules/@oh-my-pi/pi-coding-agent/src/extensibility/extensions/wrapper.ts:230-304`). A `{ block: true, reason }` result fails the call. `idea-mode-guard.ts` already relies on this. Two constraints apply: - Sessions with `restrictToolNames: true` (all trigger sessions) ignore `options.extensions` (`sdk.ts:2593-2625`) and bind only `preloadedPreparedExtensions` (`sdk.ts:2655-2666`). - `tool_call` handlers run under a fail-closed timeout (`extensionHandlers.toolCallTimeoutMs`, default 30 000 ms). It pauses only during `ctx.ui.*` dialogs, so a deck permission prompt that waits longer than 30 s would be auto-denied. Decided scope: - Gate every tool that is not already gated. - Make rules match each path of a multi-path call. - Confine `read`/`grep`/`find`/`glob` to the checkout in conversation, pull-request and CI-watchdog trigger sessions. The implementer stays unconfined, since it has `bash`. - In confined sessions, allow `http(s)://`, `local://` and `artifact://`, and deny every other URI scheme. - Raise the handler timeout. - Leave deck custom tools on their existing wrapper. ## Plan ### Context Send every tool call the SDK does not already route to `PermissionFilter` through it, using a deck `tool_call` extension that is active in both normal and trigger sessions. Fix path matching for path-taking tools. Use the new reach to confine read tools to the checkout in the no-bash trigger session kinds. ### Approach 1. **Expose the filter check and fix path matching** (`apps/server/src/bridge/permission-filter.ts`). - Remove `private` from `evaluatePermission` (signature unchanged: `(toolCallId, toolName, params, signal?) => Promise<"allow" | { deny: string }>`). - Replace `PARAM_FIELD_MAP` with: `bash: "command"`, `read: "path"`, `write: "path"`, `edit: "path"`, `grep: "path"`, `find: "path"`, `glob: "path"`, `lsp: "file"`. Drop `search`. - Add `const MULTI_PATH_TOOLS = new Set(["grep", "find", "glob"])`. Change `resolveParamValue` to return `string[]`. When `paramField === null`, the tool is in `MULTI_PATH_TOOLS`, and the value is a string, return `value.split(";").map(s => s.trim()).filter(Boolean)`; if that list is empty, return `["."]`. If the default field is missing from params for one of these three tools, also return `["."]`, because the tool searches cwd. Otherwise return a one-element array, keeping the current array-first / stringify behaviour. - In `matchRule`, evaluate `paramPattern` per value. A rule with `action === "allow"` matches only when **every** value matches. `deny`/`ask` rules match when **any** value matches. This prevents `src;/etc` from passing an allow rule on `^src`. - Update the header comment (lines 15-19) to say that the remaining built-ins are gated through the `permissionGate` extension (step 2). 2. **Add the gate extension**: new file `apps/server/src/bridge/permission-gate.ts`. It follows the shape of `idea-mode-guard.ts`; no equivalent exists. ```ts /** Tools PermissionFilter already sees via makeClientBridge(). */ export const CLIENT_BRIDGE_TOOLS: ReadonlySet<string> = new Set(["bash", "edit", "delete", "move"]); export function permissionGate( filterRef: { current: PermissionFilter | null }, skip: ReadonlySet<string>, ): ExtensionFactory ``` - The handler is `pi.on("tool_call", async (event) => …)`. Return `undefined` when `CLIENT_BRIDGE_TOOLS.has(event.toolName)` or `skip.has(event.toolName)`, because those tools are already gated and checking them again would double-prompt. Also return `undefined` when `filterRef.current` is null; this is the zero-prompt window before `attach()`, the same reasoning as `wrapToolWithFilterRef`. - Otherwise call `await filterRef.current.evaluatePermission(event.toolCallId, event.toolName, event.input)` and map `{ deny }` to `{ block: true, reason: deny }`. Pass no signal. The runner already settles the gate on abort (`raceHandlerWithTimeout`), and turn abort/dispose call `cancelAll()`, which clears the pending prompt. - Write a doc comment explaining why the extension exists: the SDK routes only the ClientBridge tools to `requestPermission`. 3. **Install the gate in every session** (`apps/server/src/bridge/in-process.ts`, `createSession` ~L285-318 and `resumeSession` ~L416-436). - Build the custom-tool array as a local `customTools` (the same `[...buildDeckTools, ...buildGitTools, ...forgejoTools, ...portainerTools]`) and compute `const gate = permissionGate(filterRef, new Set(customTools.map(t => t.name)))`. Map `customTools` through `wrapToolWithFilterRef(wrapRebaseGate(…))` as today. - Normal sessions: prepend `gate` to `extensions` (`[gate, rebaseStopGuard(cwd), ideaModeGuard(ideaBridgeRef)]`). - Trigger sessions: give `restrictedToolOptions` a third parameter `gate: ExtensionFactory`. In the trigger branch, add `preloadedPreparedExtensions: [{ path: "deck:permission-gate", resolvedPath: "deck:permission-gate", factory: gate, error: null }]`. Update both callers. Update the `restrictedToolOptions` doc comment: the tool list is the surface, and the gate adds per-call checks. - Update the comments at L933-935: deck custom tools are wrapped, ClientBridge tools go through the bridge, and everything else goes through `permissionGate`. 4. **Raise the handler timeout** (`apps/server/src/model-config.ts` L530). Add `"extensionHandlers.toolCallTimeoutMs": 86_400_000` to `DECK_SESSION_OVERRIDES`, with a comment: a `tool_call` handler that awaits a deck permission prompt must not fail closed after the SDK's 30 s default; turn abort still cancels it. 5. **Confine read tools in no-bash trigger sessions.** - New file `apps/server/src/bridge/path-confinement.ts`: ```ts export const READ_TOOLS: ReadonlySet<string> = new Set(["read", "grep", "find", "glob"]); /** Why `toolName(params)` reaches outside `root`, or null when every target is inside it or an allowed URI. */ export function confinementDenial(toolName: string, params: unknown, root: string): string | null ``` - Targets: `read` → `[params.path]`. `grep`/`find`/`glob` → `params.path` split on `;`, trimmed, empties dropped. A missing or empty path becomes `["."]`. A non-string `path` is denied with `"path must be a string"`. - For each target: - `/^https?:\/\//i` → allowed. - Starts with `local://` or `artifact://` → allowed. - Matches `/^[a-z][a-z0-9+.-]*:\/\//i` → denied. - Otherwise it is a filesystem path. A leading `~` expands to `os.homedir()`; then `abs = path.resolve(root, target)`. Build candidates from `abs` plus `abs.slice(0, i)` for every index `i` of `:` in `abs` past the root portion; these cover `file:50-100` selectors and `archive.zip:member`. Each candidate that exists (`fs.existsSync`) must have a `fs.realpathSync` inside `realRoot`. If no candidate exists, walk `path.dirname` up from `abs` to the nearest existing ancestor; its realpath must be inside `realRoot`. This covers glob patterns and not-yet-existing files. - `realRoot = fs.realpathSync(root)`. "Inside" means `p === realRoot || p.startsWith(realRoot + path.sep)`. Realpath is required because checkout content is untrusted, and a committed symlink could point outside. - Deny message (exact): `` `${target} is outside this session's checkout; only files in the checkout, http(s) URLs, local:// and artifact:// can be read` ``. - `apps/server/src/bridge/session-policy.ts`: - Add a `cwd: string` parameter to `SessionPolicy.decide` (after `sessionId`) and to `decidePolicy`, and forward it in all four `decidePolicy` switch arms. - In `forgejoConversationPolicy`, `forgejoPullRequestPolicy` and `forgejoCiWatchdogPolicy`, start `decide` with: `if (READ_TOOLS.has(toolName)) { const d = confinementDenial(toolName, params, cwd); if (d) return { deny: d }; }`. Do not change `forgejoImplementerPolicy` beyond the new unused parameter (`_cwd`). - Rewrite the header comment (L10-20) and the comment above `CONVERSATION_TOOL_NAMES` (L134-138): all tools now reach `PermissionFilter` through the bridge, the custom-tool wrapper, or `permissionGate`; `toolNames` remains the surface, and read tools are confined to the checkout in the no-bash kinds. - `permission-filter.ts`: store `this.cwd = opts.cwd` in the constructor and pass it as the new last argument to `decidePolicy` in `evaluatePermission`. Trigger sessions are constructed with `cwd` = the checkout (`in-process.ts` L932). - Fix every other `decidePolicy(` / `.decide(` caller found with `grep -rn "decidePolicy(\|\.decide(" apps/server/src`, including tests, by passing a cwd. ### Critical files & anchors - `apps/server/src/bridge/idea-mode-guard.ts` — the pattern for the `tool_call` extension. - `apps/server/src/bridge/in-process.ts` — `restrictedToolOptions` (L150), `createSession`/`resumeSession` `createAgentSession` calls, `attach` L931-939. - `apps/server/src/bridge/permission-filter.ts` — `PARAM_FIELD_MAP`/`resolveParamValue`/`matchRule` (L99-151), `evaluatePermission` (L213). - `apps/server/src/bridge/session-policy.ts` — `SessionPolicy.decide`, the three no-bash policies, `decidePolicy` (L566). - `node_modules/@oh-my-pi/pi-coding-agent/src/sdk.ts:2593-2666` — why trigger sessions need `preloadedPreparedExtensions`. ### Verification Run from `apps/server`: `bun test src/bridge` and `bun run typecheck`. Add tests following `permission-filter.test.ts` conventions (`bootDb()`, `createPermission`): - `permission-gate.test.ts`: call `permissionGate(ref, new Set(["deck_x"]))` with a fake `pi` whose `on` captures the handler. - With a global deny rule `toolPattern: "^read$"`, `paramPattern: "secret"`, the handler returns `{ block: true, reason: "Tool call denied for read: …" }` for `{ path: "secret.txt" }` and `undefined` for `{ path: "ok.txt" }`. - It returns `undefined` for `bash` and `deck_x` even under a deny-all rule `toolPattern: ".*"`. - An ask rule on `glob` emits a `tool_permission_prompt` frame, and `respond(id, "approve")` resolves the handler to `undefined`. - `permission-filter.test.ts`: a deny rule with `paramPattern: "^/etc"` on `grep` blocks `{ pattern: "x", path: "src;/etc" }`. An allow rule `^src` on `grep` combined with an ask-by-default setup does not allow `src;/etc`. - `path-confinement.test.ts` with a tmp checkout: - Allowed: `"."`, missing path, `"src/a.ts:10-20"`, `"src/**/*.ts"`, `"local://x.md"`, `"artifact://3"`, `"https://example.com"`. - Denied: `"../x"`, `"/etc/passwd"`, `"/etc/passwd:1"`, `"skill://grill-me"`, `"src;/etc"` (on grep). - Symlink cases: `link -> /etc` committed in the checkout is denied for both `"link/passwd"` and `"link:1"`. - Manual check: in a normal deck session, create a deny rule (Settings → permissions) `toolPattern ^read$`, `paramPattern \.env`. Ask the agent to read `.env`; the call fails with `Tool call denied for read: …`. Add an ask rule on `glob` and confirm the prompt appears and still works after waiting more than 30 s. ### Assumptions & contingencies - `bindPreparedExtensions` calls a non-null `factory` without importing `path` (`loader.ts:491-512` → `bindExtension`; unverified — confirm first). If it tries to import the path, use `path: "<inline-0>"`, `resolvedPath: "<inline-0>"` instead. - If `Settings.isolated` rejects or ignores `extensionHandlers.toolCallTimeoutMs` (check by reading `cfgExtensionHandlersToolCallTimeoutMs.get(settings)` in a throwaway script), set the key with `settings.set(...)` after `reloadForCwd` in `buildDeckSessionSettings`, in both the main and the fallback path.