Merge pull request #165 from buildkite-plugins/use-xargs-for-tokenizing

Replace eval with xargs for parsing BUILDKITE_COMMAND

commands/run.sh

@@ -95,12 +95,12 @@ while IFS=$'\n' read -r vol ; do
   [[ -n "${vol:-}" ]] && run_params+=("-v" "$(expand_relative_volume_path "$vol")")
 done <<< "$(plugin_read_list VOLUMES)"
 
+# Parse BUILDKITE_DOCKER_DEFAULT_VOLUMES delimited by semi-colons, normalized to
+# ignore spaces and leading or trailing semi-colons
 IFS=';' read -r -a default_volumes <<< "${BUILDKITE_DOCKER_DEFAULT_VOLUMES:-}"
-for vol in "${default_volumes[@]:-}"
-do
-  # Trim all whitespace when checking for variable definition, handling cases
-  # with repeated delimiters.
-  [[ ! -z "${vol// }" ]] && run_params+=("-v" "$(expand_relative_volume_path "$vol")")
+for vol in "${default_volumes[@]:-}" ; do
+  trimmed_vol="$(echo -n "$vol" | sed -e 's/^[[:space:]]*//' | sed -e 's/[[:space:]]*$//')"
+  [[ -n "$trimmed_vol" ]] && run_params+=("-v" "$(expand_relative_volume_path "$trimmed_vol")")
 done
 
 # Optionally disable allocating a TTY
@@ -135,18 +135,21 @@ fi
 set +e
 
 (
-  # Reset bash to the default IFS with no glob expanding and no failing on error
+  # Reset bash to the default IFS
   unset IFS
-  set -f
 
-  # The eval statements below are used to allow $BUILDKITE_COMMAND to be interpolated correctly
-  # When paired with -f we ensure that it word splits correctly, e.g bash -c "pwd" should split
-  # into [bash, -c, "pwd"]. Eval ends up the simplest way to do this, and when paired with the
-  # set -f above we ensure globs aren't expanded (imagine a command like `cat *`, which bash would
-  # helpfully expand prior to passing it to docker-compose)
+  # Because we receive $BUILDKITE_COMMAND as a single string, it needs to be tokenized as a shell
+  # would to respect things like quoted strings. Previously we used eval to do this with some
+  # careful control around preventing glob expansion, but this ends up being difficult to get just
+  # right and also very risky. The new implementation uses xargs and printf to tokenize the string,
+  # which is portable and much harder to shoot ourselves in the foot 🎉
+
+  while IFS= read -rd '' token; do
+    [[ -n "$token" ]] && run_params+=("$token")
+  done < <(xargs printf '%s\0' <<< "$BUILDKITE_COMMAND")
 
   echo "+++ :docker: Running command in Docker Compose service: $run_service" >&2
-  eval "run_docker_compose \${run_params[@]} $BUILDKITE_COMMAND"
+  run_docker_compose "${run_params[@]}"
 )
 
 exitcode=$?

tests/run.bats

@@ -121,10 +121,11 @@ load '../lib/run'
   export BUILDKITE_PLUGIN_DOCKER_COMPOSE_ENV_1=MYENV
   export BUILDKITE_PLUGIN_DOCKER_COMPOSE_ENVIRONMENT_0=MYENV=2
   export BUILDKITE_PLUGIN_DOCKER_COMPOSE_ENVIRONMENT_1=MYENV
+  export BUILDKITE_PLUGIN_DOCKER_COMPOSE_ENVIRONMENT_2=ANOTHER="this is a long string with spaces; and semi-colons"
 
   stub docker-compose \
     "-f docker-compose.yml -p buildkite1111 build --pull myservice : echo built myservice" \
-    "-f docker-compose.yml -p buildkite1111 run --name buildkite1111_myservice_build_1 -e MYENV=0 -e MYENV -e MYENV=2 -e MYENV myservice pwd : echo ran myservice"
+    "-f docker-compose.yml -p buildkite1111 run --name buildkite1111_myservice_build_1 -e MYENV=0 -e MYENV -e MYENV=2 -e MYENV -e ANOTHER=this\ is\ a\ long\ string\ with\ spaces\;\ and\ semi-colons myservice pwd : echo ran myservice"
 
   stub buildkite-agent \
     "meta-data get docker-compose-plugin-built-image-tag-myservice : exit 1"