git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, derrickstolee@github.com,
	mjcheetham@outlook.com,
	Matthew John Cheetham <mjcheetham@github.com>
Subject: Re: [PATCH 1/3] wincred: ignore unknown lines (do not die)
Date: Thu, 22 Sep 2022 14:19:43 -0700	[thread overview]
Message-ID: <xmqqbkr7xg28.fsf@gitster.g> (raw)
In-Reply-To: <6426f9c3954866b3fd9259d1a58d2c41dc42e17f.1663865974.git.gitgitgadget@gmail.com> (Matthew John Cheetham via GitGitGadget's message of "Thu, 22 Sep 2022 16:59:32 +0000")

"Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
> It is the expectation that credential helpers be liberal in what they
> accept and conservative in what they return, to allow for future growth
> and evolution of the protocol/interaction.

That is nice in principle, and the updated code below may work well
with existing "other side of the connection" (codepaths in "git"
that asks credential API to talk to the helpers), but I am not sure
if this is always a safe thing to do.

When we gain a new "command" in the protocol, if we just read it
without understanding it, would we open ourselves to a risk of
breaking the protocol communication, worst of which may be to
deadlock?  A new command, when received by a more recent helper that
understands how to react to it, may _require_ it to write more than
"username" and "password" back to "git" from get_credential(), for
example, but the helper with this patch alone, while not complaining
about seeing such a new and unknown command, would not know how to
compute and write that third thing other than "username" and
"password"---would the other side who issued that new command get
stuck waiting for us to return the third thing?  Worse yet, the new
command may expect us to read further in get_credential()
(e.g. maybe they will give us a challenge, which may need to be used
when yielding the "username" and "password" things), but because we
do not even know we need to read, their attempt to write to us may
get them stuck, and that is when we are expecting to write to them,
easily leading to a deadlock, no?

> All of the other helpers (store, cache, osxkeychain, libsecret,
> gnome-keyring) except `netrc` currently ignore any credential lines
> that are not recognised, whereas the Windows helper (wincred) instead
> dies.

Is that different from saying "everybody other than netrc and win
ignore unknown"?

> Fix the discrepancy and ignore unknown lines in the wincred helper.

OK.  As long as everybody consistently ignores, and possibly opens
themselves up consistently to a protocol mismatch issue, it is OK.
We will know if that can be a real problem or does not happen in
practice.

> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
> ---
>  contrib/credential/wincred/git-credential-wincred.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
> index 5091048f9c6..ead6e267c78 100644
> --- a/contrib/credential/wincred/git-credential-wincred.c
> +++ b/contrib/credential/wincred/git-credential-wincred.c
> @@ -278,8 +278,11 @@ static void read_credential(void)
>  			wusername = utf8_to_utf16_dup(v);
>  		} else if (!strcmp(buf, "password"))
>  			password = utf8_to_utf16_dup(v);
> -		else
> -			die("unrecognized input");
> +		/*
> +		 * Ignore other lines; we don't know what they mean, but
> +		 * this future-proofs us when later versions of git do
> +		 * learn new lines, and the helpers are updated to match.
> +		 */
>  	}
>  }

  reply	other threads:[~2022-09-22 21:19 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-22 16:59 [PATCH 0/3] Correct credential helper discrepancies handling input Matthew John Cheetham via GitGitGadget
2022-09-22 16:59 ` [PATCH 1/3] wincred: ignore unknown lines (do not die) Matthew John Cheetham via GitGitGadget
2022-09-22 21:19   ` Junio C Hamano [this message]
2022-09-22 22:09     ` Jeff King
2022-09-22 16:59 ` [PATCH 2/3] netrc: " Matthew John Cheetham via GitGitGadget
2022-09-22 16:59 ` [PATCH 3/3] osxkeychain: clarify that we ignore unknown lines Matthew John Cheetham via GitGitGadget
2022-09-22 18:31 ` [PATCH 0/3] Correct credential helper discrepancies handling input Derrick Stolee
2022-09-22 22:11 ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqbkr7xg28.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=mjcheetham@github.com \
    --cc=mjcheetham@outlook.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).