Use safer env-var expansion where available

README.md

@@ -211,15 +211,19 @@ Additionally, volumes may be specified via the agent environment variable `BUILD
 
 When set to true, it will activate interpolation of variables in the elements within the `push` configuration array. When turned off (the default), attempting to use variables will fail as the literal `$VARIABLE_NAME` string will be used as the target..
 
-:warning: **Important:** this is considered an unsafe option as the most compatible way to achieve this is to run the strings through `eval` which could lead to arbitrary code execution or information leaking if you don't have complete control of the pipeline
+:warning: **Important:** this is considered an unsafe option as the most compatible way to achieve this is to run the strings through `eval` which could lead to arbitrary code execution or information leaking if you don't have complete control of the pipeline. On a system with `envsubst` available, that tool will be used in preference to `eval`. See `expand-vars-allowlist` to restrict the variables allowed to be substituted when using `envsubst`.
 
 Note that rules regarding [environment variable interpolation](https://buildkite.com/docs/pipelines/environment-variables#runtime-variable-interpolation) apply here. That means that `$VARIABLE_NAME` is resolved at pipeline upload time, whereas `$$VARIABLE_NAME` will be at run time. All things being equal, you likely want to use `$$VARIABLE_NAME` on the variables mentioned in this option.
 
+#### `expand-vars-allowlist` (push or run, string, when using one of the `expand-*-vars` options, and on a system with `envsubst`)
+
+This variable is used to define the commands `SHELL-FORMAT` argument to `envsubst`, which defines the environment variable allowlist so that unexpected interpolations are not permitted. It is only relevant when using `expand-push-vars` or `expand-volume-vars`
+
 #### `expand-volume-vars` (run only, boolean, unsafe)
 
 When set to true, it will activate interpolation of variables in the elements of the `volumes` configuration array. When turned off (the default), attempting to use variables will fail as the literal `$VARIABLE_NAME` string will be passed to the `-v` option.
 
-:warning: **Important:** this is considered an unsafe option as the most compatible way to achieve this is to run the strings through `eval` which could lead to arbitrary code execution or information leaking if you don't have complete control of the pipeline
+:warning: **Important:** this is considered an unsafe option as the most compatible way to achieve this is to run the strings through `eval` which could lead to arbitrary code execution or information leaking if you don't have complete control of the pipeline. On a system with `envsubst` available, that tool will be used in preference to `eval`. See `expand-vars-allowlist` to restrict the variables allowed to be substituted when using `envsubst`.
 
 Note that rules regarding [environment variable interpolation](https://buildkite.com/docs/pipelines/environment-variables#runtime-variable-interpolation) apply here. That means that `$VARIABLE_NAME` is resolved at pipeline upload time, whereas `$$VARIABLE_NAME` will be at run time. All things being equal, you likely want to use `$$VARIABLE_NAME` on the variables mentioned in this option.
 

commands/push.sh

@@ -41,7 +41,7 @@ prebuilt_image_namespace="$(plugin_read_config PREBUILT_IMAGE_NAMESPACE 'docker-
 # Then we figure out what to push, and where
 for line in $(plugin_read_list PUSH) ; do
   if [[ "$(plugin_read_config EXPAND_PUSH_VARS 'false')" == "true" ]]; then
-    push_target=$(eval echo "$line")
+    push_target="$(expand_var "$line")"
   else
     push_target="$line"
   fi

lib/run.bash

@@ -92,7 +92,7 @@ expand_relative_volume_path() {
   local path
 
   if [[ "$(plugin_read_config EXPAND_VOLUME_VARS 'false')" == "true" ]]; then
-    path=$(eval echo "$1")
+    path="$(expand_var "$1")"
   else
     path="$1"
   fi

lib/shared.bash

@@ -322,6 +322,31 @@ function retry {
   done
 }
 
+# Expands the env vars in a string, using envsubst if present, falling back to
+# eval when it's missing.
+function expand_var() {
+  # Try to use the safest approach possible
+  if command -v envsubst > /dev/null; then
+    expand_var_with_envsubst "$@"
+  else
+    expand_var_with_eval "$1"
+  fi
+}
+
+function expand_var_with_envsubst() {
+  allowlist="$(plugin_read_config 'EXPAND_VARS_ALLOWLIST' '')"
+  if [[ "$allowlist" == "" ]]; then
+    envsubst <<<"$1"
+  else
+    envsubst "$allowlist" <<<"$1"
+  fi
+}
+
+function expand_var_with_eval() {
+  echo "$(eval echo "$1")"
+}
+
+
 function is_windows() {
   [[ "$OSTYPE" =~ ^(win|msys|cygwin) ]]
 }

plugin.yml

@@ -96,6 +96,8 @@ configuration:
       type: string
     expand-push-vars:
       type: boolean
+    expand-vars-allowlist:
+      type: string
     expand-volume-vars:
       type: boolean
     graceful-shutdown:
@@ -203,6 +205,7 @@ configuration:
     env-propagation-list: [ run ]
     environment: [ run ]
     expand-push-vars: [ push ]
+    expand-vars-allowlist: [ push, run ]
     expand-volume-vars: [ volumes ]
     graceful-shutdown: [ run ]
     leave-volumes: [ run ]

tests/shared_lib.bats

@@ -0,0 +1,89 @@
+#!/usr/bin/env bats
+
+load "${BATS_PLUGIN_PATH}/load.bash"
+load '../lib/shared.bash'
+
+@test "expand_var works" {
+  export MY_VAR="llamas"
+  MY_STRING="foo:bar:\$MY_VAR"
+
+  run expand_var "$MY_STRING"
+
+  assert_success
+  assert_output "foo:bar:llamas"
+}
+
+@test "expand_var works via envsubst" {
+  export MY_VAR="llamas"
+  MY_STRING="foo:bar:\$MY_VAR"
+
+  ENVSUB_STUB_STDIN="$BATS_TEST_TMPDIR/envsubst_input"
+  stub envsubst  "cat > '$ENVSUB_STUB_STDIN'; echo 'foo:bar:llamas'"
+
+  run expand_var_with_envsubst "$MY_STRING"
+
+  assert_success
+  assert_output "foo:bar:llamas"
+
+  run cat "$ENVSUB_STUB_STDIN"
+
+  assert_success
+  assert_output "$MY_STRING"
+
+  unstub envsubst
+}
+
+@test "expand_var works via envsubst with an allowlist" {
+  export MY_VAR="llamas"
+  MY_STRING="foo:bar:\$MY_VAR"
+  ALLOWLIST='$MY_VAR'
+  export BUILDKITE_PLUGIN_DOCKER_COMPOSE_EXPAND_VARS_ALLOWLIST="$ALLOWLIST"
+
+  ENVSUB_STUB_STDIN="$BATS_TEST_TMPDIR/envsubst_input"
+  stub envsubst  "'$ALLOWLIST' : cat > '$ENVSUB_STUB_STDIN'; echo 'foo:bar:llamas'"
+
+  run expand_var_with_envsubst "$MY_STRING"
+
+  assert_success
+  assert_output "foo:bar:llamas"
+
+  run cat "$ENVSUB_STUB_STDIN"
+
+  assert_success
+  assert_output "$MY_STRING"
+
+  unstub envsubst
+}
+
+@test "expand_var works via envsubst with an allowlist not including the var" {
+  export MY_VAR="llamas"
+  export MY_OTHER_VAR="more_llamas"
+  MY_STRING="foo:bar:\$MY_VAR:\$MY_OTHER_VAR"
+  ALLOWLIST='$MY_OTHER_VAR'
+  export BUILDKITE_PLUGIN_DOCKER_COMPOSE_EXPAND_VARS_ALLOWLIST="$ALLOWLIST"
+
+  ENVSUB_STUB_STDIN="$BATS_TEST_TMPDIR/envsubst_input"
+  stub envsubst  "'$ALLOWLIST' : cat > '$ENVSUB_STUB_STDIN'; echo 'foo:bar:\$MY_VAR:more_llamas'"
+
+  run expand_var_with_envsubst "$MY_STRING"
+
+  assert_success
+  assert_output "foo:bar:\$MY_VAR:more_llamas"
+
+  run cat "$ENVSUB_STUB_STDIN"
+
+  assert_success
+  assert_output "$MY_STRING"
+
+  unstub envsubst
+}
+
+@test "expand_var works via eval" {
+  export MY_VAR="llamas"
+  MY_STRING="foo:bar:\$MY_VAR"
+
+  run expand_var_with_eval "$MY_STRING"
+
+  assert_success
+  assert_output "foo:bar:llamas"
+}