bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Stephen Berman]

On one of my systems, when using ssh in shell-mode (M-x shell), the
password prompt does not appear in the minibuffer but only in the
*shell* buffer directly under the ssh invocation, and the password is
not hidden when it's entered.  I've determined that reason is that the
password prompt string passed to comint-output-filter begins with a
carriage return character (^M), and when the rest of the prompt string
begins with "Password" (which it can on this system),
comint-password-prompt-regexp does not match it and thus
comint-watch-for-password-prompt returns nil instead of calling
comint-send-invisible.

So far I haven't found out where the carriage return is coming from,
maybe it's from openssh (on the system where the carriage return occurs
in the prompt string I have openssh-8.2p1, another system where the
carriage return does not occur has openssh-7.9p1; if anyone knows the
answer, or has a suggestion for how to find out, I'd appreciate hearing
it).

As a workaround, I can customize comint-password-prompt-regexp to match
the prompt with the carriage return.  But this problem could be seen as
a datapoint in favor of changing the default value of
comint-password-prompt-regexp.  Already in bug#31075 Noam Postavsky
pondered whether eshell-password-prompt-regexp should replace
comint-password-prompt-regexp, noting that "the eshell regexp looks much
simpler".  And indeed, on the system where the carriage return occurs in
the prompt string, using ssh in the Emacs shell (M-x eshell) does use
the minibuffer for password input and it is hidden; and if I set
comint-password-prompt-regexp to eshell-password-prompt-regexp, then I
also get minibuffer hidden input in shell-mode.

So this bug report is basically a plea to change the default value of
comint-password-prompt-regexp to be the same as that of
eshell-password-prompt-regexp.  Or is there a good reason not to do
this?


In GNU Emacs 28.0.50 (build 16, x86_64-pc-linux-gnu, GTK+ Version 3.24.17, cairo version 1.17.3)
 of 2020-08-21 built on strobe-jhalfs
Repository revision: 3e10174fb65f4eb601b1921271bdcf10c933b879
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: Linux From Scratch SVN-20200401

Configured using:
 'configure 'CFLAGS=-Og -g3' PKG_CONFIG_PATH=/opt/qt5/lib/pkgconfig'

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL GNUTLS LIBXML2 FREETYPE HARFBUZZ ZLIB TOOLKIT_SCROLL_BARS
GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD PDUMPER LCMS2

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Lars Ingebrigtsen]

Stephen Berman <stephen.berman@gmx.net> writes:

> So this bug report is basically a plea to change the default value of
> comint-password-prompt-regexp to be the same as that of
> eshell-password-prompt-regexp.  Or is there a good reason not to do
> this?

The difference is that the former looks for something like "enter"
before something like "password:", while the latter is just looking for
variations of "password:"?

My guess would be that the latter would have more false positives...
but you'd think those would be pretty rare.  I can't imagine many people
having shells with prompts like "Type your user name here but definitely
not your password:".

So I think replacing the former with the latter might work, but comint
is very old code, so perhaps it supports older shells better here...

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

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Stephen Berman]

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

On Mon, 24 Aug 2020 14:27:41 +0200 Lars Ingebrigtsen <larsi@gnus.org> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> So this bug report is basically a plea to change the default value of
>> comint-password-prompt-regexp to be the same as that of
>> eshell-password-prompt-regexp.  Or is there a good reason not to do
>> this?
>
> The difference is that the former looks for something like "enter"
> before something like "password:", while the latter is just looking for
> variations of "password:"?
>
> My guess would be that the latter would have more false positives...
> but you'd think those would be pretty rare.  I can't imagine many people
> having shells with prompts like "Type your user name here but definitely
> not your password:".
>
> So I think replacing the former with the latter might work, but comint
> is very old code, so perhaps it supports older shells better here...

The minimal change to fix my use case is to add the carriage return
character as an alternative to comint-password-prompt-regexp, as in the
attached patch.  If even that is too risky, I'll just stick with a local
fix.

Steve


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: comint-password-prompt-regexp patch --]
[-- Type: text/x-patch, Size: 471 bytes --]

diff --git a/lisp/comint.el b/lisp/comint.el
index 092902d865..b6098f449d 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -365,7 +365,7 @@ comint-process-echoes
 ;; See ert test `comint-test-password-regexp'.
 (defcustom comint-password-prompt-regexp
   (concat
-   "\\(^ *\\|"
+   "\\(^ *\\|\r"
    (regexp-opt
     '("Enter" "enter" "Enter same" "enter same" "Enter the" "enter the"
       "Enter Auth" "enter auth" "Old" "old" "New" "new" "'s" "login"

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Lars Ingebrigtsen]

Stephen Berman <stephen.berman@gmx.net> writes:

> The minimal change to fix my use case is to add the carriage return
> character as an alternative to comint-password-prompt-regexp, as in the
> attached patch.  If even that is too risky, I'll just stick with a local
> fix.
>
> Steve
>
> diff --git a/lisp/comint.el b/lisp/comint.el
> index 092902d865..b6098f449d 100644
> --- a/lisp/comint.el
> +++ b/lisp/comint.el
> @@ -365,7 +365,7 @@ comint-process-echoes
>  ;; See ert test `comint-test-password-regexp'.
>  (defcustom comint-password-prompt-regexp
>    (concat
> -   "\\(^ *\\|"
> +   "\\(^ *\\|"
>     (regexp-opt
>      '("Enter" "enter" "Enter same" "enter same" "Enter the" "enter the"
>        "Enter Auth" "enter auth" "Old" "old" "New" "new" "'s" "login"

I don't quite understand how this patch helps -- doesn't this just
require a ^M character before Enter/Old/etc?  Did you mean 

> +   "\\(^ *\\|\\|"

?

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

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Stephen Berman]

On Thu, 27 Aug 2020 15:02:44 +0200 Lars Ingebrigtsen <larsi@gnus.org> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> The minimal change to fix my use case is to add the carriage return
>> character as an alternative to comint-password-prompt-regexp, as in the
>> attached patch.  If even that is too risky, I'll just stick with a local
>> fix.
>>
>> Steve
>>
>> diff --git a/lisp/comint.el b/lisp/comint.el
>> index 092902d865..b6098f449d 100644
>> --- a/lisp/comint.el
>> +++ b/lisp/comint.el
>> @@ -365,7 +365,7 @@ comint-process-echoes
>>  ;; See ert test `comint-test-password-regexp'.
>>  (defcustom comint-password-prompt-regexp
>>    (concat
>> -   "\\(^ *\\|"
>> +   "\\(^ *\\|"
>>     (regexp-opt
>>      '("Enter" "enter" "Enter same" "enter same" "Enter the" "enter the"
>>        "Enter Auth" "enter auth" "Old" "old" "New" "new" "'s" "login"
>
> I don't quite understand how this patch helps -- doesn't this just
> require a ^M character before Enter/Old/etc?  Did you mean
>
>> +   "\\(^ *\\|\\|"

(It looks like Gnus removed the ^M characters.)  Yes, I just tested
again and the password is indeed hidden only when `\\|' follows `^M' in
the regexp.  Apparently I omitted that when I made the diff for the
patch after testing yesterday...

Steve Berman

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Lars Ingebrigtsen]

Stephen Berman <stephen.berman@gmx.net> writes:

>>> +   "\\(^ *\\|\\|"
>
> (It looks like Gnus removed the ^M characters.)

Yeah, probably better to use \r instead of the ^M character directly.

> Yes, I just tested again and the password is indeed hidden only when
> `\\|' follows `^M' in the regexp.  Apparently I omitted that when I
> made the diff for the patch after testing yesterday...

That makes more sense.  But I don't think having ^M as a password prompt
regexp makes all that much sense, though -- surely there's no ^M
involved in prompting on non-Windows systems, and there you'll surely
find ^Ms all over the place, not just in password prompts?

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

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Stephen Berman]

On Fri, 28 Aug 2020 16:08:58 +0200 Lars Ingebrigtsen <larsi@gnus.org> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>>>> +   "\\(^ *\\|\\|"
>>
>> (It looks like Gnus removed the ^M characters.)
>
> Yeah, probably better to use \r instead of the ^M character directly.

Oh, yes, of course.

>> Yes, I just tested again and the password is indeed hidden only when
>> `\\|' follows `^M' in the regexp.  Apparently I omitted that when I
>> made the diff for the patch after testing yesterday...
>
> That makes more sense.  But I don't think having ^M as a password prompt
> regexp makes all that much sense, though -- surely there's no ^M
> involved in prompting on non-Windows systems,

except, apparently, on mine ;-)

>                                               and there you'll surely
> find ^Ms all over the place, not just in password prompts?

True, I wasn't thinking about that.  Then I guess it's best to leave
comint-password-prompt-regexp as is and I'll just change it locally.
The problem I have with it doesn't seem to be widespread, to say the
least.  So feel free to close the bug.

Steve Berman

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Lars Ingebrigtsen]

Stephen Berman <stephen.berman@gmx.net> writes:

> True, I wasn't thinking about that.  Then I guess it's best to leave
> comint-password-prompt-regexp as is and I'll just change it locally.
> The problem I have with it doesn't seem to be widespread, to say the
> least.  So feel free to close the bug.

OK; done.

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

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Stephen Berman]

On Sun, 30 Aug 2020 15:41:57 +0200 Lars Ingebrigtsen <larsi@gnus.org> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> True, I wasn't thinking about that.  Then I guess it's best to leave
>> comint-password-prompt-regexp as is and I'll just change it locally.
>> The problem I have with it doesn't seem to be widespread, to say the
>> least.  So feel free to close the bug.
>
> OK; done.

A postscript with additional datapoints: In my OP, the the system that
the problem occurs on (Linux From Scratch) uses openssh 8.2p1, while the
system without the problem (an older Linux From Scratch) uses openssh
7.9p1.  I've now reproduced the problem on two other GNU/Linux systems:
current openSUSE Tumbleweed, which uses openssh 8.3p1 (and I see the
problem here on two different machines), and openSUSE Leap 15.2, which
uses openssh 8.1p1.  This supports my conjecture that the problematic
carriage return in the prompt comes from openssh.  If anyone else
running a system with openssh 8.{1,2,3}p1 can reproduce the problem,
then perhaps a different way of handling it than changing the default
value of comint-password-prompt-regexp can be found.

Steve Berman

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Robert Pluim]

>>>>> On Mon, 31 Aug 2020 11:26:10 +0200, Stephen Berman <stephen.berman@gmx.net> said:

    Stephen> On Sun, 30 Aug 2020 15:41:57 +0200 Lars Ingebrigtsen <larsi@gnus.org> wrote:
    >> Stephen Berman <stephen.berman@gmx.net> writes:
    >> 
    >>> True, I wasn't thinking about that.  Then I guess it's best to leave
    >>> comint-password-prompt-regexp as is and I'll just change it locally.
    >>> The problem I have with it doesn't seem to be widespread, to say the
    >>> least.  So feel free to close the bug.
    >> 
    >> OK; done.

    Stephen> A postscript with additional datapoints: In my OP, the the system that
    Stephen> the problem occurs on (Linux From Scratch) uses openssh 8.2p1, while the
    Stephen> system without the problem (an older Linux From Scratch) uses openssh
    Stephen> 7.9p1.  I've now reproduced the problem on two other GNU/Linux systems:
    Stephen> current openSUSE Tumbleweed, which uses openssh 8.3p1 (and I see the
    Stephen> problem here on two different machines), and openSUSE Leap 15.2, which
    Stephen> uses openssh 8.1p1.  This supports my conjecture that the problematic
    Stephen> carriage return in the prompt comes from openssh.  If anyone else
    Stephen> running a system with openssh 8.{1,2,3}p1 can reproduce the problem,
    Stephen> then perhaps a different way of handling it than changing the default
    Stephen> value of comint-password-prompt-regexp can be found.

That matches the analysis done in

https://emacs.stackexchange.com/questions/60394/unable-to-push-into-github-using-magit

which leads to:

https://github.com/magit/magit/issues/3843

TL;DR magit decided to strip the \r from the prompt received from
openssh.

We could strip control characters in comint-watch-for-password-prompt,
but then people who've legitimately put control characters in
comint-password-prompt-regexp will get annoyed.

Perhaps just strip '\r'? (And document it, of course)

diff --git a/lisp/comint.el b/lisp/comint.el
index be0e32b9e0..74c5c7f94f 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -2427,7 +2427,8 @@ comint-watch-for-password-prompt
 
 This function could be in the list `comint-output-filter-functions'."
   (when (let ((case-fold-search t))
-	  (string-match comint-password-prompt-regexp string))
+	  (string-match comint-password-prompt-regexp
+                        (replace-regexp-in-string "\r" "" string)))
     (when (string-match "^[ \n\r\t\v\f\b\a]+" string)
       (setq string (replace-match "" t t string)))
     (let ((comint--prompt-recursion-depth (1+ comint--prompt-recursion-depth)))

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Stephen Berman]

On Mon, 31 Aug 2020 12:11:54 +0200 Robert Pluim <rpluim@gmail.com> wrote:

>>>>>> On Mon, 31 Aug 2020 11:26:10 +0200, Stephen Berman
> <stephen.berman@gmx.net> said:
>
>     Stephen> On Sun, 30 Aug 2020 15:41:57 +0200 Lars Ingebrigtsen
>     Stephen> <larsi@gnus.org> wrote:
>     >> Stephen Berman <stephen.berman@gmx.net> writes:
>     >>
>     >>> True, I wasn't thinking about that.  Then I guess it's best to leave
>     >>> comint-password-prompt-regexp as is and I'll just change it locally.
>     >>> The problem I have with it doesn't seem to be widespread, to say the
>     >>> least.  So feel free to close the bug.
>     >>
>     >> OK; done.
>
>     Stephen> A postscript with additional datapoints: In my OP, the the system that
>     Stephen> the problem occurs on (Linux From Scratch) uses openssh 8.2p1, while the
>     Stephen> system without the problem (an older Linux From Scratch) uses openssh
>     Stephen> 7.9p1.  I've now reproduced the problem on two other GNU/Linux systems:
>     Stephen> current openSUSE Tumbleweed, which uses openssh 8.3p1 (and I see the
>     Stephen> problem here on two different machines), and openSUSE Leap 15.2, which
>     Stephen> uses openssh 8.1p1.  This supports my conjecture that the problematic
>     Stephen> carriage return in the prompt comes from openssh.  If anyone else
>     Stephen> running a system with openssh 8.{1,2,3}p1 can reproduce the problem,
>     Stephen> then perhaps a different way of handling it than changing the default
>     Stephen> value of comint-password-prompt-regexp can be found.
>
> That matches the analysis done in
>
> https://emacs.stackexchange.com/questions/60394/unable-to-push-into-github-using-magit
>
> which leads to:
>
> https://github.com/magit/magit/issues/3843
>
> TL;DR magit decided to strip the \r from the prompt received from
> openssh.

Ah, thanks for the pointer.  I'd done a web search before my first post
about this but didn't find those pages.

> We could strip control characters in comint-watch-for-password-prompt,
> but then people who've legitimately put control characters in
> comint-password-prompt-regexp will get annoyed.
>
> Perhaps just strip '\r'? (And document it, of course)
>
> diff --git a/lisp/comint.el b/lisp/comint.el
> index be0e32b9e0..74c5c7f94f 100644
> --- a/lisp/comint.el
> +++ b/lisp/comint.el
> @@ -2427,7 +2427,8 @@ comint-watch-for-password-prompt
>
>  This function could be in the list `comint-output-filter-functions'."
>    (when (let ((case-fold-search t))
> -	  (string-match comint-password-prompt-regexp string))
> +	  (string-match comint-password-prompt-regexp
> +                        (replace-regexp-in-string "\r" "" string)))
>      (when (string-match "^[ \n\r\t\v\f\b\a]+" string)
>        (setq string (replace-match "" t t string)))
>      (let ((comint--prompt-recursion-depth (1+ comint--prompt-recursion-depth)))

That works for me.  Thanks,

Steve Berman

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> That matches the analysis done in
>
> https://emacs.stackexchange.com/questions/60394/unable-to-push-into-github-using-magit
>
> which leads to:
>
> https://github.com/magit/magit/issues/3843

Thanks for doing the detective work here.  Summary:

----
 After a bit of digging, the change does indeed come from OpenSSH:
 
 https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/readpass.c.diff?r1=1.52&r2=1.53 
  
  Print an \r in front of the password prompt so parts of a password
  that was     entered too early are likely clobbered by the prompt.
----

To which I can only say: "Ok...?"

> TL;DR magit decided to strip the \r from the prompt received from
> openssh.

[...]

> Perhaps just strip '\r'? (And document it, of course)

[...]

> -	  (string-match comint-password-prompt-regexp string))
> +	  (string-match comint-password-prompt-regexp
> +                        (replace-regexp-in-string "\r" "" string)))

Yes, that makes sense to me.

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

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Robert Pluim]

>>>>> On Tue, 01 Sep 2020 16:28:17 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> [...]

    >> -	  (string-match comint-password-prompt-regexp string))
    >> +	  (string-match comint-password-prompt-regexp
    >> +                        (replace-regexp-in-string "\r" "" string)))

    Lars> Yes, that makes sense to me.

Done in 2e3ec40293

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

>     >> -	  (string-match comint-password-prompt-regexp string))
>     >> +	  (string-match comint-password-prompt-regexp
>     >> +                        (replace-regexp-in-string "\r" "" string)))
>
>     Lars> Yes, that makes sense to me.
>
> Done in 2e3ec40293

One thought -- just as a precaution, would it make sense to do this
replacement only if the \r is at the start of a line, and it's not
followed by a \n?  I'm just slightly worried that there may be
repercussions on systems that use \r\n as end-of-line markers...  it
probably makes no difference here, but...

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

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Robert Pluim]

>>>>> On Tue, 01 Sep 2020 17:06:50 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Robert Pluim <rpluim@gmail.com> writes:
    >> >> -	  (string-match comint-password-prompt-regexp string))
    >> >> +	  (string-match comint-password-prompt-regexp
    >> >> +                        (replace-regexp-in-string "\r" "" string)))
    >> 
    Lars> Yes, that makes sense to me.
    >> 
    >> Done in 2e3ec40293

    Lars> One thought -- just as a precaution, would it make sense to do this
    Lars> replacement only if the \r is at the start of a line, and it's not
    Lars> followed by a \n?  I'm just slightly worried that there may be
    Lars> repercussions on systems that use \r\n as end-of-line markers...  it
    Lars> probably makes no difference here, but...

Iʼm finding it hard to come up with a case where stripping the \r
fails (famous last words).

Robert

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> Iʼm finding it hard to come up with a case where stripping the \r
> fails (famous last words).

Yeah, it's pretty unlikely.  Perhaps if somebody has customised
comint-password-prompt-regexp to be "Password:\r\n", and then it'd no
longer match...

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

bug#43003: 28.0.50; comint-password-prompt-regexp too restrictive

[Robert Pluim]

>>>>> On Tue, 01 Sep 2020 18:46:24 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Robert Pluim <rpluim@gmail.com> writes:
    >> Iʼm finding it hard to come up with a case where stripping the \r
    >> fails (famous last words).

    Lars> Yeah, it's pretty unlikely.  Perhaps if somebody has customised
    Lars> comint-password-prompt-regexp to be "Password:\r\n", and then it'd no
    Lars> longer match...

Iʼm not aware of any tools that emit \r and/or \n *after* the password
prompt, so I think thatʼs unlikely.

Robert