From 8585c50b05c46f0b5d604a404273705a724b19f5 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Wed, 8 Jul 2026 17:03:34 +0200 Subject: [PATCH 1/2] bundle-uri: drain remaining response on invalid bundle-uri lines On clone, when the client sends the `bundle-uri` command, the server might respond with invalid data. For example if it sends information about a bundle where the 'uri' is empty, it produces the following error: Cloning into 'foo'... error: bundle-uri: line has empty key or value error: error on bundle-uri response line 4: bundle.bundle-1.uri= error: could not retrieve server-advertised bundle-uri list This error is bubbled up to `transport_get_remote_bundle_uri()`, which is called by `cmd_clone()` in builtin/clone.c. Over here, the return value is ignored, so clone continues. Despite this, it still dies with this error: fatal: expected 'packfile' This happens because `get_remote_bundle_uri()` exited early, leaving some unprocessed packet data behind in the read buffer. This is misleading to the user, because it suggests a problem with the packfile exchange, when in reality it's caused by a misconfigured bundle-URI on the server-side. Fix this by continuing to read packets when an error was encountered, but without processing the remaining lines. This drains the protocol stream so no stale data is left behind and the caller can use it if they like. With this, clone now continues successfully if invalid bundle-URI data was sent by the server. This is intentional, because since the inception of `transport_get_remote_bundle_uri()` in 0cfde740f0 (clone: request the 'bundle-uri' command when available, 2022-12-22) the return value of that function is ignored in `cmd_clone()` so the clone can continue without bundles. Signed-off-by: Toon Claes Signed-off-by: Junio C Hamano --- connect.c | 15 ++++++++++++--- t/t5558-clone-bundle-uri.sh | 29 +++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/connect.c b/connect.c index 47e39d2a73..1d74c1eda2 100644 --- a/connect.c +++ b/connect.c @@ -517,7 +517,7 @@ static void send_capabilities(int fd_out, struct packet_reader *reader) int get_remote_bundle_uri(int fd_out, struct packet_reader *reader, struct bundle_list *bundles, int stateless_rpc) { - int line_nr = 1; + int line_nr = 1, err = 0; /* Assert bundle-uri support */ ensure_server_supports_v2("bundle-uri"); @@ -536,10 +536,19 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader, const char *line = reader->line; line_nr++; + /* + * Do not parse if an error was encountered, but + * continue draining the response so no stale data + * is left in the reader for subsequent protocol + * exchanges. + */ + if (err) + continue; + if (!bundle_uri_parse_line(bundles, line)) continue; - return error(_("error on bundle-uri response line %d: %s"), + err = error(_("error on bundle-uri response line %d: %s"), line_nr, line); } @@ -554,7 +563,7 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader, check_stateless_delimiter(stateless_rpc, reader, _("expected response end packet after ref listing")); - return 0; + return err; } struct ref **get_remote_refs(int fd_out, struct packet_reader *reader, diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh index 7a0943bd36..7cc8627e17 100755 --- a/t/t5558-clone-bundle-uri.sh +++ b/t/t5558-clone-bundle-uri.sh @@ -1302,6 +1302,35 @@ test_expect_success 'bundles with newline in target path are rejected' ' test_path_is_missing escape ' +test_expect_success 'bundles advertised with missing URI' ' + git clone --no-local --mirror clone-from \ + "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" && + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config uploadpack.advertiseBundleURIs true && + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config bundle.version 1 && + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config bundle.mode all && + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config bundle.bundle-1.creationToken 1 && + + git -c transfer.bundleURI=true clone \ + "$HTTPD_URL/smart/no-uri.git" target-no-uri 2>err && + test_grep "bundle ${SQ}bundle-1${SQ} has no uri" err && + test_grep ! "expected packfile" err +' + +test_expect_success 'bundles advertised with empty URI' ' + git clone --no-local --mirror clone-from \ + "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" && + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config uploadpack.advertiseBundleURIs true && + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.version 1 && + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.mode all && + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.bundle-1.uri "" && + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.bundle-1.creationToken 1 && + + git -c transfer.bundleURI=true clone \ + "$HTTPD_URL/smart/empty-uri.git" target-empty-uri 2>err && + test_grep "bundle ${SQ}bundle-1${SQ} has no uri" err && + test_grep ! "expected packfile" err +' + # Do not add tests here unless they use the HTTP server, as they will # not run unless the HTTP dependencies exist. From 50de1169e4bf5cff947e10f64e9855669fdd2849 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Wed, 8 Jul 2026 17:03:35 +0200 Subject: [PATCH 2/2] bundle-uri: stop sending invalid bundle configuration When bundle-URI info is requested by the client, the server responds with all "bundle.*" config lines as key=value packet lines. On the client-side, the received bundle config packet lines are always expected to contain both a key and a value otherwise the client errors out during parsing. The server performs no validation of the read bundle configuration though which results in any misconfiguration on the server-side, such as bundle configuration with an empty value, being blindly sent to the client. To avoid having the server transmit invalid configuration to clients, only send bundle configuration that has non-empty values. This change makes bundle-URI information sent by the server syntactically correct, but semantically it still can be invalid. For example the server may end up sending `bundle.bundle-1.creationToken`, but be lacking a `bundle.bundle-1.uri` for that bundle. The `uri` is mandatory, thus the client cannot process this bundle and will error with the message: error: bundle 'bundle-1' has no uri Fixing this would require a more complex solution, because bundles need to be validated as a whole and not line-by-line. This is considered outside the scope of this change. Co-authored-by: Toon Claes Signed-off-by: Justin Tobler Signed-off-by: Toon Claes Signed-off-by: Junio C Hamano --- bundle-uri.c | 8 ++++++-- t/lib-bundle-uri-protocol.sh | 23 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/bundle-uri.c b/bundle-uri.c index 3b2e347288..f956d3db7b 100644 --- a/bundle-uri.c +++ b/bundle-uri.c @@ -946,8 +946,12 @@ static int config_to_packet_line(const char *key, const char *value, { struct packet_reader *writer = data; - if (starts_with(key, "bundle.")) - packet_write_fmt(writer->fd, "%s=%s", key, value); + if (starts_with(key, "bundle.")) { + if (value && *value) + packet_write_fmt(writer->fd, "%s=%s", key, value); + else + warning(_("config '%s' has no value"), key); + } return 0; } diff --git a/t/lib-bundle-uri-protocol.sh b/t/lib-bundle-uri-protocol.sh index de09b6b02e..e0e19715cd 100644 --- a/t/lib-bundle-uri-protocol.sh +++ b/t/lib-bundle-uri-protocol.sh @@ -214,3 +214,26 @@ test_expect_success "test bundle-uri with $BUNDLE_URI_PROTOCOL:// using protocol >actual && test_cmp_config_output expect actual ' + +test_expect_success "test bundle-uri with $BUNDLE_URI_PROTOCOL:// using protocol v2 with empty value" ' + test_config -C "$BUNDLE_URI_PARENT" \ + bundle.bundle1.uri "$BUNDLE_URI_BUNDLE_URI_ESCAPED-1.bdl" && + test_config -C "$BUNDLE_URI_PARENT" \ + bundle.bundle2.uri "" && + + # The empty bundle.bundle2.uri value is invalid configuration and the + # server must not advertise it to the client. + cat >expect <<-EOF && + [bundle] + version = 1 + mode = all + [bundle "bundle1"] + uri = $BUNDLE_URI_BUNDLE_URI_ESCAPED-1.bdl + EOF + + test-tool bundle-uri \ + ls-remote \ + "$BUNDLE_URI_REPO_URI" \ + >actual && + test_cmp_config_output expect actual +'