# [PATCH] http: handle curl stripping creds from effective url 5 messages from 2026-09-28 to 2026-09-29. Participants: Jeff King, Patrick Steinhardt, Junio C Hamano. Thread: https://gitlist.dev/t/66402 ## Jeff King, 2026-09-28 04:01 Subject: [PATCH] http: handle curl stripping creds from effective url Message-ID: <20260928040149.GA498186@coredump.intra.peff.net> ``` When we detect that curl performed a redirect of a URL we requested, we update our base URL to match the new location and flush the http_auth credentials. This goes back to c93c92f309 (http: update base URLs when we see redirects, 2013-09-28). We detect the redirect by comparing the requested URL to the response from CURLINFO_EFFECTIVE_URL, using a simple string comparison. This has worked fine for years, but a change in the upcoming curl 8.23.0 adds a complication. If our URL directly contains credentials (like "https://user:pass@example.com/foo.git"), then as of 7a6bd027d0 (getinfo: make sure CURLINFO_EFFECTIVE_URL does not contain creds, 2026-09-21), curl will strip the credentials from what it returns (so just "https://example.com/foo.git" in this case). This breaks our direct string comparison, and we believe that we've been redirected. We flush our http_auth credentials, and now subsequent requests will use the reduced URL, causing us to re-request credentials from the user. Notably this causes t5550.15 (among others) to complain; it tries a clone with credentials in the URL, and fails if the user is prompted at all. We can handle this new behavior by doing a more careful comparison: if the direct string comparison fails, we'll strip out the credentials ourselves and compare. This is a little extra work, but in practice it should only happen once per process. I've used curl's curl_url() interface to do the stripping here, mostly because its behavior should match the stripping it does internally. And also, though we have code to parse a URL, we don't have any to reconstruct it, making a single string comparison hard. One alternative would be to parse with url_parse() or similar, and compare the individual fields (skipping username/password). I think that would probably also work in practice, but it seemed to me that the simplest change would be sticking with string comparisons. The curl_url() interface appeared in 7.62.0. We document that 7.61.0 is still supported, so I've made it conditional here. Only new versions strip the result from CURLINFO_EFFECTIVE_URL, so it's OK for very old versions to skip the extra comparison. Likewise if we encounter any errors, we just quietly skip the comparison. That's fine if you don't have creds in your URLs, and if you do, you'll get end up in the existing error path (a redirect warning, and eventually an auth failure). Signed-off-by: Jeff King --- I hit this in Debian unstable's packaging of libcurl; the new behavior is in 8.23.0-rc2, but not -rc1. We could probably declare 7.62.0 the oldest supported version of curl, but it would really only save a few lines of #ifdef here. I'd prefer to consider that question separately. git-curl-compat.h | 7 +++++++ http.c | 41 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/git-curl-compat.h b/git-curl-compat.h index 032aaf7126..25678b5dbd 100644 --- a/git-curl-compat.h +++ b/git-curl-compat.h @@ -28,6 +28,13 @@ * introduced, oldest first, in the official version of cURL library. */ +/** + * curl_url() interface added in 7.62.0 (October 2018) + */ +#if LIBCURL_VERSION_NUM >= 0x073e00 +#define GIT_CURL_HAVE_CURL_URL +#endif + /** * Versions before curl 7.66.0 (September 2019) required manually setting the * transfer-encoding for a streaming POST; after that this is handled diff --git a/http.c b/http.c index c8fcfd7693..4af3c29c76 100644 --- a/http.c +++ b/http.c @@ -2315,6 +2315,45 @@ static int http_request(const char *url, return ret; } +#ifndef GIT_CURL_HAVE_CURL_URL +#define strip_url_credential(in) NULL +#else +static char *strip_url_credential(const char *in) +{ + char *ret = NULL; + CURLU *url; + + url = curl_url(); + if (!url) + goto out; + + if (curl_url_set(url, CURLUPART_URL, in, 0)) + goto out; + + curl_url_set(url, CURLUPART_USER, NULL, 0); + curl_url_set(url, CURLUPART_PASSWORD, NULL, 0); + curl_url_get(url, CURLUPART_URL, &ret, 0); + +out: + curl_url_cleanup(url); + return ret; +} +#endif + +static int match_effective_url(const char *asked, const char *got) +{ + char *stripped; + int ret; + + if (!strcmp(asked, got)) + return 1; + + stripped = strip_url_credential(asked); + ret = stripped && !strcmp(stripped, got); + curl_free(stripped); + return ret; +} + /* * Update the "base" url to a more appropriate value, as deduced by * redirects seen when requesting a URL starting with "url". @@ -2347,7 +2386,7 @@ static int update_url_from_redirect(struct strbuf *base, const char *tail; size_t new_len; - if (!strcmp(asked, got->buf)) + if (match_effective_url(asked, got->buf)) return 0; if (!skip_prefix(asked, base->buf, &tail)) -- 2.56.0.rc2.338.gcaacf6bdf7 ``` ## Patrick Steinhardt, 2026-09-28 13:01 Subject: Re: [PATCH] http: handle curl stripping creds from effective url Message-ID: In-Reply-To: <20260928040149.GA498186@coredump.intra.peff.net> ``` On Mon, Sep 28, 2026 at 12:01:49AM -0400, Jeff King wrote: > When we detect that curl performed a redirect of a URL we requested, we > update our base URL to match the new location and flush the http_auth > credentials. This goes back to c93c92f309 (http: update base URLs when > we see redirects, 2013-09-28). > > We detect the redirect by comparing the requested URL to the response > from CURLINFO_EFFECTIVE_URL, using a simple string comparison. This has > worked fine for years, but a change in the upcoming curl 8.23.0 adds a > complication. If our URL directly contains credentials (like > "https://user:pass@example.com/foo.git"), then as of 7a6bd027d0 > (getinfo: make sure CURLINFO_EFFECTIVE_URL does not contain creds, > 2026-09-21), curl will strip the credentials from what it returns (so > just "https://example.com/foo.git" in this case). > > This breaks our direct string comparison, and we believe that we've been > redirected. We flush our http_auth credentials, and now subsequent > requests will use the reduced URL, causing us to re-request credentials > from the user. Notably this causes t5550.15 (among others) to complain; > it tries a clone with credentials in the URL, and fails if the user is > prompted at all. > > We can handle this new behavior by doing a more careful comparison: if > the direct string comparison fails, we'll strip out the credentials > ourselves and compare. This is a little extra work, but in practice it > should only happen once per process. So in my own words, we want to detect the case where we have been redirected and, if we have been, we want to strip credentials. But this logic is about to break as curl starts to rewrite EFFECTIVE_URL more aggressively, and that makes us detect redirects in cases where there were none. > I've used curl's curl_url() interface to do the stripping here, mostly > because its behavior should match the stripping it does internally. And > also, though we have code to parse a URL, we don't have any to > reconstruct it, making a single string comparison hard. > > One alternative would be to parse with url_parse() or similar, and > compare the individual fields (skipping username/password). I think that > would probably also work in practice, but it seemed to me that the > simplest change would be sticking with string comparisons. It still feels rather roundabout to compare URLs only to figure out whether we have been redirected. I wondered whether there is maybe a more direct way to get that info, and there indeed is CURLINFO_REDIRECT_COUNT, which allows us to retrieve the number of redirects that have happened. Is that interface maybe a more direct way to get what we're after? Patrick ``` ## Junio C Hamano, 2026-09-28 15:01 Subject: Re: [PATCH] http: handle curl stripping creds from effective url Message-ID: In-Reply-To: <20260928040149.GA498186@coredump.intra.peff.net> ``` Jeff King writes: > I've used curl's curl_url() interface to do the stripping here, mostly > because its behavior should match the stripping it does internally. And > also, though we have code to parse a URL, we don't have any to > reconstruct it, making a single string comparison hard. > > One alternative would be to parse with url_parse() or similar, and > compare the individual fields (skipping username/password). I think that > would probably also work in practice, but it seemed to me that the > simplest change would be sticking with string comparisons. Very nice. > The curl_url() interface appeared in 7.62.0. We document that 7.61.0 is > still supported, so I've made it conditional here. Only new versions > strip the result from CURLINFO_EFFECTIVE_URL, so it's OK for very old > versions to skip the extra comparison. ;-) > We could probably declare 7.62.0 the oldest supported version of curl, > but it would really only save a few lines of #ifdef here. I'd prefer to > consider that question separately. That is very sensible. > +#ifndef GIT_CURL_HAVE_CURL_URL > +#define strip_url_credential(in) NULL > +#else > +static char *strip_url_credential(const char *in) > +{ > + char *ret = NULL; > + CURLU *url; > + > + url = curl_url(); > + if (!url) > + goto out; > + > + if (curl_url_set(url, CURLUPART_URL, in, 0)) > + goto out; > + > + curl_url_set(url, CURLUPART_USER, NULL, 0); > + curl_url_set(url, CURLUPART_PASSWORD, NULL, 0); > + curl_url_get(url, CURLUPART_URL, &ret, 0); > + > +out: > + curl_url_cleanup(url); > + return ret; > +} > +#endif ``` ## Jeff King, 2026-09-28 19:36 Subject: Re: [PATCH] http: handle curl stripping creds from effective url Message-ID: <20260928193644.GA1075764@coredump.intra.peff.net> In-Reply-To: ``` On Mon, Sep 28, 2026 at 03:01:07PM +0200, Patrick Steinhardt wrote: > > We can handle this new behavior by doing a more careful comparison: if > > the direct string comparison fails, we'll strip out the credentials > > ourselves and compare. This is a little extra work, but in practice it > > should only happen once per process. > > So in my own words, we want to detect the case where we have been > redirected and, if we have been, we want to strip credentials. But this > logic is about to break as curl starts to rewrite EFFECTIVE_URL more > aggressively, and that makes us detect redirects in cases where there > were none. Yes, though I'd be careful to distinguish "strip" credentials versus "flush" credentials. I'd take the former to mean "remove them from the URL if they are embedded in it", whereas the latter to mean "throw away any credentials in the http_auth credential struct". Those credentials in http_auth might have come from the user, or even been extracted from the URL originally (or some combination; e.g., getting the user from the URL and the password from the user). But we want to flush them so that we won't provide them to a redirected destination. I think we are on the same page and this is just wording pedantry, but wanted to make sure. You might also reasonably ask: if we have already extracted the credentials from the original URL into http_auth, can't we just strip them immediately afterwards, and always deal with a vanilla URL? That was my original approach, but sadly it does not work because we then pass that URL across process boundaries (e.g., to http-push), but the extracted http_auth credential struct doesn't make it. One of the tests in t5540 notices those. > > I've used curl's curl_url() interface to do the stripping here, mostly > > because its behavior should match the stripping it does internally. And > > also, though we have code to parse a URL, we don't have any to > > reconstruct it, making a single string comparison hard. > > > > One alternative would be to parse with url_parse() or similar, and > > compare the individual fields (skipping username/password). I think that > > would probably also work in practice, but it seemed to me that the > > simplest change would be sticking with string comparisons. > > It still feels rather roundabout to compare URLs only to figure out > whether we have been redirected. I wondered whether there is maybe a > more direct way to get that info, and there indeed is > CURLINFO_REDIRECT_COUNT, which allows us to retrieve the number of > redirects that have happened. > > Is that interface maybe a more direct way to get what we're after? Hmm, interesting. We need to grab the effective URL anyway in order to actually do the base-url update. But in theory we could replace the "did we redirect at all" early return with a check of the redirect count. And indeed, the patch looks much cleaner (see below). But sadly, it doesn't work! Curl reports that we did 1 redirect for the initial request. I think it is counting the extra request it does for the auth (we get a 401, then it auto-retries with the password to get a 200). So we really do need to do our string-based check for "did the URL meaningfully change", and all of the annoying cred-stripping that comes with it. Too bad, because your solution looks much nicer. ;) --- diff --git a/http.c b/http.c index c8fcfd7693..e95fe4ba7b 100644 --- a/http.c +++ b/http.c @@ -2305,6 +2305,9 @@ static int http_request(const char *url, strbuf_release(&raw); } + curl_easy_getinfo(slot->curl, CURLINFO_REDIRECT_COUNT, + &options->redirects); + if (options->effective_url) curlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL, options->effective_url); @@ -2325,8 +2328,6 @@ static int http_request(const char *url, * The "got" parameter is the URL that curl reported to us as where we ended * up. * - * Returns 1 if we updated the base url, 0 otherwise. - * * Our basic strategy is to compare "base" and "asked" to find the bits * specific to our request. We then strip those bits off of "got" to yield the * new base. So for example, if our base is "http://example.com/foo.git", @@ -2340,16 +2341,13 @@ static int http_request(const char *url, * scheme is unlikely to represent a real git repository, and failing to * rewrite the base opens options for malicious redirects to do funny things. */ -static int update_url_from_redirect(struct strbuf *base, - const char *asked, - const struct strbuf *got) +static void update_url_from_redirect(struct strbuf *base, + const char *asked, + const struct strbuf *got) { const char *tail; size_t new_len; - if (!strcmp(asked, got->buf)) - return 0; - if (!skip_prefix(asked, base->buf, &tail)) BUG("update_url_from_redirect: %s is not a superset of %s", asked, base->buf); @@ -2363,8 +2361,6 @@ static int update_url_from_redirect(struct strbuf *base, strbuf_reset(base); strbuf_add(base, got->buf, new_len); - - return 1; } /* @@ -2426,12 +2422,12 @@ static int http_request_recoverable(const char *url, if (ret == HTTP_RATE_LIMITED && !http_max_retries) return HTTP_ERROR; - if (options->effective_url && options->base_url) { - if (update_url_from_redirect(options->base_url, - url, options->effective_url)) { - credential_from_url(&http_auth, options->base_url->buf); - url = options->effective_url->buf; - } + if (options->redirects > 0 && + options->effective_url && options->base_url) { + update_url_from_redirect(options->base_url, url, + options->effective_url); + credential_from_url(&http_auth, options->base_url->buf); + url = options->effective_url->buf; } while ((ret == HTTP_REAUTH && --i) || diff --git a/http.h b/http.h index 729c51904d..79cd0a2c1a 100644 --- a/http.h +++ b/http.h @@ -171,6 +171,12 @@ struct http_get_options { * libcurl 7.66.0 or later), or -1 if no such header was present. */ long retry_after; + + /* + * After a request completes, contains the number of redirects reported + * by curl. + */ + long redirects; }; /* Return values for http_get_*() */ ``` ## Patrick Steinhardt, 2026-09-29 05:42 Subject: Re: [PATCH] http: handle curl stripping creds from effective url Message-ID: In-Reply-To: <20260928193644.GA1075764@coredump.intra.peff.net> ``` On Mon, Sep 28, 2026 at 03:36:44PM -0400, Jeff King wrote: > On Mon, Sep 28, 2026 at 03:01:07PM +0200, Patrick Steinhardt wrote: [snip] > > > I've used curl's curl_url() interface to do the stripping here, mostly > > > because its behavior should match the stripping it does internally. And > > > also, though we have code to parse a URL, we don't have any to > > > reconstruct it, making a single string comparison hard. > > > > > > One alternative would be to parse with url_parse() or similar, and > > > compare the individual fields (skipping username/password). I think that > > > would probably also work in practice, but it seemed to me that the > > > simplest change would be sticking with string comparisons. > > > > It still feels rather roundabout to compare URLs only to figure out > > whether we have been redirected. I wondered whether there is maybe a > > more direct way to get that info, and there indeed is > > CURLINFO_REDIRECT_COUNT, which allows us to retrieve the number of > > redirects that have happened. > > > > Is that interface maybe a more direct way to get what we're after? > > Hmm, interesting. We need to grab the effective URL anyway in order to > actually do the base-url update. But in theory we could replace the "did > we redirect at all" early return with a check of the redirect count. And > indeed, the patch looks much cleaner (see below). > > But sadly, it doesn't work! Curl reports that we did 1 redirect for the > initial request. I think it is counting the extra request it does for > the auth (we get a 401, then it auto-retries with the password to get a > 200). > > So we really do need to do our string-based check for "did the URL > meaningfully change", and all of the annoying cred-stripping that comes > with it. > > Too bad, because your solution looks much nicer. ;) Oh, well, that really is too bad indeed. Anyway, let's go with your first version in that case. Thanks! Patrick ```