[pass] [PATCH v2] Team pass: enable multiple keys and per directory

[Matthew Cengia]

I'm going to be fairly brutal in the following code review, so feel free
to take my advice or leave it. I realise that some of the code I'm
commenting on isn't necessarily the focus of this patch

On 2014-03-19 17:04, Jason A. Donenfeld wrote:
[...]
> +	if [[ -n $PASSWORD_STORE_KEY ]]; then
> +		for gpg_id in $PASSWORD_STORE_KEY; do

The above will fail spectacularly if "$PASSWORD_STORE_KEY" contains the
following:

  PASSWORD_STORE_KEY="Matthew Cengia
  Jason A. Donenfeld"

because you're splitting it indiscriminantly on whitespace. The least
bad way I can think of handling this, assuming PASSWORD_STORE_KEY is
expected to be an environment variable read by pass(1) is to insist that
each key exists in the variable on a new line, and read it as follows:

  while read -r gpg_id; do
    gpg_recipient_args+=( "-r" "$gpg_id" )
  done <<<"$PASSWORD_STORE_KEY"

> +	while read gpg_id; do

  Always use read -r unless you are sure it's not what you want.

> +			-p|--path) id_path="$2"; shift; shift ;;

Shift takes an argument: 'shift 2' instead of 'shift; shift;'

> +		id_print="$(printf "%s, " "$@" | head -c -2)"

I wouldn't have used head here; I'd instead use "${id_print%, }" below.

> +		git_add_file "$gpg_id" "Set GPG id to ${id_print}."

GPG id(s), plural?

> +			find "$PREFIX/$id_path" -iname '*.gpg' | while read passfile; do

I realise the code was already like this, and this is an extreme edge
case, but to guard against people trying to break this by doing dumb
things like put newlines in filenames:

  find "$PREFIX/$id_path" -iname '*.gpg' -print0 | while IFS= read -rd '' passfile; do

> +				set_gpg_recipients "$(sed "s:^$PREFIX/::" <<<"$(dirname "$passfile")")"

Again, I'd try to avoid external utilities if it's not too much trouble
to do so:

  passfile_dir=${passfile%/*} passfile_dir=${passfile_dir#$PREFIX}
  set_gpg_recipients "$passfile_dir"

> +		set_gpg_recipients "$(dirname "$path")"

Another brace expansion above.

>  			read -r -p "Enter password for $path: " -e password
> -			gpg2 -e -r "$ID" -o "$passfile" $GPG_OPTS <<<"$password"
> +			gpg2 -e "${gpg_recipient_args[@]}" -o "$passfile" $GPG_OPTS <<<"$password"

Um... reading a password with read, and echoing it to the terminal?
Shouldn't this use 'read -s'?

I could probably keep going, as there's always more to complain about,
but that's a reasonable first pass in my opinion, and I've got real work
to do.

-- 
Regards,
Matthew Cengia
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 966 bytes
Desc: Digital signature
URL: <http://lists.zx2c4.com/pipermail/password-store/attachments/20140320/8d49cb87/attachment.asc>