-
Notifications
You must be signed in to change notification settings - Fork 333
Fix Trivy action inputs leaking between invocations (#422) #454
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
Changes from all commits
05cf4b1
c40c353
e92b296
4b9dd90
8691ff7
624b908
a777679
1007f3a
5be77a0
036b675
a4fc06b
01b0d73
732bcb3
145a38b
830ec6f
de6e5d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -147,21 +147,41 @@ runs: | |
| env: | ||
| GITHUB_ACTION_PATH: ${{ github.action_path }} | ||
|
|
||
| # Create and Clear Trivy Envs file | ||
| # See #422 for context | ||
| - name: Clear Trivy Envs file | ||
| shell: bash | ||
| run: | | ||
| rm -f trivy_envs.txt | ||
| touch trivy_envs.txt | ||
|
|
||
| - name: Set Trivy environment variables | ||
| shell: bash | ||
| run: | | ||
| # Note: There is currently no way to distinguish between undefined variables and empty strings in GitHub Actions. | ||
| # This limitation affects how we handle default values and empty inputs. | ||
| # For more information, see: https://github.com/actions/runner/issues/924 | ||
|
|
||
| # Function to set environment variable only if the input is provided and different from default | ||
| # The following logic implements the configuration priority described in the README: | ||
| # | ||
| # Inputs | ||
| # Environment Variables | ||
| # Config File | ||
| # Defaults | ||
| # | ||
| # As noted above defaults are awkward to handle as GitHub Actions will inject those values as the input | ||
| # if the caller doesn't provide them, thus if the input matches the default we don't set it as we | ||
| # can't tell the difference. Plus if we did set it when it was the default value then it could potentially | ||
| # override an external environment variable, or something in the callers configuration file, which then wouldn't | ||
| # match the configuration priority that is documented. | ||
| set_env_var_if_provided() { | ||
| local var_name="$1" | ||
| local input_value="$2" | ||
| local default_value="$3" | ||
|
|
||
| if [ ! -z "$input_value" ] && [ "$input_value" != "$default_value" ]; then | ||
| echo "$var_name=$input_value" >> $GITHUB_ENV | ||
| # If action was provided with explicit input by the caller set that | ||
| echo "export $var_name=$input_value" >> trivy_envs.txt | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| fi | ||
| } | ||
|
|
||
|
|
@@ -202,3 +222,11 @@ runs: | |
|
|
||
| # For Trivy | ||
| TRIVY_CACHE_DIR: ${{ inputs.cache-dir }} # Always set | ||
|
|
||
| # Remove Trivy envs to keep envs within this action and avoid env leaks | ||
| # See #422 for context | ||
| - name: Remove Trivy Envs file | ||
| if: ${{ always() }} | ||
| shell: bash | ||
| run: | | ||
| rm -f trivy_envs.txt | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is pretty good! Thanks.