ML-157: iterate on plan

2026-05-03 14:59:14 +01:00
parent 84012cbd1c
commit 9fda537bf9
1 changed files with 136 additions and 11 deletions
@@ -65,14 +65,46 @@ This is the simplest viable approach:

 File: `.pi/sensitive-paths.json`

-Content: JSON object with `blocked_paths` (glob-like patterns for path matching) and `blocked_command_patterns` (regex patterns for bash command scanning).
+Content: JSON object with `blocked_paths` (glob-like patterns for path matching) and `blocked_commands` (regex patterns for bash command scanning of commands that leak secrets without a path in the string, e.g., `printenv`, `env`, `set`).
+
+The config includes a `_comment` field (ignored by the extension) to document each pattern's purpose, since JSON does not support inline comments.
+
+Example config:
+
+```json
+{
+  "_comment": "Patterns for blocking sensitive file access by pi. Glob-like for paths, regex for commands.",
+  "blocked_paths": [
+    ".env",
+    ".envrc",
+    "*.pem",
+    "*.key",
+    "*.key.pub",
+    "*secret*",
+    "*credential*",
+    "*credentials*",
+    ".ssh/",
+    ".aws/",
+    ".gnupg/",
+    "~/.pi/agent/sessions/"
+  ],
+  "blocked_commands": [
+    "printenv",
+    "\\benv\\b",
+    "\\bset\\b"
+  ]
+}
+```

 Initial patterns cover:
- `.env` files: `.env`, `.env.*`, `.envrc`
+- `.env` files: `.env`, `.envrc` (`.env.*` is NOT included — it would false-positive on `.env.example`, which must remain readable)
 - Key files: `*.pem`, `*.key`, `*.key.pub`
 - Secret/credential files: paths containing `secret`, `credential`, `credentials`
 - Sensitive directories: `.ssh/`, `.aws/`, `.gnupg/`
 - Pi session files (contain tool outputs): `~/.pi/agent/sessions/`
+- Bash commands that dump environment: `printenv`, `env`, `set` (match as whole words via `\b` anchors)
+
+**`.env.example` exclusion:** The pattern `.env.*` is deliberately omitted from `blocked_paths` so `.env.example` is not caught. If a `.env.production` file exists and should be blocked, add the specific pattern `.env.production` rather than the wildcard `.env.*`.

 Verification: File exists at `.pi/sensitive-paths.json` and is valid JSON.

@@ -82,21 +114,97 @@ File: `.pi/extensions/sensitive-file-guard.ts`

 Implementation:
 1. Read `.pi/sensitive-paths.json` at extension load time (synchronous, using `readFileSync`)
-2. Compile `blocked_paths` entries into a regex matcher (glob → regex conversion)
+2. Compile `blocked_paths` entries into a regex matcher (glob → regex conversion, all patterns compiled with the case-insensitive `i` flag to handle case-insensitive filesystems like macOS):
+   - `*` → `.*`
+   - `.` → `\.` (escape literal dots)
+   - `/` → `\/` (escape path separators)
+   - Wrap in anchors: `^` and `$` for exact patterns, or allow partial match for glob-like patterns
+   - Apply `i` flag for case-insensitive matching
 3. Subscribe to `tool_call` event
 4. Handler logic:
-   - For `read`, `grep`, `write`, `edit`, `find`, `ls`: check `event.input.path` against the compiled blocked path regexes. Resolve relative paths against `ctx.cwd`.
-   - For `bash`: check `event.input.command` against `blocked_command_patterns` regexes AND scan for blocked paths in the command string
+   - **Path-based tools** (`read`, `grep`, `write`, `edit`, `find`, `ls`):
+     a. Resolve `event.input.path` to an absolute path using `path.resolve(ctx.cwd, event.input.path)`, then normalize with `path.normalize()` to collapse `..` segments. This defends against path traversal (`../.env`, `../../.env`).
+     b. Check the resolved absolute path against the compiled blocked path regexes.
+   - **Bash tool**: Two separate checks with distinct purposes:
+     a. **Blocked path scan:** Search the raw `event.input.command` string for fragments that match blocked path patterns (e.g., `.env`, `.ssh/`, `.aws/`). This catches commands like `cat .env`, `grep foo ~/.ssh/config`, `cat /absolute/path/.env`. The scan is a simple substring/regex match against the command text — it does not parse shell syntax. This covers the most common accidental access patterns.
+     b. **Blocked command check:** Test `event.input.command` against `blocked_commands` regexes (from config). These cover commands that leak secrets *without* a path argument in the command string (e.g., `printenv`, `env`, `set`, `export`). Each regex uses `\b` word-boundary anchors to avoid false positives (e.g., `env` should match `env` but not `environment`).
+   - **Blocking and notification:**
     - If match found and `ctx.hasUI` is true: show a warning notification via `ctx.ui.notify()`
-   - If match found: return `{ block: true, reason: "Blocked sensitive path: <path>" }` (error message for non-interactive mode)
+     - If match found: return `{ block: true, reason: "Blocked sensitive path: <path or command>" }` (error message for non-interactive mode)
     - If no match: return `undefined` (allow execution)

+**Edge Cases and Accepted Limitations:**
+
+| Scenario | Handling | Rationale |
+|----------|----------|-----------|
+| `cat ".env"` (quoted) | Caught by path substring scan | Quotes don't hide the path fragment |
+| `cat ./.env` (relative with `./`) | Caught by path substring scan | `.env` fragment still present |
+| `cat /absolute/path/.env` | Caught by path substring scan | `.env` fragment still present |
+| `cat $HOME/project/.env` | Caught (if `.env` appears verbatim) | Variable expansion at start doesn't mask the path fragment |
+| `head .env`, `tail .env`, `less .env` | Caught by path substring scan | All contain `.env` fragment |
+| `echo $MY_SECRET` (no file access) | NOT caught | This reads from environment memory, not disk. PI would need separate treatment for this (out of scope) |
+| `cat $(echo .env)` (command substitution) | NOT caught | Requires shell parsing; accepted limitation for accidental-access use case |
+| `eval 'cat .env'` | NOT caught | Requires shell parsing; accepted limitation |
+| `sh -c 'cat .env'` | Caught by path substring scan | `.env` appears in the command string |
+| Case variants (`.ENV`, `.Env`) | Caught by case-insensitive regex (`i` flag) | macOS filesystems are case-insensitive |
+| Path traversal (`../.env`) | Caught by `path.resolve` + `path.normalize` | Resolved absolute path contains `.env` fragment |
+| Symlinks (`config -> ~/.aws/`) | NOT caught in initial implementation | Resolving symlinks requires I/O (`realpathSync`) per tool call. Documented limitation; users must not symlink sensitive directories into the project tree. Can be added later via a config option `followSymlinks: true` |
+| `/etc/passwd` (system file) | NOT caught unless pattern added to config | Config is the source of truth for blocked paths; system files are not blocked by default |
+
+**Bash scanning scope:** The bash scanner uses simple substring/regex matching on the raw command string. It is designed to catch *accidental* access by the LLM, not adversarial bypass by a human. Commands using shell variable expansion, command substitution, or indirect execution may evade detection; this is an accepted limitation documented above.
+
 Verification:
 - Start pi in this project, ask it to read `.env.example` (should work — this is NOT a secret, it's the example)
 - Ask pi to read `.env` (should be blocked with a notification)
 - Ask pi to run `cat .env` via bash (should be blocked)
+- Ask pi to run `cat ../.env` via bash (should be blocked — path traversal)
 - Ask pi to run `ls .` (should work — listing the project root is fine)
 - Run `pi -p "read .env"` (non-interactive mode — should report error)
+- Run `pi -p "read lib/music_library.ex"` (non-interactive mode — should succeed with normal output)
+
+**Step 2.5: Write automated path-matching tests**
+
+File: `.pi/extensions/sensitive-file-guard.test.ts`
+
+Extract the path-matching logic into a pure function (`isBlockedPath(resolvedPath, blockedPatterns)` and `isBlockedCommand(command, blockedCommands)`) that takes config as input with no side effects (no filesystem I/O, no pi API calls). Write a table-driven test suite that asserts expected block/allow outcomes:
+
+```typescript
+// Example test table structure
+const pathTests = [
+  // [description, resolvedPath, expectedBlocked]
+  [".env file", "/project/.env", true],
+  [".env.example file", "/project/.env.example", false],
+  ["nested .env", "/project/config/.env", true],
+  ["normal source file", "/project/lib/music_library.ex", false],
+  ["SSH config", "/Users/me/.ssh/config", true],
+  ["AWS credentials", "/Users/me/.aws/credentials", true],
+  [".env with case variant", "/project/.ENV", true],
+  [".Env mixed case", "/project/.Env", true],
+  ["secret in filename", "/project/config/secrets.yml", true],
+  ["credential in path", "/project/config/credentials.json", true],
+  [".pem key file", "/project/keys/server.pem", true],
+  [".key file", "/project/keys/private.key", true],
+];
+
+const commandTests = [
+  // [description, command, expectedBlocked]
+  ["cat .env", "cat .env", true],
+  ["cat ./.env", "cat ./.env", true],
+  ["grep in .ssh", "grep -r foo ~/.ssh/", true],
+  ["cat lib/music_library.ex", "cat lib/music_library.ex", false],
+  ["printenv", "printenv", true],
+  ["env command", "env", true],
+  ["mix test (safe)", "mix test", false],
+  ["echo .env (mentioning, not accessing)", "echo .env", true],
+  ["environment variable ref (false positive)", "echo $NODE_ENV", false],
+];
+```
+
+Run with: `node --import tsx .pi/extensions/sensitive-file-guard.test.ts`
+
+This guarantees path-matching correctness independently of pi's runtime, catches regressions when patterns are updated, and documents expected behavior for each edge case.
+
+Verification: All tests pass before proceeding to integration verification.

 **Step 3: Verify non-interactive mode behavior**

@@ -127,25 +235,34 @@ Each step includes concrete verification instructions above. Overall verificatio
 | Test Case | Tool | Target | Expected |
 |-----------|------|--------|----------|
 | Read .env | read | `.env` | Blocked |
-| Read .env.production | read | `.env.production` | Blocked |
+| Read .env via path traversal | read | `../.env` | Blocked |
+| Read .env.production | read | `.env.production` | Blocked (if pattern added) |
 | Read .env.example | read | `.env.example` | **Allowed** (example file) |
+| Read .ENV (case variant) | read | `.ENV` | Blocked (case-insensitive) |
 | Read secrets file | read | `config/secrets.yml` | Blocked |
 | Cat .env via bash | bash | `cat .env` | Blocked |
+| Cat .env via bash with quotes | bash | `cat ".env"` | Blocked |
+| Cat ./.env via bash | bash | `cat ./.env` | Blocked |
 | Grep in .ssh | grep | `~/.ssh/config` | Blocked |
 | Ls in .aws | ls | `~/.aws/` | Blocked |
 | Write to .env | write | `.env` | Blocked |
 | Edit .env | edit | `.env` | Blocked |
+| Bash printenv (no path) | bash | `printenv` | Blocked |
+| Bash env command | bash | `env` | Blocked |
+| Bash echo $NODE_ENV (false positive check) | bash | `echo $NODE_ENV` | **Allowed** (`\b` anchor) |
 | Read normal .ex file | read | `lib/music_library.ex` | Allowed |
 | Run mix test | bash | `mix test` | Allowed |
 | Non-interactive read .env | read | `.env` (print mode) | Error reported |
+| Non-interactive read source file | read | `lib/music_library.ex` (print mode) | Normal output |
+| Add pattern + /reload | read | newly blocked path | Blocked after reload |

 ### 5. Architecture Impact Analysis

 | Touchpoint | Impact |
 |------------|--------|
-| `.pi/extensions/sensitive-file-guard.ts` | **New file** — extension entry point |
+| `.pi/extensions/sensitive-file-guard.ts` | **New file** — extension entry point. Named `000-sensitive-file-guard.ts` (or similar numeric prefix) to ensure it loads **first** among project-local extensions. Since `tool_call` handlers chain in load order and later handlers can mutate `event.input` before this guard sees it, loading first guarantees the guard inspects the original, unmodified tool arguments. |
 | `.pi/sensitive-paths.json` | **New file** — declarative blocked patterns config |
-| `tool_call` event | New subscriber, chains with existing extensions (e.g., MCP adapter) |
+| `tool_call` event | New subscriber, chains with existing extensions (e.g., MCP adapter). The guard runs **before** the MCP adapter's `tool_call` handler, so sensitive file access is blocked before any MCP forwarding occurs. |
 | Built-in tools | **Unchanged** — interception is non-invasive |
 | Phoenix/Ecto/Oban | **No impact** — pi-only concern, not part of the Elixir application |
 | Production infrastructure | **No impact** — this is developer-local tooling, not server configuration |
@@ -200,8 +317,16 @@ If future patterns grow to hundreds of entries, a trie-based matcher could be co
 - The existence of the sensitive file guard extension
 - The location and format of `.pi/sensitive-paths.json`
 - How to add or remove blocked patterns
+- The `_comment` field convention for documenting patterns inline
 - The behavior in interactive vs. non-interactive modes
 - How to temporarily disable (comment out patterns, or start pi with `--no-extensions`)
+- Accepted limitations (no symlink resolution, no shell parsing for command substitution)
+
+**`.pi/extensions/sensitive-file-guard.test.ts`** — Already included as Step 2.5. This file serves as living documentation of expected behavior for each edge case.
+
+**`docs/available-tasks.md`** — Add a note that `mise run pi` loads the sensitive file guard, so pi commands through mise are protected.
+
+**SKILL.md (optional)** — Consider creating `.claude/skills/sensitive-file-guard/SKILL.md` so pi itself understands the guard's behavior. Without this, pi may try to read a blocked file, receive the block reason in the next turn, and need to re-plan. A SKILL.md pre-loads this knowledge so pi avoids attempting blocked paths proactively. This is a nice-to-have, not required for the initial implementation.

 **No other documentation files need updates.** This is a pi-level concern, not an application architecture concern.
 <!-- SECTION:PLAN:END -->