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

[Matt Armstrong]

Mark Walters <markwalters1009 at gmail.com> writes:

> Hi
>
> On Fri, 16 Sep 2016, Matt Armstrong <marmstrong at 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 at 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 at notmuchmail.org
>> https://notmuchmail.org/mailman/listinfo/notmuch