{"thread":{"id":"58012","subject":"[RFC PATCH 0/5] fix issues in transfer.credentialsInUrl","startedAt":"2022-06-15T10:53:42Z","lastAt":"2022-06-15T12:52:47Z","messageCount":9,"participants":["Ævar Arnfjörð Bjarmason","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"457263","messageId":"RFC-cover-0.5-00000000000-20220615T104503Z-avarab@gmail.com","threadId":"58012","inReplyTo":null,"subject":"[RFC PATCH 0/5] fix issues in transfer.credentialsInUrl","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-15T10:53:27Z","receivedAt":"2022-06-15T10:53:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This is on top of [1], and given the \"rc\" phase is an RFC. This:\n\n * Fixes the issue of the transfer.credentialsInUrl (now renamed) not\n   finding passwords in \"pushurl\" URLs (in my case, the only place\n   where I'd actually put a password in a URL in a config...)\n\n * 1/5 fixes a bug in an existing test, but I didn't think it was\n   worth bothering with for 2.37.0.\n\n * Adds missing test coverage for reading the config from a file, not\n   the CLI.\n\n * 3/5 is a WIP CI target to spot the type of issue I fixed in [2],\n   it's not the first time where we have a NO_CURL=Y breakage land on\n   master...\n\n * 4/5 attemps to \"really\" fix the duplicate warnings we emit, I think\n   the approach there is good, especially the part where we shouldn't\n   emit it twice in-process.\n\n   But this currently misses e.g. \"git ls-remote\". I wonder if we\n   should just stick that git_config_push_parameter() condition into\n   packet_trace_identity() and call it a day.\n\n * 5/5 fixes the (major) blind spot of the warning missing \"pushurl\" config.\n\nI think this is all non-RFC quality, except the \"ls-remote\" case, and\nus missing tests for that & other transport users that aren't\nclone/fetch/push.\n\nDerrick: Are you interested in picking this up & pursuing it after the\nrelease, with whatever fix-ups/rewrites etc. that you find\nappropriate?\n\n1. https://lore.kernel.org/git/cover-0.2-00000000000-20220615T103852Z-avarab@gmail.com/\n2. https://lore.kernel.org/git/cover-0.1-00000000000-20220615T103609Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (5):\n  push tests: add a missing \"test_line_count\"\n  fetch+push tests: add missing coverage for 6dcbdc0d661\n  CI: add a linux-BUILD-vars job\n  fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings\n  transport: check remote.<name>pushurl with transfer.credentialsInUrl\n\n .github/workflows/main.yml |  3 ++\n builtin/clone.c            |  5 ++-\n builtin/fetch.c            |  4 ++\n builtin/push.c             |  6 ++-\n ci/run-build-and-tests.sh  | 30 ++++++++++++++\n remote.c                   | 82 +++++++++++++++++++++++++++-----------\n remote.h                   | 14 +++++++\n t/t5516-fetch-push.sh      | 46 ++++++++++++++++++++-\n t/t5601-clone.sh           |  2 +-\n 9 files changed, 164 insertions(+), 28 deletions(-)\n\n-- \n2.36.1.1239.gfba91521d90\n\n"},{"id":"457264","messageId":"RFC-patch-1.5-a1245bd3139-20220615T104503Z-avarab@gmail.com","threadId":"58012","inReplyTo":"RFC-cover-0.5-00000000000-20220615T104503Z-avarab@gmail.com","subject":"[RFC PATCH 1/5] push tests: add a missing \"test_line_count\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-15T10:53:28Z","receivedAt":"2022-06-15T10:53:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a \"test_line_count\" missing from 6dcbdc0d661, we'd clobber\n\"warnings\" here, but never test its contents.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t5516-fetch-push.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 79d8a7b3675..4b32ae39a39 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1860,6 +1860,8 @@ test_expect_success 'push warns or fails when using username:password' '\n \n \ttest_must_fail git -c transfer.credentialsInUrl=warn push https://username:password@localhost 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings &&\n+\n \ttest_must_fail git -c transfer.credentialsInUrl=die push https://username:password@localhost 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n \ttest_line_count = 1 warnings\n-- \n2.36.1.1239.gfba91521d90\n\n"},{"id":"457265","messageId":"RFC-patch-2.5-38af32c5b96-20220615T104503Z-avarab@gmail.com","threadId":"58012","inReplyTo":"RFC-cover-0.5-00000000000-20220615T104503Z-avarab@gmail.com","subject":"[RFC PATCH 2/5] fetch+push tests: add missing coverage for 6dcbdc0d661","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-15T10:53:29Z","receivedAt":"2022-06-15T10:53:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add tests that were missing from 6dcbdc0d661 (remote: create\nfetch.credentialsInUrl config, 2022-06-06), we want to test how we\nhandle cases where the config comes from a file, and that we handle\n\"pushURL\" correctly.\n\nCurrently the \"pushURL\" case isn't handled at all, i.e. URLs aren't\nwarned about in \"remote.*pushurl\" , only for \"remote.*.url\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t5516-fetch-push.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 41 insertions(+)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 4b32ae39a39..51d695e475a 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1852,6 +1852,26 @@ test_expect_success 'fetch warns or fails when using username:password' '\n \ttest_line_count = 1 warnings\n '\n \n+test_expect_success CURL 'fetch warns or fails when using username:password in config' '\n+\tmessage=\"URL '\\''https://username:<redacted>@localhost/'\\'' uses plaintext credentials\" &&\n+\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\ttest_commit -C repo A &&\n+\tgit -C repo remote add pwd-url https://username:password@localhost &&\n+\ttest_must_fail git -C repo -c transfer.credentialsInUrl=allow fetch pwd-url 2>err &&\n+\t! grep \"$message\" err &&\n+\n+\ttest_must_fail git -C repo -c transfer.credentialsInUrl=warn fetch pwd-url 2>err &&\n+\tgrep \"warning: $message\" err >warnings &&\n+\ttest_line_count = 3 warnings &&\n+\n+\tgit -C repo remote set-url --push pwd-url https://username:password@localhost &&\n+\tgit -C repo remote set-url pwd-url https://localhost &&\n+\n+\ttest_must_fail git -C repo -c transfer.credentialsInUrl=warn fetch pwd-url 2>err &&\n+\t! grep \"fatal: $message\" err\n+'\n \n test_expect_success 'push warns or fails when using username:password' '\n \tmessage=\"URL '\\''https://username:<redacted>@localhost/'\\'' uses plaintext credentials\" &&\n@@ -1867,4 +1887,25 @@ test_expect_success 'push warns or fails when using username:password' '\n \ttest_line_count = 1 warnings\n '\n \n+test_expect_success CURL 'push warns or fails when using username:password in config' '\n+\tmessage=\"URL '\\''https://username:<redacted>@localhost/'\\'' uses plaintext credentials\" &&\n+\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\ttest_commit -C repo A &&\n+\tgit -C repo remote add pwd-url https://username:password@localhost &&\n+\ttest_must_fail git -C repo -c transfer.credentialsInUrl=allow push pwd-url HEAD:refs/heads/branch 2>err &&\n+\t! grep \"$message\" err &&\n+\n+\ttest_must_fail git -C repo -c transfer.credentialsInUrl=warn push pwd-url HEAD:refs/heads/branch 2>err &&\n+\tgrep \"warning: $message\" err >warnings &&\n+\ttest_line_count = 2 warnings &&\n+\n+\tgit -C repo remote set-url --push pwd-url https://username:password@localhost &&\n+\tgit -C repo remote set-url pwd-url https://localhost &&\n+\n+\ttest_must_fail git -C repo -c transfer.credentialsInUrl=warn push pwd-url HEAD:refs/heads/branch 2>err &&\n+\t! grep \"warning: $message\" err\n+'\n+\n test_done\n-- \n2.36.1.1239.gfba91521d90\n\n"},{"id":"457266","messageId":"RFC-patch-3.5-d69368c5b39-20220615T104503Z-avarab@gmail.com","threadId":"58012","inReplyTo":"RFC-cover-0.5-00000000000-20220615T104503Z-avarab@gmail.com","subject":"[RFC PATCH 3/5] CI: add a linux-BUILD-vars job","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-15T10:53:30Z","receivedAt":"2022-06-15T10:53:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a linux-BUILD-vars job, in a preceding commit we fixed a bug that\nwould have been spotted by testing under NO_CURL=Y.\n\nThis CI job attempts to unset various settings in config.mak.uname and\nthe Makefile, so that we'll stress our fallbacks and conditionally\ncompiled code as much as possible.\n\nIf there is a missing setting here that we can enable under\n\"ubuntu-latest\" the omission isn't intentional, this list came from a\nquick skimming of the relevant parts of the Makefile and\nconfig.mak.uname.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n .github/workflows/main.yml |  3 +++\n ci/run-build-and-tests.sh  | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 3fa88b78b6d..25263c6b17a 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -229,6 +229,9 @@ jobs:\n             cc: gcc\n             os: ubuntu\n             cc_package: gcc-8\n+          - jobname: linux-BUILD-vars\n+            cc: gcc\n+            os: ubuntu\n             pool: ubuntu-latest\n           - jobname: osx-clang\n             cc: clang\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex 8ebff425967..786285c5071 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -31,6 +31,36 @@ linux-TEST-vars)\n \texport GIT_TEST_WRITE_REV_INDEX=1\n \texport GIT_TEST_CHECKOUT_WORKERS=2\n \t;;\n+linux-BUILD-vars)\n+\texport NO_CURL=Y\n+\texport NO_PTHREADS=Y\n+\texport NO_GETTEXT=Y\n+\n+\t# Undo settings in config.mak.uname\n+\texport HAVE_ALLOCA_H=\n+\n+\t# Various compat/ fallbacks, with \"FAIL\" omitted if faking it\n+\t# doesn't work on Linux.\n+\texport NO_REGEX=Y\n+\texport NO_MEMMEM=Y\n+\texport INTERNAL_QSORT=Y\n+\texport SNPRINTF_RETURNS_BOGUS=Y\n+\texport FREAD_READS_DIRECTORIES=Y\n+\texport OPEN_RETURNS_EINTR=Y\n+\texport NO_HSTRERROR= # compat/hstrerror.c FAIL\n+\texport NO_POLL=Y\n+\texport NO_STRCASESTR=Y\n+\texport NO_STRTOUMAX=Y\n+\texport NO_SETENV=Y\n+\texport NO_UNSETENV=Y\n+\texport NO_MMAP=Y\n+\texport NO_PREAD=Y\n+\texport NEEDS_MODE_TRANSLATION= # compat/stat.c FAIL\n+\texport NO_IPV6=Y\n+\texport NO_INET_NTOP=Y\n+\texport NO_INET_PTON=Y\n+\texport NO_UNIX_SOCKETS=Y\n+\t;;\n linux-clang)\n \texport GIT_TEST_DEFAULT_HASH=sha1\n \t;;\n-- \n2.36.1.1239.gfba91521d90\n\n"},{"id":"457267","messageId":"RFC-patch-4.5-3861546bd84-20220615T104503Z-avarab@gmail.com","threadId":"58012","inReplyTo":"RFC-cover-0.5-00000000000-20220615T104503Z-avarab@gmail.com","subject":"[RFC PATCH 4/5] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-15T10:53:31Z","receivedAt":"2022-06-15T10:53:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Improve on 6dcbdc0d661 (remote: create fetch.credentialsInUrl config,\n2022-06-06) by fixing the cases where we emit duplicate warnings,\nwhich were:\n\n * In the same process, as we'd get the same \"struct remote *\"\n   again. We could probably save ourselves more work in those scenarios,\n   but adding a flag to the \"struct remote\" indicating that we've validated\n   the URLs will fix those issues.\n\n * When we invoke e.g. \"git-remote-https\" (aka. \"git-remote-curl\")\n   from \"git fetch\". For those cases let's pass down the equivalent of a\n   \"-c transfer.credentialsInUrl=allow\", since we know that we've already\n   inspected our remotes in the parent process.\n\n   See 7390f05a3c6 (fetch: after refetch, encourage auto gc repacking,\n   2022-03-28) for prior use of git_config_push_parameter() for this\n   purpose, i.e. to push config parameters before invoking a\n   sub-process.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/clone.c       |  5 ++++-\n builtin/fetch.c       |  4 ++++\n builtin/push.c        |  6 ++++-\n remote.c              | 51 +++++++++++++++++++++++++++++++++----------\n remote.h              | 14 ++++++++++++\n t/t5516-fetch-push.sh |  6 ++---\n t/t5601-clone.sh      |  2 +-\n 7 files changed, 70 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 89a91b00177..96a94621a09 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -886,12 +886,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tint err = 0, complete_refs_before_fetch = 1;\n \tint submodule_progress;\n \tint filter_submodules = 0;\n-\n+\tenum credentials_in_url cred_in_url_cfg = get_credentials_in_url();\n \tstruct transport_ls_refs_options transport_ls_refs_options =\n \t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n \n \tpacket_trace_identity(\"clone\");\n \n+\tif (cred_in_url_cfg == CRED_IN_URL_WARN)\n+\t\tgit_config_push_parameter(\"transfer.credentialsInUrl=allow\");\n+\n \tgit_config(git_clone_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, builtin_clone_options,\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ac29c2b1ae3..bf67ef8c3d8 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2101,9 +2101,13 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tstruct remote *remote = NULL;\n \tint result = 0;\n \tint prune_tags_ok = 1;\n+\tenum credentials_in_url cred_in_url_cfg = get_credentials_in_url();\n \n \tpacket_trace_identity(\"fetch\");\n \n+\tif (cred_in_url_cfg == CRED_IN_URL_WARN)\n+\t\tgit_config_push_parameter(\"transfer.credentialsInUrl=allow\");\n+\n \t/* Record the command line for the reflog */\n \tstrbuf_addstr(&default_rla, \"fetch\");\n \tfor (i = 1; i < argc; i++) {\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 86b44f8aa71..6dd1b20dda0 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -576,7 +576,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tstruct string_list *push_options;\n \tconst struct string_list_item *item;\n \tstruct remote *remote;\n-\n+\tenum credentials_in_url cred_in_url_cfg = get_credentials_in_url();\n \tstruct option options[] = {\n \t\tOPT__VERBOSITY(&verbosity),\n \t\tOPT_STRING( 0 , \"repo\", &repo, N_(\"repository\"), N_(\"repository\")),\n@@ -619,6 +619,10 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t};\n \n \tpacket_trace_identity(\"push\");\n+\n+\tif (cred_in_url_cfg == CRED_IN_URL_WARN)\n+\t\tgit_config_push_parameter(\"transfer.credentialsInUrl=allow\");\n+\n \tgit_config(git_push_config, &flags);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n \tpush_options = (push_options_cmdline.nr\ndiff --git a/remote.c b/remote.c\nindex 42c891d44fd..61add35be2f 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -616,25 +616,43 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)\n \treturn NULL;\n }\n \n-static void validate_remote_url(struct remote *remote)\n+static enum credentials_in_url cred_in_url = CRED_IN_URL_UNKNOWN;\n+enum credentials_in_url get_credentials_in_url(void)\n {\n-\tint i;\n+\tenum credentials_in_url new = CRED_IN_URL_ALLOW;\n \tconst char *value;\n-\tstruct strbuf redacted = STRBUF_INIT;\n-\tint warn_not_die;\n+\n+\tif (cred_in_url != CRED_IN_URL_UNKNOWN)\n+\t\treturn cred_in_url;\n \n \tif (git_config_get_string_tmp(\"transfer.credentialsinurl\", &value))\n-\t\treturn;\n+\t\tgoto done;\n \n \tif (!strcmp(\"warn\", value))\n-\t\twarn_not_die = 1;\n+\t\tnew = CRED_IN_URL_WARN;\n \telse if (!strcmp(\"die\", value))\n-\t\twarn_not_die = 0;\n+\t\tnew = CRED_IN_URL_DIE;\n \telse if (!strcmp(\"allow\", value))\n-\t\treturn;\n+\t\tgoto done;\n \telse\n \t\tdie(_(\"unrecognized value transfer.credentialsInURL: '%s'\"), value);\n \n+done:\n+\tcred_in_url = new;\n+\treturn cred_in_url;\n+}\n+\n+static void validate_remote_url(struct remote *remote)\n+{\n+\tint i;\n+\tstruct strbuf redacted = STRBUF_INIT;\n+\tenum credentials_in_url cfg = get_credentials_in_url();\n+\n+\tif (remote->validated_urls)\n+\t\tgoto done;\n+\tif (cfg == CRED_IN_URL_ALLOW)\n+\t\tgoto done;\n+\n \tfor (i = 0; i < remote->url_nr; i++) {\n \t\tstruct url_info url_info = { 0 };\n \n@@ -648,16 +666,25 @@ static void validate_remote_url(struct remote *remote)\n \t\tstrbuf_addstr(&redacted,\n \t\t\t      url_info.url + url_info.passwd_off + url_info.passwd_len);\n \n-\t\tif (warn_not_die)\n+\t\tswitch (cfg) {\n+\t\tcase CRED_IN_URL_WARN:\n \t\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n-\t\telse\n+\t\t\tbreak;\n+\t\tcase CRED_IN_URL_DIE:\n \t\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n-\n-loop_cleanup:\n+\t\t\tbreak;\n+\t\tcase CRED_IN_URL_ALLOW:\n+\t\tcase CRED_IN_URL_UNKNOWN:\n+\t\t\tBUG(\"unreachable\");\n+\t\t\tbreak;\n+\t\t}\n+\tloop_cleanup:\n \t\tfree(url_info.url);\n \t}\n \n \tstrbuf_release(&redacted);\n+done:\n+\tremote->validated_urls = 1;\n }\n \n static struct remote *\ndiff --git a/remote.h b/remote.h\nindex dd4402436f1..4ef359e4d4a 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -98,6 +98,8 @@ struct remote {\n \tint prune;\n \tint prune_tags;\n \n+\tint validated_urls;\n+\n \t/**\n \t * The configured helper programs to run on the remote side, for\n \t * Git-native protocols.\n@@ -441,4 +443,16 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);\n char *relative_url(const char *remote_url, const char *url,\n \t\t   const char *up_path);\n \n+enum credentials_in_url {\n+\tCRED_IN_URL_UNKNOWN,\n+\tCRED_IN_URL_ALLOW,\n+\tCRED_IN_URL_WARN,\n+\tCRED_IN_URL_DIE,\n+};\n+\n+/**\n+ * Get the transfer.credentialsInUrl config setting as an \"enum\n+ * credentials_in_url\".\n+ */\n+enum credentials_in_url get_credentials_in_url(void);\n #endif\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 51d695e475a..c7a21d7cfb5 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1841,7 +1841,7 @@ test_expect_success 'fetch warns or fails when using username:password' '\n \n \ttest_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@localhost 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n-\ttest_line_count = 3 warnings &&\n+\ttest_line_count = 1 warnings &&\n \n \ttest_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@localhost 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n@@ -1864,7 +1864,7 @@ test_expect_success CURL 'fetch warns or fails when using username:password in c\n \n \ttest_must_fail git -C repo -c transfer.credentialsInUrl=warn fetch pwd-url 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n-\ttest_line_count = 3 warnings &&\n+\ttest_line_count = 1 warnings &&\n \n \tgit -C repo remote set-url --push pwd-url https://username:password@localhost &&\n \tgit -C repo remote set-url pwd-url https://localhost &&\n@@ -1899,7 +1899,7 @@ test_expect_success CURL 'push warns or fails when using username:password in co\n \n \ttest_must_fail git -C repo -c transfer.credentialsInUrl=warn push pwd-url HEAD:refs/heads/branch 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n-\ttest_line_count = 2 warnings &&\n+\ttest_line_count = 1 warnings &&\n \n \tgit -C repo remote set-url --push pwd-url https://username:password@localhost &&\n \tgit -C repo remote set-url pwd-url https://localhost &&\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex e6a248bbdcc..e449ccb54e3 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -78,7 +78,7 @@ test_expect_success 'clone warns or fails when using username:password' '\n \n \ttest_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n-\ttest_line_count = 2 warnings &&\n+\ttest_line_count = 1 warnings &&\n \n \ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n-- \n2.36.1.1239.gfba91521d90\n\n"},{"id":"457268","messageId":"RFC-patch-5.5-6cafd6368a2-20220615T104503Z-avarab@gmail.com","threadId":"58012","inReplyTo":"RFC-cover-0.5-00000000000-20220615T104503Z-avarab@gmail.com","subject":"[RFC PATCH 5/5] transport: check remote.<name>pushurl with transfer.credentialsInUrl","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-15T10:53:32Z","receivedAt":"2022-06-15T10:53:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Expand the checks added in 6dcbdc0d661 (remote: create\nfetch.credentialsInUrl config, 2022-06-06) to also check the\nremote.<name>.pushurl setting. Before this it would only check the\nremote.<name>.url setting, and would thus miss potential passwords in\nthe config.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n remote.c              | 63 ++++++++++++++++++++++++-------------------\n t/t5516-fetch-push.sh |  3 ++-\n 2 files changed, 37 insertions(+), 29 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 61add35be2f..d4dcc02a827 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -642,6 +642,37 @@ enum credentials_in_url get_credentials_in_url(void)\n \treturn cred_in_url;\n }\n \n+static void validate_one_remote_url(enum credentials_in_url cfg,\n+\t\t\t\t    const char *url, struct strbuf *redacted)\n+{\n+\tstruct url_info url_info = { 0 };\n+\n+\tif (!url_normalize(url, &url_info) || !url_info.passwd_off)\n+\t\tgoto cleanup;\n+\n+\tstrbuf_reset(redacted);\n+\tstrbuf_add(redacted, url_info.url, url_info.passwd_off);\n+\tstrbuf_addstr(redacted, \"<redacted>\");\n+\tstrbuf_addstr(redacted, url_info.url + url_info.passwd_off +\n+\t\t      url_info.passwd_len);\n+\n+\tswitch (cfg) {\n+\tcase CRED_IN_URL_WARN:\n+\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted->buf);\n+\t\tbreak;\n+\tcase CRED_IN_URL_DIE:\n+\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted->buf);\n+\t\tbreak;\n+\tcase CRED_IN_URL_ALLOW:\n+\tcase CRED_IN_URL_UNKNOWN:\n+\t\tBUG(\"unreachable\");\n+\t\tbreak;\n+\t}\n+cleanup:\n+\tfree(url_info.url);\n+}\n+\n+\n static void validate_remote_url(struct remote *remote)\n {\n \tint i;\n@@ -653,34 +684,10 @@ static void validate_remote_url(struct remote *remote)\n \tif (cfg == CRED_IN_URL_ALLOW)\n \t\tgoto done;\n \n-\tfor (i = 0; i < remote->url_nr; i++) {\n-\t\tstruct url_info url_info = { 0 };\n-\n-\t\tif (!url_normalize(remote->url[i], &url_info) ||\n-\t\t    !url_info.passwd_off)\n-\t\t\tgoto loop_cleanup;\n-\n-\t\tstrbuf_reset(&redacted);\n-\t\tstrbuf_add(&redacted, url_info.url, url_info.passwd_off);\n-\t\tstrbuf_addstr(&redacted, \"<redacted>\");\n-\t\tstrbuf_addstr(&redacted,\n-\t\t\t      url_info.url + url_info.passwd_off + url_info.passwd_len);\n-\n-\t\tswitch (cfg) {\n-\t\tcase CRED_IN_URL_WARN:\n-\t\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n-\t\t\tbreak;\n-\t\tcase CRED_IN_URL_DIE:\n-\t\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n-\t\t\tbreak;\n-\t\tcase CRED_IN_URL_ALLOW:\n-\t\tcase CRED_IN_URL_UNKNOWN:\n-\t\t\tBUG(\"unreachable\");\n-\t\t\tbreak;\n-\t\t}\n-\tloop_cleanup:\n-\t\tfree(url_info.url);\n-\t}\n+\tfor (i = 0; i < remote->url_nr; i++)\n+\t\tvalidate_one_remote_url(cfg, remote->url[i], &redacted);\n+\tfor (i = 0; i < remote->pushurl_nr; i++)\n+\t\tvalidate_one_remote_url(cfg, remote->pushurl[i], &redacted);\n \n \tstrbuf_release(&redacted);\n done:\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex c7a21d7cfb5..cdecc1c049c 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1905,7 +1905,8 @@ test_expect_success CURL 'push warns or fails when using username:password in co\n \tgit -C repo remote set-url pwd-url https://localhost &&\n \n \ttest_must_fail git -C repo -c transfer.credentialsInUrl=warn push pwd-url HEAD:refs/heads/branch 2>err &&\n-\t! grep \"warning: $message\" err\n+\tgrep \"warning: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings\n '\n \n test_done\n-- \n2.36.1.1239.gfba91521d90\n\n"},{"id":"457272","messageId":"ffa6d63c-0efb-d805-dc73-a781e5a92f22@github.com","threadId":"58012","inReplyTo":"RFC-patch-2.5-38af32c5b96-20220615T104503Z-avarab@gmail.com","subject":"Re: [RFC PATCH 2/5] fetch+push tests: add missing coverage for 6dcbdc0d661","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-15T12:39:02Z","receivedAt":"2022-06-15T12:39:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/15/22 6:53 AM, Ævar Arnfjörð Bjarmason wrote:\n> Add tests that were missing from 6dcbdc0d661 (remote: create\n> fetch.credentialsInUrl config, 2022-06-06), we want to test how we\n> handle cases where the config comes from a file, and that we handle\n> \"pushURL\" correctly.\n\nThe tests use your renamed transfer.credentialsInUrl, but you haven't\nrenamed at this point.\n\n> Currently the \"pushURL\" case isn't handled at all, i.e. URLs aren't\n> warned about in \"remote.*pushurl\" , only for \"remote.*.url\".\n\nGood find. I suppose that the second test_expect_success will _fail_\nat the point of this patch?\n\nThanks,\n-Stolee\n"},{"id":"457273","messageId":"cbba9f52-8ebe-40f2-739e-0796cf60b05c@github.com","threadId":"58012","inReplyTo":"RFC-patch-3.5-d69368c5b39-20220615T104503Z-avarab@gmail.com","subject":"Re: [RFC PATCH 3/5] CI: add a linux-BUILD-vars job","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-15T12:41:38Z","receivedAt":"2022-06-15T12:41:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/15/22 6:53 AM, Ævar Arnfjörð Bjarmason wrote:\n> Add a linux-BUILD-vars job, in a preceding commit we fixed a bug that\n> would have been spotted by testing under NO_CURL=Y.\n\nYou'll want to double-check patch order before sending a full version\nfor review.\n\nAlso, why capitalize \"BUILD\"? I suppose the variables are all-caps,\nbut our job names are all lower case to this point, right?\n\n> This CI job attempts to unset various settings in config.mak.uname and\n> the Makefile, so that we'll stress our fallbacks and conditionally\n> compiled code as much as possible.\n\nI think it is a good idea to include a job that tries these \"as\nminimal as possible\" builds.\n\n> If there is a missing setting here that we can enable under\n> \"ubuntu-latest\" the omission isn't intentional, this list came from a\n> quick skimming of the relevant parts of the Makefile and\n> config.mak.uname.\n\nWe can extend this in the future if we discover something new to add.\nThe important thing is that this moves us in the right direction.\n\nThanks,\n-Stolee\n"},{"id":"457275","messageId":"7e5488bd-a17b-3997-072d-69e3ae0a9821@github.com","threadId":"58012","inReplyTo":"RFC-cover-0.5-00000000000-20220615T104503Z-avarab@gmail.com","subject":"Re: [RFC PATCH 0/5] fix issues in transfer.credentialsInUrl","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-15T12:52:38Z","receivedAt":"2022-06-15T12:52:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/15/22 6:53 AM, Ævar Arnfjörð Bjarmason wrote:\n> This is on top of [1], and given the \"rc\" phase is an RFC. This:\n> \n>  * Fixes the issue of the transfer.credentialsInUrl (now renamed)\n\nYou are burying the lede here (and I had to look hard to see where\nyou renamed it in patch 4). I think this rename makes sense, as long\nas we do that replacement before the release. Having a patch that\ndoes only that rename as the first patch (and updates all docs, too)\nwould be helpful.\n\nThe doc updates should include the reference in the release notes,\ntoo.\n\n> not\n>    finding passwords in \"pushurl\" URLs (in my case, the only place\n>    where I'd actually put a password in a URL in a config...)\n...\n>  * 5/5 fixes the (major) blind spot of the warning missing \"pushurl\" config.\n\nI think this is a valuable extension of the feature, and justifies\nthe rename. I'm mixed on whether we should add this before the\nrelease or save it for the next cycle.\n\n>  * 1/5 fixes a bug in an existing test, but I didn't think it was\n>    worth bothering with for 2.37.0.\n\nIt's a good find, and a test fix is easy to do during the release\ncycle, I think.\n\n>  * Adds missing test coverage for reading the config from a file, not\n>    the CLI.\n\nAgain, test coverage is good. No functionality needs to change.\nHowever, one test added requires the pushurl change.\n\n>  * 3/5 is a WIP CI target to spot the type of issue I fixed in [2],\n>    it's not the first time where we have a NO_CURL=Y breakage land on\n>    master...\n\nI think that we should use CI to prevent these kinds of issues, so\nI support adding this as well as leaving room for it to be changed\nin the future if we notice other build issues that we can group into\nthis build.\n\n>  * 4/5 attemps to \"really\" fix the duplicate warnings we emit, I think\n>    the approach there is good, especially the part where we shouldn't\n>    emit it twice in-process.\n> \n>    But this currently misses e.g. \"git ls-remote\". I wonder if we\n>    should just stick that git_config_push_parameter() condition into\n>    packet_trace_identity() and call it a day.\n\nI think these duplicate warning things should absolutely be left\nuntil after the release. We do not have \"warn\" on by default, so it\nwill not disturb users who don't opt-in. We _should_ pursue this in\nthe next cycle, but with the time we can take to really be sure it\nis the right approach to solving that problem.\n\n> I think this is all non-RFC quality, except the \"ls-remote\" case, and\n> us missing tests for that & other transport users that aren't\n> clone/fetch/push.\n\nThere are some patch-order things that need to be cleaned up. I\nagree that most of the code looks right.\n\n> Derrick: Are you interested in picking this up & pursuing it after the\n> release, with whatever fix-ups/rewrites etc. that you find\n> appropriate?\n\nI'm happy to review a new version of this series during the release\nwindow that takes the high-priority items (renaming the config,\nfixing tests, adding the CI build). I'd also be happy to review the\nother changes as a follow-up.\n\nThanks,\n-Stolee\n"}]}