feat: even more precision for bash steps in github-env - #208
Conversation
| let bash = tree_sitter_bash::LANGUAGE; | ||
| let mut parser = Parser::new(); |
There was a problem hiding this comment.
Nit: creating the parser with each evaluation is probably slower than necessary; we could probably reuse parser by making it a property of GitHubEnv that gets loaded when the audit itself is loaded with GitHubEnv::new 🙂
| if let StepBody::Run { run, .. } = &step.deref().body { | ||
| if Self::uses_github_environment(run) { | ||
| if let StepBody::Run { run, shell, .. } = &step.deref().body { | ||
| let interpreter = shell.clone().unwrap_or("bash".into()); |
There was a problem hiding this comment.
This is unfortunately not guaranteed to be the right default, since Windows runners default to pwsh. I think what we need to do is look at the surrounding NormalJob::runs_on and maybe add some kind of RunsOn::default_shell() helper. I'll look into that a bit.
(Nonblocker, since this is still improving on the precision of the current approach 🙂)
There was a problem hiding this comment.
Actually, it's a bit more annoying than even that: we need to determine the step's shell via the following precedence:
- defaults.run.shell
- jobs.<job_id>.defaults.run.shell
- jobs.<job_id>.steps[*].shell
- If none of the above are set, use the default shell implied by the runner from
runs-on(which might be indeterminate in the self-hosted runner case)
There was a problem hiding this comment.
@woodruffw I totally missed pwsh on Windows as the default, I never used Windows runners and I though that bash was the default (via WLS). Another day, another learning I guess 🙂
There was a problem hiding this comment.
I forget about too 🙂 -- the changes in #213 should enable this, but I'll rebase off of your work so that it's not a blocker here.
|
|
||
| #[test] | ||
| fn test_shell_patterns() { | ||
| fn test_exploitable_bash_patterns() { |
There was a problem hiding this comment.
I think we can fold these two test functions into one, with (case, expected: bool) as the parametrization 🙂
|
@woodruffw I lack the energy to work on anything else today, I'll resume the work on this PR tomorrow, addressing all your suggestions, and perhaps making the most of #213 🙂 Meanwhile, thanks again for the great review (as usual), it's a pleasure contributing to and learning from this project ❤️ |
|
Thank you too! And there's no rush or urgency at all, it's all supposed to be open source fun 🙂 |
79fd0d6 to
bc0e914
Compare
Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>
woodruffw
left a comment
woodruffw
left a comment
There was a problem hiding this comment.
LGTM, thanks a ton @ubiratansoares!
2 participants
A follow-up for #197
Here we leverage tree-sitter-bash rather than regexes to match usages of
$GITHUB_ENVwithin a Bash script body.I took a simple approach, getting the full expansion for a
file_redirectorpipelineand matching inner contents. Perhaps I'm missing some corner cases, but at least we have a starting point to iterate over 🙂I also added some additional test cases (comments) and prepared the implementation to match against other types of shells in upcoming PRs.