[CICD-583] Split FLAGS variable into an array (#32)

* Split FLAGS variable into an array

Rsync flags sometimes include whitespace in the flag's value. For
example, --filter=':- .gitignore' includes whitespace between the filter
rule and the file name. The single quotes surrounding the flag's value
are meant to prevent splitting. Unfortunately, when bash expands the
$FLAGS variable as part of the main rsync command it escapes the single
quotes and then splits on the whitespace between :- and .gitignore. This
was causing rsync to see the filter flag as two separate arguments,
--filter=:- and .gitignore.

Simply double-quoting $FLAGS string doesn't help because it prevents
splitting between flags when multiple are provided.

To fix this, we needed to split the FLAGS variable into an array while
respecting single quotes. This pre-split array can be passed to rsync
with double-quotes to prevent further splitting.

Using the --filter example from before, our new strategy will insert
--filter=':- .gitignore' as a single item in the FLAGS array which is then
passed to rsync as '--filter=:- .gitignore'.

* Add scripts to test FLAGS parsing and quoting strategies

.changeset/nervous-bobcats-fry.md

@@ -0,0 +1,5 @@
+---
+"@wpengine/site-deploy": patch
+---
+
+Fixes a bug that caused certain flags in the FLAGS option to be incorrectly parsed by rsync

Dockerfile

@@ -7,6 +7,7 @@ RUN apk update \
             php \
  && rm -rf /var/cache/apk/*
 # Add entrypoint and excludes
+ADD functions.sh /functions.sh
 ADD entrypoint.sh /entrypoint.sh
 ADD exclude.txt /exclude.txt
 ENTRYPOINT ["/entrypoint.sh"]

Makefile

@@ -21,3 +21,6 @@ version: build
 	docker image tag $(IMAGE) $(IMAGE_NAME):v$(MAJOR_VERSION) && \
 	docker image tag $(IMAGE) $(IMAGE_NAME):v$(MAJOR_VERSION).$(MINOR_VERSION) && \
 	docker image tag $(IMAGE) $(IMAGE_NAME):v$(MAJOR_VERSION).$(MINOR_VERSION).$(PATCH_VERSION)
+
+test:
+	./tests/test_functions.sh

README.md

@@ -20,13 +20,22 @@ You can use this image to deploy a site from your local machine.
 2. Change directories into the root of the local site you'd like to deploy.
 3. Create a `.env` file with the following variables, changing their values as needed.
 
+> [!WARNING]  
+> Since `docker run` does not strip double-quotes from variables in the .env file, we don't use them
+> to wrap entire variable values. Instead, we must use single or double-quotes around flag values that
+> contain whitespace to prevent splitting (`--filter=':= .gitignore'`). The `=` sign between the flag
+> and its value is also required (`--filter=':= .gitignore'` rather than `--filter ':= .gitignore'`).
+
 ```sh
-WPE_ENV=yourinstall # The target WP Engine install name.
+# Required. The target WP Engine install name.
+WPE_ENV=yourinstall
+# Optional. Default values shown.
 REMOTE_PATH=
 SRC_PATH=.
-PHP_LINT=TRUE
+PHP_LINT=FALSE
 CACHE_CLEAR=TRUE
 SCRIPT=
+FLAGS=-azvr --inplace --exclude='.*'
 ```
 
 3. Set an environment variable with your private SSH key, replacing the key file name with your own.

entrypoint.sh

@@ -2,13 +2,19 @@
 
 set -e
 
+# Get the directory of the current script
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+
+# Source the functions.sh file relative to the current script's location
+source "${SCRIPT_DIR}/functions.sh"
+
 validate() {
   # mandatory params
   : WPE_SSHG_KEY_PRIVATE="${WPE_SSHG_KEY_PRIVATE:?'WPE_SSHG_KEY_PRIVATE variable missing from Repo or Workspace variables.'}"
   # optional params
   : REMOTE_PATH="${REMOTE_PATH:=""}"
   : SRC_PATH="${SRC_PATH:="."}"
-  : FLAGS="${FLAGS:="-azvr --inplace --exclude=".*""}"
+  : FLAGS="${FLAGS:="-azvr --inplace --exclude='.*'"}"
   : PHP_LINT="${PHP_LINT:="FALSE"}"
   : CACHE_CLEAR="${CACHE_CLEAR:="TRUE"}"
   : SCRIPT="${SCRIPT:=""}"
@@ -33,8 +39,8 @@ setup_env() {
     else CICD_VENDOR="wpe_cicd"
   fi
 
-  echo "Deploying your code to:"
-  echo "${WPE_ENV_NAME}"
+  parse_flags "$FLAGS"
+  print_deployment_info
 
   WPE_SSH_HOST="${WPE_ENV_NAME}.ssh.wpengine.net"
   DIR_PATH="${REMOTE_PATH}"
@@ -105,8 +111,9 @@ sync_files() {
   ssh -nNf -v -i "${WPE_SSHG_KEY_PRIVATE_PATH}" -o StrictHostKeyChecking=no -o ControlMaster=yes -o ControlPath="$SSH_PATH/ctl/%C" "$WPE_FULL_HOST"
   echo "!!! MULTIPLEX SSH CONNECTION ESTABLISHED !!!"
 
-  # shellcheck disable=SC2086
-  rsync --rsh="ssh -v -p 22 -i ${WPE_SSHG_KEY_PRIVATE_PATH} -o StrictHostKeyChecking=no -o 'ControlPath=$SSH_PATH/ctl/%C'" ${FLAGS} --exclude-from='/exclude.txt' --chmod=D775,F664 "${SRC_PATH}" "${WPE_DESTINATION}"
+  set -x
+  rsync --rsh="ssh -v -p 22 -i ${WPE_SSHG_KEY_PRIVATE_PATH} -o StrictHostKeyChecking=no -o 'ControlPath=$SSH_PATH/ctl/%C'" "${FLAGS_ARRAY[@]}" --exclude-from='/exclude.txt' --chmod=D775,F664 "${SRC_PATH}" "${WPE_DESTINATION}"
+  set +x
 
   if [[ -n ${SCRIPT} || -n ${CACHE_CLEAR} ]]; then
 
@@ -139,4 +146,4 @@ setup_env
 setup_ssh_dir
 check_lint
 check_cache
-sync_files
+sync_files

functions.sh

@@ -0,0 +1,27 @@
+#!/bin/bash
+
+set -e
+
+# Function to parse FLAGS into an array
+# 
+# Bash doesn't respect quotes when splitting the FLAGS string on whitespace
+# which can lead to incorrect splitting on arguments like --filter=':- .gitignore'.
+#
+# xargs does respect quotes, so we use that here to convert the string into a null-delimited
+# sequence of arguments. We then read that sequence into an array by splitting on the null character.
+#
+# https://superuser.com/questions/1529226/get-bash-to-respect-quotes-when-word-splitting-subshell-output
+parse_flags() {
+  local flags="$1"
+  FLAGS_ARRAY=()
+  while IFS= read -r -d '' flag; do FLAGS_ARRAY+=("$flag"); done < <(echo "$flags" | xargs printf '%s\0')
+}
+
+print_deployment_info() {
+  echo "Deploying your code to:"
+  echo -e "\t${WPE_ENV_NAME}"
+  echo -e "with the following ${#FLAGS_ARRAY[@]} rsync argument(s):"
+  for flag in "${FLAGS_ARRAY[@]}"; do
+    echo -e "\t$flag"
+  done
+}

tests/common.sh

@@ -0,0 +1,5 @@
+#!/bin/bash
+
+GREEN='\033[0;32m'
+RED='\033[0;31m'
+NC='\033[0m' # No Color

tests/test_flag_formats.sh

@@ -0,0 +1,55 @@
+#!/bin/bash
+
+# Demonstrates the different ways to pass flags to rsync.
+# 
+# When determining the correct way to pass flags to rsync, it is helpful to
+# understand how the shell interprets quotes and whitespace. This script runs
+# in debug mode to show how the shell interprets the flags for each test case.
+#
+# This is a stand-alone script. It is only meant for demonstration and does not
+# directly test code in this project.
+
+# Get the directory of the current script
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+
+source "${SCRIPT_DIR}/common.sh"
+
+set -x
+
+FLAGS="-avzr --filter=':- .gitignore' --exclude='.*'"
+FLAGS_ARRAY=("-avzr" "--filter=:- .gitignore" "--exclude='.*'")
+
+test_flags_no_quotes() {
+  rsync $FLAGS --dry-run "$SCRIPT_DIR"/data . > /dev/null
+  if [ $? -eq 0 ]; then
+    echo -e "${GREEN}test_flags_no_quotes: Success${NC}"
+  else
+    echo -e "${RED}test_flags_no_quotes: Failure${NC}"
+  fi
+}
+
+test_flags_double_quotes() {
+  rsync "$FLAGS" --dry-run "$SCRIPT_DIR"/data . > /dev/null
+  if [ $? -eq 0 ]; then
+    echo -e "${GREEN}test_flags_double_quotes: Success${NC}"
+  else
+    echo -e "${RED}test_flags_double_quotes: Failure${NC}"
+  fi
+}
+
+test_flags_array() {
+  rsync "${FLAGS_ARRAY[@]}" --dry-run "$SCRIPT_DIR"/data . > /dev/null
+  if [ $? -eq 0 ]; then
+    echo -e "${GREEN}test_flags_array: Success${NC}"
+  else
+    echo -e "${RED}test_flags_array: Failure${NC}"
+  fi
+}
+
+main() {
+  test_flags_no_quotes
+  test_flags_double_quotes
+  test_flags_array
+}
+
+main

tests/test_functions.sh

@@ -0,0 +1,78 @@
+#!/bin/bash
+
+# Get the directory of the current script
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+
+source "${SCRIPT_DIR}/common.sh"
+source "${SCRIPT_DIR}/../functions.sh"
+
+# First argument represents the value of the FLAGS variable.
+# The rest of the arguments represent the expected values of the FLAGS_ARRAY.
+#
+# usage: test_parse_flags <FLAGS> <EXPECTED_FLAGS...>
+test_parse_flags() {
+  local test_case=$1
+  shift
+  local expected_args=("$@")
+
+  parse_flags "$test_case"
+
+  local actual_count="${#FLAGS_ARRAY[@]}"
+  local expected_count="${#expected_args[@]}"
+
+  if [[ "$actual_count" -ne "$expected_count" ]]; then
+    echo -e "${RED}Test failed for FLAGS='$test_case': expected $expected_count arguments, got $actual_count."
+    echo -e "\tActual arguments: ${FLAGS_ARRAY[*]}${NC}"
+    return
+  fi
+
+  for i in "${!expected_args[@]}"; do
+    if [[ "${FLAGS_ARRAY[$i]}" != "${expected_args[$i]}" ]]; then
+      echo -e "${RED}Test failed for FLAGS='$test_case': expected '${expected_args[$i]}', got '${FLAGS_ARRAY[$i]}'.${NC}"
+      return
+    fi
+  done
+
+  echo -e "${GREEN}Test passed for FLAGS=\"$test_case\".${NC}"
+}
+
+# Test cases
+test_parse_flags \
+  "-azvr --inplace --exclude='.*'" \
+  "-azvr" \
+  "--inplace" \
+  "--exclude=.*"
+
+test_parse_flags \
+  '-azvr --inplace --exclude=".*"' \
+  "-azvr" \
+  "--inplace" \
+  "--exclude=.*"
+
+test_parse_flags \
+  "-azvr --filter=':- .gitignore' --exclude='.*'" \
+  "-azvr" \
+  "--filter=:- .gitignore" \
+  "--exclude=.*"
+
+test_parse_flags \
+  "-avzr --delete --filter='P /wp-uploads/**'" \
+  "-avzr" \
+  "--delete" \
+  "--filter=P /wp-uploads/**"
+
+test_parse_flags \
+  "-avzr --delete --exclude='\$dollar'" \
+  "-avzr" \
+  "--delete" \
+  "--exclude=\$dollar"
+
+test_parse_flags \
+  "-avzr --exclude='\`back-ticks\`'" \
+  "-avzr" \
+  "--exclude=\`back-ticks\`"
+
+test_parse_flags \
+  "-avzr --exclude='path\\with\\backslash'" \
+  "-avzr" \
+  "--exclude=path\\with\\backslash"