{"thread":{"id":"52803","subject":"[PATCH] credential.c: fix credential reading with regards to CR/LF","startedAt":"2020-02-14T13:50:01Z","lastAt":"2020-10-03T13:29:17Z","messageCount":32,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Jeff King","Nikita Leonov via GitGitGadget","Carlo Arenas","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"391750","messageId":"pull.710.git.git.1581688196706.gitgitgadget@gmail.com","threadId":"52803","inReplyTo":null,"subject":"[PATCH] credential.c: fix credential reading with regards to CR/LF","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-14T13:49:56Z","receivedAt":"2020-02-14T13:50:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Nikita Leonov \n\nThis fix makes using Git credentials more friendly to Windows users. In\nprevious version it was unable to finish input correctly without\nconfiguration changes (tested in PowerShell, CMD, Cygwin).\n\nWe know credential filling should be finished by empty input, but the\ncurrent implementation does not take into account CR/LF ending, and\nhence instead of the empty string we get '\\r', which is interpreted as\nan incorrect string.\n\nSo this commit changes default reading function to a more Windows\ncompatible reading function.\n\nSigned-off-by: Nikita Leonov \nSigned-off-by: Johannes Schindelin \n---\n Prepare git credential to read input with DOS line endings\n \n This just came in via Git for Windows\n [https://github.com/git-for-windows/git/pull/2516].\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-710%2Fdscho%2Fcrlf-aware-git-credential-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-710/dscho/crlf-aware-git-credential-v1\nPull-Request: https://github.com/git/git/pull/710\n\n credential.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/credential.c b/credential.c\nindex 62be651b03..65989dfa4d 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -146,7 +146,7 @@ int credential_read(struct credential *c, FILE *fp)\n {\n \tstruct strbuf line = STRBUF_INIT;\n \n-\twhile (strbuf_getline_lf(&line, fp) != EOF) {\n+\twhile (strbuf_getline(&line, fp) != EOF) {\n \t\tchar *key = line.buf;\n \t\tchar *value = strchr(key, '=');\n \n\nbase-commit: d8437c57fa0752716dde2d3747e7c22bf7ce2e41\n-- \ngitgitgadget\n"},{"id":"391775","messageId":"xmqqmu9lnjdh.fsf@gitster-ct.c.googlers.com","threadId":"52803","inReplyTo":"pull.710.git.git.1581688196706.gitgitgadget@gmail.com","subject":"Re: [PATCH] credential.c: fix credential reading with regards to CR/LF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-14T17:55:54Z","receivedAt":"2020-02-14T17:56:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Nikita Leonov \n>\n> This fix makes using Git credentials more friendly to Windows users. In\n> previous version it was unable to finish input correctly without\n> configuration changes (tested in PowerShell, CMD, Cygwin).\n>\n> We know credential filling should be finished by empty input, but the\n> current implementation does not take into account CR/LF ending, and\n> hence instead of the empty string we get '\\r', which is interpreted as\n> an incorrect string.\n>\n> So this commit changes default reading function to a more Windows\n> compatible reading function.\n>\n> Signed-off-by: Nikita Leonov \n> Signed-off-by: Johannes Schindelin \n> ---\n\nIn the older days, strbuf_getline() used to be what we have as\nstrbuf_getdelim() today, i.e. explicitly request what record\nseparator to use to grab a line.\n\nThe use of strbuf_getline_lf() we see in the pre-image of this patch\ncame from 8f309aeb (\"strbuf: introduce strbuf_getline_{lf,nul}()\",\n2016-01-13), which mechanically replaced the use of\nstrbuf_getline(... '\\n'), in anticipation for later effort to\nidentify ones that are better to accept CRLF and turn them into\n_crlf variant. Later all the callers of (old) strbuf_getline() went\naway, and strbuf_getline_crlf() that allowed both LF and CRLF\ntermination (which is most friendly to human-readable line of text)\ntook over the shortest and sweetest name, strbuf_getline().\n\nSo, the \"later\" effort just happend to this code. It was a bit\noverdue, but it's better late than never.\n\n> credential.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/credential.c b/credential.c\n> index 62be651b03..65989dfa4d 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -146,7 +146,7 @@ int credential_read(struct credential *c, FILE *fp)\n> {\n> \tstruct strbuf line = STRBUF_INIT;\n> \n> -\twhile (strbuf_getline_lf(&line, fp) != EOF) {\n> +\twhile (strbuf_getline(&line, fp) != EOF) {\n> \t\tchar *key = line.buf;\n> \t\tchar *value = strchr(key, '=');\n\nThere are many more conversions of strbuf_getline(..., '\\n') to\nstrbuf_getline_lf(...) made by 8f309aeb to other parts of credential\nstuff. Has anybody from the Windows side made sure these other ones\nare also better to accept CRLF, too? \n\nI'd wait for a bit to hear either \"oh, we found two more and here is\nan updated patch\" or \"we looked at them and this is the only one\",\nbefore touching this patch.\n\nThanks.\n\n\n\n"},{"id":"391793","messageId":"20200214183200.GP150965@coredump.intra.peff.net","threadId":"52803","inReplyTo":"pull.710.git.git.1581688196706.gitgitgadget@gmail.com","subject":"Re: [PATCH] credential.c: fix credential reading with regards to CR/LF","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-14T18:32:00Z","receivedAt":"2020-02-14T18:32:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 14, 2020 at 01:49:56PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Nikita Leonov \n> \n> This fix makes using Git credentials more friendly to Windows users. In\n> previous version it was unable to finish input correctly without\n> configuration changes (tested in PowerShell, CMD, Cygwin).\n> \n> We know credential filling should be finished by empty input, but the\n> current implementation does not take into account CR/LF ending, and\n> hence instead of the empty string we get '\\r', which is interpreted as\n> an incorrect string.\n> \n> So this commit changes default reading function to a more Windows\n> compatible reading function.\n\nThis does make it impossible to have a CR at the end of a data value. I\nthink that should be OK (we already disallow LF with no mechanism for\nquoting, because who the hell puts a LF in their password?).\n\nBut we should perhaps update the section in git-credential(1) that\ndescribes the rules. I had trouble coming up with a wording that wasn't\ntotally awkward, though:\n\ndiff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\nindex 6f0c7ca80f..09e4b58321 100644\n--- a/Documentation/git-credential.txt\n+++ b/Documentation/git-credential.txt\n@@ -112,7 +112,9 @@ specified by a key-value pair, separated by an `=` (equals) sign,\n followed by a newline. The key may contain any bytes except `=`,\n newline, or NUL. The value may contain any bytes except newline or NUL.\n In both cases, all bytes are treated as-is (i.e., there is no quoting,\n-and one cannot transmit a value with newline or NUL in it). The list of\n+and one cannot transmit a value with newline or NUL in it). Note that\n+Git will treat a carriage return before the final newline as part of\n+line ending, and not part of the data. The list of\n attributes is terminated by a blank line or end-of-file.\n Git understands the following attributes:\n \n\nThis is talking about the git-credential tool itself, but the actual\nhelper protocol documentation links to this. (As an aside, I notice that\nthe protocol documentation recently got moved into credential.h along\nwith the C API bits. Yuck. That probably should be in\ngitcredentials(7)).\n\n-Peff\n"},{"id":"406529","messageId":"pull.710.v2.git.git.1601293224.gitgitgadget@gmail.com","threadId":"52803","inReplyTo":"pull.710.git.git.1581688196706.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Prepare git credential to read input with DOS line endings","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-28T11:40:21Z","receivedAt":"2020-09-28T11:40:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This contribution came in via Git for Windows\n[https://github.com/git-for-windows/git/pull/2516].\n\nSadly, I did not find the time to go through all the changes of 8f309aeb\n(\"strbuf: introduce strbuf_getline_{lf,nul}()\", 2016-01-13) (as Junio asked\n[https://public-inbox.org/git/xmqqmu9lnjdh.fsf@gitster-ct.c.googlers.com]).\nRather than delaying this patch indefinitely, I admit defeat on that angle.\n\nChanges since v1:\n\n * Added a commit to adjust credential-daemon and credential-store in the\n same manner.\n * Adjusted the documentation accordingly.\n\nNikita Leonov (3):\n credential.c: fix credential reading with regards to CR/LF\n credentials: make line reading Windows compatible\n docs: make notes regarding credential line reading\n\n Documentation/git-credential.txt | 4 +++-\n Documentation/gitcredentials.txt | 2 ++\n builtin/credential-cache--daemon.c | 4 ++--\n builtin/credential-store.c | 2 +-\n credential.c | 2 +-\n t/t0302-credential-store.sh | 16 ++++++----------\n 6 files changed, 15 insertions(+), 15 deletions(-)\n\n\nbase-commit: 9bc233ae1cf19a49e51842c7959d80a675dbd1c0\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-710%2Fdscho%2Fcrlf-aware-git-credential-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-710/dscho/crlf-aware-git-credential-v2\nPull-Request: https://github.com/git/git/pull/710\n\nRange-diff vs v1:\n\n 1: 2a9cc710bb = 1: 27f6400a21 credential.c: fix credential reading with regards to CR/LF\n -: ---------- > 2: f69076036f credentials: make line reading Windows compatible\n -: ---------- > 3: 61baea1061 docs: make notes regarding credential line reading\n\n-- \ngitgitgadget\n"},{"id":"406530","messageId":"27f6400a21412d762b290a34a78ebe7296d36bf3.1601293224.git.gitgitgadget@gmail.com","threadId":"52803","inReplyTo":"pull.710.v2.git.git.1601293224.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] credential.c: fix credential reading with regards to CR/LF","fromName":"Nikita Leonov via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-28T11:40:22Z","receivedAt":"2020-09-28T11:40:30Z","isPatch":true,"sender":{"key":"name:Nikita Leonov","avatar":null},"body":"From: Nikita Leonov \n\nThis fix makes using Git credentials more friendly to Windows users. In\nprevious version it was unable to finish input correctly without\nconfiguration changes (tested in PowerShell, CMD, Cygwin).\n\nWe know credential filling should be finished by empty input, but the\ncurrent implementation does not take into account CR/LF ending, and\nhence instead of the empty string we get '\\r', which is interpreted as\nan incorrect string.\n\nSo this commit changes default reading function to a more Windows\ncompatible reading function.\n\nSigned-off-by: Nikita Leonov \nSigned-off-by: Johannes Schindelin \n---\n credential.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/credential.c b/credential.c\nindex efc29dc5e1..e5202fbef2 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -202,7 +202,7 @@ int credential_read(struct credential *c, FILE *fp)\n {\n \tstruct strbuf line = STRBUF_INIT;\n \n-\twhile (strbuf_getline_lf(&line, fp) != EOF) {\n+\twhile (strbuf_getline(&line, fp) != EOF) {\n \t\tchar *key = line.buf;\n \t\tchar *value = strchr(key, '=');\n \n-- \ngitgitgadget\n\n"},{"id":"406531","messageId":"61baea106115277fb0c39d3b2a79770306f5a465.1601293224.git.gitgitgadget@gmail.com","threadId":"52803","inReplyTo":"pull.710.v2.git.git.1601293224.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] docs: make notes regarding credential line reading","fromName":"Nikita Leonov via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-28T11:40:24Z","receivedAt":"2020-09-28T11:40:34Z","isPatch":true,"sender":{"key":"name:Nikita Leonov","avatar":null},"body":"From: Nikita Leonov \n\nThis commit adds notes to git-credential.txt and to gitcredentials.txt\nspecifying that 'LF' and 'CR/LF' endings are treated the same.\n\nSigned-off-by: Nikita Leonov \nSigned-off-by: Johannes Schindelin \n---\n Documentation/git-credential.txt | 4 +++-\n Documentation/gitcredentials.txt | 2 ++\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\nindex 31c81c4c02..9cf25126f2 100644\n--- a/Documentation/git-credential.txt\n+++ b/Documentation/git-credential.txt\n@@ -114,7 +114,9 @@ The key may contain any bytes except `=`, newline, or NUL. The value may\n contain any bytes except newline or NUL.\n \n In both cases, all bytes are treated as-is (i.e., there is no quoting,\n-and one cannot transmit a value with newline or NUL in it). The list of\n+and one cannot transmit a value with newline or NUL in it). Note that\n+Git will treat a carriage return before the final newline as part of\n+line ending, and not part of the data. The list of\n attributes is terminated by a blank line or end-of-file.\n \n Git understands the following attributes:\ndiff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\nindex 758bf39ba3..184079eaa4 100644\n--- a/Documentation/gitcredentials.txt\n+++ b/Documentation/gitcredentials.txt\n@@ -141,6 +141,8 @@ entry for `https://example.com/bar/baz.git` (in addition to matching the config\n entry for `https://example.com`) but will not match a config entry for\n `https://example.com/bar`.\n \n+Note that Git will treat a carriage return before the final newline as part\n+of line ending, and not part of the data.\n \n CONFIGURATION OPTIONS\n ---------------------\n-- \ngitgitgadget\n"},{"id":"406532","messageId":"f69076036fe4dfe8b57fc1d4329c7be3f7346850.1601293224.git.gitgitgadget@gmail.com","threadId":"52803","inReplyTo":"pull.710.v2.git.git.1601293224.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Nikita Leonov via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-28T11:40:23Z","receivedAt":"2020-09-28T11:40:34Z","isPatch":true,"sender":{"key":"name:Nikita Leonov","avatar":null},"body":"From: Nikita Leonov \n\nThis commit makes reading process regarding credentials compatible with\n'CR/LF' line ending. It makes using git more convenient on systems like\nWindows.\n\nSigned-off-by: Nikita Leonov \nSigned-off-by: Johannes Schindelin \n---\n builtin/credential-cache--daemon.c | 4 ++--\n builtin/credential-store.c | 2 +-\n t/t0302-credential-store.sh | 16 ++++++----------\n 3 files changed, 9 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex c61f123a3b..17664bb0d5 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -99,12 +99,12 @@ static int read_request(FILE *fh, struct credential *c,\n \tstatic struct strbuf item = STRBUF_INIT;\n \tconst char *p;\n \n-\tstrbuf_getline_lf(&item, fh);\n+\tstrbuf_getline(&item, fh);\n \tif (!skip_prefix(item.buf, \"action=\", &p))\n \t\treturn error(\"client sent bogus action line: %s\", item.buf);\n \tstrbuf_addstr(action, p);\n \n-\tstrbuf_getline_lf(&item, fh);\n+\tstrbuf_getline(&item, fh);\n \tif (!skip_prefix(item.buf, \"timeout=\", &p))\n \t\treturn error(\"client sent bogus timeout line: %s\", item.buf);\n \t*timeout = atoi(p);\ndiff --git a/builtin/credential-store.c b/builtin/credential-store.c\nindex 5331ab151a..d4e90b68df 100644\n--- a/builtin/credential-store.c\n+++ b/builtin/credential-store.c\n@@ -23,7 +23,7 @@ static int parse_credential_file(const char *fn,\n \t\treturn found_credential;\n \t}\n \n-\twhile (strbuf_getline_lf(&line, fh) != EOF) {\n+\twhile (strbuf_getline(&line, fh) != EOF) {\n \t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n \t\t entry.username && entry.password &&\n \t\t credential_match(c, &entry)) {\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex 716bf1af9f..f2c672e4b6 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -142,7 +142,7 @@ invalid_credential_test \"scheme\" ://user:pass@example.com\n invalid_credential_test \"valid host/path\" https://user:pass@\n invalid_credential_test \"username/password\" https://pass@example.com\n \n-test_expect_success 'get: credentials with DOS line endings are invalid' '\n+test_expect_success 'get: credentials with DOS line endings are valid' '\n \tprintf \"https://user:pass@example.com\\r\\n\" >\"$HOME/.git-credentials\" &&\n \tcheck fill store <<-\\EOF\n \tprotocol=https\n@@ -150,11 +150,9 @@ test_expect_success 'get: credentials with DOS line endings are invalid' '\n \t--\n \tprotocol=https\n \thost=example.com\n-\tusername=askpass-username\n-\tpassword=askpass-password\n+\tusername=user\n+\tpassword=pass\n \t--\n-\taskpass: Username for '\\''https://example.com'\\'':\n-\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n \t--\n \tEOF\n '\n@@ -172,7 +170,7 @@ test_expect_success 'get: credentials with path and DOS line endings are valid'\n \tEOF\n '\n \n-test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' '\n+test_expect_success 'get: credentials with DOS line endings are valid if path is relevant' '\n \tprintf \"https://user:pass@example.com/repo.git\\r\\n\" >\"$HOME/.git-credentials\" &&\n \ttest_config credential.useHttpPath true &&\n \tcheck fill store <<-\\EOF\n@@ -181,11 +179,9 @@ test_expect_success 'get: credentials with DOS line endings are invalid if path\n \tprotocol=https\n \thost=example.com\n \tpath=repo.git\n-\tusername=askpass-username\n-\tpassword=askpass-password\n+\tusername=user\n+\tpassword=pass\n \t--\n-\taskpass: Username for '\\''https://example.com/repo.git'\\'':\n-\taskpass: Password for '\\''https://askpass-username@example.com/repo.git'\\'':\n \t--\n \tEOF\n '\n-- \ngitgitgadget\n\n"},{"id":"406568","messageId":"xmqqpn65lk2s.fsf@gitster.c.googlers.com","threadId":"52803","inReplyTo":"pull.710.v2.git.git.1601293224.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] Prepare git credential to read input with DOS line endings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-28T20:31:23Z","receivedAt":"2020-09-28T20:31:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> This contribution came in via Git for Windows\n> [https://github.com/git-for-windows/git/pull/2516].\n>\n> Sadly, I did not find the time to go through all the changes of 8f309aeb\n> (\"strbuf: introduce strbuf_getline_{lf,nul}()\", 2016-01-13) (as Junio asked\n> [https://public-inbox.org/git/xmqqmu9lnjdh.fsf@gitster-ct.c.googlers.com]).\n> Rather than delaying this patch indefinitely, I admit defeat on that angle.\n\nOh, that's OK. Thanks for resurrecting the stalled patch. I didn't\nmean the suggested action while I was waiting to be an exhaustive\naudit---I merely wanted to avoid having to queue a few more separate\nones soon after applying a patch for just a single place.\n\n> Changes since v1:\n>\n> * Added a commit to adjust credential-daemon and credential-store in the\n> same manner.\n\nI am kind of surprised to see that these need to be touched at all.\n\nThe change to credential-store is sort-of understandable, as the\nfile it uses could be hand-modified in an editor and its line ending\ncould have been changed to CRLF, hence it needs to be prepared.\n\nBut does inter-process communication with the daemon need to care\nabout CRLF line endings? Do parts of the protocol within the\ncredential system use platform specific line ending?\n"},{"id":"406569","messageId":"xmqqk0wdliuc.fsf@gitster.c.googlers.com","threadId":"52803","inReplyTo":"f69076036fe4dfe8b57fc1d4329c7be3f7346850.1601293224.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-28T20:58:03Z","receivedAt":"2020-09-28T20:58:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nikita Leonov via GitGitGadget\" writes:\n\n> From: Nikita Leonov \n>\n> This commit makes reading process regarding credentials compatible with\n> 'CR/LF' line ending. It makes using git more convenient on systems like\n> Windows.\n\nI can see why this is a good thing for \"store\" and the two updated\npieces of the test script demonstrate it very well. \n\nBut it is unclear why and how cache-daemon benefits from this change.\nThat needs to be justified.\n\nAs to the log message (this is 70% style, but there are\nconsequences), we tend not to say \"This commit does X\"---because\nsuch a statement is often insufficient to illustrate if the commit\nindeed does X, and explain why it is a good thing to do X in the\nfirst place.\n\nInstead, we first explain that the current system does not do X (in\npresent tense, so we do NOT say \"previously we did not do X\"), then\nexplain why doing X would be a good thing, and finally give an order\nto the codebase to start doing X. For this change, it might look\nlike this:\n\n The credential subsystem has assumed that lines always end with\n LF. On a system whose text file ends with CRLF (e.g. Windows),\n this assumption causes the CR at the end of the line as an extra\n byte appended to the data, and worse, there is no way to send an\n \"empty line\" to signal an end of a group of lines, because such\n a line would be taken as a line with a lone CR on it.\n\n Because credential-store writes to and reads from a text file on\n disk, which users may manually edit with their editor and turn\n the line-ending to CRLF.\n\n Accept lines that end with CR/LF, as well as those that end with\n LF.\n\n Note that the credential-cache--daemon is not touched, because\n it is only about interacting with in-core cache, and there is\n nowhere CRLF comes into the picture.\n\n\n\nNote: I didn't know why we need to touch the daemon, and that is why\nI wrote the paragraph on it as such, but I expect that the real log\nmessage to justify the change would be quite different and explain\nwhy the daemon code needs the same change well---such a description\nwould not be \"Note that\", but would come before the \"Accept lines\nthat end with...\" and after the paragraph about credential-store.\n\nThanks.\n\n> Signed-off-by: Nikita Leonov \n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/credential-cache--daemon.c | 4 ++--\n> builtin/credential-store.c | 2 +-\n> t/t0302-credential-store.sh | 16 ++++++----------\n> 3 files changed, 9 insertions(+), 13 deletions(-)\n>\n> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\n> index c61f123a3b..17664bb0d5 100644\n> --- a/builtin/credential-cache--daemon.c\n> +++ b/builtin/credential-cache--daemon.c\n> @@ -99,12 +99,12 @@ static int read_request(FILE *fh, struct credential *c,\n> \tstatic struct strbuf item = STRBUF_INIT;\n> \tconst char *p;\n> \n> -\tstrbuf_getline_lf(&item, fh);\n> +\tstrbuf_getline(&item, fh);\n> \tif (!skip_prefix(item.buf, \"action=\", &p))\n> \t\treturn error(\"client sent bogus action line: %s\", item.buf);\n> \tstrbuf_addstr(action, p);\n> \n> -\tstrbuf_getline_lf(&item, fh);\n> +\tstrbuf_getline(&item, fh);\n> \tif (!skip_prefix(item.buf, \"timeout=\", &p))\n> \t\treturn error(\"client sent bogus timeout line: %s\", item.buf);\n> \t*timeout = atoi(p);\n> diff --git a/builtin/credential-store.c b/builtin/credential-store.c\n> index 5331ab151a..d4e90b68df 100644\n> --- a/builtin/credential-store.c\n> +++ b/builtin/credential-store.c\n> @@ -23,7 +23,7 @@ static int parse_credential_file(const char *fn,\n> \t\treturn found_credential;\n> \t}\n> \n> -\twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> +\twhile (strbuf_getline(&line, fh) != EOF) {\n> \t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n> \t\t entry.username && entry.password &&\n> \t\t credential_match(c, &entry)) {\n> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n> index 716bf1af9f..f2c672e4b6 100755\n> --- a/t/t0302-credential-store.sh\n> +++ b/t/t0302-credential-store.sh\n> @@ -142,7 +142,7 @@ invalid_credential_test \"scheme\" ://user:pass@example.com\n> invalid_credential_test \"valid host/path\" https://user:pass@\n> invalid_credential_test \"username/password\" https://pass@example.com\n> \n> -test_expect_success 'get: credentials with DOS line endings are invalid' '\n> +test_expect_success 'get: credentials with DOS line endings are valid' '\n> \tprintf \"https://user:pass@example.com\\r\\n\" >\"$HOME/.git-credentials\" &&\n> \tcheck fill store <<-\\EOF\n> \tprotocol=https\n> @@ -150,11 +150,9 @@ test_expect_success 'get: credentials with DOS line endings are invalid' '\n> \t--\n> \tprotocol=https\n> \thost=example.com\n> -\tusername=askpass-username\n> -\tpassword=askpass-password\n> +\tusername=user\n> +\tpassword=pass\n> \t--\n> -\taskpass: Username for '\\''https://example.com'\\'':\n> -\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n> \t--\n> \tEOF\n> '\n> @@ -172,7 +170,7 @@ test_expect_success 'get: credentials with path and DOS line endings are valid'\n> \tEOF\n> '\n> \n> -test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' '\n> +test_expect_success 'get: credentials with DOS line endings are valid if path is relevant' '\n> \tprintf \"https://user:pass@example.com/repo.git\\r\\n\" >\"$HOME/.git-credentials\" &&\n> \ttest_config credential.useHttpPath true &&\n> \tcheck fill store <<-\\EOF\n> @@ -181,11 +179,9 @@ test_expect_success 'get: credentials with DOS line endings are invalid if path\n> \tprotocol=https\n> \thost=example.com\n> \tpath=repo.git\n> -\tusername=askpass-username\n> -\tpassword=askpass-password\n> +\tusername=user\n> +\tpassword=pass\n> \t--\n> -\taskpass: Username for '\\''https://example.com/repo.git'\\'':\n> -\taskpass: Password for '\\''https://askpass-username@example.com/repo.git'\\'':\n> \t--\n> \tEOF\n> '\n"},{"id":"406588","messageId":"CAPUEspgW9CFO3WtbiuTUsmXp05fPqr2Cs81piDJFJ0g3KcTy3A@mail.gmail.com","threadId":"52803","inReplyTo":"f69076036fe4dfe8b57fc1d4329c7be3f7346850.1601293224.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2020-09-28T23:26:38Z","receivedAt":"2020-09-28T23:26:51Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Sep 28, 2020 at 4:41 AM Nikita Leonov via GitGitGadget\n wrote:\n> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n> index 716bf1af9f..f2c672e4b6 100755\n> --- a/t/t0302-credential-store.sh\n> +++ b/t/t0302-credential-store.sh\n> @@ -142,7 +142,7 @@ invalid_credential_test \"scheme\" ://user:pass@example.com\n> invalid_credential_test \"valid host/path\" https://user:pass@\n> invalid_credential_test \"username/password\" https://pass@example.com\n>\n> -test_expect_success 'get: credentials with DOS line endings are invalid' '\n> +test_expect_success 'get: credentials with DOS line endings are valid' '\n> printf \"https://user:pass@example.com\\r\\n\" >\"$HOME/.git-credentials\" &&\n> check fill store <<-\\EOF\n> protocol=https\n> @@ -150,11 +150,9 @@ test_expect_success 'get: credentials with DOS line endings are invalid' '\n> --\n> protocol=https\n> host=example.com\n> - username=askpass-username\n> - password=askpass-password\n> + username=user\n> + password=pass\n> --\n> - askpass: Username for '\\''https://example.com'\\'':\n> - askpass: Password for '\\''https://askpass-username@example.com'\\'':\n> --\n> EOF\n> '\n> @@ -172,7 +170,7 @@ test_expect_success 'get: credentials with path and DOS line endings are valid'\n> EOF\n> '\n>\n> -test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' '\n> +test_expect_success 'get: credentials with DOS line endings are valid if path is relevant' '\n\nnote that this test was put in place to protect users from regressions\nlike the one we got after the release of 2.26.1 where users that had\n'\\r' as part of their credentials were getting an error[1]\n\nwhile I am sympathetic to the change (indeed I proposed something\nsimilar, but was reminded by Peff that while it looks like a text file\nit was designed to be considered opaque and therefore should use UNIX\nLF as record terminator by specification), I am concerned this could\nresult in a similar regression since we know they are still users out\nthere that had modified this file manually (something that was not\nrecommended) and are currently relying on the fact that these lines\nare invalid and therefore silently ignored.\n\nCarlo\n\n[1] https://lore.kernel.org/git/ad80aa0d-3a35-6d7e-7958-b3520e16c855@ed4u.de/\n"},{"id":"406592","messageId":"xmqqd025jwpc.fsf@gitster.c.googlers.com","threadId":"52803","inReplyTo":"CAPUEspgW9CFO3WtbiuTUsmXp05fPqr2Cs81piDJFJ0g3KcTy3A@mail.gmail.com","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-28T23:41:35Z","receivedAt":"2020-09-28T23:41:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n>> -test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' '\n>> +test_expect_success 'get: credentials with DOS line endings are valid if path is relevant' '\n>\n> note that this test was put in place to protect users from regressions\n> like the one we got after the release of 2.26.1 where users that had\n> '\\r' as part of their credentials were getting an error[1]\n>\n> while I am sympathetic to the change (indeed I proposed something\n> similar, but was reminded by Peff that while it looks like a text file\n> it was designed to be considered opaque and therefore should use UNIX\n> LF as record terminator by specification), I am concerned this could\n> result in a similar regression since we know they are still users out\n> there that had modified this file manually (something that was not\n> recommended) and are currently relying on the fact that these lines\n> are invalid and therefore silently ignored.\n>\n> Carlo\n>\n> [1] https://lore.kernel.org/git/ad80aa0d-3a35-6d7e-7958-b3520e16c855@ed4u.de/\n\nI think you meant to remind us and this thread of the earlier review\nand discussion thread, which begins at\n\n https://lore.kernel.org/git/20200426234750.40418-1-carenas@gmail.com/\n\nAnd thanks for doing so---I totally forgot about it.\n\n"},{"id":"406602","messageId":"20200929003000.GA898702@coredump.intra.peff.net","threadId":"52803","inReplyTo":"CAPUEspgW9CFO3WtbiuTUsmXp05fPqr2Cs81piDJFJ0g3KcTy3A@mail.gmail.com","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-29T00:30:00Z","receivedAt":"2020-09-29T00:30:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 28, 2020 at 04:26:38PM -0700, Carlo Arenas wrote:\n\n> > -test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' '\n> > +test_expect_success 'get: credentials with DOS line endings are valid if path is relevant' '\n> \n> note that this test was put in place to protect users from regressions\n> like the one we got after the release of 2.26.1 where users that had\n> '\\r' as part of their credentials were getting an error[1]\n> \n> while I am sympathetic to the change (indeed I proposed something\n> similar, but was reminded by Peff that while it looks like a text file\n> it was designed to be considered opaque and therefore should use UNIX\n> LF as record terminator by specification), I am concerned this could\n> result in a similar regression since we know they are still users out\n> there that had modified this file manually (something that was not\n> recommended) and are currently relying on the fact that these lines\n> are invalid and therefore silently ignored.\n\nGoing over that old thread, I'm not sure that anybody raised a real use\ncase where somebody expected a CR at the end of a line. The discussion\nwas mostly about whether proposed changes would or would not be\ncompatible with existing behavior.\n\nI think that:\n\n - we'd never write a raw CR ourselves, as we'd urlencode the character\n\n - if somebody did put in a raw CR manually like:\n\n https://example.com\\r\\n\n\n then we'd currently fail to match \"example.com\". Which is probably\n not what they wanted. I suspect that \\r in a hostname is bogus\n anyway (certainly curl will complain about it).\n\n - you could do the same in a path:\n\n https://example.com/foo\\r\\n\n\n which _is_ legal, but I think we'd generally ignore it completely\n unless credential.usehttppath is set (for network-accessible\n requests, that is; you could probably point your local cert file at\n something bogus, but I think the main areas of interest here are\n people manipulating the credential protocol via malicious urls).\n\nSo I'm OK loosening the format in the name of convenience, as long as\nwe're confident that it's not opening up any security problems. I can't\nthink of any such problems, though.\n\nIt sounds from your email like you may have found me arguing the\nopposite. That's entirely possible. ;) But I couldn't find it in the\nthread Junio linked.\n\n-Peff\n"},{"id":"406603","messageId":"20200929003519.GB898702@coredump.intra.peff.net","threadId":"52803","inReplyTo":"xmqqk0wdliuc.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-29T00:35:19Z","receivedAt":"2020-09-29T00:35:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 28, 2020 at 01:58:03PM -0700, Junio C Hamano wrote:\n\n> \"Nikita Leonov via GitGitGadget\" writes:\n> \n> > From: Nikita Leonov \n> >\n> > This commit makes reading process regarding credentials compatible with\n> > 'CR/LF' line ending. It makes using git more convenient on systems like\n> > Windows.\n> \n> I can see why this is a good thing for \"store\" and the two updated\n> pieces of the test script demonstrate it very well. \n> \n> But it is unclear why and how cache-daemon benefits from this change.\n> That needs to be justified.\n\nI suspect it doesn't need touched, because it is internal to git-daemon.\nBut it does handle CRLF for some lines, because the first patch\nmodified credential_read(), which the daemon builds on (and which _is_\nuser-facing via git-credential). So there's perhaps an argument that\nthese calls should just be made consistent, even though the only one who\nwould write them is our matching client. If that is the argument to be\nmade, I think it would make sense to do so in a separate patch, since\nthere's no functional change.\n\n(I'm also slightly puzzled that anybody on Windows would care about\ncredential-cache, since it require unix sockets. But maybe in a world of\nWSL people are actually able to mix the two. I confess I haven't kept up\nwith the state of things in Windows).\n\n-Peff\n"},{"id":"406604","messageId":"xmqqwo0difdh.fsf@gitster.c.googlers.com","threadId":"52803","inReplyTo":"20200929003000.GA898702@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-29T00:41:14Z","receivedAt":"2020-09-29T00:41:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think that:\n>\n> - we'd never write a raw CR ourselves, as we'd urlencode the character\n>\n> - if somebody did put in a raw CR manually like:\n>\n> https://example.com\\r\\n\n>\n> then we'd currently fail to match \"example.com\". Which is probably\n> not what they wanted. I suspect that \\r in a hostname is bogus\n> anyway (certainly curl will complain about it).\n\nI may be misremembering, but an argument I recall against the kind\nof change we are dicussing now was that we ignore such an entry\nright now, and the user may have added an entry for the host anew,\npossibly with a more recent password. Changing the parsing to\nignore CR would silently resurrect such a stale entry that the user\nhas written off as unused, and depending on the order of entries in\nthe file, a site that used to work may start failing suddenly.\n\nI don't think I'd care too heavily either way. As long as other\npeople will deal with possible backward-incompatibility fallout,\nI do not mind seeing the change ;-).\n\nI still don't see why we need to touch the cache-daemon, though.\n\nThanks.\n\n"},{"id":"406605","messageId":"20200929004220.GC898702@coredump.intra.peff.net","threadId":"52803","inReplyTo":"27f6400a21412d762b290a34a78ebe7296d36bf3.1601293224.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] credential.c: fix credential reading with regards to CR/LF","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-29T00:42:20Z","receivedAt":"2020-09-29T00:42:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 28, 2020 at 11:40:22AM +0000, Nikita Leonov via GitGitGadget wrote:\n\n> From: Nikita Leonov \n> \n> This fix makes using Git credentials more friendly to Windows users. In\n> previous version it was unable to finish input correctly without\n> configuration changes (tested in PowerShell, CMD, Cygwin).\n> \n> We know credential filling should be finished by empty input, but the\n> current implementation does not take into account CR/LF ending, and\n> hence instead of the empty string we get '\\r', which is interpreted as\n> an incorrect string.\n> \n> So this commit changes default reading function to a more Windows\n> compatible reading function.\n\nUnlike the credential-store file case, where we expect the data to be\nURL-encoded anyway (and so any true \"\\r\" in the data would not be found\nin raw form), this means that the credential protocol can no longer\nrepresent \"\\r\" at the end of a value.\n\nAnd we'd match \"example.com\\r\" and \"example.com\" as the same (unlikely,\nsince carriage returns aren't allowed in hostnames, and curl will\ncomplain about this). We'd also match \"cert://some/path\\r\" and\n\"cert://some/path\". Or \"https://example.com/path\\r\" and its match, if\nyou have credential.useHTTPPath set.\n\nThat may be acceptable if it makes things more convenient. Those are all\npretty obscure cases, and I find it hard to believe an attacker could\nhijack credentials using this (it implies that the only difference\nbetween their malicious url and a known-good one is a trailing CR).\n\nThis part of the commit message confused me a little:\n\n> We know credential filling should be finished by empty input, but the\n> current implementation does not take into account CR/LF ending, and\n> hence instead of the empty string we get '\\r', which is interpreted as\n> an incorrect string.\n\nIf all we care about is the empty line, and not data lines, then we\ncould do this:\n\ndiff --git a/credential.c b/credential.c\nindex efc29dc5e1..73143c5ed0 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -206,7 +206,7 @@ int credential_read(struct credential *c, FILE *fp)\n \t\tchar *key = line.buf;\n \t\tchar *value = strchr(key, '=');\n \n-\t\tif (!line.len)\n+\t\tif (!line.len || (line.len == 1 && line.buf[0] == '\\r'))\n \t\t\tbreak;\n \n \t\tif (!value) {\n\nwithout impacting the ability to send raw CR in the lines with actual\ndata. But I imagine that a trailing CR in all of the data would also\ncause problems.\n\n-Peff\n"},{"id":"406606","messageId":"20200929004448.GD898702@coredump.intra.peff.net","threadId":"52803","inReplyTo":"xmqqwo0difdh.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-29T00:44:48Z","receivedAt":"2020-09-29T00:44:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 28, 2020 at 05:41:14PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I think that:\n> >\n> > - we'd never write a raw CR ourselves, as we'd urlencode the character\n> >\n> > - if somebody did put in a raw CR manually like:\n> >\n> > https://example.com\\r\\n\n> >\n> > then we'd currently fail to match \"example.com\". Which is probably\n> > not what they wanted. I suspect that \\r in a hostname is bogus\n> > anyway (certainly curl will complain about it).\n> \n> I may be misremembering, but an argument I recall against the kind\n> of change we are dicussing now was that we ignore such an entry\n> right now, and the user may have added an entry for the host anew,\n> possibly with a more recent password. Changing the parsing to\n> ignore CR would silently resurrect such a stale entry that the user\n> has written off as unused, and depending on the order of entries in\n> the file, a site that used to work may start failing suddenly.\n\nYeah, that is probably what would happen. I have to admit that it's such\nan obscure case that I'm not sure I really care. It's unlikely in\npractice, and if somebody did report such a case, I think my first\nresponse would be \"well, why did you have a broken entry stuck in your\nfile?\".\n\n> I still don't see why we need to touch the cache-daemon, though.\n\nYeah, I touched on that more in another response.\n\n-Peff\n"},{"id":"406607","messageId":"xmqqsgb1ier6.fsf@gitster.c.googlers.com","threadId":"52803","inReplyTo":"20200929004448.GD898702@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-29T00:54:37Z","receivedAt":"2020-09-29T00:54:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, that is probably what would happen. I have to admit that it's such\n> an obscure case that I'm not sure I really care. It's unlikely in\n> practice, and if somebody did report such a case, I think my first\n> response would be \"well, why did you have a broken entry stuck in your\n> file?\".\n\nI think we know the likely answer. \"I once used Windows to edit the\nfile manually\".\n\nAfter which the file looks broken, so the user may have re-added via\nthe credential API (with LF line ending) a new entry for the host\nand have been happily using the result.\n\n"},{"id":"406609","messageId":"20200929030043.GA905754@coredump.intra.peff.net","threadId":"52803","inReplyTo":"xmqqsgb1ier6.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-29T03:00:43Z","receivedAt":"2020-09-29T03:00:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 28, 2020 at 05:54:37PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Yeah, that is probably what would happen. I have to admit that it's such\n> > an obscure case that I'm not sure I really care. It's unlikely in\n> > practice, and if somebody did report such a case, I think my first\n> > response would be \"well, why did you have a broken entry stuck in your\n> > file?\".\n> \n> I think we know the likely answer. \"I once used Windows to edit the\n> file manually\".\n> \n> After which the file looks broken, so the user may have re-added via\n> the credential API (with LF line ending) a new entry for the host\n> and have been happily using the result.\n\nI wrote something less charitable towards the user at first, and then\ntoned it down. But I think I toned it down too much. Maybe my response\nwould be \"garbage in, garbage out; it was lucky that it worked before\".\n\n-Peff\n"},{"id":"406723","messageId":"xmqqeemigawq.fsf@gitster.c.googlers.com","threadId":"52803","inReplyTo":"20200929030043.GA905754@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-30T22:25:09Z","receivedAt":"2020-09-30T22:25:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Sep 28, 2020 at 05:54:37PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > Yeah, that is probably what would happen. I have to admit that it's such\n>> > an obscure case that I'm not sure I really care. It's unlikely in\n>> > practice, and if somebody did report such a case, I think my first\n>> > response would be \"well, why did you have a broken entry stuck in your\n>> > file?\".\n>> \n>> I think we know the likely answer. \"I once used Windows to edit the\n>> file manually\".\n>> \n>> After which the file looks broken, so the user may have re-added via\n>> the credential API (with LF line ending) a new entry for the host\n>> and have been happily using the result.\n>\n> I wrote something less charitable towards the user at first, and then\n> toned it down. But I think I toned it down too much. Maybe my response\n> would be \"garbage in, garbage out; it was lucky that it worked before\".\n\nOK, so what's the final verdict from us? This is good enough to\nmove forward as-is?\n\n"},{"id":"406724","messageId":"xmqqa6x6gajn.fsf@gitster.c.googlers.com","threadId":"52803","inReplyTo":"20200929003519.GB898702@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-30T22:33:00Z","receivedAt":"2020-09-30T22:33:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Sep 28, 2020 at 01:58:03PM -0700, Junio C Hamano wrote:\n>\n>> \"Nikita Leonov via GitGitGadget\" writes:\n>> \n>> > From: Nikita Leonov \n>> >\n>> > This commit makes reading process regarding credentials compatible with\n>> > 'CR/LF' line ending. It makes using git more convenient on systems like\n>> > Windows.\n>> \n>> I can see why this is a good thing for \"store\" and the two updated\n>> pieces of the test script demonstrate it very well. \n>> \n>> But it is unclear why and how cache-daemon benefits from this change.\n>> That needs to be justified.\n>\n> I suspect it doesn't need touched, because it is internal to git-daemon.\n> But it does handle CRLF for some lines, because the first patch\n> modified credential_read(), which the daemon builds on (and which _is_\n> user-facing via git-credential). So there's perhaps an argument that\n> these calls should just be made consistent, even though the only one who\n> would write them is our matching client. If that is the argument to be\n> made, I think it would make sense to do so in a separate patch, since\n> there's no functional change.\n>\n> (I'm also slightly puzzled that anybody on Windows would care about\n> credential-cache, since it require unix sockets. But maybe in a world of\n> WSL people are actually able to mix the two. I confess I haven't kept up\n> with the state of things in Windows).\n\nTrue, so some, if not all, parts of these changes start to look more\nand more like \"I change LF to CRLF purely for political correctness,\neven I know nobody sends CRLF in these cases (or \"even I do not know\nif anybody sends CRLF in these cases\", which essentially amounts to\nthe same thing but may be worse)\", needless code churns.\n\nSigh.\n"},{"id":"406726","messageId":"20200930223913.GB1908000@coredump.intra.peff.net","threadId":"52803","inReplyTo":"xmqqeemigawq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-30T22:39:13Z","receivedAt":"2020-09-30T22:39:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 30, 2020 at 03:25:09PM -0700, Junio C Hamano wrote:\n\n> OK, so what's the final verdict from us? This is good enough to\n> move forward as-is?\n\nI'd prefer to see the credential-cache--daemon changes either dropped,\nor split out into a separate patch with the justification of: this\nprobably doesn't matter in practice, but it makes the whole protocol\nbetween client and server treat CRLF consistently.\n\nI had also raised a question in:\n\n https://lore.kernel.org/git/20200929004220.GC898702@coredump.intra.peff.net/\n\nabout whether they just care about the empty line, or about CRLF on data\nlines. If the former (which is what the commit message claims), then I\nthink we can do something much simpler. But I suspect it is the latter,\nand it is simply the commit message that is a bit misleading.\n\nOther than those nits, I think the series is OK.\n\n-Peff\n"},{"id":"406729","messageId":"xmqq5z7ug9gk.fsf@gitster.c.googlers.com","threadId":"52803","inReplyTo":"20200930223913.GB1908000@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-30T22:56:27Z","receivedAt":"2020-09-30T22:56:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Sep 30, 2020 at 03:25:09PM -0700, Junio C Hamano wrote:\n>\n>> OK, so what's the final verdict from us? This is good enough to\n>> move forward as-is?\n>\n> I'd prefer to see the credential-cache--daemon changes either dropped,\n> or split out into a separate patch with the justification of: this\n> probably doesn't matter in practice, but it makes the whole protocol\n> between client and server treat CRLF consistently.\n>\n> I had also raised a question in:\n>\n> https://lore.kernel.org/git/20200929004220.GC898702@coredump.intra.peff.net/\n>\n> about whether they just care about the empty line, or about CRLF on data\n> lines. If the former (which is what the commit message claims), then I\n> think we can do something much simpler. But I suspect it is the latter,\n> and it is simply the commit message that is a bit misleading.\n>\n> Other than those nits, I think the series is OK.\n\nSure. But credential-store side is also iffy; it is not like they\nwant CRLF on data lines (if they want CR in data, that needs to be\nencoded). The only reason I can think of that the change to\n\"-store\" makes any difference is when people edit it, but the file\nis not designed to be manually edited, so even that part of the\nseries needs a better justification. It's not like \"We want to be\ncompatible\" without \"why it is better to be compatible\" is a good\nrationale, when we define the file format not to be manually edited\nin the first place.\n\nThanks.\n"},{"id":"406761","messageId":"20201001135417.GA3145610@coredump.intra.peff.net","threadId":"52803","inReplyTo":"xmqq5z7ug9gk.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-01T13:54:17Z","receivedAt":"2020-10-01T13:54:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 30, 2020 at 03:56:27PM -0700, Junio C Hamano wrote:\n\n> > Other than those nits, I think the series is OK.\n> \n> Sure. But credential-store side is also iffy; it is not like they\n> want CRLF on data lines (if they want CR in data, that needs to be\n> encoded). The only reason I can think of that the change to\n> \"-store\" makes any difference is when people edit it, but the file\n> is not designed to be manually edited, so even that part of the\n> series needs a better justification. It's not like \"We want to be\n> compatible\" without \"why it is better to be compatible\" is a good\n> rationale, when we define the file format not to be manually edited\n> in the first place.\n\nYeah, I agree that just teaching git-credential's stdin to handle CR\nwould be an OK stopping point. I don't have a strong opinion on\ncredential-store's on disk format. At least allowing CRLF there is\n_plausibly_ useful, unlike credential-cache--daemon's pipe. And I doubt\nthat making the change would hurt anybody.\n\n-Peff\n"},{"id":"406762","messageId":"xmqqpn62c5q7.fsf@gitster.c.googlers.com","threadId":"52803","inReplyTo":"20201001135417.GA3145610@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-01T15:42:56Z","receivedAt":"2020-10-01T15:43:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I agree that just teaching git-credential's stdin to handle CR\n> would be an OK stopping point. I don't have a strong opinion on\n> credential-store's on disk format. At least allowing CRLF there is\n> _plausibly_ useful, unlike credential-cache--daemon's pipe. And I doubt\n> that making the change would hurt anybody.\n\nI agree 100% with all of the above, including the part that says it\nis also OK to let credential-store read from its files with line\nending converted to CRLF.\n\nThanks.\n\n"},{"id":"406809","messageId":"nycvar.QRO.7.76.6.2010020952330.50@tvgsbejvaqbjf.bet","threadId":"52803","inReplyTo":"xmqqa6x6gajn.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-02T07:53:46Z","receivedAt":"2020-10-02T11:04:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio & Peff,\n\nOn Wed, 30 Sep 2020, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n> > On Mon, Sep 28, 2020 at 01:58:03PM -0700, Junio C Hamano wrote:\n> >\n> >> \"Nikita Leonov via GitGitGadget\" writes:\n> >>\n> >> > From: Nikita Leonov \n> >> >\n> >> > This commit makes reading process regarding credentials compatible with\n> >> > 'CR/LF' line ending. It makes using git more convenient on systems like\n> >> > Windows.\n> >>\n> >> I can see why this is a good thing for \"store\" and the two updated\n> >> pieces of the test script demonstrate it very well.\n> >>\n> >> But it is unclear why and how cache-daemon benefits from this change.\n> >> That needs to be justified.\n> >\n> > I suspect it doesn't need touched, because it is internal to git-daemon.\n> > But it does handle CRLF for some lines, because the first patch\n> > modified credential_read(), which the daemon builds on (and which _is_\n> > user-facing via git-credential). So there's perhaps an argument that\n> > these calls should just be made consistent, even though the only one who\n> > would write them is our matching client. If that is the argument to be\n> > made, I think it would make sense to do so in a separate patch, since\n> > there's no functional change.\n> >\n> > (I'm also slightly puzzled that anybody on Windows would care about\n> > credential-cache, since it require unix sockets. But maybe in a world of\n> > WSL people are actually able to mix the two. I confess I haven't kept up\n> > with the state of things in Windows).\n>\n> True, so some, if not all, parts of these changes start to look more\n> and more like \"I change LF to CRLF purely for political correctness,\n> even I know nobody sends CRLF in these cases (or \"even I do not know\n> if anybody sends CRLF in these cases\", which essentially amounts to\n> the same thing but may be worse)\", needless code churns.\n\nIndeed. I did not make the connection that `credential-cache--daemon` uses\nUnix sockets internally (and hence if you try to use it on Windows, you\nare greeted with the message \"credential-cache--daemon unavailable; no\nunix socket support\").\n\nWill drop this part from the next iteration.\n\nCiao,\nDscho\n"},{"id":"406811","messageId":"nycvar.QRO.7.76.6.2010021004080.50@tvgsbejvaqbjf.bet","threadId":"52803","inReplyTo":"xmqqpn62c5q7.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] credentials: make line reading Windows compatible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-02T08:07:17Z","receivedAt":"2020-10-02T11:17:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 1 Oct 2020, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n> > Yeah, I agree that just teaching git-credential's stdin to handle CR\n> > would be an OK stopping point. I don't have a strong opinion on\n> > credential-store's on disk format. At least allowing CRLF there is\n> > _plausibly_ useful, unlike credential-cache--daemon's pipe. And I doubt\n> > that making the change would hurt anybody.\n>\n> I agree 100% with all of the above, including the part that says it\n> is also OK to let credential-store read from its files with line\n> ending converted to CRLF.\n\nAfter mulling over this, I am more inclined to drop the credential-store\nchanges, too. That file _is_ an implementation detail, and not intended\nfor interactive editing. And as Carlo pointed out so correctly, the\nregression tests were not introduced \"just for fun\", they really want to\nmake sure that we keep handling CR/LF the way we currently do.\n\nSo I will drop both credential-cache--daemon and credential-store changes\nfrom the next iteration.\n\nCiao,\nDscho\n"},{"id":"406812","messageId":"nycvar.QRO.7.76.6.2010021011540.50@tvgsbejvaqbjf.bet","threadId":"52803","inReplyTo":"20200929004220.GC898702@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/3] credential.c: fix credential reading with regards to CR/LF","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-02T11:37:23Z","receivedAt":"2020-10-02T11:37:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 28 Sep 2020, Jeff King wrote:\n\n> On Mon, Sep 28, 2020 at 11:40:22AM +0000, Nikita Leonov via GitGitGadget wrote:\n>\n> > From: Nikita Leonov \n> >\n> > This fix makes using Git credentials more friendly to Windows users. In\n> > previous version it was unable to finish input correctly without\n> > configuration changes (tested in PowerShell, CMD, Cygwin).\n> >\n> > We know credential filling should be finished by empty input, but the\n> > current implementation does not take into account CR/LF ending, and\n> > hence instead of the empty string we get '\\r', which is interpreted as\n> > an incorrect string.\n> >\n> > So this commit changes default reading function to a more Windows\n> > compatible reading function.\n>\n> Unlike the credential-store file case, where we expect the data to be\n> URL-encoded anyway (and so any true \"\\r\" in the data would not be found\n> in raw form), this means that the credential protocol can no longer\n> represent \"\\r\" at the end of a value.\n\nIndeed.\n\n> And we'd match \"example.com\\r\" and \"example.com\" as the same (unlikely,\n> since carriage returns aren't allowed in hostnames, and curl will\n> complain about this). We'd also match \"cert://some/path\\r\" and\n> \"cert://some/path\". Or \"https://example.com/path\\r\" and its match, if\n> you have credential.useHTTPPath set.\n\nTrue. It's a problem with all of those URLs that end in Carriage Returns\n;-)\n\n> That may be acceptable if it makes things more convenient. Those are all\n> pretty obscure cases, and I find it hard to believe an attacker could\n> hijack credentials using this (it implies that the only difference\n> between their malicious url and a known-good one is a trailing CR).\n\nIndeed.\n\n> This part of the commit message confused me a little:\n>\n> > We know credential filling should be finished by empty input, but the\n> > current implementation does not take into account CR/LF ending, and\n> > hence instead of the empty string we get '\\r', which is interpreted as\n> > an incorrect string.\n>\n> If all we care about is the empty line, and not data lines, then we\n> could do this:\n>\n> diff --git a/credential.c b/credential.c\n> index efc29dc5e1..73143c5ed0 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -206,7 +206,7 @@ int credential_read(struct credential *c, FILE *fp)\n> \t\tchar *key = line.buf;\n> \t\tchar *value = strchr(key, '=');\n>\n> -\t\tif (!line.len)\n> +\t\tif (!line.len || (line.len == 1 && line.buf[0] == '\\r'))\n> \t\t\tbreak;\n>\n> \t\tif (!value) {\n>\n> without impacting the ability to send raw CR in the lines with actual\n> data. But I imagine that a trailing CR in all of the data would also\n> cause problems.\n\nI checked again with github.com/git-for-windows/git/pull/2516, where the\npatch originally entered the public eye, but could not find any background\ninformation.\n\nBut I would highly doubt that the empty lines were the biggest problem:\nSure, we would fail to recognize an empty line with CR/LF line endings\nwhen reading with `strbuf_getline_lf()`, but we would totally\nmisunderstand the entire rest of the lines, too. For example, we would\nmistake `quit\\r` for an unknown command, and hence simply ignore it.\n\nI do agree, however, that your confusion validly points out a flaw in the\ncommit message: the \"empty line\" comment is a red herring.\n\nTherefore, I spent some time pouring over the commit message. This is my\ncurrent version:\n\n credential: treat CR/LF as line endings in the credential protocol\n\n This fix makes using Git credentials more friendly to Windows users: it\n allows a credential helper to communicate using CR/LF line endings (\"DOS\n line endings\" commonly found on Windows) instead of LF-only line endings\n (\"Unix line endings\").\n\n Note that this changes the behavior a bit: if a credential helper\n produces, say, a password with a trailing Carriage Return character,\n that will now be culled even when the rest of the lines end only in Line\n Feed characters, indicating that the Carriage Return was not meant to be\n part of the line ending.\n\n In practice, it seems _very_ unlikely that something like this happens.\n Passwords usually need to consist of non-control characters, URLs need\n to have special characters URL-encoded, and user names, well, are names.\n\n So let's change the credential machinery to accept both CR/LF and LF\n line endings.\n\n While we do this for the credential helper protocol, we do _not_ do\n adjust `git credential-cache--daemon` (which won't work on Windows,\n anyway, because it requires Unix sockets) nor `git credential-store`\n (which writes the file `~/.git-credentials` which we consider an\n implementation detail that should be opaque to the user, read: we do\n expect users _not_ to edit this file manually).\n\nWhat do you think?\n\nCiao,\nDscho\n"},{"id":"406814","messageId":"20201002120126.GA2255314@coredump.intra.peff.net","threadId":"52803","inReplyTo":"nycvar.QRO.7.76.6.2010021011540.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/3] credential.c: fix credential reading with regards to CR/LF","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-02T12:01:26Z","receivedAt":"2020-10-02T12:01:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 02, 2020 at 01:37:23PM +0200, Johannes Schindelin wrote:\n\n> But I would highly doubt that the empty lines were the biggest problem:\n> Sure, we would fail to recognize an empty line with CR/LF line endings\n> when reading with `strbuf_getline_lf()`, but we would totally\n> misunderstand the entire rest of the lines, too. For example, we would\n> mistake `quit\\r` for an unknown command, and hence simply ignore it.\n> \n> I do agree, however, that your confusion validly points out a flaw in the\n> commit message: the \"empty line\" comment is a red herring.\n> \n> Therefore, I spent some time pouring over the commit message. This is my\n> current version:\n> [...]\n> What do you think?\n\nI think we are on the same page, and this revision does a good job of\nfixing my complaint about the commit message. Thanks. One minor typo:\n\n> While we do this for the credential helper protocol, we do _not_ do\n> adjust `git credential-cache--daemon` (which won't work on Windows,\n\ns/do$//\n\n-Peff\n"},{"id":"406815","messageId":"nycvar.QRO.7.76.6.2010021427080.50@tvgsbejvaqbjf.bet","threadId":"52803","inReplyTo":"20201002120126.GA2255314@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/3] credential.c: fix credential reading with regards to CR/LF","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-02T12:27:23Z","receivedAt":"2020-10-02T12:27:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 2 Oct 2020, Jeff King wrote:\n\n> On Fri, Oct 02, 2020 at 01:37:23PM +0200, Johannes Schindelin wrote:\n>\n> > But I would highly doubt that the empty lines were the biggest problem:\n> > Sure, we would fail to recognize an empty line with CR/LF line endings\n> > when reading with `strbuf_getline_lf()`, but we would totally\n> > misunderstand the entire rest of the lines, too. For example, we would\n> > mistake `quit\\r` for an unknown command, and hence simply ignore it.\n> >\n> > I do agree, however, that your confusion validly points out a flaw in the\n> > commit message: the \"empty line\" comment is a red herring.\n> >\n> > Therefore, I spent some time pouring over the commit message. This is my\n> > current version:\n> > [...]\n> > What do you think?\n>\n> I think we are on the same page, and this revision does a good job of\n> fixing my complaint about the commit message. Thanks. One minor typo:\n>\n> > While we do this for the credential helper protocol, we do _not_ do\n> > adjust `git credential-cache--daemon` (which won't work on Windows,\n>\n> s/do$//\n\nThanks, fixed!\n\nCiao,\nDscho\n"},{"id":"406827","messageId":"xmqqk0w8a8re.fsf@gitster.c.googlers.com","threadId":"52803","inReplyTo":"nycvar.QRO.7.76.6.2010021011540.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/3] credential.c: fix credential reading with regards to CR/LF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-02T16:32:37Z","receivedAt":"2020-10-02T16:32:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Therefore, I spent some time pouring over the commit message. This is my\n> current version:\n>\n> credential: treat CR/LF as line endings in the credential protocol\n>\n> This fix makes using Git credentials more friendly to Windows users: it\n> allows a credential helper to communicate using CR/LF line endings (\"DOS\n> line endings\" commonly found on Windows) instead of LF-only line endings\n> (\"Unix line endings\").\n>\n> Note that this changes the behavior a bit: if a credential helper\n> produces, say, a password with a trailing Carriage Return character,\n> that will now be culled even when the rest of the lines end only in Line\n> Feed characters, indicating that the Carriage Return was not meant to be\n> part of the line ending.\n>\n> In practice, it seems _very_ unlikely that something like this happens.\n> Passwords usually need to consist of non-control characters, URLs need\n> to have special characters URL-encoded, and user names, well, are names.\n>\n> So let's change the credential machinery to accept both CR/LF and LF\n> line endings.\n>\n> While we do this for the credential helper protocol, we do _not_ do\n> adjust `git credential-cache--daemon` (which won't work on Windows,\n> anyway, because it requires Unix sockets) nor `git credential-store`\n> (which writes the file `~/.git-credentials` which we consider an\n> implementation detail that should be opaque to the user, read: we do\n> expect users _not_ to edit this file manually).\n>\n> What do you think?\n\nI am not Peff, but I was also drawn into the same confusion by the\n\"we never see an empty line\" red herring. \n\nThere are some micronits, but the above made a lot easier to\nunderstand (I think you could even add \"quit\\r\" bit to make it even\neasier to understand) than the original description.\n\nThanks.\n"},{"id":"406865","messageId":"nycvar.QRO.7.76.6.2010031527200.50@tvgsbejvaqbjf.bet","threadId":"52803","inReplyTo":"xmqqk0w8a8re.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/3] credential.c: fix credential reading with regards to CR/LF","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-03T13:28:09Z","receivedAt":"2020-10-03T13:28:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 2 Oct 2020, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > Therefore, I spent some time pouring over the commit message. This is my\n> > current version:\n> >\n> > credential: treat CR/LF as line endings in the credential protocol\n> >\n> > This fix makes using Git credentials more friendly to Windows users: it\n> > allows a credential helper to communicate using CR/LF line endings (\"DOS\n> > line endings\" commonly found on Windows) instead of LF-only line endings\n> > (\"Unix line endings\").\n> >\n> > Note that this changes the behavior a bit: if a credential helper\n> > produces, say, a password with a trailing Carriage Return character,\n> > that will now be culled even when the rest of the lines end only in Line\n> > Feed characters, indicating that the Carriage Return was not meant to be\n> > part of the line ending.\n> >\n> > In practice, it seems _very_ unlikely that something like this happens.\n> > Passwords usually need to consist of non-control characters, URLs need\n> > to have special characters URL-encoded, and user names, well, are names.\n> >\n> > So let's change the credential machinery to accept both CR/LF and LF\n> > line endings.\n> >\n> > While we do this for the credential helper protocol, we do _not_ do\n> > adjust `git credential-cache--daemon` (which won't work on Windows,\n> > anyway, because it requires Unix sockets) nor `git credential-store`\n> > (which writes the file `~/.git-credentials` which we consider an\n> > implementation detail that should be opaque to the user, read: we do\n> > expect users _not_ to edit this file manually).\n> >\n> > What do you think?\n>\n> I am not Peff, but I was also drawn into the same confusion by the\n> \"we never see an empty line\" red herring.\n\n:-)\n\n> There are some micronits, but the above made a lot easier to\n> understand (I think you could even add \"quit\\r\" bit to make it even\n> easier to understand) than the original description.\n\nOkay, I incorporated a comment talking about `quit\\r` and will submit\na new iteration right now.\n\nThanks,\nDscho\n"},{"id":"406866","messageId":"pull.710.v3.git.git.1601731752695.gitgitgadget@gmail.com","threadId":"52803","inReplyTo":"pull.710.v2.git.git.1601293224.gitgitgadget@gmail.com","subject":"[PATCH v3] credential: treat CR/LF as line endings in the credential protocol","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-03T13:29:12Z","receivedAt":"2020-10-03T13:29:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Nikita Leonov \n\nThis fix makes using Git credentials more friendly to Windows users: it\nallows a credential helper to communicate using CR/LF line endings (\"DOS\nline endings\" commonly found on Windows) instead of LF-only line endings\n(\"Unix line endings\").\n\nNote that this changes the behavior a bit: if a credential helper\nproduces, say, a password with a trailing Carriage Return character,\nthat will now be culled even when the rest of the lines end only in Line\nFeed characters, indicating that the Carriage Return was not meant to be\npart of the line ending.\n\nIn practice, it seems _very_ unlikely that something like this happens.\nPasswords usually need to consist of non-control characters, URLs need\nto have special characters URL-encoded, and user names, well, are names.\n\nHowever, it _does_ help on Windows, where CR/LF line endings are common:\nas unrecognized commands are simply ignored by the credential machinery,\neven a command like `quit\\r` (which is clearly intended to abort) would\nsimply be ignored (silently) by Git.\n\nSo let's change the credential machinery to accept both CR/LF and LF\nline endings.\n\nWhile we do this for the credential helper protocol, we do _not_ adjust\n`git credential-cache--daemon` (which won't work on Windows, anyway,\nbecause it requires Unix sockets) nor `git credential-store` (which\nwrites the file `~/.git-credentials` which we consider an implementation\ndetail that should be opaque to the user, read: we do expect users _not_\nto edit this file manually).\n\nSigned-off-by: Nikita Leonov \nSigned-off-by: Johannes Schindelin \n---\n Prepare git credential to read input with DOS line endings\n \n This contribution came in via Git for Windows\n [https://github.com/git-for-windows/git/pull/2516].\n \n Sadly, I did not find the time to go through all the changes of 8f309aeb\n (\"strbuf: introduce strbuf_getline_{lf,nul}()\", 2016-01-13) (as Junio\n asked\n [https://public-inbox.org/git/xmqqmu9lnjdh.fsf@gitster-ct.c.googlers.com]\n ). Rather than delaying this patch indefinitely, I admit defeat on that\n angle.\n \n Changes since v2:\n \n * Dropped the credential-cache--daemon and credential-store changes\n again.\n * Enhanced the commit message (also explaining why we don't touch the\n daemon and the store).\n \n Changes since v1:\n \n * Added a commit to adjust credential-daemon and credential-store in\n the same manner.\n * Adjusted the documentation accordingly.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-710%2Fdscho%2Fcrlf-aware-git-credential-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-710/dscho/crlf-aware-git-credential-v3\nPull-Request: https://github.com/git/git/pull/710\n\nRange-diff vs v2:\n\n 1: 27f6400a21 ! 1: f6eeb18d3a credential.c: fix credential reading with regards to CR/LF\n @@ Metadata\n Author: Nikita Leonov \n \n ## Commit message ##\n - credential.c: fix credential reading with regards to CR/LF\n + credential: treat CR/LF as line endings in the credential protocol\n \n - This fix makes using Git credentials more friendly to Windows users. In\n - previous version it was unable to finish input correctly without\n - configuration changes (tested in PowerShell, CMD, Cygwin).\n + This fix makes using Git credentials more friendly to Windows users: it\n + allows a credential helper to communicate using CR/LF line endings (\"DOS\n + line endings\" commonly found on Windows) instead of LF-only line endings\n + (\"Unix line endings\").\n \n - We know credential filling should be finished by empty input, but the\n - current implementation does not take into account CR/LF ending, and\n - hence instead of the empty string we get '\\r', which is interpreted as\n - an incorrect string.\n + Note that this changes the behavior a bit: if a credential helper\n + produces, say, a password with a trailing Carriage Return character,\n + that will now be culled even when the rest of the lines end only in Line\n + Feed characters, indicating that the Carriage Return was not meant to be\n + part of the line ending.\n \n - So this commit changes default reading function to a more Windows\n - compatible reading function.\n + In practice, it seems _very_ unlikely that something like this happens.\n + Passwords usually need to consist of non-control characters, URLs need\n + to have special characters URL-encoded, and user names, well, are names.\n +\n + However, it _does_ help on Windows, where CR/LF line endings are common:\n + as unrecognized commands are simply ignored by the credential machinery,\n + even a command like `quit\\r` (which is clearly intended to abort) would\n + simply be ignored (silently) by Git.\n +\n + So let's change the credential machinery to accept both CR/LF and LF\n + line endings.\n +\n + While we do this for the credential helper protocol, we do _not_ adjust\n + `git credential-cache--daemon` (which won't work on Windows, anyway,\n + because it requires Unix sockets) nor `git credential-store` (which\n + writes the file `~/.git-credentials` which we consider an implementation\n + detail that should be opaque to the user, read: we do expect users _not_\n + to edit this file manually).\n \n Signed-off-by: Nikita Leonov \n Signed-off-by: Johannes Schindelin \n 2: f69076036f < -: ---------- credentials: make line reading Windows compatible\n 3: 61baea1061 < -: ---------- docs: make notes regarding credential line reading\n\n\n credential.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/credential.c b/credential.c\nindex efc29dc5e1..e5202fbef2 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -202,7 +202,7 @@ int credential_read(struct credential *c, FILE *fp)\n {\n \tstruct strbuf line = STRBUF_INIT;\n \n-\twhile (strbuf_getline_lf(&line, fp) != EOF) {\n+\twhile (strbuf_getline(&line, fp) != EOF) {\n \t\tchar *key = line.buf;\n \t\tchar *value = strchr(key, '=');\n \n\nbase-commit: 9bc233ae1cf19a49e51842c7959d80a675dbd1c0\n-- \ngitgitgadget\n"}]}