Skip to content

Fix 'allow always' patterns matching paths in bash/grep commands - #23

Merged
yogthos merged 1 commit into
mainfrom
fix/permission-allowlist
May 19, 2026
Merged

Fix 'allow always' patterns matching paths in bash/grep commands#23
yogthos merged 1 commit into
mainfrom
fix/permission-allowlist

Conversation

@yogthos

@yogthos yogthos commented May 19, 2026

Copy link
Copy Markdown
Collaborator

Reported scenario

```
[permission] bash: cd /Users/yogthos/src/work/rigging-workshop
(y) allow once (a) allow always (n) deny (ESC) abort
-> will allow: cd *
allowed bash cd * (saved to session)
◈ bash "cd /Users/yogthos/src/work/rigging-workshop && git diff"
[permission] bash: cd /Users/yogthos/src/work/rigging-workshop <-- re-prompts!
(y) allow once (a) allow always (n) deny (ESC) abort
-> will allow: cd *
```

Root cause

`glob_to_regex` compiled `` to `[^/]` for all patterns — classic filesystem-glob semantics where `` doesn't cross slashes. For shell commands like `cd /Users/...` the regex `^cd [^/]$` rejects the path argument, so the session-allowlist entry never matched. Every subsequent command re-prompted.

Fix

Split `Pattern` into two compile modes:

Constructor `*` `?` Used for
`Pattern::new` `[^/]*` `[^/]` path tools: read/write/edit/list_dir
`Pattern::new_command` `.*` `.` bash, grep, find_files, write_todo_list, ...

A `pattern_for_tool(tool, pat)` helper picks the right variant based on `is_path_tool_name(tool)`. Wired through `PermissionChecker::new` (config rules + default bash rules), `add_session_allowlist`, and `load_session_allowlist`. External-directory rules retain path semantics since they're explicitly path patterns.

What stays the same

  • `src/*` (path) still matches `src/main.rs` and not `src/agent/main.rs` — one segment only.
  • `src/**` (path) still spans directories.
  • Static config rules retain whatever semantics map to their tool.

What changes

  • `cd *` (bash) now matches `cd /any/path`, `cd /any/path && git diff`, etc.
  • `grep TODO*` matches commands beyond a single segment.
  • The fix is broader-permissive: nothing that previously matched now fails to match; some previously-too-restrictive matches now succeed.

Test coverage

11 new tests covering:

  • The exact bug scenario (full session-allowlist roundtrip via `PermissionChecker::check`).
  • `cd *` anchored at start (doesn't match `echo cd /foo`).
  • Path patterns retain one-segment / recursive semantics.
  • `?` differs across modes.
  • `~/foo` expansion works in both modes.
  • Regex metachars (`()`, etc.) are escaped, not interpreted.
  • `load_session_allowlist` preserves command semantics across reload.

Test plan

  • `cargo build`
  • `cargo test --bin dirge -- --skip plugin` → 266 passed, 0 failed
  • Manual: `allow always` on a `cd /foo` command, run a second `cd /bar`, observe no re-prompt

Reported scenario:
  [permission] bash: cd /Users/yogthos/src/work/rigging-workshop
    (y)/(a)/(n)/(ESC)  -> will allow: cd *
    allowed bash cd * (saved to session)
  ... next command ...
  [permission] bash: cd /Users/yogthos/src/work/rigging-workshop
    (y)/(a)/(n)/(ESC)  -> will allow: cd *
The 'allow always' pattern wasn't firing on subsequent bash commands.

Root cause: glob_to_regex compiled `*` to `[^/]*` for all patterns,
using classic filesystem-glob semantics (one segment, no slash crossing).
For shell commands like `cd /Users/...` the path argument contains
slashes, so the regex never matched. The session allowlist entry was
silently inert.

Fix: split Pattern into two compile modes.
- Pattern::new(s)          — path-style: `*` -> `[^/]*`, `?` -> `[^/]`
- Pattern::new_command(s)  — command-style: `*` -> `.*`, `?` -> `.`
Add pattern_for_tool(tool, pat) that picks the right variant based on
is_path_tool_name(tool). Wired through PermissionChecker::new (rules
loaded from config + default bash rules) and add_session_allowlist /
load_session_allowlist. External-directory rules keep path semantics
since they are path patterns by definition.

Path tools (read/write/edit/list_dir) keep their existing one-segment
behavior — `src/*` still doesn't auto-match nested files. Only bash,
grep, find_files, write_todo_list, etc. get the relaxed semantics.

11 regression + behavioral tests covering:
- cd * matches absolute paths and command pipelines (the exact bug)
- cd * anchors at start (doesn't match 'echo cd /foo')
- path `src/*` still excludes nested files
- path `src/**` spans directories
- ? exclusion differs across modes
- ~/foo expansion works in both modes
- regex metachars escaped not interpreted
- full session-allowlist roundtrip via PermissionChecker::check
- load_session_allowlist preserves command semantics across reload
@yogthos
yogthos force-pushed the fix/permission-allowlist branch from 2a6cb64 to d4bc1d1 Compare May 19, 2026 20:44
@yogthos
yogthos merged commit a5af4e6 into main May 19, 2026
1 check passed
@yogthos
yogthos deleted the fix/permission-allowlist branch May 19, 2026 20:46
yogthos added a commit that referenced this pull request May 21, 2026
…aths (#111)

23 audit findings verified REAL via parallel agent verification +
cross-check against opencode/pi reference patterns. Shipping the
10 most concrete fixes here; the rest go in a follow-up docs/test
batch.

## Security

- **#9 bash quote_aware_split missed bare `|`** —
  `safe_cmd | rm -rf /` was treated as one segment; only the
  LHS got permission-checked. Pipe RHS rode in unchecked under
  the fallback (non-semantic-bash) path. Added single-byte `|`
  split after `||` is matched. The tree-sitter path was already
  correct.

- **#4 read.rs no binary detection** — feeding a PDF/ELF/.pyc
  into the LLM as lossy UTF-8 wasted tokens and confused the
  model. Ported opencode `read.ts:153-198`: reject by
  extension list (zip/exe/.o/.pdf/.png/etc.), then sniff the
  first 4 KiB — null byte = binary, >30% non-printable = binary.
  Clear error message tells the agent to use bash + xxd instead.

## Correctness

- **#2 skill override inverted** — README contract: "Project
  skills override global skills by name". Code used
  `map.entry(name).or_insert(skill)` which KEEPS the first
  (global) value and silently drops project overrides. Switch
  to `map.insert` (last-write-wins) since globals iterate
  first and project iterates second.

- **#37 skill empty name** — frontmatter `name:` with empty
  value parsed to "", which then matched any `skill ""` call
  silently. Fall back to directory name when frontmatter name
  is empty/whitespace-only.

- **#1 session_tree.janet hook never fired** — plugin defined
  `(defn on-message ...)` but `(def hooks [])` was empty AND
  the hook name doesn't exist (dirge uses `on-message-update`).
  `/label` was permanently broken ("no entry yet"). Fix:
  rename to `on-message-update` + register in hooks vector.

- **#7 workflow.janet hooks vector missing entries** — plugin
  defined `workflow-on-tool-end`, `-on-error`, `-on-complete`
  but only registered the first four hook names. Three hooks
  were dead. Added them.

- **#26 MCP malformed JSON silently empty args** —
  `serde_json::from_str(&args).unwrap_or_default()` turned bad
  JSON into None, sending the server an empty argument set.
  Server then errored with confusing "missing required field"
  instead of dirge surfacing the actual parse error. Now returns
  ToolError with the parse error message + first 200 chars of
  the offending JSON.

- **#22 /prompt default unreachable** — README documents
  `default` as a built-in prompt (prompts/default.md exists),
  but `/prompt default` was intercepted as a magic "clear"
  keyword. If `default` is registered in `context.prompts`,
  the new branch falls through to the normal name-lookup. Only
  acts as clear-keyword when no `default` prompt is present
  (legacy fallback).

- **#23 /allow add accepted invalid tools** — typo
  `/allow add bsah ...` silently created an inert rule the
  user couldn't debug. Added a known-tools whitelist matching
  PermissionConfig fields; unknown tools error with the valid
  list.

## Performance + correctness

- **#11 grep loaded whole files into memory** — no size cap
  meant a 9MB file got fully buffered. Added 10 MiB per-file
  cap via metadata pre-check.

- **#15 Python dunder methods marked non-exported** —
  `!name.starts_with('_')` treats `__init__`/`__call__`/etc.
  as private, even though they're Python's standard public
  protocol. Recognize `__x__` dunder pattern as exported.

## UI

- **#36 panel char-count truncation vs Unicode width** — panel
  truncation used `chars().count()` while wide emoji and CJK
  take 2 cells. A status line with an emoji overflowed the
  right border by one cell. Switched to
  `UnicodeWidthStr::width` for both truncation and padding.

## Tests

4 new regression tests:
- `test_is_binary_extension_known` — pdf/tgz/.so/.jpg/.pyc
- `test_is_binary_content_null_byte` — null byte trigger,
  UTF-8 Japanese stays clean, all-non-printable triggers
- `quote_aware_split_splits_on_bare_pipe` — pipe security
- `quote_aware_split_or_and_pipe_distinct` — `a || b | c`
  produces 3 segments, not 2

725 plugin / 599 default pass. All build profiles clean.

## Verified false positives (not fixed, audit was wrong)

- #3 cache.rs clear() race — generation counter gating in
  `get` makes stale entries invisible, no correctness impact.
- #17 DeepSeek auto-detect priority — auto-detect only fires
  when env vars present; default-default is still OpenRouter.
- #19 semantic tools in collision filter — semantic tools
  added separately, can't be shadowed by MCP.
- #20 glob global gitignore — intentionally disabled to match
  grep behavior.
- #28 nearest_root blocking std::fs — function doesn't exist
  in current code.
- #32 ReadArgs.path vs GrepArgs.path — semantically different
  by design (file vs dir), documented in schema.
- #33 install_plugin_providers dead-without-feature — gated
  with explicit `#[cfg_attr(not(feature), allow(dead_code))]`.
- #34 websearch double-gated — config + API key serve distinct
  purposes (enable + auth).

## Deferred to follow-up batches

Docs-only fixes (#6 CONFIG.md tools, #12 temperature, #13
--api-key, #14 acp_host/port), MCP/LSP architecture (#8, #25,
#27), test gaps (#38-40), and lower-priority polish — all in
a follow-up PR.

Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch pushed a commit to allen-munsch/dirge that referenced this pull request Jun 3, 2026
…aths (dirge-code#111)

23 audit findings verified REAL via parallel agent verification +
cross-check against opencode/pi reference patterns. Shipping the
10 most concrete fixes here; the rest go in a follow-up docs/test
batch.

## Security

- **dirge-code#9 bash quote_aware_split missed bare `|`** —
  `safe_cmd | rm -rf /` was treated as one segment; only the
  LHS got permission-checked. Pipe RHS rode in unchecked under
  the fallback (non-semantic-bash) path. Added single-byte `|`
  split after `||` is matched. The tree-sitter path was already
  correct.

- **#4 read.rs no binary detection** — feeding a PDF/ELF/.pyc
  into the LLM as lossy UTF-8 wasted tokens and confused the
  model. Ported opencode `read.ts:153-198`: reject by
  extension list (zip/exe/.o/.pdf/.png/etc.), then sniff the
  first 4 KiB — null byte = binary, >30% non-printable = binary.
  Clear error message tells the agent to use bash + xxd instead.

## Correctness

- **#2 skill override inverted** — README contract: "Project
  skills override global skills by name". Code used
  `map.entry(name).or_insert(skill)` which KEEPS the first
  (global) value and silently drops project overrides. Switch
  to `map.insert` (last-write-wins) since globals iterate
  first and project iterates second.

- **dirge-code#37 skill empty name** — frontmatter `name:` with empty
  value parsed to "", which then matched any `skill ""` call
  silently. Fall back to directory name when frontmatter name
  is empty/whitespace-only.

- **#1 session_tree.janet hook never fired** — plugin defined
  `(defn on-message ...)` but `(def hooks [])` was empty AND
  the hook name doesn't exist (dirge uses `on-message-update`).
  `/label` was permanently broken ("no entry yet"). Fix:
  rename to `on-message-update` + register in hooks vector.

- **dirge-code#7 workflow.janet hooks vector missing entries** — plugin
  defined `workflow-on-tool-end`, `-on-error`, `-on-complete`
  but only registered the first four hook names. Three hooks
  were dead. Added them.

- **dirge-code#26 MCP malformed JSON silently empty args** —
  `serde_json::from_str(&args).unwrap_or_default()` turned bad
  JSON into None, sending the server an empty argument set.
  Server then errored with confusing "missing required field"
  instead of dirge surfacing the actual parse error. Now returns
  ToolError with the parse error message + first 200 chars of
  the offending JSON.

- **dirge-code#22 /prompt default unreachable** — README documents
  `default` as a built-in prompt (prompts/default.md exists),
  but `/prompt default` was intercepted as a magic "clear"
  keyword. If `default` is registered in `context.prompts`,
  the new branch falls through to the normal name-lookup. Only
  acts as clear-keyword when no `default` prompt is present
  (legacy fallback).

- **dirge-code#23 /allow add accepted invalid tools** — typo
  `/allow add bsah ...` silently created an inert rule the
  user couldn't debug. Added a known-tools whitelist matching
  PermissionConfig fields; unknown tools error with the valid
  list.

## Performance + correctness

- **dirge-code#11 grep loaded whole files into memory** — no size cap
  meant a 9MB file got fully buffered. Added 10 MiB per-file
  cap via metadata pre-check.

- **dirge-code#15 Python dunder methods marked non-exported** —
  `!name.starts_with('_')` treats `__init__`/`__call__`/etc.
  as private, even though they're Python's standard public
  protocol. Recognize `__x__` dunder pattern as exported.

## UI

- **dirge-code#36 panel char-count truncation vs Unicode width** — panel
  truncation used `chars().count()` while wide emoji and CJK
  take 2 cells. A status line with an emoji overflowed the
  right border by one cell. Switched to
  `UnicodeWidthStr::width` for both truncation and padding.

## Tests

4 new regression tests:
- `test_is_binary_extension_known` — pdf/tgz/.so/.jpg/.pyc
- `test_is_binary_content_null_byte` — null byte trigger,
  UTF-8 Japanese stays clean, all-non-printable triggers
- `quote_aware_split_splits_on_bare_pipe` — pipe security
- `quote_aware_split_or_and_pipe_distinct` — `a || b | c`
  produces 3 segments, not 2

725 plugin / 599 default pass. All build profiles clean.

## Verified false positives (not fixed, audit was wrong)

- #3 cache.rs clear() race — generation counter gating in
  `get` makes stale entries invisible, no correctness impact.
- dirge-code#17 DeepSeek auto-detect priority — auto-detect only fires
  when env vars present; default-default is still OpenRouter.
- dirge-code#19 semantic tools in collision filter — semantic tools
  added separately, can't be shadowed by MCP.
- dirge-code#20 glob global gitignore — intentionally disabled to match
  grep behavior.
- dirge-code#28 nearest_root blocking std::fs — function doesn't exist
  in current code.
- dirge-code#32 ReadArgs.path vs GrepArgs.path — semantically different
  by design (file vs dir), documented in schema.
- dirge-code#33 install_plugin_providers dead-without-feature — gated
  with explicit `#[cfg_attr(not(feature), allow(dead_code))]`.
- dirge-code#34 websearch double-gated — config + API key serve distinct
  purposes (enable + auth).

## Deferred to follow-up batches

Docs-only fixes (dirge-code#6 CONFIG.md tools, dirge-code#12 temperature, dirge-code#13
--api-key, dirge-code#14 acp_host/port), MCP/LSP architecture (dirge-code#8, dirge-code#25,
dirge-code#27), test gaps (dirge-code#38-40), and lower-priority polish — all in
a follow-up PR.

Co-authored-by: Yogthos <yogthos@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant