[PATCH] http: handle curl stripping creds from effective url

[PATCH] http: handle curl stripping creds from effective url

[Jeff King]

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 <peff@peff.net>
---
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

Re: [PATCH] http: handle curl stripping creds from effective url

[Patrick Steinhardt]

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

Re: [PATCH] http: handle curl stripping creds from effective url

[Jeff King]

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_*() */

Re: [PATCH] http: handle curl stripping creds from effective url

[Patrick Steinhardt]

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

Re: [PATCH] http: handle curl stripping creds from effective url

[Junio C Hamano]

Jeff King <peff@peff.net> 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