Skip to content

Security: replace Bash LLM tool with read-only, shell-free tools - #5

Merged
bilikaz merged 1 commit into
mainfrom
read-only-llm-tools
Jun 10, 2026
Merged

bilikaz merged 1 commit into
mainfrom
read-only-llm-tools

Conversation

@bilikaz

@bilikaz bilikaz commented Jun 10, 2026

Copy link
Copy Markdown
Owner

The review input is untrusted PR content while the container holds live VCS + LLM credentials, so bash -lc was arbitrary command execution one prompt injection away. The LLM tool set is now Read (with start_line/lines paging), Grep, Ls, Glob, Tail — structured args, never a shell; Grep spawns the grep binary with an argv array so model input stays inert data. Decision recorded in ADR-0014; ARCHITECTURE.md pattern 4.11 carries the invariant for future tools.

Also:

  • pnpm build now wipes dist/ before compiling, so deleted sources can't leave stale artifacts (flushes the orphaned dist bash.js / shared.js)
  • import shared docs contributed by the harness repo: ADR-0000 (ADR scope gate), conventions i18n / react / constants-and-identifiers, and the CLAUDE.md working procedure
  • ignore .claude/ local config

The review input is untrusted PR content while the container holds live
VCS + LLM credentials, so `bash -lc` was arbitrary command execution one
prompt injection away. The LLM tool set is now Read (with start_line/lines
paging), Grep, Ls, Glob, Tail — structured args, never a shell; Grep spawns
the grep binary with an argv array so model input stays inert data.
Decision recorded in ADR-0014; ARCHITECTURE.md pattern 4.11 carries the
invariant for future tools.

Also:
- `pnpm build` now wipes dist/ before compiling, so deleted sources can't
  leave stale artifacts (flushes the orphaned dist bash.js / shared.js)
- import shared docs contributed by the harness repo: ADR-0000 (ADR scope
  gate), conventions i18n / react / constants-and-identifiers, and the
  CLAUDE.md working procedure
- ignore .claude/ local config

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@bilikaz bilikaz left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 informational finding(s) (nits / suggestions, not blocking — listed here as summary, not tracked as individual threads):

  • src/llm/tools/grep.ts — Good use of -e to treat pattern as data rather than a flag. This correctly handles patterns starting with -. The argv array approach prevents shell injection.
  • src/llm/tools/read.ts — The start_line validation correctly handles past-end-of-file cases. The error message is clear and actionable.
  • src/llm/tools/index.ts — The tool registry correctly lists all five read-only tools. The comment explaining why Bash was removed is valuable for future maintainers.
  • package.json — The build script now wipes dist/ before compiling, preventing stale artifacts from deleted sources. Consider extracting this to a separate clean script if the build command grows further.
  • docs/adr/0014-llm-tools-read-only-no-shell.md — ADR-0014 clearly documents the security rationale and the three options considered. The consequences section honestly notes what's lost (git log/blame/show) and the path forward if needed.

@bilikaz
bilikaz merged commit 0f262ed into main Jun 10, 2026
2 checks passed
@bilikaz
bilikaz deleted the read-only-llm-tools branch June 10, 2026 17:01
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