Fix Trivy action inputs leaking between invocations (#422) #454
Conversation
- Explicitly rm -f it at start and end of action - Also remove temporary test steps from action
d167969 to
a4fc06b
Compare
There was a problem hiding this comment.
@rvesse Thanks for your work!
LGTM
left small comment.
Can you also add examples with multiple few test runs (as example - #397)?
I do that in test repository (https://github.com/DmitriyLewen/test-trivy-action/actions)
you can do same.
@simar7 Can you also take a look?
I have a repo where we're wrapping our usage of Trivy up in a composite action which was what led me to discovering this bug and the various proposed fixes. I can likely get that made public tomorrow and add a link here when that's available |
Dump the generated environment variables file only when tests are run with actions debug logging
cf8c1ca to
c0352d3
Compare
This is done as long as they have a non-empty input value, or a non-empty default value.
c0352d3 to
732bcb3
Compare
|
@DmitriyLewen Forked your tests and modified the test workflow so it tests current Did spot an issue with my fix around environment variables not being set when they were the defaults, since the defaults for the action inputs don't necessarily match the defaults for the You can also see my fix in action over on my $dayjob wrapper action around our usage of Trivy - https://github.com/telicent-oss/trivy-action/actions/runs/14127012407 - where we're using a second invocation with |
There was a problem hiding this comment.
Looks very good for me.
@rvesse Thanks for your work, help and good testsing!
Left small comments.
Can you also add https://github.com/rvesse/test-trivy-action/actions/runs/14127078253 into PR description?
@simar7 Can you also take a look?
|
@rvesse thanks for the PR! Only one comment related to documentation: Old GitHub action runs are pruned for logs over a period of time. Therefore, we should document this in the readme of this project. Screenshots or other associated materials are fine. |
|
Thanks for the reviews, didn't get chance to work on this today, will tidy everything up as requested tomorrow |
- Remove unnecessary debug statements in the action used during testing - Additional explanatory comments - Fix to address case where caller explicitly injects environment variables, either via env: block on the action call or via GITHUB_ENV
93f8287 to
2895385
Compare
Noted in documenting this fix that what had been implemented deviated from the existing configuration priority documentation. Amended the implementation of the Action to try and restore that consistency.
2895385 to
830ec6f
Compare
| is the case by looking at the GitHub Actions step information, if the `env` section shown in your Actions output | ||
| contains `TRIVY_*` environment variables you did not explicitly set then you may be affected by this bug and should | ||
| upgrade to the latest Action version. | ||
|
|
There was a problem hiding this comment.
@simar7 Wasn't really sure how/what you wanted to document in the README for this PR, let me know if this is appropriate or if you had something else in mind?
There was a problem hiding this comment.
Maybe a screenshot showing Actions output with env leakage happening to help users spot if they're affected by this, similar to what I showed in the PR description?
There was a problem hiding this comment.
I think this is pretty good! Thanks.
| echo "$var_name=$input_value" >> $GITHUB_ENV | ||
| if [ ! -z "$input_value" && "$input_value" != "$default_value" ]; then | ||
| # If action was provided with explicit input by the caller set that | ||
| echo "export $var_name=$input_value" >> trivy_envs.txt |
There was a problem hiding this comment.
Ending up removing the extra logic for defaults I previously added as realised this wasn't needed, and would cause behaviour to deviate from configuration priority that is documented/intended for the action
The weird behaviour I'd seen in tests of defaults apparently not being honoured was actually due to the tests repository having a trivy.yaml file present which was automatically getting picked up and throwing off behaviour for the default output test
As discussed in #422 the current way that the Trivy action injects the GitHub Actions inputs to
trivyis by adding these to theGITHUB_ENVfile which is then used to populate the environment for all subsequent job steps. This leads to a problem where a subsequent call toaquasecurity/trivy-actionmay incorrectly pick up inputs from a previous call because those previous inputs are already set in the environment file, and the action only sets those environment variables to new values if the new inputs differ from the defaults.This PR takes an approach that @DmitriyLewen was already developing in his fork and finishes it up ready for review. Basically instead of writing the inputs to the
GITHUB_ENVfile it writes them, in the form ofexport <var>=<value>statements to a temporarytrivy_envs.txtfile, and has theentrypoint.shscriptsourcethat file. The file is cleaned up as part of the action running. This ensures that inputs from one call to the action cannot be leaked into a subsequent call to the action.Note that supersedes another approach proposed in #437 since as discussed there that approach still potentially leaks the Trivy action inputs across job steps. As noted in that PR the alternative approach in this PR also likely fixes other related issues #438 and #435
Testing
entrypoint.shscript does load thetrivy_envs.txtfile correctlytrivy-actioninvocations as we had previously noted. Now the only env variables we are seeing passed intotrivy-actionare the ones we explicitly add ourselves.master, the previous proposed fix and my proposed fix - https://github.com/rvesse/test-trivy-action/actions/runs/14216797998E.g. With current
master, environment leakage occurs:With the changes from this branch, environment leakage no longer occurs and all inputs are honoured as expected: