-
Notifications
You must be signed in to change notification settings - Fork 7k
feat: glob pattern-based external_directory permission #5841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
|
Great to see PR #5841 addressing the core read/write split! We've been working on a related design (not yet posted) that extends this further with pattern-based path permissions (like "~/reference/**": "allow") and TUI folder-level approvals. Happy to share proposed design doc if useful as a follow-on enhancement.
{
"external_directory": {
"read": {
"~/reference/**": "allow",
"~/.ssh/**": "deny",
"*": "ask"
},
"write": {
"/tmp/**": "allow",
"*": "deny"
}
}
} |
|
@3leapsdave Great to see too ;) Thanks for sharing diagram too, I'll try to implement with given (glob-based)syntax for even further granularity |
6604143 to
c5c7b3f
Compare
|
@fpdy (and maintainers) — I’ve posted the full design write-up as a gist for easier reference: https://gist.github.com/3leapsdave/5b7cf0980af70b8b07af968bac4b4371 It consolidates a few follow-up hardening items that seem relevant to the current implementation:
Happy to help with follow-on commits or a follow-up PR for any of the above — just point me at what you’d like tackled first. @fpdy (and maintainers) — I’ve posted the full design write-up as a gist for easier reference:
In particular, one thing that stood out to me: when a pattern-map is used and there’s no match (and no Other follow-ups captured in the gist:
Happy to help with follow-on commits or a follow-up PR for any of the above — just point me at what you’d like tackled first. |
|
@3leapsdave Thanks again for yr kindness, your point (about the implicit behavior when there’s no catch-all) is exactly what I was concerned about too. Personally, I also think returning That said, the current behavior ( |
Summary
Fixes #5395 - Extends the
external_directorypermission with:readandwritepermissions for external directory accessConfiguration Syntax
Simple permission (backward compatible)
{ "permission": { "external_directory": "allow" } }Read/write split
{ "permission": { "external_directory": { "read": "allow", "write": "deny" } } }Glob pattern-based
{ "permission": { "external_directory": { "read": { "~/reference/**": "allow", "~/.ssh/**": "deny", "*": "ask" }, "write": { "/tmp/**": "allow", "*": "deny" } } } }Note on pattern evaluation:
*serves as catch-all default (evaluated last)~expands to home directoryTool Behavior
Note on
bashtoolUses
writepermission conservatively since bash commands can potentially modify files.Tests
bash.test.tsread.test.tswith tests for external directory read permissionsAppendix: Mermaid Flowchart on pattern evaluation
flowchart TD A[Receive file path] --> B{Is permission a string?} B -->|Yes| C[Return that value] B -->|No| D{Is read/write a string?} D -->|Yes| E[Return that value] D -->|No| F[Iterate through pattern map] F --> G{Is current pattern '*'?} G -->|Yes| H[Skip and continue to next] G -->|No| I{Does pattern match?} I -->|Yes| J[Return that value] I -->|No| H H --> K{More patterns remaining?} K -->|Yes| G K -->|No| L{Does map have '*' key?} L -->|Yes| M["Return map['*']"] L -->|No| N[Return undefined = evaluated as allowed]