{"thread":{"id":"53595","subject":"[PATCH] clone/fetch: anonymize URLs in the reflog","startedAt":"2020-06-01T19:20:08Z","lastAt":"2020-06-04T20:30:11Z","messageCount":5,"participants":["Johannes Schindelin via GitGitGadget","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"398990","messageId":"pull.797.git.git.1591039202561.gitgitgadget@gmail.com","threadId":"53595","inReplyTo":null,"subject":"[PATCH] clone/fetch: anonymize URLs in the reflog","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-01T19:20:02Z","receivedAt":"2020-06-01T19:20:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nEven if we strongly discourage putting credentials into the URLs passed\nvia the command-line, there _is_ support for that, and users _do_ do\nthat.\n\nLet's scrub them before writing them to the reflog.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    Anonymize URLs in the reflog\n    \n    This came up in an internal audit, but we do not consider this to be a\n    big deal: the reflog is local and not really shared with anybody.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-797%2Fdscho%2Fanonymize-clone-reflog-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-797/dscho/anonymize-clone-reflog-v1\nPull-Request: https://github.com/git/git/pull/797\n\n builtin/clone.c            | 10 ++++++----\n builtin/fetch.c            |  9 +++++++--\n t/t5541-http-push-smart.sh | 15 +++++++++++++++\n 3 files changed, 28 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 1ad26f4d8c8..5fe637a6702 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -938,7 +938,7 @@ static int path_exists(const char *path)\n int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0, is_local;\n-\tconst char *repo_name, *repo, *work_tree, *git_dir;\n+\tconst char *repo_name, *repo, *display_repo, *work_tree, *git_dir;\n \tchar *path, *dir;\n \tint dest_exists;\n \tconst struct ref *refs, *remote_head;\n@@ -993,11 +993,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tpath = get_repo_path(repo_name, &is_bundle);\n \tif (path)\n-\t\trepo = absolute_pathdup(repo_name);\n+\t\tdisplay_repo = repo = absolute_pathdup(repo_name);\n \telse if (!strchr(repo_name, ':'))\n \t\tdie(_(\"repository '%s' does not exist\"), repo_name);\n-\telse\n+\telse {\n \t\trepo = repo_name;\n+\t\tdisplay_repo = transport_anonymize_url(repo);\n+\t}\n \n \t/* no need to be strict, transport_set_option() will validate it again */\n \tif (option_depth && atoi(option_depth) < 1)\n@@ -1014,7 +1016,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"destination path '%s' already exists and is not \"\n \t\t\t\"an empty directory.\"), dir);\n \n-\tstrbuf_addf(&reflog_msg, \"clone: from %s\", repo);\n+\tstrbuf_addf(&reflog_msg, \"clone: from %s\", display_repo);\n \n \tif (option_bare)\n \t\twork_tree = NULL;\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex bf6bab80fab..d58b7572114 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1765,8 +1765,13 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \t/* Record the command line for the reflog */\n \tstrbuf_addstr(&default_rla, \"fetch\");\n-\tfor (i = 1; i < argc; i++)\n-\t\tstrbuf_addf(&default_rla, \" %s\", argv[i]);\n+\tfor (i = 1; i < argc; i++) {\n+\t\t/* This handles non-URLs gracefully */\n+\t\tchar *anon = transport_anonymize_url(argv[i]);\n+\n+\t\tstrbuf_addf(&default_rla, \" %s\", anon);\n+\t\tfree(anon);\n+\t}\n \n \tfetch_config_from_gitmodules(&submodule_fetch_jobs_config,\n \t\t\t\t     &recurse_submodules);\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 23be8ce92d6..2d60381a5e7 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -456,6 +456,21 @@ test_expect_success 'push status output scrubs password' '\n \tgrep \"^To $HTTPD_URL/smart/test_repo.git\" status\n '\n \n+test_expect_success 'clone/fetch scrubs password from reflogs' '\n+\tcd \"$ROOT_PATH\" &&\n+\tgit clone \"$HTTPD_URL_USER_PASS/smart/test_repo.git\" \\\n+\t\treflog-test &&\n+\tcd reflog-test &&\n+\ttest_commit prepare-for-force-fetch &&\n+\tgit switch -c away &&\n+\tgit fetch \"$HTTPD_URL_USER_PASS/smart/test_repo.git\" \\\n+\t\t+master:master &&\n+\t# should have been scrubbed down to vanilla URL\n+\tgit log -g master >reflog &&\n+\tgrep \"$HTTPD_URL\" reflog &&\n+\t! grep \"$HTTPD_URL_USER_PASS\" reflog\n+'\n+\n test_expect_success 'colorize errors/hints' '\n \tcd \"$ROOT_PATH\"/test_repo_clone &&\n \ttest_must_fail git -c color.transport=always -c color.advice=always \\\n\nbase-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec\n-- \ngitgitgadget\n"},{"id":"399002","messageId":"20200601214715.GB3309882@coredump.intra.peff.net","threadId":"53595","inReplyTo":"pull.797.git.git.1591039202561.gitgitgadget@gmail.com","subject":"Re: [PATCH] clone/fetch: anonymize URLs in the reflog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-06-01T21:47:15Z","receivedAt":"2020-06-01T21:47:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 01, 2020 at 07:20:02PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> \n> Even if we strongly discourage putting credentials into the URLs passed\n> via the command-line, there _is_ support for that, and users _do_ do\n> that.\n> \n> Let's scrub them before writing them to the reflog.\n\nGood idea.\n\n>     This came up in an internal audit, but we do not consider this to be a\n>     big deal: the reflog is local and not really shared with anybody.\n\nAgreed.\n\n>  builtin/clone.c            | 10 ++++++----\n>  builtin/fetch.c            |  9 +++++++--\n>  t/t5541-http-push-smart.sh | 15 +++++++++++++++\n\nThe patch itself looks very neatly done.\n\n> @@ -993,11 +993,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \n>  \tpath = get_repo_path(repo_name, &is_bundle);\n>  \tif (path)\n> -\t\trepo = absolute_pathdup(repo_name);\n> +\t\tdisplay_repo = repo = absolute_pathdup(repo_name);\n>  \telse if (!strchr(repo_name, ':'))\n>  \t\tdie(_(\"repository '%s' does not exist\"), repo_name);\n> -\telse\n> +\telse {\n>  \t\trepo = repo_name;\n> +\t\tdisplay_repo = transport_anonymize_url(repo);\n> +\t}\n\nNot introduced by your patch, but I had to read this a few times to make\nsure we always end up with repo and display_repo set. IMHO it would be\neasier to read as:\n\n  if (this) {\n     repo = ...;\n     display_repo = ...;\n  } else if (that) {\n     repo = ...;\n     display_repo = ...;\n  } else {\n     die(...);\n  }\n\ninstead of sticking the die() in the middle.  Maybe just personal\npreference, though. :)\n\n> +\t# should have been scrubbed down to vanilla URL\n> +\tgit log -g master >reflog &&\n> +\tgrep \"$HTTPD_URL\" reflog &&\n> +\t! grep \"$HTTPD_URL_USER_PASS\" reflog\n> +'\n\nAnd you make sure we retain the username. Nice.\n\n-Peff\n"},{"id":"399035","messageId":"xmqqimg9tnkk.fsf@gitster.c.googlers.com","threadId":"53595","inReplyTo":"20200601214715.GB3309882@coredump.intra.peff.net","subject":"Re: [PATCH] clone/fetch: anonymize URLs in the reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-02T16:55:07Z","receivedAt":"2020-06-02T16:55:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Jun 01, 2020 at 07:20:02PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n>> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>> \n>> Even if we strongly discourage putting credentials into the URLs passed\n>> via the command-line, there _is_ support for that, and users _do_ do\n>> that.\n>> \n>> Let's scrub them before writing them to the reflog.\n>\n> Good idea.\n>\n>>     This came up in an internal audit, but we do not consider this to be a\n>>     big deal: the reflog is local and not really shared with anybody.\n>\n> Agreed.\n\nNice.\n\n>>  builtin/clone.c            | 10 ++++++----\n>>  builtin/fetch.c            |  9 +++++++--\n>>  t/t5541-http-push-smart.sh | 15 +++++++++++++++\n>\n> The patch itself looks very neatly done.\n>\n>> @@ -993,11 +993,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>>  \n>>  \tpath = get_repo_path(repo_name, &is_bundle);\n>>  \tif (path)\n>> -\t\trepo = absolute_pathdup(repo_name);\n>> +\t\tdisplay_repo = repo = absolute_pathdup(repo_name);\n>>  \telse if (!strchr(repo_name, ':'))\n>>  \t\tdie(_(\"repository '%s' does not exist\"), repo_name);\n>> -\telse\n>> +\telse {\n>>  \t\trepo = repo_name;\n>> +\t\tdisplay_repo = transport_anonymize_url(repo);\n>> +\t}\n>\n> Not introduced by your patch, but I had to read this a few times to make\n> sure we always end up with repo and display_repo set. IMHO it would be\n> easier to read as:\n>\n>   if (this) {\n>      repo = ...;\n>      display_repo = ...;\n>   } else if (that) {\n>      repo = ...;\n>      display_repo = ...;\n>   } else {\n>      die(...);\n>   }\n>\n> instead of sticking the die() in the middle.  Maybe just personal\n> preference, though. :)\n\nFor a if/elseif cascade of few-liner blocks each, I do not think it\nwould matter, but if a block were larger, having the die() case at\nthe beginning or at the end would indeed make it easier to spot any\nanomalies.\n\n>> +\t# should have been scrubbed down to vanilla URL\n>> +\tgit log -g master >reflog &&\n>> +\tgrep \"$HTTPD_URL\" reflog &&\n>> +\t! grep \"$HTTPD_URL_USER_PASS\" reflog\n>> +'\n>\n> And you make sure we retain the username. Nice.\n"},{"id":"399149","messageId":"pull.797.v2.git.git.1591301309308.gitgitgadget@gmail.com","threadId":"53595","inReplyTo":"pull.797.git.git.1591039202561.gitgitgadget@gmail.com","subject":"[PATCH v2] clone/fetch: anonymize URLs in the reflog","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-04T20:08:29Z","receivedAt":"2020-06-04T20:08:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nEven if we strongly discourage putting credentials into the URLs passed\nvia the command-line, there _is_ support for that, and users _do_ do\nthat.\n\nLet's scrub them before writing them to the reflog.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    Anonymize URLs in the reflog\n    \n    This came up in an internal audit, but we do not consider this to be a\n    big deal: the reflog is local and not really shared with anybody.\n    \n    Changes since v1:\n    \n     * Changed the if...else if...else cadence to move the die() to the last\n       arm\n     * Stopped the memory leak of display_repo (allocated by \n       transport_anonymize_url())\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-797%2Fdscho%2Fanonymize-clone-reflog-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-797/dscho/anonymize-clone-reflog-v2\nPull-Request: https://github.com/git/git/pull/797\n\nRange-diff vs v1:\n\n 1:  11c0d47c95e ! 1:  933a7353847 clone/fetch: anonymize URLs in the reflog\n     @@ Commit message\n          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n      \n       ## builtin/clone.c ##\n     -@@ builtin/clone.c: static int path_exists(const char *path)\n     - int cmd_clone(int argc, const char **argv, const char *prefix)\n     +@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n       {\n       \tint is_bundle = 0, is_local;\n     --\tconst char *repo_name, *repo, *work_tree, *git_dir;\n     -+\tconst char *repo_name, *repo, *display_repo, *work_tree, *git_dir;\n     - \tchar *path, *dir;\n     + \tconst char *repo_name, *repo, *work_tree, *git_dir;\n     +-\tchar *path, *dir;\n     ++\tchar *path, *dir, *display_repo = NULL;\n       \tint dest_exists;\n       \tconst struct ref *refs, *remote_head;\n     + \tconst struct ref *remote_head_points_at;\n      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n     - \n       \tpath = get_repo_path(repo_name, &is_bundle);\n       \tif (path)\n     --\t\trepo = absolute_pathdup(repo_name);\n     -+\t\tdisplay_repo = repo = absolute_pathdup(repo_name);\n     - \telse if (!strchr(repo_name, ':'))\n     - \t\tdie(_(\"repository '%s' does not exist\"), repo_name);\n     + \t\trepo = absolute_pathdup(repo_name);\n     +-\telse if (!strchr(repo_name, ':'))\n     +-\t\tdie(_(\"repository '%s' does not exist\"), repo_name);\n      -\telse\n     -+\telse {\n     ++\telse if (strchr(repo_name, ':')) {\n       \t\trepo = repo_name;\n      +\t\tdisplay_repo = transport_anonymize_url(repo);\n     -+\t}\n     ++\t} else\n     ++\t\tdie(_(\"repository '%s' does not exist\"), repo_name);\n       \n       \t/* no need to be strict, transport_set_option() will validate it again */\n       \tif (option_depth && atoi(option_depth) < 1)\n     @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n       \t\t\t\"an empty directory.\"), dir);\n       \n      -\tstrbuf_addf(&reflog_msg, \"clone: from %s\", repo);\n     -+\tstrbuf_addf(&reflog_msg, \"clone: from %s\", display_repo);\n     ++\tstrbuf_addf(&reflog_msg, \"clone: from %s\",\n     ++\t\t    display_repo ? display_repo : repo);\n     ++\tfree(display_repo);\n       \n       \tif (option_bare)\n       \t\twork_tree = NULL;\n\n\n builtin/clone.c            | 13 ++++++++-----\n builtin/fetch.c            |  9 +++++++--\n t/t5541-http-push-smart.sh | 15 +++++++++++++++\n 3 files changed, 30 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 1ad26f4d8c8..002d23ab0a2 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -939,7 +939,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0, is_local;\n \tconst char *repo_name, *repo, *work_tree, *git_dir;\n-\tchar *path, *dir;\n+\tchar *path, *dir, *display_repo = NULL;\n \tint dest_exists;\n \tconst struct ref *refs, *remote_head;\n \tconst struct ref *remote_head_points_at;\n@@ -994,10 +994,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tpath = get_repo_path(repo_name, &is_bundle);\n \tif (path)\n \t\trepo = absolute_pathdup(repo_name);\n-\telse if (!strchr(repo_name, ':'))\n-\t\tdie(_(\"repository '%s' does not exist\"), repo_name);\n-\telse\n+\telse if (strchr(repo_name, ':')) {\n \t\trepo = repo_name;\n+\t\tdisplay_repo = transport_anonymize_url(repo);\n+\t} else\n+\t\tdie(_(\"repository '%s' does not exist\"), repo_name);\n \n \t/* no need to be strict, transport_set_option() will validate it again */\n \tif (option_depth && atoi(option_depth) < 1)\n@@ -1014,7 +1015,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"destination path '%s' already exists and is not \"\n \t\t\t\"an empty directory.\"), dir);\n \n-\tstrbuf_addf(&reflog_msg, \"clone: from %s\", repo);\n+\tstrbuf_addf(&reflog_msg, \"clone: from %s\",\n+\t\t    display_repo ? display_repo : repo);\n+\tfree(display_repo);\n \n \tif (option_bare)\n \t\twork_tree = NULL;\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex bf6bab80fab..d58b7572114 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1765,8 +1765,13 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \t/* Record the command line for the reflog */\n \tstrbuf_addstr(&default_rla, \"fetch\");\n-\tfor (i = 1; i < argc; i++)\n-\t\tstrbuf_addf(&default_rla, \" %s\", argv[i]);\n+\tfor (i = 1; i < argc; i++) {\n+\t\t/* This handles non-URLs gracefully */\n+\t\tchar *anon = transport_anonymize_url(argv[i]);\n+\n+\t\tstrbuf_addf(&default_rla, \" %s\", anon);\n+\t\tfree(anon);\n+\t}\n \n \tfetch_config_from_gitmodules(&submodule_fetch_jobs_config,\n \t\t\t\t     &recurse_submodules);\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 23be8ce92d6..2d60381a5e7 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -456,6 +456,21 @@ test_expect_success 'push status output scrubs password' '\n \tgrep \"^To $HTTPD_URL/smart/test_repo.git\" status\n '\n \n+test_expect_success 'clone/fetch scrubs password from reflogs' '\n+\tcd \"$ROOT_PATH\" &&\n+\tgit clone \"$HTTPD_URL_USER_PASS/smart/test_repo.git\" \\\n+\t\treflog-test &&\n+\tcd reflog-test &&\n+\ttest_commit prepare-for-force-fetch &&\n+\tgit switch -c away &&\n+\tgit fetch \"$HTTPD_URL_USER_PASS/smart/test_repo.git\" \\\n+\t\t+master:master &&\n+\t# should have been scrubbed down to vanilla URL\n+\tgit log -g master >reflog &&\n+\tgrep \"$HTTPD_URL\" reflog &&\n+\t! grep \"$HTTPD_URL_USER_PASS\" reflog\n+'\n+\n test_expect_success 'colorize errors/hints' '\n \tcd \"$ROOT_PATH\"/test_repo_clone &&\n \ttest_must_fail git -c color.transport=always -c color.advice=always \\\n\nbase-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec\n-- \ngitgitgadget\n"},{"id":"399151","messageId":"xmqqy2p2o9pw.fsf@gitster.c.googlers.com","threadId":"53595","inReplyTo":"pull.797.v2.git.git.1591301309308.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] clone/fetch: anonymize URLs in the reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-04T20:30:03Z","receivedAt":"2020-06-04T20:30:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 1ad26f4d8c8..002d23ab0a2 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -939,7 +939,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  {\n>  \tint is_bundle = 0, is_local;\n>  \tconst char *repo_name, *repo, *work_tree, *git_dir;\n> -\tchar *path, *dir;\n> +\tchar *path, *dir, *display_repo = NULL;\n>  \tint dest_exists;\n>  \tconst struct ref *refs, *remote_head;\n>  \tconst struct ref *remote_head_points_at;\n> @@ -994,10 +994,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \tpath = get_repo_path(repo_name, &is_bundle);\n>  \tif (path)\n>  \t\trepo = absolute_pathdup(repo_name);\n> -\telse if (!strchr(repo_name, ':'))\n> -\t\tdie(_(\"repository '%s' does not exist\"), repo_name);\n> -\telse\n> +\telse if (strchr(repo_name, ':')) {\n>  \t\trepo = repo_name;\n> +\t\tdisplay_repo = transport_anonymize_url(repo);\n> +\t} else\n> +\t\tdie(_(\"repository '%s' does not exist\"), repo_name);\n>  \n>  \t/* no need to be strict, transport_set_option() will validate it again */\n>  \tif (option_depth && atoi(option_depth) < 1)\n> @@ -1014,7 +1015,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\tdie(_(\"destination path '%s' already exists and is not \"\n>  \t\t\t\"an empty directory.\"), dir);\n>  \n> -\tstrbuf_addf(&reflog_msg, \"clone: from %s\", repo);\n> +\tstrbuf_addf(&reflog_msg, \"clone: from %s\",\n> +\t\t    display_repo ? display_repo : repo);\n> +\tfree(display_repo);\n\nThe new patch is easier to see because display_repo becomes non NULL\nonly when anonymization was necessary and done.  Makes sense.\n\nWill queue.  Thanks.\n"}]}