Use safer env-var expansion where available

[lparry]

Hey there,

This is a small follow-up pr to #493, that looks for envsubst on the host system, and when found opts to use that instead of eval for the string interpolation.

It also adds a expand-vars-allowlist configuration option which enables the stricter use of envsubst where only expected/permitted env vars are substituted and anything else is left alone, which I believe allows users to reduce the unsafe-ness of expand-push-vars if that's something they care about.

I've tested the scenarios for this in some real pipelines (✅), and have added some bats tests for the two new types of expansion, but I couldn't find a reliable way to stub the command built-in in order to test expand_var, or add another properly different example to push.bats. Happy to add them too if someone knows the secret sauce 🙏

edit: now also hooks in for expand-volume-vars too. One shared allowlist expand-vars-allowlist for both spots using envsubst over eval

[toote]

This is actually a very good idea but, in addition to the comments I left, I think that it should not be limited to the push-vars. It could be applied to all unsafe options that use eval. Which means that the allowlist option would need to be renamed to something more general. What do you think?

[lparry]

Thanks, that's good feedback. I will try and find some time to update this later in the week

[lparry]

Hi @toote, I've done all the suggested tidying up, and have now made expand-volume-vars use the same function so it'll use envsubst and a shared allowlist one systems where envsubst is present 🙏

[lparry]

It's occurred to me just now that some folk could potentially be leaning on that other pre-existing eval method in ways that had not occurred to me before

eg.

    plugins:
      - docker-compose: 
          expand-volume-vars: true
          build: target
          run: target
          volumes: 
            - "$(command -v oh-no):/bin/oh-no"

Assuming that's possible, I guess we'd want to tie this to an option rather than just finding it on the system. Not sure if it would make sense to have an option for each case, or just a single option that covers all cases.

What do you think?

[toote]

that is indeed a very good point... a good middle-ground could be changing the existing options to not be booleans but instead one of true/false/eval/envsubst so that users can decide what to do on each case. A modular implementation of that would be to change the expand_var function to accept the value of the corresponding setting and use the corresponding implementation. What do you think?

[lparry]

Sounds pretty sensible 👍 I'll find some time this week to update again

[pzeballos]

Hey @lparry! Thanks for your contributions here! Are you able to update this PR based on the previous recommendations? 🙏 thanks!