On Fri, Sep 16 2016, Matt Armstrong <marmstrong@google.com> wrote:

> Tomi, thanks for your reply asking for some motivation behind this
> patch.  I can't reply directly to your message because, for some reason,
> it doesn't appear in my mailbox (I discovered your message while reading
> the mail archive on notmuchmail.org).

I know the reason -- I just cannot send to gmail addresses from this host;
there is some nitpi^H^H^H^H^H stricter rules in gmail for sender hosts,
which we don't obey and we've been too lazy to figure those out...

> The code dealing with this quoting issue was last touched in commit
> b57d9635f50d5e9b53092218e81f6d2c391c363e, where Carl recognizes the
> quoting is a bit of a hack and asks for a better fix.  This is my
> attempt.
>
> I am motivated by a concern for code health.  I saw the quoting, did not
> understand it, recognized it as probably wrong, investigated how the
> quotes were actually passed from Emacs to the shell, and still believed
> it wrong.
>
> I think this kind of flaw can be placed in the category of security fix.
> Quoting issues often are.  But, I'm not a security person.
>
> By my reasoning, the rationale for the change is simple:
>
> a) It is the job of notmuch elisp to pass call-process the args in an
> appropriate manner for notmuch-command (which is always a local
> command).  Because call-process takes a list of strings, and no shell is
> involved, using shell quotes is wrong.  It just so happens that Xapian
> ignores the quotes, but taking advantage of that is not a great thing.
>
> b) If notmuch-command is doing something fancy, as is the case with the
> "remote" script on https://notmuchmail.org/remoteusage/, it is the job
> of that script to quote its own args properly for ssh.  It looks like it
> already does this.
>
> So, the quoting is unnecessary on both accounts.


I hoped you'd answer in a way that I don't have to dig... but it seems that
I eventually had to go there...

Currently (if notmuch-query-context is not nil, simpler if it is nil) the
query part done by notmuch-show--build-buffer looks like

    ' basic-args and ( notmuch-show-query-context )'

if this were passed via shell (and neiher basic-args nor
notmuch-show-query-context contained ' -characters !!!), it would work as
expected. using " or not using either would work if the character set in
those variables were even more restricted.

Now, as you mentioned, these are passed to call-process instead. In that
case all the whitespace-separated strings above goes to separate arguments
(and, basic-args and notmuch-show-query-context being as lists, in even
more separate arguments there...)

After notmuch search^H^H^H^H^H show (*) has gone through optional arguments,
and it is going through ... search-terms, it will concatenate those to one
string, inserting space between those.

(*) Just now I checked a line from my wrapper logs, and found e.g.

2013-08-29 (Thu) 18:57:21: show --format=sexp --format-version=1 --decrypt --exclude=false ' thread:0000000000001ccd and ( tag:unread )'

(*) and went to fix that 'search' to 'show' ;)

... so, the search-term part of the query starts with ' and ends with )' --
and that is what goes to xapian and it seems to work fine.

what is here to notice, that that ' ends option parsing -- if it weren't
there, one could start search string with e.g. --format=json ...

from the point of notmuch it is irrelevant whether the wrapper shell
script used any of these 4 variants: $@, $*, "$@" or "$*" in this case.
The relevant part is that of anyone still uses such a wrapper script:

    $SSH_BIN $NOTMUCH_HOST $NOTMUCH_REMOTE_PATH $@

(again, $@ could be $*, "$@" or "$*") they may encounter regressions if
this is changed ( e.g. query will fail is there are '(' or ')' in there)
how probable is that and should we care is another issue.
(we can help users upgrade unless it breaks some users' 'workflow':
 https://xkcd.com/1172/ -- one never knows ;)

btw: I wonder whether just writing the above as

    $SSH_BIN $NOTMUCH_HOST $NOTMUCH_REMOTE_PATH \' $@ \'

would have worked (before the better alternative we now have in remoteusage)

If we decide that this change is good (like you said, it simplifies it a
bit and is clearer -- and, expecially as it was (only) half-baked solution,
handling (only) the common case where 's were not included in search terms...)

1) add '--' to the command line (first check that it is supported by
notmuch show)

2) commit message needs some adjustments (and irrelevant parts could be
removed, (IMO) it is just noise)


Tomi




>
> Matt Armstrong <marmstrong@google.com> writes:
>
>> Remove shell quoting from notmuch-show--build-buffer.  The args list
>> is ultimately passed to call-process, which passes them verbatim to
>> the subprocess (typically, notmuch).  The quoting, intended for a
>> shell, is unnecessary and confusing.