fix: audit batch 2 — docs accuracy + 2 small code touch-ups - #112
Merged
Conversation
Follow-up to PR #111. Tier-2 items from the 23-bug audit batch: docs corrections and two small correctness/UX fixes. ## Docs - **#12 temperature** — CONFIG.md claimed "parsed but not currently applied". Actually applied since PR #105 with a clamp warning. Rewrote the cell. - **#13 --api-key** — flag existed but neither README nor CONFIG.md mentioned it. Added a Quick-start example noting the process-list visibility caveat. - **#14 acp_host/acp_port** — CONFIG.md documented both keys but the CLI flags were intentionally removed (stdio-only transport). Removed both from the keys table + ACP section. - **#6 tools** — `Config::tools` (per-tool enable map) was fully wired in code but undocumented. Added a row to the keys table covering `tools.websearch` and `tools.webfetch`. - **#21 find_callers** — README claimed "word-boundary regex" but the impl uses the tree-sitter symbol index. Updated to reflect actual behavior; the user-visible word-boundary semantics are preserved. ## Code - **#16 semantic index skip_dir** — `SymbolIndex::find_callers` filter had its own hardcoded `matches!(name, "node_modules" | "target" | ".git" | "__pycache__")` while the rest of the codebase uses `agent::tools::is_skip_dir`. Switched to the shared helper so future additions stay in lockstep. - **#18 context::load_file** — silently swallowed `read_to_string` errors via `.ok()`. A permission-denied AGENTS.md looked identical to a missing file. Now emits a stderr warning naming the path + reason; still returns None so callers' behavior is unchanged. 725 plugin / 599 default pass. All build profiles clean. ## Remaining audit items (deferred to feature work) - **#8 LSP no crash restart**: needs broken-pipe IO error handling + exponential backoff. Touches manager state machine. - **#10 task tool fire-and-forget**: needs timeout + cleanup coordination via JoinHandle tracking. - **#25 MCP no reconnection**: similar architectural concern to #8. - **#27 LSP didClose**: client lifecycle hook missing. - **#29 token estimation len/4**: needs per-provider usage extraction (Phase 6 work). - **#5 MCP shutdown**: rmcp Drop semantics need verification. - **#38/39/40 semantic test gaps**: get_symbol_body untested, list_symbols kind_filter untested, find_definition test vacuous. Sat down to add but each requires a fixture build. Together with PR #111 (10 code fixes), 17 of the 23 verified items are now shipped. Remaining 6 are architectural or test-infrastructure work better tackled as discrete PRs.
yogthos
added a commit
that referenced
this pull request
May 21, 2026
…1 leftovers (#113) Verified the 11 new round-2 findings and 6 round-1 leftovers against the actual code. Shipping the 7 concrete fixes that map to clear before/after behavior; the rest are deferred-architectural or already-fixed-elsewhere. ## N1 — webfetch SSRF (CRITICAL) `http://169.254.169.254/latest/meta-data/` (AWS metadata) and all private/loopback ranges sailed straight through. An LLM that gets prompt-injected into fetching a private IP could exfiltrate IAM credentials, hit internal admin endpoints, or scrape internal network maps. New `validate_url_host_safety()` blocks: - Hostname literal "localhost" / "ip6-localhost" / "ip6-loopback" - IPv4 loopback (127/8), private (10/8, 172.16/12, 192.168/16), link-local (169.254/16 — cloud metadata), unspecified, broadcast, class E. - IPv6 loopback, ULA (fc00::/7), link-local (fe80::/10), multicast, unspecified. Bracketed IPv6 (`[::1]`) is parsed correctly. Opt-in escape hatch: `DIRGE_WEBFETCH_ALLOW_PRIVATE=1` for dev workflows that need to hit localhost. Tool description documents the env var so the agent can see it. ## N8 — webfetch body OOM (HIGH) `resp.text().await` buffered the entire response into memory before any truncation. A 500 MB page would OOM the agent process. Now streams via `bytes_stream` with a 10 MiB cap; bails at the cap. ## N2 — MCP result size cap (CRITICAL) `McpTool::call` concatenated every `RawContent::Text` block into `content` without limit. A 200 KB MCP response flooded every subsequent turn until compaction. Cap aggregate at 256 KiB (≈65k tokens worst case); UTF-8 char-boundary slice; append a clear truncation marker naming the offending server::tool so the agent can adjust its calls. ## N3 — bash output cap (CRITICAL) The UI's `render_tool_output` truncated the DISPLAY but the full string was persisted to `ToolCallState::Completed` and fed back to the LLM on the next turn. `cat /dev/urandom | head -c 10M` would have shoved millions of tokens. Cap at 256 KiB at the bash tool source, with a clear truncation message pointing the agent at head/grep filtering. ## N5 — sandbox hardening (HIGH) Added `--new-session` (drops the ability to gain new privs via setuid) and `--unshare-user-try` (explicit user-ns isolation, defensive against future bwrap default changes). `--unshare-all` already covers most of this; the explicit flags pin the guarantees. ## N7 — sanitize_output strips \\r (HIGH) Previously `\\r` was preserved (originally for CRLF lines). A bash tool printing progress with `\\rstep N/M` would move the cursor to column 0 mid-line, overwriting chamber borders. Now stripped (`\\n` and `\\t` still pass; display path normalizes CRLF before reaching here). ## H10 — task subagent timeout `tokio::spawn(btw_query(...))` with no timeout meant a stuck provider would keep the task in `Running` forever. Cap at 10 minutes; on timeout, the task transitions to `Failed` with a clear message and the system-reminder still fires. ## M22 — /prompt default help text Behavior was fixed in PR #111 (activates `default.md` if installed). Help text still said "clear active prompt". Now explains both cases. ## Verified false positives / non-issues this round - **N4 sandbox --ro-bind / /**: by design — the sandbox is a WRITE guard, not exfiltration prevention. Defense for read access requires `--ro-bind` per path (much narrower scope + breaks read-only tooling like bash `ls`). Document as feature, not bug. - **N6 sandbox current_dir fallback**: `unwrap_or_else(|_| ".".into())` is the standard fallback; `current_dir()` failing means cwd is deleted/inaccessible which is its own fatal-soon problem. - **N9 token estimation len/4**: Phase 6 work; per-provider usage extraction needs distinct fix. - **N10 sandbox /dev/tty**: `--dev /dev` mounts a tmpfs with only essential nodes, not the full host /dev. /dev/tty IS available to the sandboxed process so it can write to its own controlling terminal (needed for bash interactive features); not the host's /dev/tty0. - **H17 DeepSeek auto-detect order**: confirmed false positive in earlier rounds — only fires when env var present. - **H18 load_file path in error**: already fixed in PR #112. - **M19 semantic tools in BUILTIN_TOOL_NAMES**: semantic tools added separately, can't be shadowed by MCP collision. ## Tests 2 new regression tests: - `validate_url_host_safety_blocks_ssrf_targets`: pins 169.254.x, 127.x, RFC 1918, ::1, fc00::, fe80:: as refused; public IPs + domains pass; bracketed IPv6 parses correctly. - `validate_url_host_safety_handles_malformed_hosts`: garbage hosts don't panic. 727 plugin / 601 default pass. ## Deferred to architectural follow-ups - H8 LSP no crash restart — broken-pipe handling design - N4 sandbox --ro-bind / — fundamental design tradeoff - N9 token estimation — per-provider usage extraction Co-authored-by: Yogthos <yogthos@gmail.com>
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>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…de#112) Follow-up to PR dirge-code#111. Tier-2 items from the 23-bug audit batch: docs corrections and two small correctness/UX fixes. ## Docs - **dirge-code#12 temperature** — CONFIG.md claimed "parsed but not currently applied". Actually applied since PR dirge-code#105 with a clamp warning. Rewrote the cell. - **dirge-code#13 --api-key** — flag existed but neither README nor CONFIG.md mentioned it. Added a Quick-start example noting the process-list visibility caveat. - **dirge-code#14 acp_host/acp_port** — CONFIG.md documented both keys but the CLI flags were intentionally removed (stdio-only transport). Removed both from the keys table + ACP section. - **dirge-code#6 tools** — `Config::tools` (per-tool enable map) was fully wired in code but undocumented. Added a row to the keys table covering `tools.websearch` and `tools.webfetch`. - **dirge-code#21 find_callers** — README claimed "word-boundary regex" but the impl uses the tree-sitter symbol index. Updated to reflect actual behavior; the user-visible word-boundary semantics are preserved. ## Code - **dirge-code#16 semantic index skip_dir** — `SymbolIndex::find_callers` filter had its own hardcoded `matches!(name, "node_modules" | "target" | ".git" | "__pycache__")` while the rest of the codebase uses `agent::tools::is_skip_dir`. Switched to the shared helper so future additions stay in lockstep. - **dirge-code#18 context::load_file** — silently swallowed `read_to_string` errors via `.ok()`. A permission-denied AGENTS.md looked identical to a missing file. Now emits a stderr warning naming the path + reason; still returns None so callers' behavior is unchanged. 725 plugin / 599 default pass. All build profiles clean. ## Remaining audit items (deferred to feature work) - **dirge-code#8 LSP no crash restart**: needs broken-pipe IO error handling + exponential backoff. Touches manager state machine. - **dirge-code#10 task tool fire-and-forget**: needs timeout + cleanup coordination via JoinHandle tracking. - **dirge-code#25 MCP no reconnection**: similar architectural concern to dirge-code#8. - **dirge-code#27 LSP didClose**: client lifecycle hook missing. - **dirge-code#29 token estimation len/4**: needs per-provider usage extraction (Phase 6 work). - **dirge-code#5 MCP shutdown**: rmcp Drop semantics need verification. - **dirge-code#38/39/40 semantic test gaps**: get_symbol_body untested, list_symbols kind_filter untested, find_definition test vacuous. Sat down to add but each requires a fixture build. Together with PR dirge-code#111 (10 code fixes), 17 of the 23 verified items are now shipped. Remaining 6 are architectural or test-infrastructure work better tackled as discrete PRs. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…1 leftovers (dirge-code#113) Verified the 11 new round-2 findings and 6 round-1 leftovers against the actual code. Shipping the 7 concrete fixes that map to clear before/after behavior; the rest are deferred-architectural or already-fixed-elsewhere. ## N1 — webfetch SSRF (CRITICAL) `http://169.254.169.254/latest/meta-data/` (AWS metadata) and all private/loopback ranges sailed straight through. An LLM that gets prompt-injected into fetching a private IP could exfiltrate IAM credentials, hit internal admin endpoints, or scrape internal network maps. New `validate_url_host_safety()` blocks: - Hostname literal "localhost" / "ip6-localhost" / "ip6-loopback" - IPv4 loopback (127/8), private (10/8, 172.16/12, 192.168/16), link-local (169.254/16 — cloud metadata), unspecified, broadcast, class E. - IPv6 loopback, ULA (fc00::/7), link-local (fe80::/10), multicast, unspecified. Bracketed IPv6 (`[::1]`) is parsed correctly. Opt-in escape hatch: `DIRGE_WEBFETCH_ALLOW_PRIVATE=1` for dev workflows that need to hit localhost. Tool description documents the env var so the agent can see it. ## N8 — webfetch body OOM (HIGH) `resp.text().await` buffered the entire response into memory before any truncation. A 500 MB page would OOM the agent process. Now streams via `bytes_stream` with a 10 MiB cap; bails at the cap. ## N2 — MCP result size cap (CRITICAL) `McpTool::call` concatenated every `RawContent::Text` block into `content` without limit. A 200 KB MCP response flooded every subsequent turn until compaction. Cap aggregate at 256 KiB (≈65k tokens worst case); UTF-8 char-boundary slice; append a clear truncation marker naming the offending server::tool so the agent can adjust its calls. ## N3 — bash output cap (CRITICAL) The UI's `render_tool_output` truncated the DISPLAY but the full string was persisted to `ToolCallState::Completed` and fed back to the LLM on the next turn. `cat /dev/urandom | head -c 10M` would have shoved millions of tokens. Cap at 256 KiB at the bash tool source, with a clear truncation message pointing the agent at head/grep filtering. ## N5 — sandbox hardening (HIGH) Added `--new-session` (drops the ability to gain new privs via setuid) and `--unshare-user-try` (explicit user-ns isolation, defensive against future bwrap default changes). `--unshare-all` already covers most of this; the explicit flags pin the guarantees. ## N7 — sanitize_output strips \\r (HIGH) Previously `\\r` was preserved (originally for CRLF lines). A bash tool printing progress with `\\rstep N/M` would move the cursor to column 0 mid-line, overwriting chamber borders. Now stripped (`\\n` and `\\t` still pass; display path normalizes CRLF before reaching here). ## H10 — task subagent timeout `tokio::spawn(btw_query(...))` with no timeout meant a stuck provider would keep the task in `Running` forever. Cap at 10 minutes; on timeout, the task transitions to `Failed` with a clear message and the system-reminder still fires. ## M22 — /prompt default help text Behavior was fixed in PR dirge-code#111 (activates `default.md` if installed). Help text still said "clear active prompt". Now explains both cases. ## Verified false positives / non-issues this round - **N4 sandbox --ro-bind / /**: by design — the sandbox is a WRITE guard, not exfiltration prevention. Defense for read access requires `--ro-bind` per path (much narrower scope + breaks read-only tooling like bash `ls`). Document as feature, not bug. - **N6 sandbox current_dir fallback**: `unwrap_or_else(|_| ".".into())` is the standard fallback; `current_dir()` failing means cwd is deleted/inaccessible which is its own fatal-soon problem. - **N9 token estimation len/4**: Phase 6 work; per-provider usage extraction needs distinct fix. - **N10 sandbox /dev/tty**: `--dev /dev` mounts a tmpfs with only essential nodes, not the full host /dev. /dev/tty IS available to the sandboxed process so it can write to its own controlling terminal (needed for bash interactive features); not the host's /dev/tty0. - **H17 DeepSeek auto-detect order**: confirmed false positive in earlier rounds — only fires when env var present. - **H18 load_file path in error**: already fixed in PR dirge-code#112. - **M19 semantic tools in BUILTIN_TOOL_NAMES**: semantic tools added separately, can't be shadowed by MCP collision. ## Tests 2 new regression tests: - `validate_url_host_safety_blocks_ssrf_targets`: pins 169.254.x, 127.x, RFC 1918, ::1, fc00::, fe80:: as refused; public IPs + domains pass; bracketed IPv6 parses correctly. - `validate_url_host_safety_handles_malformed_hosts`: garbage hosts don't panic. 727 plugin / 601 default pass. ## Deferred to architectural follow-ups - H8 LSP no crash restart — broken-pipe handling design - N4 sandbox --ro-bind / — fundamental design tradeoff - N9 token estimation — per-provider usage extraction 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>
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.
Tier-2 follow-up to PR #111. 5 docs fixes (#6
tools, #12 temperature, #13 --api-key, #14 acp_host/port, #21 find_callers) + 2 code fixes (#16 share is_skip_dir between semantic + tools, #18 context::load_file error context). 725/599 pass.