awalsh128/cache-apt-pkgs-action

Multiline string for packages breaks everything

panzi opened this issue · 1 comments

panzi commented

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}")"

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.