git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Carlo Arenas <carenas@gmail.com>, Jeff King <peff@peff.net>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Nikita Leonov <nykyta.leonov@gmail.com>
Subject: [PATCH v3] credential: treat CR/LF as line endings in the credential protocol
Date: Sat, 03 Oct 2020 13:29:12 +0000	[thread overview]
Message-ID: <pull.710.v3.git.git.1601731752695.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.710.v2.git.git.1601293224.gitgitgadget@gmail.com>

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

This fix makes using Git credentials more friendly to Windows users: it
allows a credential helper to communicate using CR/LF line endings ("DOS
line endings" commonly found on Windows) instead of LF-only line endings
("Unix line endings").

Note that this changes the behavior a bit: if a credential helper
produces, say, a password with a trailing Carriage Return character,
that will now be culled even when the rest of the lines end only in Line
Feed characters, indicating that the Carriage Return was not meant to be
part of the line ending.

In practice, it seems _very_ unlikely that something like this happens.
Passwords usually need to consist of non-control characters, URLs need
to have special characters URL-encoded, and user names, well, are names.

However, it _does_ help on Windows, where CR/LF line endings are common:
as unrecognized commands are simply ignored by the credential machinery,
even a command like `quit\r` (which is clearly intended to abort) would
simply be ignored (silently) by Git.

So let's change the credential machinery to accept both CR/LF and LF
line endings.

While we do this for the credential helper protocol, we do _not_ adjust
`git credential-cache--daemon` (which won't work on Windows, anyway,
because it requires Unix sockets) nor `git credential-store` (which
writes the file `~/.git-credentials` which we consider an implementation
detail that should be opaque to the user, read: we do expect users _not_
to edit this file manually).

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 contribution came in via Git for Windows
    [https://github.com/git-for-windows/git/pull/2516].
    
    Sadly, I did not find the time to go through all the changes of 8f309aeb
    ("strbuf: introduce strbuf_getline_{lf,nul}()", 2016-01-13) (as Junio
    asked
    [https://public-inbox.org/git/xmqqmu9lnjdh.fsf@gitster-ct.c.googlers.com]
    ). Rather than delaying this patch indefinitely, I admit defeat on that
    angle.
    
    Changes since v2:
    
     * Dropped the credential-cache--daemon and credential-store changes
       again.
     * Enhanced the commit message (also explaining why we don't touch the
       daemon and the store).
    
    Changes since v1:
    
     * Added a commit to adjust credential-daemon and credential-store in
       the same manner.
     * Adjusted the documentation accordingly.

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

Range-diff vs v2:

 1:  27f6400a21 ! 1:  f6eeb18d3a credential.c: fix credential reading with regards to CR/LF
     @@ Metadata
      Author: Nikita Leonov <nykyta.leonov@gmail.com>
      
       ## Commit message ##
     -    credential.c: fix credential reading with regards to CR/LF
     +    credential: treat CR/LF as line endings in the credential protocol
      
     -    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).
     +    This fix makes using Git credentials more friendly to Windows users: it
     +    allows a credential helper to communicate using CR/LF line endings ("DOS
     +    line endings" commonly found on Windows) instead of LF-only line endings
     +    ("Unix line endings").
      
     -    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.
     +    Note that this changes the behavior a bit: if a credential helper
     +    produces, say, a password with a trailing Carriage Return character,
     +    that will now be culled even when the rest of the lines end only in Line
     +    Feed characters, indicating that the Carriage Return was not meant to be
     +    part of the line ending.
      
     -    So this commit changes default reading function to a more Windows
     -    compatible reading function.
     +    In practice, it seems _very_ unlikely that something like this happens.
     +    Passwords usually need to consist of non-control characters, URLs need
     +    to have special characters URL-encoded, and user names, well, are names.
     +
     +    However, it _does_ help on Windows, where CR/LF line endings are common:
     +    as unrecognized commands are simply ignored by the credential machinery,
     +    even a command like `quit\r` (which is clearly intended to abort) would
     +    simply be ignored (silently) by Git.
     +
     +    So let's change the credential machinery to accept both CR/LF and LF
     +    line endings.
     +
     +    While we do this for the credential helper protocol, we do _not_ adjust
     +    `git credential-cache--daemon` (which won't work on Windows, anyway,
     +    because it requires Unix sockets) nor `git credential-store` (which
     +    writes the file `~/.git-credentials` which we consider an implementation
     +    detail that should be opaque to the user, read: we do expect users _not_
     +    to edit this file manually).
      
          Signed-off-by: Nikita Leonov <nykyta.leonov@gmail.com>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
 2:  f69076036f < -:  ---------- credentials: make line reading Windows compatible
 3:  61baea1061 < -:  ---------- docs: make notes regarding credential line reading


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

diff --git a/credential.c b/credential.c
index efc29dc5e1..e5202fbef2 100644
--- a/credential.c
+++ b/credential.c
@@ -202,7 +202,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: 9bc233ae1cf19a49e51842c7959d80a675dbd1c0
-- 
gitgitgadget

      parent reply	other threads:[~2020-10-03 13:29 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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
2020-09-28 11:40 ` [PATCH v2 0/3] Prepare git credential to read input with DOS line endings Johannes Schindelin via GitGitGadget
2020-09-28 11:40   ` [PATCH v2 1/3] credential.c: fix credential reading with regards to CR/LF Nikita Leonov via GitGitGadget
2020-09-29  0:42     ` Jeff King
2020-10-02 11:37       ` Johannes Schindelin
2020-10-02 12:01         ` Jeff King
2020-10-02 12:27           ` Johannes Schindelin
2020-10-02 16:32         ` Junio C Hamano
2020-10-03 13:28           ` Johannes Schindelin
2020-09-28 11:40   ` [PATCH v2 2/3] credentials: make line reading Windows compatible Nikita Leonov via GitGitGadget
2020-09-28 20:58     ` Junio C Hamano
2020-09-29  0:35       ` Jeff King
2020-09-30 22:33         ` Junio C Hamano
2020-10-02  7:53           ` Johannes Schindelin
2020-09-28 23:26     ` Carlo Arenas
2020-09-28 23:41       ` Junio C Hamano
2020-09-29  0:30       ` Jeff King
2020-09-29  0:41         ` Junio C Hamano
2020-09-29  0:44           ` Jeff King
2020-09-29  0:54             ` Junio C Hamano
2020-09-29  3:00               ` Jeff King
2020-09-30 22:25                 ` Junio C Hamano
2020-09-30 22:39                   ` Jeff King
2020-09-30 22:56                     ` Junio C Hamano
2020-10-01 13:54                       ` Jeff King
2020-10-01 15:42                         ` Junio C Hamano
2020-10-02  8:07                           ` Johannes Schindelin
2020-09-28 11:40   ` [PATCH v2 3/3] docs: make notes regarding credential line reading Nikita Leonov via GitGitGadget
2020-09-28 20:31   ` [PATCH v2 0/3] Prepare git credential to read input with DOS line endings Junio C Hamano
2020-10-03 13:29   ` Johannes Schindelin via GitGitGadget [this message]

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=pull.710.v3.git.git.1601731752695.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=carenas@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=nykyta.leonov@gmail.com \
    --cc=peff@peff.net \
    --subject='Re: [PATCH v3] credential: treat CR/LF as line endings in the credential protocol' \
    /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

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).