[PATCH v3 0/3] bundle-uri: do not die on unresolvable relative URIs

[PATCH v3 0/3] bundle-uri: do not die on unresolvable relative URIs

[Aniket Gupta]

A bundle list can contain a relative URI that cannot be resolved, and
today one such entry makes the whole clone die. This series lets the
bundle code report it and skip that bundle instead.

Changes since v2:
- split into three patches
- reworded the commit message to describe relative_url() as a thin
  wrapper around relative_url_gently()
- the credentials test now compares the full error message
- the error names the URL that was passed in, not what is left of it
  after some components were stripped off

Aniket Gupta (3):
  remote: add relative_url_gently()
  remote: do not show credentials when relative_url() fails
  bundle-uri: do not die on unresolvable relative URIs

 bundle-uri.c                | 10 +++++++++-
 remote.c                    | 37 +++++++++++++++++++++++++++++++------
 remote.h                    |  8 ++++++++
 t/t0060-path-utils.sh       |  9 +++++++++
 t/t5750-bundle-uri-parse.sh | 15 +++++++--------
 5 files changed, 64 insertions(+), 15 deletions(-)


base-commit: ebdc087dc99d68bb340c282b2ba5f722def28d19
-- 
2.43.0

[PATCH v3 1/3] remote: add relative_url_gently()

[Aniket Gupta]

relative_url() calls die() when there are more "../" components in the
URL than the base URL can absorb. A caller that would rather carry on
has no way to do so.

Add relative_url_gently(), which returns NULL and adds the reason to a
strbuf instead of dying. relative_url() is now a thin wrapper around
it that continues to die, so existing callers behave the same.

Signed-off-by: Aniket Gupta <aniketgupta3001@gmail.com>
---
 remote.c | 34 ++++++++++++++++++++++++++++------
 remote.h |  8 ++++++++
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/remote.c b/remote.c
index 49ee2ea318..899d805b76 100644
--- a/remote.c
+++ b/remote.c
@@ -3005,7 +3005,8 @@ void remote_state_clear(struct remote_state *remote_state)
 }
 
 /*
- * Returns 1 if it was the last chop before ':'.
+ * Returns 1 if it was the last chop before ':', or -1 if there is
+ * nothing left to chop.
  */
 static int chop_last_dir(char **remoteurl, int is_relative)
 {
@@ -3022,16 +3023,15 @@ static int chop_last_dir(char **remoteurl, int is_relative)
 	}
 
 	if (is_relative || !strcmp(".", *remoteurl))
-		die(_("cannot strip one component off url '%s'"),
-			*remoteurl);
+		return -1;
 
 	free(*remoteurl);
 	*remoteurl = xstrdup(".");
 	return 0;
 }
 
-char *relative_url(const char *remote_url, const char *url,
-		   const char *up_path)
+char *relative_url_gently(const char *remote_url, const char *url,
+			  const char *up_path, struct strbuf *err)
 {
 	int is_relative = 0;
 	int colonsep = 0;
@@ -3073,8 +3073,19 @@ char *relative_url(const char *remote_url, const char *url,
 	 */
 	while (*url) {
 		if (starts_with_dot_dot_slash_native(url)) {
+			int chopped;
+
 			url += 3;
-			colonsep |= chop_last_dir(&remoteurl, is_relative);
+			chopped = chop_last_dir(&remoteurl, is_relative);
+			if (chopped < 0) {
+				strbuf_addf(err,
+					    _("cannot strip one component off url '%s'"),
+					    remoteurl);
+				free(remoteurl);
+				strbuf_release(&sb);
+				return NULL;
+			}
+			colonsep |= chopped;
 		} else if (starts_with_dot_slash_native(url))
 			url += 2;
 		else
@@ -3102,6 +3113,17 @@ char *relative_url(const char *remote_url, const char *url,
 	return strbuf_detach(&sb, NULL);
 }
 
+char *relative_url(const char *remote_url, const char *url,
+		   const char *up_path)
+{
+	struct strbuf err = STRBUF_INIT;
+	char *out = relative_url_gently(remote_url, url, up_path, &err);
+
+	if (!out)
+		die("%s", err.buf);
+	return out;
+}
+
 int valid_remote_name(const char *name)
 {
 	int result;
diff --git a/remote.h b/remote.h
index cca02033b9..940474fa71 100644
--- a/remote.h
+++ b/remote.h
@@ -478,6 +478,14 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);
 char *relative_url(const char *remote_url, const char *url,
 		   const char *up_path);
 
+/*
+ * Like relative_url(), but if `url` cannot be resolved against
+ * `remote_url`, return NULL and add the reason to `err` instead of
+ * dying. `err` must not be NULL.
+ */
+char *relative_url_gently(const char *remote_url, const char *url,
+			  const char *up_path, struct strbuf *err);
+
 int valid_remote_name(const char *name);
 
 #endif
-- 
2.43.0

[PATCH v3 2/3] remote: do not show credentials when relative_url() fails

[Aniket Gupta]

When relative_url() cannot resolve a URL, it names the URL in its error
message. If the URL contains a user name and password, both end up in
the message, and from there in terminal output and logs.

Pass the URL through transport_anonymize_url() first. Also name the
URL that was passed in rather than what is left of it after some
components have already been stripped off, which is what a user would
recognize.

Signed-off-by: Aniket Gupta <aniketgupta3001@gmail.com>
---
 remote.c              | 5 ++++-
 t/t0060-path-utils.sh | 9 +++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 899d805b76..36834b129c 100644
--- a/remote.c
+++ b/remote.c
@@ -3078,9 +3078,12 @@ char *relative_url_gently(const char *remote_url, const char *url,
 			url += 3;
 			chopped = chop_last_dir(&remoteurl, is_relative);
 			if (chopped < 0) {
+				char *anon = transport_anonymize_url(remote_url);
+
 				strbuf_addf(err,
 					    _("cannot strip one component off url '%s'"),
-					    remoteurl);
+					    anon);
+				free(anon);
 				free(remoteurl);
 				strbuf_release(&sb);
 				return NULL;
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 56faf5fe73..cea0d13429 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -394,6 +394,15 @@ test_submodule_relative_url "../" "$(pwd)/addtest" "../repo" "$(pwd)/repo"
 test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
 test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
 
+test_expect_success 'relative_url() does not show credentials when it fails' '
+	cat >expect <<-\EOF &&
+	fatal: cannot strip one component off url '\''https://host.example/a'\''
+	EOF
+	test_must_fail test-tool submodule resolve-relative-url "" \
+		"https://user:secret@host.example/a" ../../../../../../x 2>actual &&
+	test_cmp expect actual
+'
+
 test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c" "../foo/sub/a/b/c"
 test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c/" "../foo/sub/a/b/c"
 test_submodule_relative_url "(null)" "../foo/bar/" "../sub/a/b/c" "../foo/sub/a/b/c"
-- 
2.43.0

[PATCH v3 3/3] bundle-uri: do not die on unresolvable relative URIs

[Aniket Gupta]

A bundle list, either advertised by a server or given to "git clone
--bundle-uri", may contain relative URIs. They are resolved with
relative_url(), which calls die() when there are more "../" components
than the base can absorb, so one bad entry aborts the whole clone.

Resolve them with relative_url_gently() instead. When that fails,
bundle_list_update() reports the problem as an error and leaves the
bundle without a URI. The download code already skips such a bundle,
so the clone carries on with the others.

This resolves the TODO in t5750, so update the test to match.

Signed-off-by: Aniket Gupta <aniketgupta3001@gmail.com>
---
 bundle-uri.c                | 10 +++++++++-
 t/t5750-bundle-uri-parse.sh | 15 +++++++--------
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/bundle-uri.c b/bundle-uri.c
index 2bb2eb99e4..1f198ef9a9 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -210,9 +210,17 @@ static int bundle_list_update(const char *key, const char *value,
 	strbuf_release(&id);
 
 	if (!strcmp(subkey, "uri")) {
+		struct strbuf err = STRBUF_INIT;
+
 		if (bundle->uri)
 			return -1;
-		bundle->uri = relative_url(list->baseURI, value, NULL);
+		bundle->uri = relative_url_gently(list->baseURI, value, NULL,
+						  &err);
+		if (!bundle->uri) {
+			error("%s", err.buf);
+			strbuf_release(&err);
+			return -1;
+		}
 		return 0;
 	}
 
diff --git a/t/t5750-bundle-uri-parse.sh b/t/t5750-bundle-uri-parse.sh
index 70ae94c4d0..8abf8f31ae 100755
--- a/t/t5750-bundle-uri-parse.sh
+++ b/t/t5750-bundle-uri-parse.sh
@@ -68,17 +68,16 @@ test_expect_success 'bundle_uri_parse_line(): relative URIs and parent paths' '
 		uri = <uri>/bundle.bdl
 	[bundle "two"]
 		uri = bundle.bdl
-	[bundle "three"]
-		uri = <uri>/../bundle.bdl
 	EOF
 
-	# TODO: We would prefer if parsing a bundle list would not cause
-	# a die() and instead would give a warning and allow the rest of
-	# a Git command to continue. This test_must_fail is necessary for
-	# now until the interface for relative_url() allows for reporting
-	# an error instead of die()ing.
+	cat >err.expect <<-\EOF &&
+	error: cannot strip one component off url '\''<uri>'\''
+	error: bad line: '\''bundle.three.uri=../../bundle.bdl'\''
+	EOF
+
 	test_must_fail test-tool bundle-uri parse-key-values in >actual 2>err &&
-	test_grep "fatal: cannot strip one component off url" err
+	test_cmp err.expect err &&
+	test_cmp_config_output expect actual
 '
 
 test_expect_success 'bundle_uri_parse_line() parsing edge cases: empty key or value' '
-- 
2.43.0

Re: [PATCH v3 1/3] remote: add relative_url_gently()

[Derrick Stolee]

On 11/5/2026 10:28 AM, Aniket Gupta wrote:
> relative_url() calls die() when there are more "../" components in the
> URL than the base URL can absorb. A caller that would rather carry on
> has no way to do so.

I agree that we should have a mechanism to allow users to know about a
broken bundle URI and to respond somehow. 
> Add relative_url_gently(), which returns NULL and adds the reason to a
> strbuf instead of dying. relative_url() is now a thin wrapper around
> it that continues to die, so existing callers behave the same.

This method is good to have, in general, and is written well.

The only question here is if we should be changing the behavior for
bundle-URI clones from halting on bad data to skipping bundles and
doing the full operation from the remote.

(But there's more about this in patch 3, so I'll defer until then.)

Thanks,
-Stolee

Re: [PATCH v3 2/3] remote: do not show credentials when relative_url() fails

[Derrick Stolee]

On 11/5/2026 10:28 AM, Aniket Gupta wrote:
> When relative_url() cannot resolve a URL, it names the URL in its error
> message. If the URL contains a user name and password, both end up in
> the message, and from there in terminal output and logs.
> 
> Pass the URL through transport_anonymize_url() first. Also name the
> URL that was passed in rather than what is left of it after some
> components have already been stripped off, which is what a user would
> recognize.

I'm less convinced on this factor. I understand anonymizing the URL if
we are sending it to a location that is perhaps "less trusted" (like
trace logs) but the user asked us to operate on a given URL that caused
problems, so they should know the string that caused problems.

In environments that store stdout/stderr logs, those systems already
need to be careful about such strings in the commands themselves, let
alone the output strings.

And back to the bundle-URI application: if the bundle list uses such
URIs for its child bundles, then that's not a safe mechanism for
communicating those credentials. This isn't going to protect secrets
like that from being exposed.

This is just my opinion on the necessity of this change. If we _do_
want this change, then I continued my review: the patch accomplishes
its goal.
> @@ -3078,9 +3078,12 @@ char *relative_url_gently(const char *remote_url, const char *url,
>  			url += 3;
>  			chopped = chop_last_dir(&remoteurl, is_relative);
>  			if (chopped < 0) {
> +				char *anon = transport_anonymize_url(remote_url);
> +
>  				strbuf_addf(err,
>  					    _("cannot strip one component off url '%s'"),
> -					    remoteurl);
> +					    anon);
> +				free(anon);
>  				free(remoteurl);

This code looks good, assuming taht we want to make this change.

> +test_expect_success 'relative_url() does not show credentials when it fails' '
> +	cat >expect <<-\EOF &&
> +	fatal: cannot strip one component off url '\''https://host.example/a'\''
> +	EOF
> +	test_must_fail test-tool submodule resolve-relative-url "" \
> +		"https://user:secret@host.example/a" ../../../../../../x 2>actual &&
> +	test_cmp expect actual
> +'
Thank you for including a test that demonstrates this change.

Thanks,
-Stolee

Re: [PATCH v3 3/3] bundle-uri: do not die on unresolvable relative URIs

[Derrick Stolee]

On 11/5/2026 10:28 AM, Aniket Gupta wrote:
> A bundle list, either advertised by a server or given to "git clone
> --bundle-uri", may contain relative URIs. They are resolved with
> relative_url(), which calls die() when there are more "../" components
> than the base can absorb, so one bad entry aborts the whole clone.
> 
> Resolve them with relative_url_gently() instead. When that fails,
> bundle_list_update() reports the problem as an error and leaves the
> bundle without a URI. The download code already skips such a bundle,
> so the clone carries on with the others.
> 
> This resolves the TODO in t5750, so update the test to match.

This TODO was added in ebc39479558 (bundle-uri: allow relative URLs
in bundle lists, 2022-12-22) by _me_, so clearly I thought that this
was a good change at the time. I should retract my concerns about
the change from patch 1.

The only thing that is different now is that the feature has existed
for a while, so maybe some depend on this behavior that we're
changing. That's unlikely to be important.

So I'll officially say: thanks for contributing this cleanup!

> @@ -210,9 +210,17 @@ static int bundle_list_update(const char *key, const char *value,
>  	strbuf_release(&id);
>  
>  	if (!strcmp(subkey, "uri")) {
> +		struct strbuf err = STRBUF_INIT;
> +
>  		if (bundle->uri)
>  			return -1;
> -		bundle->uri = relative_url(list->baseURI, value, NULL);
> +		bundle->uri = relative_url_gently(list->baseURI, value, NULL,
> +						  &err);
> +		if (!bundle->uri) {
> +			error("%s", err.buf);
> +			strbuf_release(&err);
> +			return -1;
> +		}
>  		return 0;
>  	}
>  
> diff --git a/t/t5750-bundle-uri-parse.sh b/t/t5750-bundle-uri-parse.sh
> index 70ae94c4d0..8abf8f31ae 100755
> --- a/t/t5750-bundle-uri-parse.sh
> +++ b/t/t5750-bundle-uri-parse.sh
> @@ -68,17 +68,16 @@ test_expect_success 'bundle_uri_parse_line(): relative URIs and parent paths' '
>  		uri = <uri>/bundle.bdl
>  	[bundle "two"]
>  		uri = bundle.bdl
> -	[bundle "three"]
> -		uri = <uri>/../bundle.bdl
>  	EOF

In the context of this diff, this looks like you are removing the
bad input, but this is actually the expected output of the test-tool.

The input is a few lines earlier:

	cat >in <<-\EOF &&
	bundle.one.uri=bundle.bdl
	bundle.two.uri=../bundle.bdl
	bundle.three.uri=../../bundle.bdl
	EOF

>  
> -	# TODO: We would prefer if parsing a bundle list would not cause
> -	# a die() and instead would give a warning and allow the rest of
> -	# a Git command to continue. This test_must_fail is necessary for
> -	# now until the interface for relative_url() allows for reporting
> -	# an error instead of die()ing.
> +	cat >err.expect <<-\EOF &&
> +	error: cannot strip one component off url '\''<uri>'\''
> +	error: bad line: '\''bundle.three.uri=../../bundle.bdl'\''
> +	EOF
> +

But this err.expect matches.

>  	test_must_fail test-tool bundle-uri parse-key-values in >actual 2>err &&
> -	test_grep "fatal: cannot strip one component off url" err
> +	test_cmp err.expect err &&
> +	test_cmp_config_output expect actual

And now we're actually seeing the list match the expectation (minus
the broken item).

LGTM.
Thanks,
-Stolee

Re: [PATCH v3 0/3] bundle-uri: do not die on unresolvable relative URIs

[Derrick Stolee]

On 11/5/2026 10:28 AM, Aniket Gupta wrote:
> A bundle list can contain a relative URI that cannot be resolved, and
> today one such entry makes the whole clone die. This series lets the
> bundle code report it and skip that bundle instead.

When reading patch-by-patch, I initially was concerned about the
behavior change. However, patch 3 removes a long-standing TODO
comment _that I wrote_ so I'm inclined to take the change. I
didn't see anything critical to change about these patches.

That said, we may have something for #leftoverbits:

How could we create a toggle to re-enable the die() if desired?

Perhaps a user _wants_ to fail on bad URIs instead of spending
time on an incomplete bundle advertisement and expensive fetch
from the remote.

Such an option could be a follow-up, now that you've implemented
the _gently() version and the bundle URI machinery could decide
for itself whether this is a hard failure or not.

Thanks,
-Stolee

Re: [PATCH v3 0/3] bundle-uri: do not die on unresolvable relative URIs

[Aniket Gupta]

Hi Stolee,

Thanks for the review.

> I'm less convinced on this factor.

Fair point. I tried it with a password in the remote URL but the "failed
to download bundle from URI" warning prints it as is. I think this patch
will only cover this one message. The reason I did it is that git
already hides the password when it shows a remote URL. For eg. in the
"From" line of fetch. It is a separate patch so it is easy to drop if
you and Patrick think it is not worth it.

While checking this I noticed that my commit message for patch 3 is not
accurate. It says that the clone carries on with the other bundles, but
the list is only used up to the bad entry (not at all for a list given
to --bundle-uri) and then the clone fetches from the remote. I can send
a v4 with that fixed if its fine

Aniket

Re: [PATCH v3 2/3] remote: do not show credentials when relative_url() fails

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> In environments that store stdout/stderr logs, those systems already
> need to be careful about such strings in the commands themselves, let
> alone the output strings.

Hmph, is this true?  How can these systems be _careful_ without
knowing what these strings are?  By guessing that https:// is a common
enough thing that shoud raise red flag if there is some:thing@ that
follows it?

> And back to the bundle-URI application: if the bundle list uses such
> URIs for its child bundles, then that's not a safe mechanism for
> communicating those credentials. This isn't going to protect secrets
> like that from being exposed.

This is very true.  So the patch may be protecting us against an
issue that does not exist in practice?

Where do we use anonymize_url() in existing code paths?

 - "git clone" leaving "clone: from X" in the reflog
 - "git fetch" showing "From X" to show ref updates
 - "git push" shwoing "Pushing to X" in verbose output
 - remote-curl talking about "url" in die() messages
 - transport showing "To X" to show push_report.

These are all talking about what is shown on the screen except for
the reflog entry left by "git clone", I think.

Thanks.