Open charles-dyfis-net opened 1 year ago
BTW, one thing that may be notable in the recently added commit (returning to a separate jq call per variable) is the change from echo
to printf
. For background on that, see the excellent answer by Stéphane Chazelas on Why is printf better than echo?, or the APPLICATION USAGE and RATIONALE sections of the POSIX standard for echo
.
If we were specifying a specific shell (like bash), making assumptions about echo
behavior would be slightly justifiable (though only slightly: even with bash, configuration parameters like xpg_echo
modifying behaviors in the cases the standard describes as ambiguous can be set at compile time, or via environment variables at runtime, or via explicit runtime commands); but if we're using /bin/sh
, best to avoid cases the POSIX sh standard describes as ambiguous altogether.
Approved. Thanks @charles-dyfis-net for the contribution!
Changes proposed in this pull request:
jq
generate assignments with eval-safe escaping setting all required shell variables from a single invocation.eval aws s3 sync "s3://$bucket/$path" $dest $options
gained none of the benefit from quotes arounds3://$bucket/$path
, becauseeval
itself concatenated the literal arguments (with syntactic quotes discarded from the quote-removal parsing stage) into a single string before starting the parsing process over from the beginning; the new logic no longer useseval
for this entire command, but instead only useseval
when processing$options
into"$@"
.echo
to handle non-constant data (which introduces substantial portability problems across different implementations ofsh
; see https://unix.stackexchange.com/a/65819/3113 and https://pubs.opengroup.org/onlinepubs/9699919799/utilities/echo.html). Also avoidecho $variable
where$variable
is unquoted, which can result in values being glob-expanded before being passed toecho
, as well as transforming characters in IFS to spaces and collapsing runs of multiple such characters to a single instance per each.aws s3api list-objects
gratefully (fixes cloud-gov/s3-resource-simple#49)"$0"
inside"$(dirname "$0")"
; the modern command substitution format used here introduces a new quoting context, so directory names with spaces (or an unexpectedIFS
value) could resultdirname
being passed an unexpected number of arguments with the old code.Security considerations
Because we deliberately do not modify the behavior described in cloud-gov/s3-resource-simple#50, the preexisting opportunity for shell injection via
$options
remains intact. However, we do narrow the code to only beeval
ing$options
, so it's no longer possible to perform shell injection via$bucket
or$path
.In every other respect, this PR works to reduce runtime ambiguity, and thus to also reduce attack surface.