^D to credentials prompt results in "fatal: ... Success"

^D to credentials prompt results in "fatal: ... Success"

[Anthony Sottile]

A bit of an amusing edge case.

I'm not exactly sure the correct approach to fix this but here's my
reproduction, triage, and a few potential options I see.

Note that after the username prompt, I pressed ^D

$./prefix/bin/git --version
git version 2.18.0
$ PATH=$PWD/prefix/bin:$PATH git clone
https://github.com/asottile/this-does-not-exist-i-promise
Cloning into 'this-does-not-exist-i-promise'...
Username for 'https://github.com': fatal: could not read Username for
'https://github.com': Success

Looking at `prompt.c`, it's hitting this bit of code:

        if (git_env_bool("GIT_TERMINAL_PROMPT", 1)) {
            r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);
            err = strerror(errno);
        } else {
            err = "terminal prompts disabled";

        }
        if (!r) {
            /* prompts already contain ": " at the end */
            die("could not read %s%s", prompt, err);
        }

From `git_terminal_prompt` (compat/terminal.c):

    r = strbuf_getline_lf(&buf, input_fh);
    if (!echo) {
        putc('\n', output_fh);
        fflush(output_fh);
    }

    restore_term();
    fclose(input_fh);
    fclose(output_fh);

    if (r == EOF)
        return NULL;
    return buf.buf;


in the `EOF` case, this is returning `NULL`, but `errno = 0` at this
point causing the error string to be "Success"

I see a couple of options here:

1. special case EOF in `git_terminal_prompt` / `git_prompt` and
produce an error message such as:

fatal: could not read Username for 'https://github.com': EOF

(I tried returing `EOF` directly from `git_terminal_prompt` and was
able to get this messaging to work, however `r == EOF` is a
pointer-int comparison so this approach didn't really seem like a good
idea -- changing the signature of `git_terminal_prompt` to set a
special flag for EOF is another option, but seems a lot of work for a
case that probably doesn't happen all too often)

I also couldn't find an appropriate errno to set in the EOF case either

2. treat EOF less specially

The function this is replacing, `getpass` simply returns an empty
string on `EOF`.  This patch would implement that:

diff --git a/compat/terminal.c b/compat/terminal.c
index fa13ee672..8bd08108e 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -122,7 +122,7 @@ char *git_terminal_prompt(const char *prompt, int echo)
        fputs(prompt, output_fh);
        fflush(output_fh);

-       r = strbuf_getline_lf(&buf, input_fh);
+       strbuf_getline_lf(&buf, input_fh);
        if (!echo) {
                putc('\n', output_fh);
                fflush(output_fh);
@@ -132,8 +132,6 @@ char *git_terminal_prompt(const char *prompt, int echo)
        fclose(input_fh);
        fclose(output_fh);

-       if (r == EOF)
-               return NULL;
        return buf.buf;
 }


however then the output is a bit strange for ^D (note I pressed ^D twice):

$ PATH=$PWD/prefix/bin:$PATH git clone
https://github.com/asottile/this-does-not-exist-i-promise
Cloning into 'this-does-not-exist-i-promise'...
Username for 'https://github.com': Password for 'https://github.com':
remote: Repository not found.
fatal: Authentication failed for
'https://github.com/asottile/this-does-not-exist-i-promise/'

Anthony

Re: ^D to credentials prompt results in "fatal: ... Success"

[Jeff King]

On Fri, Jun 22, 2018 at 07:42:38PM -0700, Anthony Sottile wrote:

> A bit of an amusing edge case.
> 
> I'm not exactly sure the correct approach to fix this but here's my
> reproduction, triage, and a few potential options I see.
> 
> Note that after the username prompt, I pressed ^D
> 
> $./prefix/bin/git --version
> git version 2.18.0
> $ PATH=$PWD/prefix/bin:$PATH git clone
> https://github.com/asottile/this-does-not-exist-i-promise
> Cloning into 'this-does-not-exist-i-promise'...
> Username for 'https://github.com': fatal: could not read Username for
> 'https://github.com': Success

Yeah, agreed that is not ideal.

> I see a couple of options here:
> 
> 1. special case EOF in `git_terminal_prompt` / `git_prompt` and
> produce an error message such as:
> 
> fatal: could not read Username for 'https://github.com': EOF
> [...]
>
> 2. treat EOF less specially
> 
> The function this is replacing, `getpass` simply returns an empty
> string on `EOF`.  This patch would implement that:

Either of those would be fine with me. We do not have to adhere to
getpass() behavior exactly (the whole point of having our custom wrapper
is that getpass() behaves badly in a few situations). But I doubt
anybody really cares that much either way, so we might as well have
consistent behavior.

> diff --git a/compat/terminal.c b/compat/terminal.c
> index fa13ee672..8bd08108e 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -122,7 +122,7 @@ char *git_terminal_prompt(const char *prompt, int echo)
>         fputs(prompt, output_fh);
>         fflush(output_fh);
> 
> -       r = strbuf_getline_lf(&buf, input_fh);
> +       strbuf_getline_lf(&buf, input_fh);
>         if (!echo) {
>                 putc('\n', output_fh);
>                 fflush(output_fh);
> @@ -132,8 +132,6 @@ char *git_terminal_prompt(const char *prompt, int echo)
>         fclose(input_fh);
>         fclose(output_fh);
> 
> -       if (r == EOF)
> -               return NULL;
>         return buf.buf;
>  }

I think this goes too far, though. We get EOF from strbuf_getline on
actual EOF _or_ on a real error. And we'd want to keep reporting a real
error, since it may tell the user something useful.

I suspect you probably need to check ferror(input_fh) before closing,
and rewrite "r" to 0 in that case.

-Peff