Skip to content

Conversation

@n8henrie
Copy link
Member

  • remove sensitive files ASAP
  • add traps to also ensure their removal if script fails (belt and
    suspenders)
  • add -- to rm calls to avoid potential risks with hyphenated
    filenames

- remove sensitive files ASAP
- add traps to also ensure their removal if script fails (belt and
  suspenders)
- add `--` to `rm` calls to avoid potential risks with hyphenated
  filenames

Clean up $RUNNER_TEMP

Should ensure that $KEYCHAIN_PATH and $CERTIFICATE_PATH are both cleaned
up

chmod -> umask
@pjrobertson
Copy link
Member

Cool, have you got a ref for this trap and rm -- stuff? LGTM either way, feel free to merge

@n8henrie n8henrie merged commit d1c3d9b into main Apr 29, 2025
4 checks passed
@pjrobertson pjrobertson deleted the ci branch April 29, 2025 16:29
@n8henrie
Copy link
Member Author

Cool, have you got a ref for this trap and rm -- stuff?

Nothing specific, just stuff I've picked up over time. This should demonstrate the relevant issues:

  1. bash should generally be set to exit with an error upon a failing command
  2. this behavior means the cleanup logic may not run (if an error happens first)
  3. traps can ensure that sensitive stuff is cleaned up even if an early exit happens due to an error
  4. filenames (or variable contents) that start with - will be interpreted as flags to your commands if you're not careful
  • many commands support -- which specifies that "everything after this is a filename, not a flag"
  • a filename of --no-preserve-root (or envvar or what have you ) can give you a very bad time if you rm *
  • this is mitigated by using rm -- * (or rm -- "${MYVAR}" instead of rm "${MYVAR}"
#!/usr/bin/env bash

set -Eeu -o pipefail
set -x

rmdemo() {
  (
    umask 277
    touch needs_-f_to_remove.txt
  )
  local files=(needs_-f_to_remove.txt)
  # this will prompt you if you *really* want to remove due to 0400 perms
  rm "${files[@]}"

  ################################################################################
  (
    umask 277
    touch whups.txt
  )
  local gotcha=(-f whups.txt)
  # this will **not** prompt you as expected!
  # now just imagine deleting an envvar with scary values
  # `MY_ENV_DIR=(-rf --no-preserve-root /)
  rm "${gotcha[@]}"

  ################################################################################
  mkdir -p scary_glob
  pushd scary_glob
  (
    umask 277
    touch scary.txt
  )
  # create a file named `-f`
  : > -f

  # this `rm *` considers the file named `-f` to be a flag and will not prompt
  # Danger Will Robinson!
  rm *
  popd

  ################################################################################
  (
    umask 277
    touch also_needs_-f_to_remove.txt
  )
  local fixed=(-f also_needs_-f_to_remove.txt)
  # `-f` is considered a filename, not a flag, so will also prompt for removal
  rm -- "${fixed[@]}"
}

main() {
  # don't refuse to run the rest of the demo when these have expected errors
  rmdemo || true

  echo supersecret > yestrap.txt
  trap 'rm -f -- yestrap.txt' EXIT

  echo supersecret > notrap.txt

  # Force an error condition
  false

  # clean up sensitive files
  rm -- yestrap.txt
  rm -- notrap.txt
}
main "$@"

Hope that makes sense!

@pjrobertson
Copy link
Member

Thanks for the explanation, makes sense!

@AucaCoyan
Copy link

Thank you for the detailed explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants