@matheusbsbs / matheus-bagliano / commits / a922bfa3b5

upload-pack: only accept packfile-uris if we advertised it

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 <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Feb 28, 2024 at 17:50 UTC a922bfa3b5a6b2ac5e98f0e3405d66c1847aa7e8
2 files changed +26 -9
t/t5702-protocol-v2.sh
+19
@@ -778,6 +778,25 @@ test_expect_success 'archive with custom path does not request v2' '
778 ! grep ^GIT_PROTOCOL env.trace
779 '
780
781 +test_expect_success 'reject client packfile-uris if not advertised' '
782 + {
783 + packetize command=fetch &&
784 + packetize object-format=$(test_oid algo) &&
785 + printf 0001 &&
786 + packetize packfile-uris https &&
787 + packetize done &&
788 + printf 0000
789 + } >input &&
790 + test_must_fail env GIT_PROTOCOL=version=2 \
791 + git upload-pack client <input &&
792 + test_must_fail env GIT_PROTOCOL=version=2 \
793 + git -c uploadpack.blobpackfileuri \
794 + upload-pack client <input &&
795 + GIT_PROTOCOL=version=2 \
796 + git -c uploadpack.blobpackfileuri=anything \
797 + upload-pack client <input
798 +'
799 +
800 # Test protocol v2 with 'http://' transport
801 #
802 . "$TEST_DIRECTORY"/lib-httpd.sh
upload-pack.c
+7 -9
@@ -113,6 +113,7 @@ struct upload_pack_data {
113 unsigned done : 1; /* v2 only */
114 unsigned allow_ref_in_want : 1; /* v2 only */
115 unsigned allow_sideband_all : 1; /* v2 only */
116 + unsigned allow_packfile_uris : 1; /* v2 only */
117 unsigned advertise_sid : 1;
118 unsigned sent_capabilities : 1;
119 };
@@ -1362,6 +1363,9 @@ static int upload_pack_config(const char *var, const char *value,
1363 data->allow_ref_in_want = git_config_bool(var, value);
1364 } else if (!strcmp("uploadpack.allowsidebandall", var)) {
1365 data->allow_sideband_all = git_config_bool(var, value);
1366 + } else if (!strcmp("uploadpack.blobpackfileuri", var)) {
1367 + if (value)
1368 + data->allow_packfile_uris = 1;
1369 } else if (!strcmp("core.precomposeunicode", var)) {
1370 precomposed_unicode = git_config_bool(var, value);
1371 } else if (!strcmp("transfer.advertisesid", var)) {
@@ -1647,7 +1651,8 @@ static void process_args(struct packet_reader *request,
1651 continue;
1652 }
1653
1650 - if (skip_prefix(arg, "packfile-uris ", &p)) {
1654 + if (data->allow_packfile_uris &&
1655 + skip_prefix(arg, "packfile-uris ", &p)) {
1656 string_list_split(&data->uri_protocols, p, ',', -1);
1657 continue;
1658 }
@@ -1847,8 +1852,6 @@ int upload_pack_advertise(struct repository *r,
1852 get_upload_pack_config(r, &data);
1853
1854 if (value) {
1850 - char *str = NULL;
1851 -
1855 strbuf_addstr(value, "shallow wait-for-done");
1856
1857 if (data.allow_filter)
@@ -1860,13 +1863,8 @@ int upload_pack_advertise(struct repository *r,
1863 if (data.allow_sideband_all)
1864 strbuf_addstr(value, " sideband-all");
1865
1863 - if (!repo_config_get_string(r,
1864 - "uploadpack.blobpackfileuri",
1865 - &str) &&
1866 - str) {
1866 + if (data.allow_packfile_uris)
1867 strbuf_addstr(value, " packfile-uris");
1868 - free(str);
1869 - }
1868 }
1869
1870 upload_pack_data_clear(&data);