feat(execute_bash): untrust commands based on env parameter #1999
Conversation
| let env = crate::platform::Env::new(); | ||
| if let Ok(untrusted_patterns) = env.get("Q_EXECUTE_BASH_UNTRUSTED_PATTERNS") { | ||
| let patterns: Vec<&str> = untrusted_patterns.split(',').map(|s| s.trim()).collect(); | ||
| if patterns.iter().any(|pattern| self.command.contains(pattern)) { |
There was a problem hiding this comment.
This feels really primitive. Is it sufficient to just check "string exists in string?" Is that how other tools work?
Is this really only settable by environment vs storing it in a configuration file?
There was a problem hiding this comment.
anything else in mind in terms of feature ?
from the env var perspective I was thinking that is a lightweight approach, considering the very specific problem I am trying to solve. Seems to me also quite common for CLI tools.
There was a problem hiding this comment.
I'd look to the core team, but I would not add this on as a quick, lightweight thing. It's a key feature of the tool and shouldn't be hidden in an environment variable. It should be integrated into settings, and likely needs a more thorough design. Once people rely on this, it will be very unsafe to alter it.
This design makes it global to all profiles and all folders, which may not be desirable.
Regex are difficult for people to write correctly, and it's not obvious from your example what the syntax is. Are /.../ required to interpret it as a regex? If I enter export Q_EXECUTE_BASH_UNTRUSTED_PATTERNS='rm .*' (without slashes), should I expect that to work, or will I quietly have no protection?
You wrote as the example /^\s*rm.*/. Does that mean that the entire string needs to match, and /rm/ won't work? (That is probably surprising and needs documentation.)
How do I include , in a regex? It looks like escaping it won't work.
If I enter the regex incorrectly, I believe this code will quietly just do matching instead without giving me an error. that seems very dangerous. It's easy to mess up and hard to know that you have.
I think this is a hard feature to get right, but I wouldn't do a half-measure on it. It's important, but better to have nothing than to make people think they have protection they do not have.
There was a problem hiding this comment.
you're right, doc will be needed.
You have some exemples in the unit test that should explain more the behavior. /^\srm./ will match anything starting by rm (or starting with some spaces followed by rm).
export Q_EXECUTE_BASH_UNTRUSTED_PATTERNS='rm .*' will not protect you from rm something.txt but only from rm .*
Currently, you're right, you cant include , in a regex. Is it possible to change that i think, using something different that a simple split on ,. So that's clearly in favor of a config file.
Can you think about a format that will be more usable ? Something like :
"patterns": [
{
"type": "string",
"value": "git push",
},
{
"type": "string",
"value": "delete",
},
{
"type": "regex",
"value": "^\\s*rm.*",
}
]
}```
for the type string for `git push` meaning `.*git.*push.*` in regex
…ted bash commands
Description of changes:
After discussing with some users it appears that it's relevant to let the tools execute_bash be trusted except for some specific commands (git push for example). It seems that more flexibility is needed around this tool to enjoy a real "vibe coding" experience :).
I propose to let the user define an env var with untrusted pattern commands that the tool will check before trust a command.
CX will be :
This proposed changes seems a bit specific to execute_bash, but, right now, I cannot figure more generics way.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.