git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH] credential.c: fix credential reading with regards to CR/LF
@ 2020-02-14 13:49 Johannes Schindelin via GitGitGadget
  2020-02-14 17:55 ` Junio C Hamano
  2020-02-14 18:32 ` Jeff King
  0 siblings, 2 replies; 3+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-02-14 13:49 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Nikita Leonov

From: Nikita Leonov <nykyta.leonov@gmail.com>

This fix makes using Git credentials more friendly to Windows users. In
previous version it was unable to finish input correctly without
configuration changes (tested in PowerShell, CMD, Cygwin).

We know credential filling should be finished by empty input, but the
current implementation does not take into account CR/LF ending, and
hence instead of the empty string we get '\r', which is interpreted as
an incorrect string.

So this commit changes default reading function to a more Windows
compatible reading function.

Signed-off-by: Nikita Leonov <nykyta.leonov@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    Prepare git credential to read input with DOS line endings
    
    This just came in via Git for Windows
    [https://github.com/git-for-windows/git/pull/2516].

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-710%2Fdscho%2Fcrlf-aware-git-credential-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-710/dscho/crlf-aware-git-credential-v1
Pull-Request: https://github.com/git/git/pull/710

 credential.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/credential.c b/credential.c
index 62be651b03..65989dfa4d 100644
--- a/credential.c
+++ b/credential.c
@@ -146,7 +146,7 @@ int credential_read(struct credential *c, FILE *fp)
 {
 	struct strbuf line = STRBUF_INIT;
 
-	while (strbuf_getline_lf(&line, fp) != EOF) {
+	while (strbuf_getline(&line, fp) != EOF) {
 		char *key = line.buf;
 		char *value = strchr(key, '=');
 

base-commit: d8437c57fa0752716dde2d3747e7c22bf7ce2e41
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] credential.c: fix credential reading with regards to CR/LF
  2020-02-14 13:49 [PATCH] credential.c: fix credential reading with regards to CR/LF Johannes Schindelin via GitGitGadget
@ 2020-02-14 17:55 ` Junio C Hamano
  2020-02-14 18:32 ` Jeff King
  1 sibling, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2020-02-14 17:55 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Johannes Schindelin, Nikita Leonov

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Nikita Leonov <nykyta.leonov@gmail.com>
>
> This fix makes using Git credentials more friendly to Windows users. In
> previous version it was unable to finish input correctly without
> configuration changes (tested in PowerShell, CMD, Cygwin).
>
> We know credential filling should be finished by empty input, but the
> current implementation does not take into account CR/LF ending, and
> hence instead of the empty string we get '\r', which is interpreted as
> an incorrect string.
>
> So this commit changes default reading function to a more Windows
> compatible reading function.
>
> Signed-off-by: Nikita Leonov <nykyta.leonov@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

In the older days, strbuf_getline() used to be what we have as
strbuf_getdelim() today, i.e. explicitly request what record
separator to use to grab a line.

The use of strbuf_getline_lf() we see in the pre-image of this patch
came from 8f309aeb ("strbuf: introduce strbuf_getline_{lf,nul}()",
2016-01-13), which mechanically replaced the use of
strbuf_getline(... '\n'), in anticipation for later effort to
identify ones that are better to accept CRLF and turn them into
_crlf variant.  Later all the callers of (old) strbuf_getline() went
away, and strbuf_getline_crlf() that allowed both LF and CRLF
termination (which is most friendly to human-readable line of text)
took over the shortest and sweetest name, strbuf_getline().

So, the "later" effort just happend to this code.  It was a bit
overdue, but it's better late than never.

>  credential.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/credential.c b/credential.c
> index 62be651b03..65989dfa4d 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -146,7 +146,7 @@ int credential_read(struct credential *c, FILE *fp)
>  {
>  	struct strbuf line = STRBUF_INIT;
>  
> -	while (strbuf_getline_lf(&line, fp) != EOF) {
> +	while (strbuf_getline(&line, fp) != EOF) {
>  		char *key = line.buf;
>  		char *value = strchr(key, '=');

There are many more conversions of strbuf_getline(..., '\n') to
strbuf_getline_lf(...) made by 8f309aeb to other parts of credential
stuff.  Has anybody from the Windows side made sure these other ones
are also better to accept CRLF, too?  

I'd wait for a bit to hear either "oh, we found two more and here is
an updated patch" or "we looked at them and this is the only one",
before touching this patch.

Thanks.




^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] credential.c: fix credential reading with regards to CR/LF
  2020-02-14 13:49 [PATCH] credential.c: fix credential reading with regards to CR/LF Johannes Schindelin via GitGitGadget
  2020-02-14 17:55 ` Junio C Hamano
@ 2020-02-14 18:32 ` Jeff King
  1 sibling, 0 replies; 3+ messages in thread
From: Jeff King @ 2020-02-14 18:32 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Johannes Schindelin, Nikita Leonov

On Fri, Feb 14, 2020 at 01:49:56PM +0000, Johannes Schindelin via GitGitGadget wrote:

> From: Nikita Leonov <nykyta.leonov@gmail.com>
> 
> This fix makes using Git credentials more friendly to Windows users. In
> previous version it was unable to finish input correctly without
> configuration changes (tested in PowerShell, CMD, Cygwin).
> 
> We know credential filling should be finished by empty input, but the
> current implementation does not take into account CR/LF ending, and
> hence instead of the empty string we get '\r', which is interpreted as
> an incorrect string.
> 
> So this commit changes default reading function to a more Windows
> compatible reading function.

This does make it impossible to have a CR at the end of a data value. I
think that should be OK (we already disallow LF with no mechanism for
quoting, because who the hell puts a LF in their password?).

But we should perhaps update the section in git-credential(1) that
describes the rules. I had trouble coming up with a wording that wasn't
totally awkward, though:

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index 6f0c7ca80f..09e4b58321 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -112,7 +112,9 @@ specified by a key-value pair, separated by an `=` (equals) sign,
 followed by a newline. The key may contain any bytes except `=`,
 newline, or NUL. The value may contain any bytes except newline or NUL.
 In both cases, all bytes are treated as-is (i.e., there is no quoting,
-and one cannot transmit a value with newline or NUL in it). The list of
+and one cannot transmit a value with newline or NUL in it). Note that
+Git will treat a carriage return before the final newline as part of
+line ending, and not part of the data. The list of
 attributes is terminated by a blank line or end-of-file.
 Git understands the following attributes:
 

This is talking about the git-credential tool itself, but the actual
helper protocol documentation links to this. (As an aside, I notice that
the protocol documentation recently got moved into credential.h along
with the C API bits. Yuck. That probably should be in
gitcredentials(7)).

-Peff

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-14 13:49 [PATCH] credential.c: fix credential reading with regards to CR/LF Johannes Schindelin via GitGitGadget
2020-02-14 17:55 ` Junio C Hamano
2020-02-14 18:32 ` Jeff King

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git