fix: use PowerShell to parse PowerShell #7607
Conversation
| PowershellParseOutcome::Failed | ||
| } | ||
|
|
||
| fn powershell_candidates() -> &'static [&'static str] { |
There was a problem hiding this comment.
should we use shell detection here?
bolinfest
force-pushed
the
bolinfest-parse-powershell
branch
from
2941ceb to
7bf5726
Compare
…7893) I am trying to tighten up some of our logic around PowerShell over in #7607 and it would be helpful to be more precise about `pwsh.exe` versus `powershell.exe`, as they do not accept the exact same input language. To that end, this PR introduces utilities for detecting each on the system. I think we also want to update `get_user_shell_path()` to return PowerShell instead of `None` on Windows, but we'll consider that in a separate PR since it may require more testing.
bolinfest
force-pushed
the
bolinfest-parse-powershell
branch
6 times, most recently
from
7a03f68 to
7c88bee
Compare
change json depth from 3 to 10 move the powershell script to a separate file remove unnecessary operator checks use pwsh vs powershell consistently clean up lint warnings
bolinfest
force-pushed
the
bolinfest-parse-powershell
branch
from
7c88bee to
62c9e30
Compare
| "-Command", | ||
| "Get-Item foo.rs | Select-Object Length", | ||
| ]))); | ||
| let Some(pwsh) = try_find_pwsh_executable_blocking() else { |
There was a problem hiding this comment.
nit - maybe something for me to fix since it's pre-existing. This should probably be multiple tests not a bunch of asserts in a single test
There was a problem hiding this comment.
Thanks: it would be great if you could help improve this to your satisfaction post-merge!
| commands | ||
| .iter() | ||
| .all(|cmd| is_safe_powershell_command(cmd.as_slice())); | ||
| .all(|cmd| is_safe_powershell_command(cmd.as_slice())) |
There was a problem hiding this comment.
probably not a big concern in practice, but if the new powershell-based parser fails to parse an otherwise known-safe command, we would reject it. You might consider allowing commands that we know for sure are safe, based on our allow-list, even if the AST-based parser fails
There was a problem hiding this comment.
If the PowerShell-based parser fails to parse the input, then I don't think there's a reason we can (or should) assume it's safe.
Previous to this PR, we used a hand-rolled PowerShell parser in
windows_safe_commands.rsto take a&strof PowerShell script see if it is equivalent to a list ofexecvp(3)invocations, and if so, we then test each usingis_safe_powershell_command()to determine if the overall command is safe:codex/codex-rs/core/src/command_safety/windows_safe_commands.rs
Lines 89 to 98 in 6e6338a
Unfortunately, our PowerShell parser did not recognize
@(...)as a special construct, so it was treated as an ordinary token. This meant that the following would erroneously be considered "safe:"The fix introduced in this PR is to do something comparable what we do for Bash/Zsh, which is to use a "proper" parser to derive the list of
execvp(3)calls. For Bash/Zsh, we rely on https://crates.io/crates/tree-sitter-bash, but there does not appear to be a crate of comparable quality for parsing PowerShell statically (https://github.com/airbus-cert/tree-sitter-powershell/ is the best thing I found).Instead, in this PR, we use a PowerShell script to parse the input PowerShell program to produce the AST.