The permissions system is too YOLO - we need to add context to tool permission requests #3077
Replies: 5 comments 4 replies
-
|
I appreciate the comment and I do generally agree with this. Thanks for posting a discussion about it. One thing to keep in mind in the implementation: we’ll need to be sensitive about commands in redirections, pipelines, conditionals and so on, and well as flags and what not (for example, |
Beta Was this translation helpful? Give feedback.
-
Is |
Beta Was this translation helpful? Give feedback.
-
If I understand this correctly,
|
Beta Was this translation helpful? Give feedback.
-
|
Consider this scenario:
To avoid such trouble, I'm running Crush under Fence, and I expect to continue until a future version of Crush routes script execution through a similar sandbox by default. For protecting files, getting help from the operating system is the way to go, in my opinion. |
Beta Was this translation helpful? Give feedback.
-
|
The cross-product problem DanEble found (approve The scheme that's held up for me in production agent use is three independent layers:
One operational warning supporting meowgorithm's parsing point: text-level matching false-positives are real (a commit message containing "push" and "main" once tripped our push guard), so parse before matching wherever possible. I maintain a reference architecture documenting this pattern (impact-tier table + hook layers): https://github.com/jimy-r/agent-workspace-architecture/blob/main/PATTERNS.md#tier-by-mechanical-impact-not-by-tone |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
Hey so there has been a little bit of discussion around
allowed_commandsincluding a stale PR.I think though fundamentally the current permission system is basically just all or nothing, and for me (and I would hope for every one else too) this is just too much of a security dealbreaker to just be able to let a model go wild on my machine. I've already had one model nuke all the work I was doing, which was fortunately just me giving it a trial task to run at, but once you approve the bash tool for the duration of the session it has free reign to make whatever poor decisions it likes and for me that makes crush an unusable tool as it exists now for agentic workflows.
I usually work with the copilot cli which has this really well implemented, so I have taken inspiration from that to make crush a viable workflow tool for me. Here's the changes I have made and although I have a PR to submit for this, it's open for discussion and I can continue to use my fork - so I'm happy either way.
What This Does
It replaces the old blanket "Allow for session" permission model with granular, context-aware approvals for bash commands.
How it works
When you run a command like
cd /tmp && ls ./src && pwd, the system now:•
command:cd,command:ls,command:pwd•
path:/tmp,path:/repo/src• Running
ls /repo/srcalone → ✅ auto-approved (bothcommand:lsandpath:/repo/srcwere approved)• Running
pwdalone → ✅ auto-approved (command:pwdwas approved)• Running
ls /other→ ❌ still prompts (path:/otherwas never approved)rm /tmp/xdoes not auto-approverm /repo.allowed_commandsandallowed_pathsincrush.jsonwork the same way, using prefix matching (command:gosatisfiescommand:go test).Unsafe constructs (command substitution $() , backticks, redirects, sh -c , eval , loops, conditionals) are fail-closed — they emit an opaque command!:... token that can only be approved by exact match, never auto-approved. I will probably try to improve on this if I find it occurs frequently enough and without good reason but these scenarios only usually trigger for me when doing some analysis / data collection exercise, or a refactor that affects many files, and in these cases I don't mind just hitting approve.
Changes to the Permission Package
The crux of the changes to the permission system is effectively just adding optional context to the tool permission request and recording that context when it is approved for the duration of the session.
What Changed
Request model —
CreatePermissionRequestandPermissionRequestnow carry aContexts []stringfield. This is opaque to the service — it just checks whether every context tokenis satisfied. Tools that don't use it leave it empty.
Session grants —
PermissionKeygained aContext stringfield. When a context-aware toolis approved for the session, one key is stored per context token (
PermissionKey{SessionID, ToolName, Action, Context}). Legacy tools still use the old key shape (PermissionKey{SessionID, ToolName, Action, Path}).Resolution flow —
Request()now has two auto-approval paths:• If
Contextsis non-empty: every token must be satisfied (by config tokens or priorsession grants)
• If
Contextsis empty: falls back to legacy {ToolName, Action, Path} key lookupGrant recording —
GrantPersistentrecords one key per context token whenContextsis populated, otherwise records the legacy key. ThePathfield is intentionally omitted from contextual keys because location semantics are captured by path: tokens.Config matching —
allowed_commandsandallowed_pathsfromcrush.jsonare translated at startup intocommand:andpath:tokens and checked first incontextSatisfied().Impact on
bashtoolThis is the primary beneficiary. Before, "Allow for session" on
cd /tmp && ls ./src && pwdgranted just any use of the bash tool.Now, each constituent command and path is independently reusable. Approve the chain once, and
ls /repo/srcruns alone later with no prompt. This is the intended behaviour.Impact on Other Tools
For
edit,write,view,multiedit, etc. — no change at all. They never populateContexts, so they fall through to the legacy{ToolName, Action, Path}lookup. The old "Allow for session" behaviour is preserved because the legacy lookup is still checked in step 6 ofRequest().What It Enables
This design is tool-agnostic. Any tool can start using
Contextswithout changing the permission service. If a future tool wants to grant file operations by directory, or Git operations by repository, it just emits the appropriate tokens. The service doesn't need to know what the tokens mean.crush.jsonexampleThis is the relevant parts of the config I use
{ "$schema": "https://charm.land/crush.json", "permissions": { "allowed_tools": [ "multiedit", "view", "ls", "grep", "write", "edit" ], "allowed_commands": [ "cd", "ls", "wc", "mkdir", "sed", "awk", "grep", "head", "tail", "find", "xargs", "tee", "touch", "go fmt", "go build", "go test", "go run", "go doc", "go list", "go vet", "go tool", "make", "yarn run", "yarn build", "yarn test", "yarn tsc", "git status", "git log", "git diff", "git show" ], "allowed_paths": [ "/tmp" ] }The PR for my implemented changes is open here: #3076
It'll need some more work, but I figured I'd open this discussion and see if we agree this approach is the way forward, and come up with some actions that need to be taken to either adopt this as it is, or design the system as intended.
What would need to be done after this is probably get
editandmultieditto providepath:token context so that file modifications are limited to allowed paths; and also re-use pre-approved paths from the bash tool, and vice-versa.I think also adding the working directory to the pre-approved context on start up would be expected implied behaviour too.
This was done relatively quickly to just give me something workable so I can use crush to do my other work. Happy to find some time to finish it off though.
Beta Was this translation helpful? Give feedback.
All reactions