From patchwork Wed Feb 28 22:46:47 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13576019 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E4A151361C4 for ; Wed, 28 Feb 2024 22:46:48 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709160410; cv=none; b=REOjer6teya9dE4Tf5ois9PEVpAjoHap0yVU8YX+fbRhfsUDrAS1Bck/xeBO0xcJxSHXSJcE/P5WWGse9xNE8kkxyWYCS++YMYVgpGkpvCjoca3nJAStREUOmGN5JW/11TSq+jswy4Pk7AEg0YpYc9BWTb5YU9nmCcjGXQ443AI= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709160410; c=relaxed/simple; bh=X7SILVlPbtawJRjEJvUP5vLpwdVcHX6tfgTEDVuaY1Q=; h=Date:From:To:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=IDuS62iyqHOfm5YL+bby8sof+BRO+upfIS4xX48fW5huh4bpC639/3dmCfx8g74+3L67rzmi7McNSFgBtj07RzU3d/qZVjwd7VKzKMLiBOd3OoMR8QIxktK6Exr0AWT9sU6ujZFjk4nsMaVtUNuyjt7QYzO2AvelhlX2v7SbGXI= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 23766 invoked by uid 109); 28 Feb 2024 22:46:48 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 28 Feb 2024 22:46:48 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 26944 invoked by uid 111); 28 Feb 2024 22:46:50 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 28 Feb 2024 17:46:50 -0500 Authentication-Results: peff.net; auth=none Date: Wed, 28 Feb 2024 17:46:47 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 1/4] upload-pack: use repository struct to get config Message-ID: <20240228224647.GA1158898@coredump.intra.peff.net> References: <20240228224625.GA1158651@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240228224625.GA1158651@coredump.intra.peff.net> Our upload_pack_v2() function gets a repository struct, but we ignore it totally. In practice this doesn't cause any problems, as it will never differ from the_repository. But in the spirit of taking a small step towards getting rid of the_repository, let's at least starting using it to grab config. There are probably other spots that could benefit, but it's a start. Note that we don't need to pass the repo for protected_config(); the whole point there is that we are not looking at repo config, so there is no repo-specific version of the function. For the v0 version of the protocol, we're not passed a repository struct, so we'll continue to use the_repository there. Signed-off-by: Jeff King --- upload-pack.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/upload-pack.c b/upload-pack.c index 2537affa90..e156c1e472 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1385,9 +1385,10 @@ static int upload_pack_protected_config(const char *var, const char *value, return 0; } -static void get_upload_pack_config(struct upload_pack_data *data) +static void get_upload_pack_config(struct repository *r, + struct upload_pack_data *data) { - git_config(upload_pack_config, data); + repo_config(r, upload_pack_config, data); git_protected_config(upload_pack_protected_config, data); } @@ -1398,7 +1399,7 @@ void upload_pack(const int advertise_refs, const int stateless_rpc, struct upload_pack_data data; upload_pack_data_init(&data); - get_upload_pack_config(&data); + get_upload_pack_config(the_repository, &data); data.stateless_rpc = stateless_rpc; data.timeout = timeout; @@ -1771,7 +1772,7 @@ enum fetch_state { FETCH_DONE, }; -int upload_pack_v2(struct repository *r UNUSED, struct packet_reader *request) +int upload_pack_v2(struct repository *r, struct packet_reader *request) { enum fetch_state state = FETCH_PROCESS_ARGS; struct upload_pack_data data; @@ -1780,7 +1781,7 @@ int upload_pack_v2(struct repository *r UNUSED, struct packet_reader *request) upload_pack_data_init(&data); data.use_sideband = LARGE_PACKET_MAX; - get_upload_pack_config(&data); + get_upload_pack_config(r, &data); while (state != FETCH_DONE) { switch (state) { From patchwork Wed Feb 28 22:47:18 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13576020 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DC5F736E for ; Wed, 28 Feb 2024 22:47:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709160441; cv=none; b=N87Oa2c8uWJe4T8DbE+/QX5Jcm1TZS1E7HVXsb+rZgdO+LIe15J7q7mgm3V0YKOd8VxAQngcwsINlOq+1ogoqbBkG+xnQNU2LMAV67Y8ZX+1uBnZR8jjmhrf3NeMwVgH8fAV5Oxqw9uQjLjZI9IqyHFqfTWYG2C3W1uXqxROssM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709160441; c=relaxed/simple; bh=3ocyPhRZqIO858sqJ9tM8iU4ZGoYxJEYC6EpXcwlCW0=; h=Date:From:To:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=tdzQNhqEetbR5+L0zygdwBrlfmXdo7zuPmKTgXNkoc14Qr0HnXOVvUYRE0sSj0482z2V5+pRtYKqq5VCHLEOaLyn1RI3hQFqz5jgqwedP+62cgcWU9R+R1DT79HaI+H4VyDzuzW7Jh4zQsRjQ/WOmET731l0PRKibOXFj2BeWJA= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 23771 invoked by uid 109); 28 Feb 2024 22:47:19 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 28 Feb 2024 22:47:19 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 26950 invoked by uid 111); 28 Feb 2024 22:47:21 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 28 Feb 2024 17:47:21 -0500 Authentication-Results: peff.net; auth=none Date: Wed, 28 Feb 2024 17:47:18 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 2/4] upload-pack: centralize setup of sideband-all config Message-ID: <20240228224718.GB1158898@coredump.intra.peff.net> References: <20240228224625.GA1158651@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240228224625.GA1158651@coredump.intra.peff.net> We read uploadpack.allowsidebandall to set a matching flag in our upload_pack_data struct. But for our tests, we also respect GIT_TEST_SIDEBAND_ALL from the environment, and anybody looking at the flag in the struct needs to remember to check both. There's only one such piece of code now, but we're about to add another. So let's have the config step actually fold the environment value into the struct, letting the rest of the code use the flag in the obvious way. Signed-off-by: Jeff King --- upload-pack.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/upload-pack.c b/upload-pack.c index e156c1e472..6bda20754d 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1390,6 +1390,8 @@ static void get_upload_pack_config(struct repository *r, { repo_config(r, upload_pack_config, data); git_protected_config(upload_pack_protected_config, data); + + data->allow_sideband_all |= git_env_bool("GIT_TEST_SIDEBAND_ALL", 0); } void upload_pack(const int advertise_refs, const int stateless_rpc, @@ -1639,8 +1641,7 @@ static void process_args(struct packet_reader *request, continue; } - if ((git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) || - data->allow_sideband_all) && + if (data->allow_sideband_all && !strcmp(arg, "sideband-all")) { data->writer.use_sideband = 1; continue; From patchwork Wed Feb 28 22:48:18 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13576021 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A9BC61361CD for ; Wed, 28 Feb 2024 22:48:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709160502; cv=none; b=k9ZJuMn9niNLgskIpQk1QJFDJByLIXumgzaPJCsuvEhs4kGQpLHuIyH4y7QIXQpiyKeaFSB9U9vf8qGuJj89mErWSSOFoRIOGlzHdLJxuR8w1HbwOYv1SLgodJnanOKFJMnq1CtwE11HLXwvPkq2yJn4xgX5DKslnIFTTMlthJ8= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709160502; c=relaxed/simple; bh=Bcp53yGtifgnvINt+GVL2eDqzu7fMqY/yMh8JsXe4iM=; h=Date:From:To:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=kVxw37W055pr0voMwkeXFgOv/wGWXEChLXpWU6mSKefTGvLrDZxvQzQvsoWycic5a+l/bejl0xLqe21eyEcnXNIEJUldqZUtWvakQfWKlLp+jQMI6AEpaM4znG+K/Pumc79CgiyslZuYmoUflSdijuehahq5f3JeYjRew3DPny4= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 23785 invoked by uid 109); 28 Feb 2024 22:48:20 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 28 Feb 2024 22:48:20 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 26973 invoked by uid 111); 28 Feb 2024 22:48:21 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 28 Feb 2024 17:48:21 -0500 Authentication-Results: peff.net; auth=none Date: Wed, 28 Feb 2024 17:48:18 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 3/4] upload-pack: use existing config mechanism for advertisement Message-ID: <20240228224818.GA1158952@coredump.intra.peff.net> References: <20240228224625.GA1158651@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240228224625.GA1158651@coredump.intra.peff.net> When serving a v2 capabilities request, we call upload_pack_advertise() to tell us the set of features we can advertise to the client. That involves looking at various config options, all of which need to be kept in sync with the rules we use in upload_pack_config to set flags like allow_filter, allow_sideband_all, and so on. If these two pieces of code get out of sync then we may refuse to respect a capability we advertised, or vice versa accept one that we should not. Instead, let's call the same config helper that we'll use for processing the actual client request, and then just pick the values out of the resulting struct. This is only a little bit shorter than the current code, but we don't repeat any policy logic (e.g., we don't have to worry about the magic sideband-all environment variable here anymore). And this reveals a gap in the existing code: there is no struct flag for the packfile-uris capability (we accept it even if it is not advertised, which we should not). We'll leave the advertisement code for now and deal with it in the next patch. Signed-off-by: Jeff King --- upload-pack.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/upload-pack.c b/upload-pack.c index 6bda20754d..491ef51daa 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1841,41 +1841,35 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request) int upload_pack_advertise(struct repository *r, struct strbuf *value) { + struct upload_pack_data data; + + upload_pack_data_init(&data); + get_upload_pack_config(r, &data); + if (value) { - int allow_filter_value; - int allow_ref_in_want; - int allow_sideband_all_value; char *str = NULL; strbuf_addstr(value, "shallow wait-for-done"); - if (!repo_config_get_bool(r, - "uploadpack.allowfilter", - &allow_filter_value) && - allow_filter_value) + if (data.allow_filter) strbuf_addstr(value, " filter"); - if (!repo_config_get_bool(r, - "uploadpack.allowrefinwant", - &allow_ref_in_want) && - allow_ref_in_want) + if (data.allow_ref_in_want) strbuf_addstr(value, " ref-in-want"); - if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) || - (!repo_config_get_bool(r, - "uploadpack.allowsidebandall", - &allow_sideband_all_value) && - allow_sideband_all_value)) + if (data.allow_sideband_all) strbuf_addstr(value, " sideband-all"); if (!repo_config_get_string(r, "uploadpack.blobpackfileuri", &str) && str) { strbuf_addstr(value, " packfile-uris"); free(str); } } + upload_pack_data_clear(&data); + return 1; } From patchwork Wed Feb 28 22:50:50 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13576028 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B214276EF6 for ; Wed, 28 Feb 2024 22:50:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709160653; cv=none; b=G3qoyXrOInZVKNSzlTtP93QzWUA68qOPMoKzNCWQyP0BnLCxUY1lVpi1Io2Hn2VUWrKIZjrzlrGYsG6uG2mPnB6fhLc7D65MKapaSFWmz7TB+gqhJd5dbhgDx5ZrdlBW0FE20wO8L0GQle9A2hEhW+DJg3Lr2gXmjaQlZj+8H2Y= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709160653; c=relaxed/simple; bh=eRK/QuFG0XBBx8Fe9ak+eYHwo0Eu9HH0y9j38dyhSzg=; h=Date:From:To:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=dHlbxrfYNNCH52LIRWMO7o0EWPwIZUjGgUJjnK3wrO/VUIxJhhYk+IZHRrxAdTXGbs0lOKyNgk3/YI5rFcKdbpUNCmtUHF98wLPFub1UWXN1Zw6TriS1fz9Qou0KZFTRdPAghfoxnkWVWBuSmkxbavo809ShrcZX/Ky3L/fjb2Q= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 23798 invoked by uid 109); 28 Feb 2024 22:50:51 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 28 Feb 2024 22:50:51 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 27044 invoked by uid 111); 28 Feb 2024 22:50:52 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 28 Feb 2024 17:50:52 -0500 Authentication-Results: peff.net; auth=none Date: Wed, 28 Feb 2024 17:50:50 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it Message-ID: <20240228225050.GA1159078@coredump.intra.peff.net> References: <20240228224625.GA1158651@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240228224625.GA1158651@coredump.intra.peff.net> 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 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 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 --- 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 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);