fix: gate unattended commands at the subcommand level, not just the binary (#5808) - #5997
Merged
Conversation
…inary (#5808) The Layered Intelligence `cmd` lane runs strings from persistent config on a schedule with nobody watching. Its allowlist admitted whole binaries, and four of them are multi-purpose: `git reset --hard`, `git clean -fd`, `find . -delete`, `find . -exec`, `gh api -X POST` and `gh pr merge` all carry no shell metacharacter, so both the allowlist and the metacharacter filter waved them through — destroying uncommitted work, deleting files, running arbitrary binaries, or writing to the tracker with the operator's own credentials. `validateUnattendedCommand` now applies a per-binary subcommand check after the allowlist, keyed off the base command the same way the existing pm2 check is: - `git` — an allowlist of read-only verbs (log, show, status, diff, blame, describe, rev-parse, branch, tag, remote, config, ls-files, shortlog). Unknown verbs fail closed, and the three listing verbs are held to their listing forms (`git branch -D`, `git tag v1`, `git remote add`, and a `git config` write are all rejected). - `find` — the action flags (`-delete`, `-exec`/`-execdir`/`-ok`/`-okdir`, the `-fprint` family) are rejected anywhere in the args; inspection predicates are untouched. - `gh` / `glab` — only `<noun> list`, `<noun> view`, and `api` restricted to GET. Field flags are rejected too, since they make `api` an implicit POST. `ls`, `cat`, `head`, `tail`, `grep`, `wc`, `pwd` and `echo` have no write mode and stay binary-level. The operator-driven runner (`validateCommand` / POST /api/commands/execute) is unchanged — a human triggers and watches it — and the `trustShellSources` escape hatch still restores full-shell behavior for an install that opts in. Claude-Session: https://claude.ai/code/session_01Do7xpqX5RLHM78rnstRkzf
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
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.
Summary
The unattended Layered Intelligence
cmdlane runs command strings out of persistent, attacker-reachable config on a schedule with nobody watching. #5806 narrowed it to a read-only binary allowlist, which closed the remote-code-fetch/exec class — but the gate was binary-level, and four admitted binaries are multi-purpose.git reset --hard,git checkout .,git clean -fd,find . -delete,find . -exec …,gh api -X POST …andgh pr mergeall contain no shell metacharacter, so both the allowlist and the metacharacter filter waved them straight through — destroying uncommitted work in the app repo, deleting arbitrary files, running arbitrary binaries (find -execre-opened execution entirely), or writing to the tracker with the operator's own credentials.validateUnattendedCommandnow applies a per-binary subcommand check after the allowlist, keyed offbaseCommandthe same wayvalidateCommandalready keys its pm2 check — the shape matches the existing precedent rather than introducing a second mechanism.git— an allowlist (not a denylist; git grows verbs) of read-only subcommands:log,show,status,diff,blame,describe,rev-parse,branch,tag,remote,config,ls-files,shortlog. Unknown verbs fail closed. The three listing verbs keep only their listing forms, sogit branch -D main,git branch <new>,git tag v1,git remote add, and agit config <key> <value>write are all rejected whilegit branch -a,git tag -l v1,git remote -v,git remote show originandgit config --get user.namestill pass.find— the action flags (-delete,-exec,-execdir,-ok,-okdir,-fls,-fprint,-fprint0,-fprintf) are rejected anywhere in the args. Inspection predicates (-name,-type,-maxdepth,-print) are untouched.gh/glab— only<noun> list,<noun> view, andapilimited to GET. Field flags (-f,-F,--field,--raw-field,--input) are rejected too, because they makeapian implicit POST even with no-X.ls,cat,head,tail,grep,wc,pwdandechohave no write mode and deliberately stay binary-level.Scope held to the issue:
ALLOWED_COMMANDS,DANGEROUS_SHELL_CHARS,redactOutputandserver/routes/commands.jsare untouched; no binaries were added toUNATTENDED_READONLY_COMMANDS; the operator-driven runner (validateCommand/POST /api/commands/execute) is byte-identical, since a human triggers and watches it andgit committhere is legitimate; and the install-levelsettings.layeredIntelligence.trustShellSourcesescape hatch still restores full-shell behavior unchanged. TheUNATTENDED_READONLY_COMMANDSscope comment and thecommandSecurity.jsrow inserver/lib/README.mdnow point at the implementation instead of describing subcommand gating as tracked separately.Test plan
cd server && npm test— 1888 test files passed, 1 skipped, 0 failed; 38145 tests passed.server/lib/commandSecurity.test.jscover: the destructive git forms and an unknown-to-the-allowlist verb; the read-only git forms that must keep working; the mutating form of each listing verb; thefindaction flags (using the+terminator, since the{}/;form is already caught upstream by the metacharacter filter and would prove nothing new);gh/glabwrites including--method=PATCH, an implicit-POST field flag, and a malformed methodless-X; the read-only tracker commands; and a pin thatvalidateCommand('git commit …'),gh pr merge 1,git reset --hardandfind . -deleteall still pass on the operator lane.git diff HEAD~1as an accept case — that string is already rejected one layer earlier because~is inDANGEROUS_SHELL_CHARS, so the test usesgit diff --statand records why.Closes #5808