From 81dab9057c59ba249c813adaf797991f1435056a Mon Sep 17 00:00:00 2001
From: Charles Duffy <charles@cmdzero.io>
Date: Wed, 28 Dec 2022 10:55:03 -0600
Subject: [PATCH 1/2] General best-practices cleanup

- Avoid calling jq more than once per operation. To do this, we have `jq` generate assignments with eval-safe escaping setting shell variables.
- Avoid passing anything but options through eval (keeping options themselves from being eval'd will be a compatibility-breaking change, and is the subject of cloud-gov/s3-resource-simple#50).
- Avoid using `echo` to handle non-constant data (which introduces substantial portability problems across different implementations of `sh`; see https://unix.stackexchange.com/a/65819/3113 and https://pubs.opengroup.org/onlinepubs/9699919799/utilities/echo.html).
- Handle "null" return value from `aws s3api list-objects` gratefully (cloud-gov/s3-resource-simple#49)
---
 assets/check   | 30 ++++++++++++++++++------------
 assets/emit.sh |  6 +++---
 assets/in      | 28 +++++++++++++++-------------
 assets/out     | 34 ++++++++++++++++++----------------
 4 files changed, 54 insertions(+), 44 deletions(-)

diff --git a/assets/check b/assets/check
index fe6980c..6d08fe4 100755
--- a/assets/check
+++ b/assets/check
@@ -5,25 +5,31 @@
 set -e
 
 # parse incoming config data
-payload=`cat`
-bucket=$(echo "$payload" | jq -r '.source.bucket')
-prefix="$(echo "$payload" | jq -r '.source.path // ""')"
+eval "$(jq -r '.source | [
+  "bucket=\(.bucket | @sh)",
+  "path=\(.path // "" | @sh)",
+  "AWS_ACCESS_KEY_ID=\(.access_key_id // "" | @sh)",
+  "AWS_SECRET_ACCESS_KEY=\(.secret_access_key // "" | @sh)",
+  "AWS_DEFAULT_REGION=\(.region // "" | @sh)"
+] | join("\n")')"
 
-# export for `aws` cli
-AWS_ACCESS_KEY_ID=$(echo "$payload" | jq -r '.source.access_key_id // empty')
-AWS_SECRET_ACCESS_KEY=$(echo "$payload" | jq -r '.source.secret_access_key // empty')
-AWS_DEFAULT_REGION=$(echo "$payload" | jq -r '.source.region // empty')
+# hints for static checking
+: "${bucket:=}" "${path:=}" "${AWS_ACCESS_KEY_ID:=}" "${AWS_SECRET_ACCESS_KEY:=}" "${AWS_DEFAULT_REGION:=}"
 
 # Due to precedence rules, must be unset to support AWS IAM Roles.
 if [ -n "$AWS_ACCESS_KEY_ID" ] && [ -n "$AWS_SECRET_ACCESS_KEY" ]; then
-  export AWS_ACCESS_KEY_ID=$AWS_ACCESS_KEY_ID
-  export AWS_SECRET_ACCESS_KEY=$AWS_SECRET_ACCESS_KEY
+  export AWS_ACCESS_KEY_ID AWS_SECRET_ACCESS_KEY
 fi
 
 # Export AWS_DEFAULT_REGION if set
 [ -n "$AWS_DEFAULT_REGION" ] && export AWS_DEFAULT_REGION
 
 # Consider the most recent LastModified timestamp as the most recent version.
-timestamps=$(aws s3api list-objects --bucket $bucket --prefix "$prefix" --query 'Contents[].{LastModified: LastModified}')
-recent="$(echo $timestamps | jq -r 'max_by(.LastModified)')"
-echo "[$recent]"
+timestamps=$(aws s3api list-objects --bucket "$bucket" --prefix "$path" --query 'Contents[].{LastModified: LastModified}')
+if [ "$timestamps" != "null" ]; then
+  recent="$(printf '%s\n' "$timestamps" | jq -r 'max_by(.LastModified)')"
+else
+  recent=''
+fi
+
+printf '%s\n' "[$recent]"
diff --git a/assets/emit.sh b/assets/emit.sh
index 16f7e6c..5347052 100755
--- a/assets/emit.sh
+++ b/assets/emit.sh
@@ -3,6 +3,6 @@
 set -e
 
 # give back a(n empty) version, so that the check passes when using `in`/`out`
-echo "{
-  \"version\": {}
-}"
+echo '{
+  "version": {}
+}'
diff --git a/assets/in b/assets/in
index f20410c..d1da3c1 100755
--- a/assets/in
+++ b/assets/in
@@ -15,27 +15,29 @@ fi
 #######################################
 
 # parse incoming config data
-payload=`cat`
-bucket=$(echo "$payload" | jq -r '.source.bucket')
-path=$(echo "$payload" | jq -r '.source.path // ""')
-options=$(echo "$payload" | jq -r '.source.options // [] | join(" ")')
-
-# export for `aws` cli
-AWS_ACCESS_KEY_ID=$(echo "$payload" | jq -r '.source.access_key_id // empty')
-AWS_SECRET_ACCESS_KEY=$(echo "$payload" | jq -r '.source.secret_access_key // empty')
-AWS_DEFAULT_REGION=$(echo "$payload" | jq -r '.source.region // empty')
+eval "$(jq -r '.source | [
+  "bucket=\(.bucket | @sh)",
+  "path=\(.path // "" | @sh)",
+  "options=\(.options // [] | join(" ") | @sh)",
+  "AWS_ACCESS_KEY_ID=\(.access_key_id // "" | @sh)",
+  "AWS_SECRET_ACCESS_KEY=\(.secret_access_key // "" | @sh)",
+  "AWS_DEFAULT_REGION=\(.region // "" | @sh)"
+] | join("\n")')"
+
+# hints for static checking
+: "${bucket:=}" "${path:=}" "${options:=}" "${AWS_ACCESS_KEY_ID:=}" "${AWS_SECRET_ACCESS_KEY:=}" "${AWS_DEFAULT_REGION:=}"
 
 # Due to precedence rules, must be unset to support AWS IAM Roles.
 if [ -n "$AWS_ACCESS_KEY_ID" ] && [ -n "$AWS_SECRET_ACCESS_KEY" ]; then
-  export AWS_ACCESS_KEY_ID=$AWS_ACCESS_KEY_ID
-  export AWS_SECRET_ACCESS_KEY=$AWS_SECRET_ACCESS_KEY
+  export AWS_ACCESS_KEY_ID AWS_SECRET_ACCESS_KEY
 fi
 
 # Export AWS_DEFAULT_REGION if set
 [ -n "$AWS_DEFAULT_REGION" ] && export AWS_DEFAULT_REGION
 
 echo "Downloading from S3..."
-eval aws s3 sync "s3://$bucket/$path" $dest $options
+eval "set -- $options" # transform options list into argument vector elements
+aws s3 sync "s3://$bucket/$path" "$dest" "$@"
 echo "...done."
 
-. "$(dirname $0)/emit.sh" >&3
+. "$(dirname "$0")/emit.sh" >&3
diff --git a/assets/out b/assets/out
index 14ac646..722313b 100755
--- a/assets/out
+++ b/assets/out
@@ -16,22 +16,23 @@ fi
 
 # disable trace since we're interacting with sensitive values
 set +x
-# parse incoming config data
-payload=`cat`
-bucket=$(echo "$payload" | jq -r '.source.bucket')
-path=$(echo "$payload" | jq -r '.source.path // ""')
-options=$(echo "$payload" | jq -r '.source.options // [] | join(" ")')
-change_dir_to=$(echo "$payload" | jq -r '.source.change_dir_to // "." ')
-
-# export for `aws` cli
-AWS_ACCESS_KEY_ID=$(echo "$payload" | jq -r '.source.access_key_id // empty')
-AWS_SECRET_ACCESS_KEY=$(echo "$payload" | jq -r '.source.secret_access_key // empty')
-AWS_DEFAULT_REGION=$(echo "$payload" | jq -r '.source.region // empty')
+
+eval "$(jq -r '.source | [
+  "bucket=\(.bucket | @sh)",
+  "path=\(.path // "" | @sh)",
+  "change_dir_to=\(.change_dir_to // "." | @sh)",
+  "AWS_ACCESS_KEY_ID=\(.access_key_id // "" | @sh)",
+  "AWS_SECRET_ACCESS_KEY=\(.secret_access_key // "" | @sh)",
+  "AWS_DEFAULT_REGION=\(.region // "" | @sh)",
+  "options=\(.options // [] | join(" ") | @sh)"
+] | join("\n")')"
+
+# hints for static checking
+: "${bucket:=}" "${path:=}" "${change_dir_to:=}" "${options:=}" "${AWS_ACCESS_KEY_ID:=}" "${AWS_SECRET_ACCESS_KEY:=}" "${AWS_DEFAULT_REGION:=}"
 
 # Due to precedence rules, must be unset to support AWS IAM Roles.
 if [ -n "$AWS_ACCESS_KEY_ID" ] && [ -n "$AWS_SECRET_ACCESS_KEY" ]; then
-  export AWS_ACCESS_KEY_ID=$AWS_ACCESS_KEY_ID
-  export AWS_SECRET_ACCESS_KEY=$AWS_SECRET_ACCESS_KEY
+  export AWS_ACCESS_KEY_ID AWS_SECRET_ACCESS_KEY
 fi
 
 # re-enable trace since we're done interacting with sensitive values
@@ -40,10 +41,11 @@ set -x
 # Export AWS_DEFAULT_REGION if set
 [ -n "$AWS_DEFAULT_REGION" ] && export AWS_DEFAULT_REGION
 
-cd ${source}/${change_dir_to}
+cd "${source}/${change_dir_to}"
 
 echo "Uploading to S3..."
-eval aws s3 sync . "s3://$bucket/$path" $options
+eval "set -- $options"
+aws s3 sync . "s3://$bucket/$path" "$@"
 echo "...done."
 
-. "$(dirname $0)/emit.sh" >&3
+. "$(dirname "$0")/emit.sh" >&3

From 2c6d87048bd384352b5eaace3acb4601966bf2c9 Mon Sep 17 00:00:00 2001
From: Charles Duffy <charles@dyfis.net>
Date: Wed, 4 Jan 2023 22:14:52 -0600
Subject: [PATCH 2/2] Revert optimization to reduce jq invocations, per review
 feedback

---
 assets/check | 17 ++++++-----------
 assets/in    | 19 +++++++------------
 assets/out   | 20 ++++++++------------
 3 files changed, 21 insertions(+), 35 deletions(-)

diff --git a/assets/check b/assets/check
index 6d08fe4..0cd71d8 100755
--- a/assets/check
+++ b/assets/check
@@ -4,17 +4,12 @@
 
 set -e
 
-# parse incoming config data
-eval "$(jq -r '.source | [
-  "bucket=\(.bucket | @sh)",
-  "path=\(.path // "" | @sh)",
-  "AWS_ACCESS_KEY_ID=\(.access_key_id // "" | @sh)",
-  "AWS_SECRET_ACCESS_KEY=\(.secret_access_key // "" | @sh)",
-  "AWS_DEFAULT_REGION=\(.region // "" | @sh)"
-] | join("\n")')"
-
-# hints for static checking
-: "${bucket:=}" "${path:=}" "${AWS_ACCESS_KEY_ID:=}" "${AWS_SECRET_ACCESS_KEY:=}" "${AWS_DEFAULT_REGION:=}"
+payload=$(cat)
+bucket=$(printf '%s\n' "$payload" | jq -r '.source.bucket // ""')
+path=$(printf '%s\n' "$payload" | jq -r '.source.path // ""')
+AWS_ACCESS_KEY_ID=$(printf '%s\n' "$payload" | jq -r '.source.access_key_id // ""')
+AWS_SECRET_ACCESS_KEY=$(printf '%s\n' "$payload" | jq -r '.source.secret_access_key // ""')
+AWS_DEFAULT_REGION=$(printf '%s\n' "$payload" | jq -r '.source.region // ""')
 
 # Due to precedence rules, must be unset to support AWS IAM Roles.
 if [ -n "$AWS_ACCESS_KEY_ID" ] && [ -n "$AWS_SECRET_ACCESS_KEY" ]; then
diff --git a/assets/in b/assets/in
index d1da3c1..d2b0e79 100755
--- a/assets/in
+++ b/assets/in
@@ -14,18 +14,13 @@ if [ -z "$dest" ]; then
 fi
 #######################################
 
-# parse incoming config data
-eval "$(jq -r '.source | [
-  "bucket=\(.bucket | @sh)",
-  "path=\(.path // "" | @sh)",
-  "options=\(.options // [] | join(" ") | @sh)",
-  "AWS_ACCESS_KEY_ID=\(.access_key_id // "" | @sh)",
-  "AWS_SECRET_ACCESS_KEY=\(.secret_access_key // "" | @sh)",
-  "AWS_DEFAULT_REGION=\(.region // "" | @sh)"
-] | join("\n")')"
-
-# hints for static checking
-: "${bucket:=}" "${path:=}" "${options:=}" "${AWS_ACCESS_KEY_ID:=}" "${AWS_SECRET_ACCESS_KEY:=}" "${AWS_DEFAULT_REGION:=}"
+payload=$(cat)
+bucket=$(printf '%s\n' "$payload" | jq -r '.source.bucket // ""')
+path=$(printf '%s\n' "$payload" | jq -r '.source.path // ""')
+options=$(printf '%s\n' "$payload" | jq -r '.source.options // [] | join(" ")')
+AWS_ACCESS_KEY_ID=$(printf '%s\n' "$payload" | jq -r '.source.access_key_id // ""')
+AWS_SECRET_ACCESS_KEY=$(printf '%s\n' "$payload" | jq -r '.source.secret_access_key // ""')
+AWS_DEFAULT_REGION=$(printf '%s\n' "$payload" | jq -r '.source.region // ""')
 
 # Due to precedence rules, must be unset to support AWS IAM Roles.
 if [ -n "$AWS_ACCESS_KEY_ID" ] && [ -n "$AWS_SECRET_ACCESS_KEY" ]; then
diff --git a/assets/out b/assets/out
index 722313b..e1f207f 100755
--- a/assets/out
+++ b/assets/out
@@ -17,18 +17,14 @@ fi
 # disable trace since we're interacting with sensitive values
 set +x
 
-eval "$(jq -r '.source | [
-  "bucket=\(.bucket | @sh)",
-  "path=\(.path // "" | @sh)",
-  "change_dir_to=\(.change_dir_to // "." | @sh)",
-  "AWS_ACCESS_KEY_ID=\(.access_key_id // "" | @sh)",
-  "AWS_SECRET_ACCESS_KEY=\(.secret_access_key // "" | @sh)",
-  "AWS_DEFAULT_REGION=\(.region // "" | @sh)",
-  "options=\(.options // [] | join(" ") | @sh)"
-] | join("\n")')"
-
-# hints for static checking
-: "${bucket:=}" "${path:=}" "${change_dir_to:=}" "${options:=}" "${AWS_ACCESS_KEY_ID:=}" "${AWS_SECRET_ACCESS_KEY:=}" "${AWS_DEFAULT_REGION:=}"
+payload=$(cat)
+bucket=$(printf '%s\n' "$payload" | jq -r '.source.bucket // ""')
+path=$(printf '%s\n' "$payload" | jq -r '.source.path // ""')
+change_dir_to=$(printf '%s\n' "$payload" | jq -r '.source.change_dir_to // ""')
+options=$(printf '%s\n' "$payload" | jq -r '.source.options // [] | join(" ")')
+AWS_ACCESS_KEY_ID=$(printf '%s\n' "$payload" | jq -r '.source.access_key_id // ""')
+AWS_SECRET_ACCESS_KEY=$(printf '%s\n' "$payload" | jq -r '.source.secret_access_key // ""')
+AWS_DEFAULT_REGION=$(printf '%s\n' "$payload" | jq -r '.source.region // ""')
 
 # Due to precedence rules, must be unset to support AWS IAM Roles.
 if [ -n "$AWS_ACCESS_KEY_ID" ] && [ -n "$AWS_SECRET_ACCESS_KEY" ]; then