{"thread":{"id":"66402","subject":"[PATCH] http: handle curl stripping creds from effective url","startedAt":"2026-09-28T04:01:50Z","lastAt":"2026-09-29T05:43:12Z","messageCount":5,"participants":["Jeff King","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"553399","messageId":"20260928040149.GA498186@coredump.intra.peff.net","threadId":"66402","inReplyTo":null,"subject":"[PATCH] http: handle curl stripping creds from effective url","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-28T04:01:49Z","receivedAt":"2026-09-28T04:01:50Z","isPatch":true,"body":"When we detect that curl performed a redirect of a URL we requested, we\nupdate our base URL to match the new location and flush the http_auth\ncredentials. This goes back to c93c92f309 (http: update base URLs when\nwe see redirects, 2013-09-28).\n\nWe detect the redirect by comparing the requested URL to the response\nfrom CURLINFO_EFFECTIVE_URL, using a simple string comparison. This has\nworked fine for years, but a change in the upcoming curl 8.23.0 adds a\ncomplication. If our URL directly contains credentials (like\n\"https://user:pass@example.com/foo.git\"), then as of 7a6bd027d0\n(getinfo: make sure CURLINFO_EFFECTIVE_URL does not contain creds,\n2026-09-21), curl will strip the credentials from what it returns (so\njust \"https://example.com/foo.git\" in this case).\n\nThis breaks our direct string comparison, and we believe that we've been\nredirected. We flush our http_auth credentials, and now subsequent\nrequests will use the reduced URL, causing us to re-request credentials\nfrom the user. Notably this causes t5550.15 (among others) to complain;\nit tries a clone with credentials in the URL, and fails if the user is\nprompted at all.\n\nWe can handle this new behavior by doing a more careful comparison: if\nthe direct string comparison fails, we'll strip out the credentials\nourselves and compare. This is a little extra work, but in practice it\nshould only happen once per process.\n\nI've used curl's curl_url() interface to do the stripping here, mostly\nbecause its behavior should match the stripping it does internally. And\nalso, though we have code to parse a URL, we don't have any to\nreconstruct it, making a single string comparison hard.\n\nOne alternative would be to parse with url_parse() or similar, and\ncompare the individual fields (skipping username/password). I think that\nwould probably also work in practice, but it seemed to me that the\nsimplest change would be sticking with string comparisons.\n\nThe curl_url() interface appeared in 7.62.0. We document that 7.61.0 is\nstill supported, so I've made it conditional here. Only new versions\nstrip the result from CURLINFO_EFFECTIVE_URL, so it's OK for very old\nversions to skip the extra comparison. Likewise if we encounter any\nerrors, we just quietly skip the comparison. That's fine if you don't\nhave creds in your URLs, and if you do, you'll get end up in the\nexisting error path (a redirect warning, and eventually an auth\nfailure).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI hit this in Debian unstable's packaging of libcurl; the new behavior\nis in 8.23.0-rc2, but not -rc1.\n\nWe could probably declare 7.62.0 the oldest supported version of curl,\nbut it would really only save a few lines of #ifdef here. I'd prefer to\nconsider that question separately.\n\n git-curl-compat.h |  7 +++++++\n http.c            | 41 ++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 47 insertions(+), 1 deletion(-)\n\ndiff --git a/git-curl-compat.h b/git-curl-compat.h\nindex 032aaf7126..25678b5dbd 100644\n--- a/git-curl-compat.h\n+++ b/git-curl-compat.h\n@@ -28,6 +28,13 @@\n  * introduced, oldest first, in the official version of cURL library.\n  */\n \n+/**\n+ * curl_url() interface added in 7.62.0 (October 2018)\n+ */\n+#if LIBCURL_VERSION_NUM >= 0x073e00\n+#define GIT_CURL_HAVE_CURL_URL\n+#endif\n+\n /**\n  * Versions before curl 7.66.0 (September 2019) required manually setting the\n  * transfer-encoding for a streaming POST; after that this is handled\ndiff --git a/http.c b/http.c\nindex c8fcfd7693..4af3c29c76 100644\n--- a/http.c\n+++ b/http.c\n@@ -2315,6 +2315,45 @@ static int http_request(const char *url,\n \treturn ret;\n }\n \n+#ifndef GIT_CURL_HAVE_CURL_URL\n+#define strip_url_credential(in) NULL\n+#else\n+static char *strip_url_credential(const char *in)\n+{\n+\tchar *ret = NULL;\n+\tCURLU *url;\n+\n+\turl = curl_url();\n+\tif (!url)\n+\t\tgoto out;\n+\n+\tif (curl_url_set(url, CURLUPART_URL, in, 0))\n+\t\tgoto out;\n+\n+\tcurl_url_set(url, CURLUPART_USER, NULL, 0);\n+\tcurl_url_set(url, CURLUPART_PASSWORD, NULL, 0);\n+\tcurl_url_get(url, CURLUPART_URL, &ret, 0);\n+\n+out:\n+\tcurl_url_cleanup(url);\n+\treturn ret;\n+}\n+#endif\n+\n+static int match_effective_url(const char *asked, const char *got)\n+{\n+\tchar *stripped;\n+\tint ret;\n+\n+\tif (!strcmp(asked, got))\n+\t\treturn 1;\n+\n+\tstripped = strip_url_credential(asked);\n+\tret = stripped && !strcmp(stripped, got);\n+\tcurl_free(stripped);\n+\treturn ret;\n+}\n+\n /*\n  * Update the \"base\" url to a more appropriate value, as deduced by\n  * redirects seen when requesting a URL starting with \"url\".\n@@ -2347,7 +2386,7 @@ static int update_url_from_redirect(struct strbuf *base,\n \tconst char *tail;\n \tsize_t new_len;\n \n-\tif (!strcmp(asked, got->buf))\n+\tif (match_effective_url(asked, got->buf))\n \t\treturn 0;\n \n \tif (!skip_prefix(asked, base->buf, &tail))\n-- \n2.56.0.rc2.338.gcaacf6bdf7\n"},{"id":"553461","messageId":"arplE8-5jD-rZiyu@pks.im","threadId":"66402","inReplyTo":"20260928040149.GA498186@coredump.intra.peff.net","subject":"Re: [PATCH] http: handle curl stripping creds from effective url","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-09-28T13:01:07Z","receivedAt":"2026-09-28T13:01:13Z","isPatch":true,"body":"On Mon, Sep 28, 2026 at 12:01:49AM -0400, Jeff King wrote:\n> When we detect that curl performed a redirect of a URL we requested, we\n> update our base URL to match the new location and flush the http_auth\n> credentials. This goes back to c93c92f309 (http: update base URLs when\n> we see redirects, 2013-09-28).\n> \n> We detect the redirect by comparing the requested URL to the response\n> from CURLINFO_EFFECTIVE_URL, using a simple string comparison. This has\n> worked fine for years, but a change in the upcoming curl 8.23.0 adds a\n> complication. If our URL directly contains credentials (like\n> \"https://user:pass@example.com/foo.git\"), then as of 7a6bd027d0\n> (getinfo: make sure CURLINFO_EFFECTIVE_URL does not contain creds,\n> 2026-09-21), curl will strip the credentials from what it returns (so\n> just \"https://example.com/foo.git\" in this case).\n> \n> This breaks our direct string comparison, and we believe that we've been\n> redirected. We flush our http_auth credentials, and now subsequent\n> requests will use the reduced URL, causing us to re-request credentials\n> from the user. Notably this causes t5550.15 (among others) to complain;\n> it tries a clone with credentials in the URL, and fails if the user is\n> prompted at all.\n> \n> We can handle this new behavior by doing a more careful comparison: if\n> the direct string comparison fails, we'll strip out the credentials\n> ourselves and compare. This is a little extra work, but in practice it\n> should only happen once per process.\n\nSo in my own words, we want to detect the case where we have been\nredirected and, if we have been, we want to strip credentials. But this\nlogic is about to break as curl starts to rewrite EFFECTIVE_URL more\naggressively, and that makes us detect redirects in cases where there\nwere none.\n\n> I've used curl's curl_url() interface to do the stripping here, mostly\n> because its behavior should match the stripping it does internally. And\n> also, though we have code to parse a URL, we don't have any to\n> reconstruct it, making a single string comparison hard.\n> \n> One alternative would be to parse with url_parse() or similar, and\n> compare the individual fields (skipping username/password). I think that\n> would probably also work in practice, but it seemed to me that the\n> simplest change would be sticking with string comparisons.\n\nIt still feels rather roundabout to compare URLs only to figure out\nwhether we have been redirected. I wondered whether there is maybe a\nmore direct way to get that info, and there indeed is\nCURLINFO_REDIRECT_COUNT, which allows us to retrieve the number of\nredirects that have happened.\n\nIs that interface maybe a more direct way to get what we're after?\n\nPatrick\n"},{"id":"553486","messageId":"xmqqo6dho1xj.fsf@gitster.g","threadId":"66402","inReplyTo":"20260928040149.GA498186@coredump.intra.peff.net","subject":"Re: [PATCH] http: handle curl stripping creds from effective url","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-28T15:01:12Z","receivedAt":"2026-09-28T15:01:15Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> I've used curl's curl_url() interface to do the stripping here, mostly\n> because its behavior should match the stripping it does internally. And\n> also, though we have code to parse a URL, we don't have any to\n> reconstruct it, making a single string comparison hard.\n>\n> One alternative would be to parse with url_parse() or similar, and\n> compare the individual fields (skipping username/password). I think that\n> would probably also work in practice, but it seemed to me that the\n> simplest change would be sticking with string comparisons.\n\nVery nice.\n\n> The curl_url() interface appeared in 7.62.0. We document that 7.61.0 is\n> still supported, so I've made it conditional here. Only new versions\n> strip the result from CURLINFO_EFFECTIVE_URL, so it's OK for very old\n> versions to skip the extra comparison.\n\n;-)\n\n> We could probably declare 7.62.0 the oldest supported version of curl,\n> but it would really only save a few lines of #ifdef here. I'd prefer to\n> consider that question separately.\n\nThat is very sensible.\n\n> +#ifndef GIT_CURL_HAVE_CURL_URL\n> +#define strip_url_credential(in) NULL\n> +#else\n> +static char *strip_url_credential(const char *in)\n> +{\n> +\tchar *ret = NULL;\n> +\tCURLU *url;\n> +\n> +\turl = curl_url();\n> +\tif (!url)\n> +\t\tgoto out;\n> +\n> +\tif (curl_url_set(url, CURLUPART_URL, in, 0))\n> +\t\tgoto out;\n> +\n> +\tcurl_url_set(url, CURLUPART_USER, NULL, 0);\n> +\tcurl_url_set(url, CURLUPART_PASSWORD, NULL, 0);\n> +\tcurl_url_get(url, CURLUPART_URL, &ret, 0);\n> +\n> +out:\n> +\tcurl_url_cleanup(url);\n> +\treturn ret;\n> +}\n> +#endif\n\n"},{"id":"553516","messageId":"20260928193644.GA1075764@coredump.intra.peff.net","threadId":"66402","inReplyTo":"arplE8-5jD-rZiyu@pks.im","subject":"Re: [PATCH] http: handle curl stripping creds from effective url","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-28T19:36:44Z","receivedAt":"2026-09-28T19:36:45Z","isPatch":true,"body":"On Mon, Sep 28, 2026 at 03:01:07PM +0200, Patrick Steinhardt wrote:\n\n> > We can handle this new behavior by doing a more careful comparison: if\n> > the direct string comparison fails, we'll strip out the credentials\n> > ourselves and compare. This is a little extra work, but in practice it\n> > should only happen once per process.\n> \n> So in my own words, we want to detect the case where we have been\n> redirected and, if we have been, we want to strip credentials. But this\n> logic is about to break as curl starts to rewrite EFFECTIVE_URL more\n> aggressively, and that makes us detect redirects in cases where there\n> were none.\n\nYes, though I'd be careful to distinguish \"strip\" credentials versus\n\"flush\" credentials. I'd take the former to mean \"remove them from the\nURL if they are embedded in it\", whereas the latter to mean \"throw away\nany credentials in the http_auth credential struct\".\n\nThose credentials in http_auth might have come from the user, or even\nbeen extracted from the URL originally (or some combination; e.g.,\ngetting the user from the URL and the password from the user). But we\nwant to flush them so that we won't provide them to a redirected\ndestination.\n\nI think we are on the same page and this is just wording pedantry, but\nwanted to make sure.\n\nYou might also reasonably ask: if we have already extracted the\ncredentials from the original URL into http_auth, can't we just strip\nthem immediately afterwards, and always deal with a vanilla URL? That\nwas my original approach, but sadly it does not work because we then\npass that URL across process boundaries (e.g., to http-push), but the\nextracted http_auth credential struct doesn't make it. One of the tests\nin t5540 notices those.\n\n> > I've used curl's curl_url() interface to do the stripping here, mostly\n> > because its behavior should match the stripping it does internally. And\n> > also, though we have code to parse a URL, we don't have any to\n> > reconstruct it, making a single string comparison hard.\n> > \n> > One alternative would be to parse with url_parse() or similar, and\n> > compare the individual fields (skipping username/password). I think that\n> > would probably also work in practice, but it seemed to me that the\n> > simplest change would be sticking with string comparisons.\n> \n> It still feels rather roundabout to compare URLs only to figure out\n> whether we have been redirected. I wondered whether there is maybe a\n> more direct way to get that info, and there indeed is\n> CURLINFO_REDIRECT_COUNT, which allows us to retrieve the number of\n> redirects that have happened.\n> \n> Is that interface maybe a more direct way to get what we're after?\n\nHmm, interesting. We need to grab the effective URL anyway in order to\nactually do the base-url update. But in theory we could replace the \"did\nwe redirect at all\" early return with a check of the redirect count. And\nindeed, the patch looks much cleaner (see below).\n\nBut sadly, it doesn't work! Curl reports that we did 1 redirect for the\ninitial request. I think it is counting the extra request it does for\nthe auth (we get a 401, then it auto-retries with the password to get a\n200).\n\nSo we really do need to do our string-based check for \"did the URL\nmeaningfully change\", and all of the annoying cred-stripping that comes\nwith it.\n\nToo bad, because your solution looks much nicer. ;)\n\n---\ndiff --git a/http.c b/http.c\nindex c8fcfd7693..e95fe4ba7b 100644\n--- a/http.c\n+++ b/http.c\n@@ -2305,6 +2305,9 @@ static int http_request(const char *url,\n \t\tstrbuf_release(&raw);\n \t}\n \n+\tcurl_easy_getinfo(slot->curl, CURLINFO_REDIRECT_COUNT,\n+\t\t\t  &options->redirects);\n+\n \tif (options->effective_url)\n \t\tcurlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL,\n \t\t\t\toptions->effective_url);\n@@ -2325,8 +2328,6 @@ static int http_request(const char *url,\n  * The \"got\" parameter is the URL that curl reported to us as where we ended\n  * up.\n  *\n- * Returns 1 if we updated the base url, 0 otherwise.\n- *\n  * Our basic strategy is to compare \"base\" and \"asked\" to find the bits\n  * specific to our request. We then strip those bits off of \"got\" to yield the\n  * new base. So for example, if our base is \"http://example.com/foo.git\",\n@@ -2340,16 +2341,13 @@ static int http_request(const char *url,\n  * scheme is unlikely to represent a real git repository, and failing to\n  * rewrite the base opens options for malicious redirects to do funny things.\n  */\n-static int update_url_from_redirect(struct strbuf *base,\n-\t\t\t\t    const char *asked,\n-\t\t\t\t    const struct strbuf *got)\n+static void update_url_from_redirect(struct strbuf *base,\n+\t\t\t\t     const char *asked,\n+\t\t\t\t     const struct strbuf *got)\n {\n \tconst char *tail;\n \tsize_t new_len;\n \n-\tif (!strcmp(asked, got->buf))\n-\t\treturn 0;\n-\n \tif (!skip_prefix(asked, base->buf, &tail))\n \t\tBUG(\"update_url_from_redirect: %s is not a superset of %s\",\n \t\t    asked, base->buf);\n@@ -2363,8 +2361,6 @@ static int update_url_from_redirect(struct strbuf *base,\n \n \tstrbuf_reset(base);\n \tstrbuf_add(base, got->buf, new_len);\n-\n-\treturn 1;\n }\n \n /*\n@@ -2426,12 +2422,12 @@ static int http_request_recoverable(const char *url,\n \tif (ret == HTTP_RATE_LIMITED && !http_max_retries)\n \t\treturn HTTP_ERROR;\n \n-\tif (options->effective_url && options->base_url) {\n-\t\tif (update_url_from_redirect(options->base_url,\n-\t\t\t\t\t     url, options->effective_url)) {\n-\t\t\tcredential_from_url(&http_auth, options->base_url->buf);\n-\t\t\turl = options->effective_url->buf;\n-\t\t}\n+\tif (options->redirects > 0 &&\n+\t    options->effective_url && options->base_url) {\n+\t\tupdate_url_from_redirect(options->base_url, url,\n+\t\t\t\t\t options->effective_url);\n+\t\tcredential_from_url(&http_auth, options->base_url->buf);\n+\t\turl = options->effective_url->buf;\n \t}\n \n \twhile ((ret == HTTP_REAUTH && --i) ||\ndiff --git a/http.h b/http.h\nindex 729c51904d..79cd0a2c1a 100644\n--- a/http.h\n+++ b/http.h\n@@ -171,6 +171,12 @@ struct http_get_options {\n \t * libcurl 7.66.0 or later), or -1 if no such header was present.\n \t */\n \tlong retry_after;\n+\n+\t/*\n+\t * After a request completes, contains the number of redirects reported\n+\t * by curl.\n+\t */\n+\tlong redirects;\n };\n \n /* Return values for http_get_*() */\n"},{"id":"553541","messageId":"artP42iEIYeQxh4C@pks.im","threadId":"66402","inReplyTo":"20260928193644.GA1075764@coredump.intra.peff.net","subject":"Re: [PATCH] http: handle curl stripping creds from effective url","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-09-29T05:42:59Z","receivedAt":"2026-09-29T05:43:12Z","isPatch":true,"body":"On Mon, Sep 28, 2026 at 03:36:44PM -0400, Jeff King wrote:\n> On Mon, Sep 28, 2026 at 03:01:07PM +0200, Patrick Steinhardt wrote:\n[snip]\n> > > I've used curl's curl_url() interface to do the stripping here, mostly\n> > > because its behavior should match the stripping it does internally. And\n> > > also, though we have code to parse a URL, we don't have any to\n> > > reconstruct it, making a single string comparison hard.\n> > > \n> > > One alternative would be to parse with url_parse() or similar, and\n> > > compare the individual fields (skipping username/password). I think that\n> > > would probably also work in practice, but it seemed to me that the\n> > > simplest change would be sticking with string comparisons.\n> > \n> > It still feels rather roundabout to compare URLs only to figure out\n> > whether we have been redirected. I wondered whether there is maybe a\n> > more direct way to get that info, and there indeed is\n> > CURLINFO_REDIRECT_COUNT, which allows us to retrieve the number of\n> > redirects that have happened.\n> > \n> > Is that interface maybe a more direct way to get what we're after?\n> \n> Hmm, interesting. We need to grab the effective URL anyway in order to\n> actually do the base-url update. But in theory we could replace the \"did\n> we redirect at all\" early return with a check of the redirect count. And\n> indeed, the patch looks much cleaner (see below).\n> \n> But sadly, it doesn't work! Curl reports that we did 1 redirect for the\n> initial request. I think it is counting the extra request it does for\n> the auth (we get a 401, then it auto-retries with the password to get a\n> 200).\n> \n> So we really do need to do our string-based check for \"did the URL\n> meaningfully change\", and all of the annoying cred-stripping that comes\n> with it.\n> \n> Too bad, because your solution looks much nicer. ;)\n\nOh, well, that really is too bad indeed. Anyway, let's go with your\nfirst version in that case. Thanks!\n\nPatrick\n"}]}