Re: [PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 4, 2024, 07:45 UTC
Message-ID: <ZeV8BNCr0QN84ZQE@tanuki>
In-Reply-To: <20240228225050.GA1159078@coredump.intra.peff.net>

On Wed, Feb 28, 2024 at 05:50:50PM -0500, Jeff King wrote:

> Clients are only supposed to request particular capabilities or features
> if the server advertised them. For the "packfile-uris" feature, we only
> advertise it if uploadpack.blobpacfileuri is set, but we always accept a

Nit: s/uploadpack.blobpackfileuri. I noticed that this isn't actually
documented in git-config(1), but that's not a problem of this commit.

Show 8 quoted lines

> request from the client regardless.
> 
> In practice this doesn't really hurt anything, as we'd pass the client's
> protocol list on to pack-objects, which ends up ignoring it. But we
> should try to follow the protocol spec, and tightening this up may catch
> buggy or misbehaving clients more easily.
> 
> Thanks to recent refactoring, we can hoist the config check from

Nit: s/refactoring/&s.

Patrick

Show 104 quoted lines

> upload_pack_advertise() into upload_pack_config(). Note the subtle
> handling of a value-less bool (which does not count for triggering an
> advertisement).
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I suspect in the long term that we may have other ways to trigger this
> feature than the static blobpackfileuri config (e.g., a hook that knows
> about site-specific packfiles "somehow"). So we may need to update the
> test later for that, but presumably in the vanilla config we'll continue
> to skip advertising it.
> 
>  t/t5702-protocol-v2.sh | 18 ++++++++++++++++++
>  upload-pack.c          | 16 +++++++---------
>  2 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 6ef4971845..902e42c1c0 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -778,6 +778,24 @@ test_expect_success 'archive with custom path does not request v2' '
>  	! grep ^GIT_PROTOCOL env.trace
>  '
>  
> +test_expect_success 'reject client packfile-uris if not advertised' '
> +	{
> +		packetize command=fetch &&
> +		printf 0001 &&
> +		packetize packfile-uris https &&
> +		packetize done &&
> +		printf 0000
> +	} >input &&
> +	test_must_fail env GIT_PROTOCOL=version=2 \
> +		git upload-pack client <input &&
> +	test_must_fail env GIT_PROTOCOL=version=2 \
> +		git -c uploadpack.blobpackfileuri \
> +		upload-pack client <input &&
> +	GIT_PROTOCOL=version=2 \
> +		git -c uploadpack.blobpackfileuri=anything \
> +		upload-pack client <input
> +'
> +
>  # Test protocol v2 with 'http://' transport
>  #
>  . "$TEST_DIRECTORY"/lib-httpd.sh
> diff --git a/upload-pack.c b/upload-pack.c
> index 491ef51daa..66f4de9d87 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -113,6 +113,7 @@ struct upload_pack_data {
>  	unsigned done : 1;					/* v2 only */
>  	unsigned allow_ref_in_want : 1;				/* v2 only */
>  	unsigned allow_sideband_all : 1;			/* v2 only */
> +	unsigned allow_packfile_uris : 1;			/* v2 only */
>  	unsigned advertise_sid : 1;
>  	unsigned sent_capabilities : 1;
>  };
> @@ -1362,6 +1363,9 @@ static int upload_pack_config(const char *var, const char *value,
>  		data->allow_ref_in_want = git_config_bool(var, value);
>  	} else if (!strcmp("uploadpack.allowsidebandall", var)) {
>  		data->allow_sideband_all = git_config_bool(var, value);
> +	} else if (!strcmp("uploadpack.blobpackfileuri", var)) {
> +		if (value)
> +			data->allow_packfile_uris = 1;
>  	} else if (!strcmp("core.precomposeunicode", var)) {
>  		precomposed_unicode = git_config_bool(var, value);
>  	} else if (!strcmp("transfer.advertisesid", var)) {
> @@ -1647,7 +1651,8 @@ static void process_args(struct packet_reader *request,
>  			continue;
>  		}
>  
> -		if (skip_prefix(arg, "packfile-uris ", &p)) {
> +		if (data->allow_packfile_uris &&
> +		    skip_prefix(arg, "packfile-uris ", &p)) {
>  			string_list_split(&data->uri_protocols, p, ',', -1);
>  			continue;
>  		}
> @@ -1847,8 +1852,6 @@ int upload_pack_advertise(struct repository *r,
>  	get_upload_pack_config(r, &data);
>  
>  	if (value) {
> -		char *str = NULL;
> -
>  		strbuf_addstr(value, "shallow wait-for-done");
>  
>  		if (data.allow_filter)
> @@ -1860,13 +1863,8 @@ int upload_pack_advertise(struct repository *r,
>  		if (data.allow_sideband_all)
>  			strbuf_addstr(value, " sideband-all");
>  
> -		if (!repo_config_get_string(r,
> -					    "uploadpack.blobpackfileuri",
> -					    &str) &&
> -		    str) {
> +		if (data.allow_packfile_uris)
>  			strbuf_addstr(value, " packfile-uris");
> -			free(str);
> -		}
>  	}
>  
>  	upload_pack_data_clear(&data);
> -- 
> 2.44.0.rc2.424.gbdbf4d014b
>