[PATCH] emacs: notmuch-show: remove extraneous shell quoting

[PATCH] emacs: notmuch-show: remove extraneous shell quoting

[Matt Armstrong]

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, are unnecessary and confusing.

This code originally appeared in
9193455fa1476ea3957475e77379b75efa6dd90b, aiming to support remote
notmuch execution over ssh.  The commit message included this example
shell script (simplified):

  #!/bin/sh
  ssh remotehost notmuch $@

That script doesn't properly quote "$@".  Even if it did, it wouldn't
work because the shell ssh starts on the remote host does another
round of shell word splitting.  Fortunately, notmuch already provides
an example of a correct shell script on
https://notmuchmail.org/remoteusage/ (again simplified):

  #!/bin/bash
  printf -v ARGS "%q " "$@"
  ssh remotehost notmuch $ARGS

With the above script I'm able to use notmuch in "remote" mode.
Quoted search terms work as expected, etc.

(this is my t/remote-quoting)

---
 emacs/notmuch-show.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5a585f3..da2d685 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1267,9 +1267,9 @@ first relevant message.
 If no messages match the query return NIL."
   (let* ((basic-args (list notmuch-show-thread-id))
 	 (args (if notmuch-show-query-context
-		   (append (list "\'") basic-args
-			   (list "and (" notmuch-show-query-context ")\'"))
-		 (append (list "\'") basic-args (list "\'"))))
+		   (append basic-args
+			   (list "and (" notmuch-show-query-context ")"))
+		 basic-args))
 	 (cli-args (cons "--exclude=false"
 			 (when notmuch-show-elide-non-matching-messages
 			   (list "--entire-thread=false"))))
-- 
tg: (89c8d27..) t/remote-quoting (depends on: master)

[PATCH] emacs: notmuch-show: remove extraneous shell quoting

[Matt Armstrong]

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.

This code originally appeared in
9193455fa1476ea3957475e77379b75efa6dd90b, aiming to support remote
notmuch execution over ssh.  The commit message included this example
shell script (simplified):

  #!/bin/sh
  ssh remotehost notmuch $@

That script doesn't properly quote "$@".  Even if it did, it wouldn't
work because the shell ssh starts on the remote host does another
round of shell word splitting.  Fortunately, notmuch already provides
an example of a correct shell script on
https://notmuchmail.org/remoteusage/ (again simplified):

  #!/bin/bash
  printf -v ARGS "%q " "$@"
  ssh remotehost notmuch $ARGS

With the above script I'm able to use notmuch in "remote" mode.
Quoted search terms work as expected, etc.

---
 emacs/notmuch-show.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5a585f3..da2d685 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1267,9 +1267,9 @@ first relevant message.
 If no messages match the query return NIL."
   (let* ((basic-args (list notmuch-show-thread-id))
 	 (args (if notmuch-show-query-context
-		   (append (list "\'") basic-args
-			   (list "and (" notmuch-show-query-context ")\'"))
-		 (append (list "\'") basic-args (list "\'"))))
+		   (append basic-args
+			   (list "and (" notmuch-show-query-context ")"))
+		 basic-args))
 	 (cli-args (cons "--exclude=false"
 			 (when notmuch-show-elide-non-matching-messages
 			   (list "--entire-thread=false"))))
-- 
tg: (89c8d27..) t/remote-quoting (depends on: master)

Re: [PATCH] emacs: notmuch-show: remove extraneous shell quoting

[Matt Armstrong]

Sorry, ignore this one.
id:1473834053-17591-1-git-send-email-marmstrong@google.com has some
typo fixes to the commit message.

Re: [PATCH] emacs: notmuch-show: remove extraneous shell quoting

[Tomi Ollila]

On Wed, Sep 14 2016, Matt Armstrong <marmstrong@google.com> wrote:

> 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.

To motivate someone(tm) to test and review this patch, I'd like to know
which problem does this _fix_ (if any). If it does not then such a good
change as this looks like it is takes a bit longer to be taken to review
(and the reviewers need to be more careful for spotting potential
regressions...) 

Tomi

>
> This code originally appeared in
> 9193455fa1476ea3957475e77379b75efa6dd90b, aiming to support remote
> notmuch execution over ssh.  The commit message included this example
> shell script (simplified):
>
>   #!/bin/sh
>   ssh remotehost notmuch $@
>
> That script doesn't properly quote "$@".  Even if it did, it wouldn't
> work because the shell ssh starts on the remote host does another
> round of shell word splitting.  Fortunately, notmuch already provides
> an example of a correct shell script on
> https://notmuchmail.org/remoteusage/ (again simplified):
>
>   #!/bin/bash
>   printf -v ARGS "%q " "$@"
>   ssh remotehost notmuch $ARGS
>
> With the above script I'm able to use notmuch in "remote" mode.
> Quoted search terms work as expected, etc.
>
> ---
>  emacs/notmuch-show.el | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 5a585f3..da2d685 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1267,9 +1267,9 @@ first relevant message.
>  If no messages match the query return NIL."
>    (let* ((basic-args (list notmuch-show-thread-id))
>  	 (args (if notmuch-show-query-context
> -		   (append (list "\'") basic-args
> -			   (list "and (" notmuch-show-query-context ")\'"))
> -		 (append (list "\'") basic-args (list "\'"))))
> +		   (append basic-args
> +			   (list "and (" notmuch-show-query-context ")"))
> +		 basic-args))
>  	 (cli-args (cons "--exclude=false"
>  			 (when notmuch-show-elide-non-matching-messages
>  			   (list "--entire-thread=false"))))
> -- 
> tg: (89c8d27..) t/remote-quoting (depends on: master)
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: notmuch-show: remove extraneous shell quoting

[Matt Armstrong]

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).

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.


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.

Re: [PATCH] emacs: notmuch-show: remove extraneous shell quoting

[Mark Walters]

Hi

On Fri, 16 Sep 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).
>
> 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.

I think all the data being passed is generated by notmuch so I don't
see a security issue.

> 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.

That one script does -- there are at least two others even on the wiki
(see the links at the bottom of the above page) -- they also seem to be
fine. But there could be other user scripts that do need the quoting.

So the question is do we mind breaking a few currently working setups
for the purpose of a mild cleanup. Since the current code is confusing I
think a comment would be in order if we don't apply this patch.

Best wishes

Mark


> So, the quoting is unnecessary on both accounts.
>
>
> 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.
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: notmuch-show: remove extraneous shell quoting

[Tomi Ollila]

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.

Re: [PATCH] emacs: notmuch-show: remove extraneous shell quoting

[Matt Armstrong]

Mark Walters <markwalters1009@gmail.com> writes:

> Hi
>
> On Fri, 16 Sep 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).
>>
>> 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.
>
> I think all the data being passed is generated by notmuch so I don't
> see a security issue.

The search phrase is entered by the user.  Imagine a user searching for
an exact substring they copy/paste from source code, etc.  It is
contrived, but it isn't hard to imagine users using single quotes in
their searches.


>> 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.
>
> That one script does -- there are at least two others even on the wiki
> (see the links at the bottom of the above page) -- they also seem to be
> fine. But there could be other user scripts that do need the quoting.

I agree that there is risk to the change.  However, the scripts that
require the quoting from notmuch.el are already broken.  They will break
whenever a user happens to include a single quote in their query.


> So the question is do we mind breaking a few currently working setups
> for the purpose of a mild cleanup. Since the current code is confusing I
> think a comment would be in order if we don't apply this patch.

Yes, some debate about the importance of the cleanup, but I agree that
if we do keep the quoting it deserves some comment.



>> So, the quoting is unnecessary on both accounts.
>>
>>
>> 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.
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> https://notmuchmail.org/mailman/listinfo/notmuch