Skip to content
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

Multiline string for packages breaks everything #88

Closed
panzi opened this issue Jan 19, 2023 · 1 comment
Closed

Multiline string for packages breaks everything #88

panzi opened this issue Jan 19, 2023 · 1 comment
Assignees
Labels
duplicate This issue or pull request already exists

Comments

@panzi
Copy link

panzi commented Jan 19, 2023

If you have a lot of packages and use a multiline string for the packages parameter the setup fails, because bash attempts top run the second line as a command.

Instead of directly using the GitHub action variable interpolation you should pass the parameters as environment variables in order to prevent breakage (and command injection):

runs:
  using: "composite"
  steps:
    - id: pre-cache
      run: |
        ${GITHUB_ACTION_PATH}/pre_cache_action.sh \
          ~/cache-apt-pkgs \
          "$VERSION" \
          "$EXEC_INSTALL_SCRIPTS" \
          "$DEBUG" \
          "$PACKAGES"
        echo "CACHE_KEY=$(cat ~/cache-apt-pkgs/cache_key.md5)" >> $GITHUB_ENV
      shell: bash
      env:
         VERSION: "${{ inputs.version }}"
         EXEC_INSTALL_SCRIPTS: "${{ inputs.execute_install_scripts }}"
         DEBUG: "${{ inputs.debug }}"
         PACKAGES: "${{ inputs.packages }}"

Change here:

- id: pre-cache
run: |
${GITHUB_ACTION_PATH}/pre_cache_action.sh \
~/cache-apt-pkgs \
"${{ inputs.version }}" \
"${{ inputs.execute_install_scripts }}" \
"${{ inputs.debug }}" \
${{ inputs.packages }}
echo "CACHE_KEY=$(cat ~/cache-apt-pkgs/cache_key.md5)" >> $GITHUB_ENV

- id: post-cache
run: |
${GITHUB_ACTION_PATH}/post_cache_action.sh \
~/cache-apt-pkgs \
/ \
"${{ steps.load-cache.outputs.cache-hit }}" \
"${{ inputs.execute_install_scripts }}" \
"${{ inputs.debug }}" \
${{ inputs.packages }}
function create_list { local list=$(cat ~/cache-apt-pkgs/manifest_${1}.log | tr '\n' ','); echo ${list:0:-1}; };
echo "package-version-list=$(create_list main)" >> $GITHUB_OUTPUT
echo "all-package-version-list=$(create_list all)" >> $GITHUB_OUTPUT
shell: bash

Note that simply adding " " around ${{ inputs.packages }} in the run command will still allow command injection if someone uses a " in the packages parameter. So using env vars is cleaner.

However, this then passes the packages as a single parameter, but since you then do this anyway it shouldn't matter (in fact make that simpler, since you merge it into a single string anyway):

input_packages="${@:5}"
# Trim commas, excess spaces, and sort.
packages="$(normalize_package_list "${input_packages}")"

packages="${@:6}"
if [ "$cache_hit" == true ]; then
${script_dir}/restore_pkgs.sh "${cache_dir}" "${cache_restore_root}" "${execute_install_scripts}" "${debug}"
else
${script_dir}/install_and_cache_pkgs.sh "${cache_dir}" "${debug}" ${packages}
fi

input_packages="${@:3}"
# Trim commas, excess spaces, and sort.
normalized_packages="$(normalize_package_list "${input_packages}")"

@awalsh128
Copy link
Owner

Sorry for the lag here. Thank you for the detailed report. I have a duplicate issue open in #84. Going to dupe this against #84 since I already commited some test code under that.

@awalsh128 awalsh128 closed this as not planned Won't fix, can't repro, duplicate, stale Feb 4, 2023
awalsh128 added a commit that referenced this issue Feb 4, 2023
@awalsh128 awalsh128 added the duplicate This issue or pull request already exists label Feb 4, 2023
awalsh128 added a commit that referenced this issue Feb 4, 2023
* Address block style package issue #84 #88

* Use cache key for upload artifact name #89.

* Sync master back to dev. (#92)

* Fix if condition for upload-logs step (#87)

Previously the if condition was always evaluating to a truthy string
(e.g. 'false == "true"' or 'true == "true"') as the string comparison
(`== 'true'`) was not inside the expression syntax (`${{ }}`) and thus
being treated as a string rather than being evaluated.

* Introduce a force update value for reloading cache #82

---------

Co-authored-by: Leroy Hopson <[email protected]>

---------

Co-authored-by: Leroy Hopson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants