fix(security): perm bypass in grep/glob/find/semantic; URL scheme case-insensitive; tool defaults match docs - #120
Merged
Merged
Conversation
…ve URL scheme; tool defaults match docs
Six verified security-critical findings from a code review.
## C1: grep / glob / find_files bypassed external_directory rules
Each tool called `check_perm(name, pattern)` but the actual
filesystem path being walked (`args.path`) was never permission-
checked. `check_perm_path` is what consults `ext_dir_rules`
+ Accept-mode working-dir gating; the bare `check_perm` skips
both. An LLM doing `grep("keyword", "/etc")` or
`find_files("*", "~/.ssh")` walked external dirs without
asking — even under restrictive rules.
Added `check_perm_path` after the existing `check_perm` call
in each of the three tools. The pattern matches `read.rs` /
`list_dir.rs` which already did this.
## C2: semantic tools didn't use path-aware perm check
`find_callees`, `get_symbol_body`, `list_symbols` all
accepted a `path` arg and passed it to `check_perm` — same
class of bypass as C1. Switched the three to `check_perm_path`.
The other two semantic tools (`find_callers`, `find_definition`)
operate over symbol names only and continue to use `check_perm`.
## C3: case-sensitive scheme check enabled SSRF bypass
`starts_with("http://")` only matched lowercase. URL schemes
are case-insensitive per RFC 3986, so `HTTP://169.254.169.254/`
slipped past both the scheme guard AND the SSRF host blocker
(which also did case-sensitive `strip_prefix`). Refactored to
a shared `has_http_scheme` helper using `eq_ignore_ascii_case`,
and rewrote the host-extraction to use case-insensitive length-
matching to strip the scheme.
## C6: tools.websearch / tools.webfetch default mismatched docs
CONFIG.md states "both `bool`, default `true`". Code had
`.unwrap_or(false)` so both tools were disabled by default,
contradicting the documented behavior. Flipped to `unwrap_or(true)`.
Explicit `false` in config still disables; absent or `true`
enables (websearch additionally requires the runtime EXA_API_KEY
check to pass, unchanged).
## C5: removed dead --tools / -t CLI flag
The `tools: Vec<String>` flag was defined in clap but never read
anywhere — pure dead code. Not documented in README or CONFIG.md.
Removed the clap arg entirely. Users invoking `-t bash` will
now get a clear clap error instead of silent acceptance.
## Tests
1 new regression: `scheme_matching_is_case_insensitive` —
verifies `HTTP://`, `HTTPS://`, mixed case all pass the
scheme check AND that SSRF defense still triggers for case-
variant schemes targeting metadata IPs.
813 all-features tests pass (was 812). All build profiles clean.
## Deferred to next PR (config + cleanup batch)
- C4: unregistered tool names default to Allow. Architectural —
needs a design decision (explicit tool whitelist vs default
allow). Will tackle separately.
- C7: missing didClose in LSP. Needs lifecycle design — when to
fire (on file delete? on session end?).
- H1: unbounded edit/apply_patch file reads. Trivial cap; in
next batch.
- H3: blocking std::fs in apply_patch + list_dir. In next batch.
- H4 / H5: missing PermissionConfig fields for MCP / 8 other
tools. Schema change; in next batch.
- H6: MCP connect errors only at tracing::warn. Tiny fix;
in next batch.
- H8: LSP config extensions field ignored. In next batch.
- H9 / H10: dead acp_servers / acp_host / acp_port config
fields. In next batch.
## Verified false positives
- H2 (empty bash): `quote_aware_split` already filters empty
segments. No bug.
- H7 (LSP spawn silent): the `broken` set + backoff IS the
user-feedback mechanism for repeated failures; logging at
warn is intentional non-fatal degradation.
- H11 (plugin dedup): summary IS emitted when a different error
arrives.
- H12 (token counter field): `estimate_tokens` on the
accumulated partial string is correct.
yogthos
added a commit
that referenced
this pull request
May 21, 2026
…ig (#121) Continues from PR #120 (security-critical). Six deferred items shipped. ## H1: edit + apply_patch unbounded file reads `edit.rs` and `apply_patch.rs` (apply_update) called `tokio::fs::read` / `std::fs::read_to_string` with no size guard. An LLM pointing them at a multi-GB log/binary would OOM the agent process. Added 100 MiB pre-flight metadata check in both. The cap is generous (every realistic source file fits) but rejects the pathological case fast with a clear error pointing the agent at `bash + sed/awk` for huge files. ## H3: apply_patch blocking std::fs in async All four `apply_*` helpers (create / update / delete / rename) used synchronous `std::fs` calls inside an async `call` method. On the tokio runtime with limited worker threads, slow I/O can starve the event loop. Switched each to `tokio::fs` and made them async. Tests updated from `#[test]` to `#[tokio::test]` (8 tests touched). ## H4 + H5: PermissionConfig fields for missing tools `PermissionConfig` only had fields for 11 tools (bash, read, write, edit, grep, find_files, list_dir, write_todo_list, apply_patch, lsp, question). 12 other tools the agent can call had no way to be individually gated — they always fell through to the global default action. Added 12 fields covering the missing surface: - Web: `webfetch`, `websearch` - Subagent / state: `task`, `memory`, `skill` - Semantic (tree-sitter): `list_symbols`, `get_symbol_body`, `find_definition`, `find_callers`, `find_callees` - MCP umbrella: `mcp_tool` — pattern rules match against the full `mcp_tool:<server>:<tool>` key, so `{"mcp_tool:filesystem:*": "deny"}` blocks every tool from a filesystem MCP server. `PermissionChecker::new` now loads rules for all 23 tool names. `is_path_tool_name` extended to mark the path-bearing semantic tools (`list_symbols`, `get_symbol_body`, `find_callees`) plus `grep`/`find_files`/`glob` whose new path-side perm check (PR #120) uses path-glob semantics. The `/allow` slash command's whitelist grew to match — typos on the new tool names still get caught. ## H6: MCP connect / list_tools errors visible to user `McpClientManager::connect_all` and `collect_tools` logged failures at `tracing::warn!` only. Default-log users (no `RUST_LOG` / `--verbose`) saw nothing — configured MCP servers just silently failed to register tools. Added an `eprintln!` alongside each warn so the failure surfaces to stderr unconditionally. Message names the server and tells the user the tools won't be available this session. ## H9 + H10: removed dead acp_host / acp_port config keys Both keys were parsed by serde but never consumed anywhere. ACP transport is stdio-only by design. Removed from `config::Config`. CONFIG.md already (PR #112) documents the removal; no further docs change needed. `acp_servers` kept — it has a CONFIG.md entry, holds future TCP / Unix-socket config, and isn't actively misleading users. ## CONFIG.md Rewrote the Permission tool-keys list from a single sentence into a categorized list covering all 23 tools + the `mcp_tool:<server>:<tool>` pattern syntax. ## Tests 8 `apply_patch` tests converted to `#[tokio::test]` (no new assertions — the conversion was mechanical so the existing coverage still applies). 813 all-features / 730 plugin / 604 default pass — same as PR #120 plus one new test there. ## Not in this PR (still deferred) - **C4**: unregistered-tool default-allow. This PR ADDS fields for the 12 missing tools, so the original "unregistered" set is now mostly covered. The remaining gap (future MCP tools, third-party plugins) is by design — `mcp_tool` is the umbrella key. - **C7**: LSP didClose. Needs lifecycle design (when to fire? on file delete? session end?). Will tackle as a focused PR. - **H8**: LSP `extensions` config field ignored. Real gap but bigger plumbing change; separate PR. ## Verified false positives (re-confirmed from earlier review) - H2 empty bash: `quote_aware_split` filters empty segments. - H7 LSP spawn silent: broken-set + backoff IS the feedback. - H11 plugin dedup: summary IS emitted. - H12 token counter: estimate_tokens on partial is correct. Co-authored-by: Yogthos <yogthos@gmail.com>
yogthos
added a commit
that referenced
this pull request
May 21, 2026
…122) Two LSP gaps from the audit, both real. ## C7: missing didClose → server-side state leak LSP servers retain parse trees + diagnostic caches keyed on every `textDocument/didOpen`. dirge never sent `textDocument/didClose`, so a long session editing dozens of files left rust-analyzer / pyright / etc. holding all that state for the lifetime of the server process. Added two methods on `LspClient`: - `notify_close(path)`: send didClose for one file, drop the associated FileState / push + pull diagnostics / last-push timestamp from local tracking. Returns Ok on never-opened files (a stray didClose is harmless per the spec). - `close_all()`: fan didClose across every tracked file. Used at session shutdown. `LspManager::close_all_files` fans across every attached client. Called from main.rs after the interactive loop returns. Best-effort: an already-dead server isn't re-contacted and individual notify_close failures are swallowed. The `lsp_manager` Arc is cloned before being moved into `ui::run_interactive` so the shutdown call site still has a handle. Two new regression tests pin the behavior: - `notify_close_emits_did_close_and_clears_state` — checks that subsequent `notify_open` after close emits a fresh didOpen (version 0) rather than a didChange. - `close_all_emits_did_close_for_every_open_file` — two open files → two didClose frames sent. ## H8: LspServerConfig.extensions ignored `LspServerConfig` carries an `extensions: Option<Vec<String>>` field, parsed by serde, but `LspManager` always used the hardcoded `builtin_servers()` registry. User config like `{ "lsp": { "rust": { "extensions": ["rs", "rlib"] } } }` was silently dropped. Two changes: 1. `ServerInfo.extensions` changed from `&'static [&'static str]` to `Vec<String>` so it can be mutated post-construction. `builtin_servers()` now produces owned vecs; this is a one-time cost paid at session startup. 2. New `apply_extension_overrides` function in `lsp::server` walks the user's per-server config and replaces builtin extension lists (or removes the server entirely if `disabled: true`). Lowercases + strips leading dots to match how `servers_for_extension` looks up by extension. 3. `LspManager::with_servers` constructor accepts the modified server list. The existing `new` delegates with the unmodified builtin list. 4. `main.rs::build_channels` applies overrides between `builtin_servers()` and `LspManager::with_servers(...)`. The `compile_lsp_commands` docstring previously called the extensions field a follow-up; updated to point at the new override path. A trait shim (`AsExtensionOverride`) keeps the `lsp::server` module from depending directly on `config::LspServerConfig` — keeps the dep graph clean and lets the tests use a stub. 4 new regression tests: - Extensions override replaces builtin list. - `disabled: true` removes server from the active set. - Unknown server id silently ignored (no panic, no spurious entry added). - User-supplied extensions are normalized (leading-dot strip + lowercase). ## Status 736 plugin / 610 default pass (was 730 / 604). All build profiles + fmt clean. This closes the last two real items in the original audit's deferred list. PR #120 → C1/C2/C3/C5/C6. PR #121 → H1/H3/H4/H5/ H6/H9/H10. This PR → C7/H8. All verified false positives (H2, H7, H11, H12) remain unchanged. Co-authored-by: Yogthos <yogthos@gmail.com>
yogthos
added a commit
that referenced
this pull request
May 21, 2026
…WEBFETCH_ENABLED symmetry (#123) ## Steering the model toward the question tool opencode's `question.txt` description gets the model to actually reach for the tool by enumerating concrete use cases + usage notes. dirge's prior description was a single sentence; ported opencode's structure: - "Use this when you need to: (1) gather preferences (2) clarify ambiguity (3) get decisions (4) offer choices" - Usage notes covering the `custom` option, multi_select, and the "(Recommended)" first-option convention - Explicit "prefer asking over guessing when genuinely ambiguous, but don't over-ask" guidance System-prompt nudge: replaced the generic "ask the user directly" line with one that names the `question` tool, calls for concrete options with "(Recommended)" markers, and balances against over-asking. Matches opencode's intent without their anti-asking "beast mode" override. ## C2 remnant — find_callers optional path The earlier C2 fix (PR #120) covered 3 of 5 semantic tools. `find_callers` was missed: it accepts an optional `path` arg that scopes the search, but only the symbol name went through permission. An LLM doing `find_callers(name="foo", path="/etc")` walked /etc with no external_directory check. Added `check_perm_path` after the existing name-side `check_perm`. Defaults to "." when no path arg supplied (so the check runs against the working dir). `find_definition` remains untouched — it has no path arg (operates over the whole project index), so check_perm on the symbol name is the right granularity. ## C4 remnant — task_status missing `task_status` (the companion to `task` for querying background subagent state) had no `PermissionConfig` field and wasn't in the `PermissionChecker::new` loop. It always fell through to the `*` default, contradicting the "every tool gated explicitly" model. Added `task_status: Option<ToolPerm>` to PermissionConfig + the matching entry in the checker's load loop. `/allow`'s KNOWN_PERM_TOOLS whitelist and CONFIG.md's category list both get the new key. Read-only tool — users mostly leave it as default Allow, but the option to deny it independently exists (e.g. force background-only by allowing task but denying task_status polling). ## WEBFETCH_ENABLED env-var symmetry websearch had a `WEBSEARCH_ENABLED` env-var escape hatch (set to "true"/"1" to force-enable). webfetch had no equivalent — users had to edit config.json. Added `WEBFETCH_ENABLED` with identical semantics + a tiny `env_true` closure so the two web-tool checks share the parsing. ## Tests No new tests added — these are surface-area + steering changes. The existing 10 question-tool tests still pass and cover the JSON contract; the new path-side check on `find_callers` is exercised whenever permission tests run. 736 plugin / 610 default pass. All build profiles + fmt clean. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…ve URL scheme; tool defaults match docs (dirge-code#120) Six verified security-critical findings from a code review. ## C1: grep / glob / find_files bypassed external_directory rules Each tool called `check_perm(name, pattern)` but the actual filesystem path being walked (`args.path`) was never permission- checked. `check_perm_path` is what consults `ext_dir_rules` + Accept-mode working-dir gating; the bare `check_perm` skips both. An LLM doing `grep("keyword", "/etc")` or `find_files("*", "~/.ssh")` walked external dirs without asking — even under restrictive rules. Added `check_perm_path` after the existing `check_perm` call in each of the three tools. The pattern matches `read.rs` / `list_dir.rs` which already did this. ## C2: semantic tools didn't use path-aware perm check `find_callees`, `get_symbol_body`, `list_symbols` all accepted a `path` arg and passed it to `check_perm` — same class of bypass as C1. Switched the three to `check_perm_path`. The other two semantic tools (`find_callers`, `find_definition`) operate over symbol names only and continue to use `check_perm`. ## C3: case-sensitive scheme check enabled SSRF bypass `starts_with("http://")` only matched lowercase. URL schemes are case-insensitive per RFC 3986, so `HTTP://169.254.169.254/` slipped past both the scheme guard AND the SSRF host blocker (which also did case-sensitive `strip_prefix`). Refactored to a shared `has_http_scheme` helper using `eq_ignore_ascii_case`, and rewrote the host-extraction to use case-insensitive length- matching to strip the scheme. ## C6: tools.websearch / tools.webfetch default mismatched docs CONFIG.md states "both `bool`, default `true`". Code had `.unwrap_or(false)` so both tools were disabled by default, contradicting the documented behavior. Flipped to `unwrap_or(true)`. Explicit `false` in config still disables; absent or `true` enables (websearch additionally requires the runtime EXA_API_KEY check to pass, unchanged). ## C5: removed dead --tools / -t CLI flag The `tools: Vec<String>` flag was defined in clap but never read anywhere — pure dead code. Not documented in README or CONFIG.md. Removed the clap arg entirely. Users invoking `-t bash` will now get a clear clap error instead of silent acceptance. ## Tests 1 new regression: `scheme_matching_is_case_insensitive` — verifies `HTTP://`, `HTTPS://`, mixed case all pass the scheme check AND that SSRF defense still triggers for case- variant schemes targeting metadata IPs. 813 all-features tests pass (was 812). All build profiles clean. ## Deferred to next PR (config + cleanup batch) - C4: unregistered tool names default to Allow. Architectural — needs a design decision (explicit tool whitelist vs default allow). Will tackle separately. - C7: missing didClose in LSP. Needs lifecycle design — when to fire (on file delete? on session end?). - H1: unbounded edit/apply_patch file reads. Trivial cap; in next batch. - H3: blocking std::fs in apply_patch + list_dir. In next batch. - H4 / H5: missing PermissionConfig fields for MCP / 8 other tools. Schema change; in next batch. - H6: MCP connect errors only at tracing::warn. Tiny fix; in next batch. - H8: LSP config extensions field ignored. In next batch. - H9 / H10: dead acp_servers / acp_host / acp_port config fields. In next batch. ## Verified false positives - H2 (empty bash): `quote_aware_split` already filters empty segments. No bug. - H7 (LSP spawn silent): the `broken` set + backoff IS the user-feedback mechanism for repeated failures; logging at warn is intentional non-fatal degradation. - H11 (plugin dedup): summary IS emitted when a different error arrives. - H12 (token counter field): `estimate_tokens` on the accumulated partial string is correct. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…ig (dirge-code#121) Continues from PR dirge-code#120 (security-critical). Six deferred items shipped. ## H1: edit + apply_patch unbounded file reads `edit.rs` and `apply_patch.rs` (apply_update) called `tokio::fs::read` / `std::fs::read_to_string` with no size guard. An LLM pointing them at a multi-GB log/binary would OOM the agent process. Added 100 MiB pre-flight metadata check in both. The cap is generous (every realistic source file fits) but rejects the pathological case fast with a clear error pointing the agent at `bash + sed/awk` for huge files. ## H3: apply_patch blocking std::fs in async All four `apply_*` helpers (create / update / delete / rename) used synchronous `std::fs` calls inside an async `call` method. On the tokio runtime with limited worker threads, slow I/O can starve the event loop. Switched each to `tokio::fs` and made them async. Tests updated from `#[test]` to `#[tokio::test]` (8 tests touched). ## H4 + H5: PermissionConfig fields for missing tools `PermissionConfig` only had fields for 11 tools (bash, read, write, edit, grep, find_files, list_dir, write_todo_list, apply_patch, lsp, question). 12 other tools the agent can call had no way to be individually gated — they always fell through to the global default action. Added 12 fields covering the missing surface: - Web: `webfetch`, `websearch` - Subagent / state: `task`, `memory`, `skill` - Semantic (tree-sitter): `list_symbols`, `get_symbol_body`, `find_definition`, `find_callers`, `find_callees` - MCP umbrella: `mcp_tool` — pattern rules match against the full `mcp_tool:<server>:<tool>` key, so `{"mcp_tool:filesystem:*": "deny"}` blocks every tool from a filesystem MCP server. `PermissionChecker::new` now loads rules for all 23 tool names. `is_path_tool_name` extended to mark the path-bearing semantic tools (`list_symbols`, `get_symbol_body`, `find_callees`) plus `grep`/`find_files`/`glob` whose new path-side perm check (PR dirge-code#120) uses path-glob semantics. The `/allow` slash command's whitelist grew to match — typos on the new tool names still get caught. ## H6: MCP connect / list_tools errors visible to user `McpClientManager::connect_all` and `collect_tools` logged failures at `tracing::warn!` only. Default-log users (no `RUST_LOG` / `--verbose`) saw nothing — configured MCP servers just silently failed to register tools. Added an `eprintln!` alongside each warn so the failure surfaces to stderr unconditionally. Message names the server and tells the user the tools won't be available this session. ## H9 + H10: removed dead acp_host / acp_port config keys Both keys were parsed by serde but never consumed anywhere. ACP transport is stdio-only by design. Removed from `config::Config`. CONFIG.md already (PR dirge-code#112) documents the removal; no further docs change needed. `acp_servers` kept — it has a CONFIG.md entry, holds future TCP / Unix-socket config, and isn't actively misleading users. ## CONFIG.md Rewrote the Permission tool-keys list from a single sentence into a categorized list covering all 23 tools + the `mcp_tool:<server>:<tool>` pattern syntax. ## Tests 8 `apply_patch` tests converted to `#[tokio::test]` (no new assertions — the conversion was mechanical so the existing coverage still applies). 813 all-features / 730 plugin / 604 default pass — same as PR dirge-code#120 plus one new test there. ## Not in this PR (still deferred) - **C4**: unregistered-tool default-allow. This PR ADDS fields for the 12 missing tools, so the original "unregistered" set is now mostly covered. The remaining gap (future MCP tools, third-party plugins) is by design — `mcp_tool` is the umbrella key. - **C7**: LSP didClose. Needs lifecycle design (when to fire? on file delete? session end?). Will tackle as a focused PR. - **H8**: LSP `extensions` config field ignored. Real gap but bigger plumbing change; separate PR. ## Verified false positives (re-confirmed from earlier review) - H2 empty bash: `quote_aware_split` filters empty segments. - H7 LSP spawn silent: broken-set + backoff IS the feedback. - H11 plugin dedup: summary IS emitted. - H12 token counter: estimate_tokens on partial is correct. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…irge-code#122) Two LSP gaps from the audit, both real. ## C7: missing didClose → server-side state leak LSP servers retain parse trees + diagnostic caches keyed on every `textDocument/didOpen`. dirge never sent `textDocument/didClose`, so a long session editing dozens of files left rust-analyzer / pyright / etc. holding all that state for the lifetime of the server process. Added two methods on `LspClient`: - `notify_close(path)`: send didClose for one file, drop the associated FileState / push + pull diagnostics / last-push timestamp from local tracking. Returns Ok on never-opened files (a stray didClose is harmless per the spec). - `close_all()`: fan didClose across every tracked file. Used at session shutdown. `LspManager::close_all_files` fans across every attached client. Called from main.rs after the interactive loop returns. Best-effort: an already-dead server isn't re-contacted and individual notify_close failures are swallowed. The `lsp_manager` Arc is cloned before being moved into `ui::run_interactive` so the shutdown call site still has a handle. Two new regression tests pin the behavior: - `notify_close_emits_did_close_and_clears_state` — checks that subsequent `notify_open` after close emits a fresh didOpen (version 0) rather than a didChange. - `close_all_emits_did_close_for_every_open_file` — two open files → two didClose frames sent. ## H8: LspServerConfig.extensions ignored `LspServerConfig` carries an `extensions: Option<Vec<String>>` field, parsed by serde, but `LspManager` always used the hardcoded `builtin_servers()` registry. User config like `{ "lsp": { "rust": { "extensions": ["rs", "rlib"] } } }` was silently dropped. Two changes: 1. `ServerInfo.extensions` changed from `&'static [&'static str]` to `Vec<String>` so it can be mutated post-construction. `builtin_servers()` now produces owned vecs; this is a one-time cost paid at session startup. 2. New `apply_extension_overrides` function in `lsp::server` walks the user's per-server config and replaces builtin extension lists (or removes the server entirely if `disabled: true`). Lowercases + strips leading dots to match how `servers_for_extension` looks up by extension. 3. `LspManager::with_servers` constructor accepts the modified server list. The existing `new` delegates with the unmodified builtin list. 4. `main.rs::build_channels` applies overrides between `builtin_servers()` and `LspManager::with_servers(...)`. The `compile_lsp_commands` docstring previously called the extensions field a follow-up; updated to point at the new override path. A trait shim (`AsExtensionOverride`) keeps the `lsp::server` module from depending directly on `config::LspServerConfig` — keeps the dep graph clean and lets the tests use a stub. 4 new regression tests: - Extensions override replaces builtin list. - `disabled: true` removes server from the active set. - Unknown server id silently ignored (no panic, no spurious entry added). - User-supplied extensions are normalized (leading-dot strip + lowercase). ## Status 736 plugin / 610 default pass (was 730 / 604). All build profiles + fmt clean. This closes the last two real items in the original audit's deferred list. PR dirge-code#120 → C1/C2/C3/C5/C6. PR dirge-code#121 → H1/H3/H4/H5/ H6/H9/H10. This PR → C7/H8. All verified false positives (H2, H7, H11, H12) remain unchanged. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…WEBFETCH_ENABLED symmetry (dirge-code#123) ## Steering the model toward the question tool opencode's `question.txt` description gets the model to actually reach for the tool by enumerating concrete use cases + usage notes. dirge's prior description was a single sentence; ported opencode's structure: - "Use this when you need to: (1) gather preferences (2) clarify ambiguity (3) get decisions (4) offer choices" - Usage notes covering the `custom` option, multi_select, and the "(Recommended)" first-option convention - Explicit "prefer asking over guessing when genuinely ambiguous, but don't over-ask" guidance System-prompt nudge: replaced the generic "ask the user directly" line with one that names the `question` tool, calls for concrete options with "(Recommended)" markers, and balances against over-asking. Matches opencode's intent without their anti-asking "beast mode" override. ## C2 remnant — find_callers optional path The earlier C2 fix (PR dirge-code#120) covered 3 of 5 semantic tools. `find_callers` was missed: it accepts an optional `path` arg that scopes the search, but only the symbol name went through permission. An LLM doing `find_callers(name="foo", path="/etc")` walked /etc with no external_directory check. Added `check_perm_path` after the existing name-side `check_perm`. Defaults to "." when no path arg supplied (so the check runs against the working dir). `find_definition` remains untouched — it has no path arg (operates over the whole project index), so check_perm on the symbol name is the right granularity. ## C4 remnant — task_status missing `task_status` (the companion to `task` for querying background subagent state) had no `PermissionConfig` field and wasn't in the `PermissionChecker::new` loop. It always fell through to the `*` default, contradicting the "every tool gated explicitly" model. Added `task_status: Option<ToolPerm>` to PermissionConfig + the matching entry in the checker's load loop. `/allow`'s KNOWN_PERM_TOOLS whitelist and CONFIG.md's category list both get the new key. Read-only tool — users mostly leave it as default Allow, but the option to deny it independently exists (e.g. force background-only by allowing task but denying task_status polling). ## WEBFETCH_ENABLED env-var symmetry websearch had a `WEBSEARCH_ENABLED` env-var escape hatch (set to "true"/"1" to force-enable). webfetch had no equivalent — users had to edit config.json. Added `WEBFETCH_ENABLED` with identical semantics + a tiny `env_true` closure so the two web-tool checks share the parsing. ## Tests No new tests added — these are surface-area + steering changes. The existing 10 question-tool tests still pass and cover the JSON contract; the new path-side check on `find_callers` is exercised whenever permission tests run. 736 plugin / 610 default pass. All build profiles + fmt clean. Co-authored-by: Yogthos <yogthos@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six verified security-critical findings. C1: grep/glob/find_files skipped external_directory permission rules (only checked pattern, not path). C2: 3 of 5 semantic tools had the same path-arg bypass. C3: scheme check was case-sensitive (
HTTP://169.254.169.254/slipped past both scheme + SSRF defenses). C6: tools.websearch/webfetch defaulted to false despite docs saying true. C5: --tools CLI flag was dead code. 1 new regression test, 813 pass. Architectural items (C4 unregistered-tool allow, C7 LSP didClose, H1/H3/H4/H5/H6/H8/H9/H10) deferred to a config/cleanup PR.