bug#46609: Fix shell password prompt in minibuffer (bug 43302)

bug#46609: Fix shell password prompt in minibuffer (bug 43302)

[Ryan Prior via Bug reports for GNU Emacs, the Swiss army knife of text editors]

The current comint-password-prompt-regexp does not tolerate newlines at
the end of the prompt, so a string like "Password:\n" will not be
recognized as a password prompt in shell-mode. Before Emacs 27
(74277b0e881) newlines were tolerated here, so this is a regression, and
as a result I would sometimes echo my password in plain text where
previously it would be hidden.

The first patch in this series updates the regexp to use the :space:
Unicode character class for trailing space characters instead of
:blank:, which includes /only/ horizontal whitespace.

The second patch updates comint-watch-for-password-prompt to trim
trailing newlines from the ~string~ argument, which avoids showing the
user a fat password prompt with newlines in the middle.

I am not a Unicode expert and don't know if there might be undesirable
side-effects from using :space: instead of :blank:. However, in my
manual testing these change give me the exact behavior I'm after.

I pair-programmed with Nick Drozd to diagnose and fix this issue. Thanks
Nick!


===File
/home/ryan/dev/emacs/patches/0001-lisp-comint.el-comint-password-prompt-regexp-Allow-e.patch===
From 6c9aa1f08b215abfb69f4f53f622289494588eea Mon Sep 17 00:00:00 2001
From: Ryan Prior <ryanprior@hey.com>
Subject: [PATCH 1/2] lisp/comint.el (comint-password-prompt-regexp): Allow
 ending newline.

---
 lisp/comint.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index 57df6bfb19..0bc358bf51 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -378,7 +378,7 @@ This variable is buffer-local."
    "\\(?:" (regexp-opt password-word-equivalents) "\\|Response\\)"
    "\\(?:\\(?:, try\\)? *again\\| (empty for no passphrase)\\| (again)\\)?"
    ;; "[[:alpha:]]" used to be "for", which fails to match non-English.
-   "\\(?: [[:alpha:]]+ .+\\)?[[:blank:]]*[:：៖][[:blank:]]*\\'")
+   "\\(?: [[:alpha:]]+ .+\\)?[[:blank:]]*[:：៖][[:space:]]*\\'")
   "Regexp matching prompts for passwords in the inferior process.
 This is used by `comint-watch-for-password-prompt'."
   :version "27.1"
--
2.30.1

============================================================


===File
/home/ryan/dev/emacs/patches/0002-lisp-comint.el-comint-watch-for-password-prompt-Trim.patch===
From a9260384dfc0ced53d88380724c899f295ce0944 Mon Sep 17 00:00:00 2001
From: Ryan Prior <ryanprior@hey.com>
Subject: [PATCH 2/2] lisp/comint.el (comint-watch-for-password-prompt): Trim
 trailing newline(s)

---
 lisp/comint.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/comint.el b/lisp/comint.el
index 0bc358bf51..c2942a13da 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -2432,6 +2432,8 @@ This function could be in the list `comint-output-filter-functions'."
                         (replace-regexp-in-string "\r" "" string)))
     (when (string-match "^[ \n\r\t\v\f\b\a]+" string)
       (setq string (replace-match "" t t string)))
+    (when (string-match "[\n]+$" string)
+      (setq string (replace-match "" t t string)))
     (let ((comint--prompt-recursion-depth (1+ comint--prompt-recursion-depth)))
       (if (> comint--prompt-recursion-depth 10)
           (message "Password prompt recursion too deep")
--
2.30.1

============================================================

bug#46609: Fix shell password prompt in minibuffer (bug 43302)

[Lars Ingebrigtsen]

Ryan Prior <rprior@protonmail.com> writes:

> The current comint-password-prompt-regexp does not tolerate newlines at
> the end of the prompt, so a string like "Password:\n" will not be
> recognized as a password prompt in shell-mode. Before Emacs 27
> (74277b0e881) newlines were tolerated here, so this is a regression, and
> as a result I would sometimes echo my password in plain text where
> previously it would be hidden.

Thanks; applied to Emacs 27 with one change:

> +    (when (string-match "[\n]+$" string)
> +      (setq string (replace-match "" t t string)))

This should probably be "\n+\\'", because we only want to remove
newlines from the end of the string, and not newlines from the middle of
the string, presumably?  ("$" means "match end of line", not "match end
of string".)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46609: Fix shell password prompt in minibuffer (bug 43302)

[Andreas Schwab]

On Feb 18 2021, Lars Ingebrigtsen wrote:

> Ryan Prior <rprior@protonmail.com> writes:
>
>> The current comint-password-prompt-regexp does not tolerate newlines at
>> the end of the prompt, so a string like "Password:\n" will not be
>> recognized as a password prompt in shell-mode. Before Emacs 27
>> (74277b0e881) newlines were tolerated here, so this is a regression, and
>> as a result I would sometimes echo my password in plain text where
>> previously it would be hidden.
>
> Thanks; applied to Emacs 27 with one change:
>
>> +    (when (string-match "[\n]+$" string)
>> +      (setq string (replace-match "" t t string)))
>
> This should probably be "\n+\\'", because we only want to remove
> newlines from the end of the string, and not newlines from the middle of
> the string, presumably?  ("$" means "match end of line", not "match end
> of string".)

The preceding line should probably use "\\`" as well.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#46609: Fix shell password prompt in minibuffer (bug 43302)

[Lars Ingebrigtsen]

Andreas Schwab <schwab@linux-m68k.org> writes:

> The preceding line should probably use "\\`" as well.

Yup, but that's not a regression, I think?  So it wouldn't be
appropriate to fix on the emacs-27 branch.  But we should fix it in
Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46609: Fix shell password prompt in minibuffer (bug 43302)

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 18 Feb 2021 12:44:32 +0100
> Cc: 46609@debbugs.gnu.org
> 
> Thanks; applied to Emacs 27 with one change:
> 
> > +    (when (string-match "[\n]+$" string)
> > +      (setq string (replace-match "" t t string)))
> 
> This should probably be "\n+\\'", because we only want to remove
> newlines from the end of the string, and not newlines from the middle of
> the string, presumably?  ("$" means "match end of line", not "match end
> of string".)

Do we want to remove more than one newline?

bug#46609: Fix shell password prompt in minibuffer (bug 43302)

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> > +    (when (string-match "[\n]+$" string)
>> > +      (setq string (replace-match "" t t string)))
>> 
>> This should probably be "\n+\\'", because we only want to remove
>> newlines from the end of the string, and not newlines from the middle of
>> the string, presumably?  ("$" means "match end of line", not "match end
>> of string".)
>
> Do we want to remove more than one newline?

It's just used as the prompt for a `read-string' call, so I think so?
Having a prompt end with a newline is just confusing.

(This (and the preceding two lines) should just be rewritten to use
`string-trim'.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46609: Fix shell password prompt in minibuffer (bug 43302)

[Eli Zaretskii]

> Date: Thu, 18 Feb 2021 01:13:58 +0000
> From:  Ryan Prior via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I am not a Unicode expert and don't know if there might be undesirable
> side-effects from using :space: instead of :blank:. However, in my
> manual testing these change give me the exact behavior I'm after.

I'm a bit worried by the [[:space:]]* part: it would match any number
of newlines and ^L characters, right?  Is that what we want here?

Perhaps it would be safer to just add a literal newline, and only
once?  Especially for the emacs-27 branch?

bug#46609: Fix shell password prompt in minibuffer (bug 43302)

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> I am not a Unicode expert and don't know if there might be undesirable
>> side-effects from using :space: instead of :blank:. However, in my
>> manual testing these change give me the exact behavior I'm after.
>
> I'm a bit worried by the [[:space:]]* part: it would match any number
> of newlines and ^L characters, right?  Is that what we want here?

I think so -- matching "Password: \n\n" makes as much sense as matching
"Password: \n", I think?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46609: Fix shell password prompt in minibuffer (bug 43302)

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Ryan Prior <rprior@protonmail.com>,  46609@debbugs.gnu.org
> Date: Thu, 18 Feb 2021 15:47:39 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I am not a Unicode expert and don't know if there might be undesirable
> >> side-effects from using :space: instead of :blank:. However, in my
> >> manual testing these change give me the exact behavior I'm after.
> >
> > I'm a bit worried by the [[:space:]]* part: it would match any number
> > of newlines and ^L characters, right?  Is that what we want here?
> 
> I think so -- matching "Password: \n\n" makes as much sense as matching
> "Password: \n", I think?

It actually happens in Real Life?

What bothers me is that we could takes something unrelated as a
password prompt.

bug#46609: Fix shell password prompt in minibuffer (bug 43302)

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> I think so -- matching "Password: \n\n" makes as much sense as matching
>> "Password: \n", I think?
>
> It actually happens in Real Life?

I wouldn't have thought so, but then again, I wouldn't have thought that
anybody had even a single newline in the prompt, so my intuitions here
don't count for much.  

> What bothers me is that we could takes something unrelated as a
> password prompt.

If I understand the comint code correctly, it'll match this stuff if
it's the final thing that has been output.  So there should be little
danger in faulty matching here.  But I'm not overly confident in my
understanding of that code.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no