A previous commit allowed a server to pass additional fields through
the "promisor-remote" protocol capability after the "name" and "url"
fields, specifically the "partialCloneFilter" and "token" fields.
Let's make it possible for a client to check if these fields match
what it expects before accepting a promisor remote.
We allow this by introducing a new "promisor.checkFields"
configuration variable. It should contain a comma or space separated
list of fields that will be checked.
By limiting the protocol to specific well-defined fields, we ensure
both server and client have a shared understanding of field
semantics and usage.
Signed-off-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
---
Documentation/config/promisor.adoc | 25 ++++++
promisor-remote.c | 105 +++++++++++++++++++++++---
t/t5710-promisor-remote-capability.sh | 35 +++++++++
3 files changed, 154 insertions(+), 11 deletions(-)
diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc
index 71311b70c8..4147d2cf44 100644
--- a/Documentation/config/promisor.adoc
+++ b/Documentation/config/promisor.adoc
@@ -46,3 +46,28 @@ promisor.acceptFromServer::
lazily fetchable from this promisor remote from its responses
to "fetch" and "clone" requests from the client. Name and URL
comparisons are case sensitive. See linkgit:gitprotocol-v2[5].
+
+promisor.checkFields::
+ A comma or space separated list of additional remote related
+ fields that a client will check before accepting a promisor
+ remote. Currently, only the "partialCloneFilter" and "token"
+ fields are supported.
++
+When a field is part of this list and a corresponding
+"remote.foo.<field>" config variable is set locally for remote "foo",
+then the value of this config variable will be checked against the
+value of the same field passed by the server for the remote "foo". The
+remote "foo" will be rejected if the values don't match.
++
+For the "partialCloneFilter" field, this allows the client to ensure
+that the server's filter matches what it expects locally, preventing
+inconsistencies in filtering behavior. For the "token" field, this can
+be used to verify that authentication credentials match expected
+values.
++
+The fields should be passed by the server through the
+"promisor-remote" capability by using the `promisor.sendFields` config
+variable. The fields will be checked only if the
+`promisor.acceptFromServer` config variable is not set to "None". If
+set to "None", this config variable will have no effect. See
+linkgit:gitprotocol-v2[5].
diff --git a/promisor-remote.c b/promisor-remote.c
index 70abec4c24..97f81a0e08 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -377,6 +377,20 @@ static struct string_list *fields_sent(void)
return &fields_list;
}
+static struct string_list *fields_checked(void)
+{
+ static struct string_list fields_list = STRING_LIST_INIT_NODUP;
+ static int initialized = 0;
+
+ if (!initialized) {
+ fields_list.cmp = strcasecmp;
+ fields_from_config(&fields_list, "promisor.checkFields");
+ initialized = 1;
+ }
+
+ return &fields_list;
+}
+
static void append_fields(struct string_list *fields,
struct string_list *field_names,
const char *name)
@@ -533,15 +547,65 @@ enum accept_promisor {
ACCEPT_ALL
};
+static int check_field_one(struct string_list_item *item_value,
+ struct promisor_info *p)
+{
+ struct string_list_item *item;
+
+ item = unsorted_string_list_lookup(&p->fields, item_value->string);
+ if (!item)
+ return 0;
+
+ return !strcmp(item->util, item_value->util);
+}
+
+
+static int check_field(struct string_list_item *item_value,
+ struct promisor_info *p, int in_list)
+{
+ if (!in_list)
+ return check_field_one(item_value, p);
+
+ for (; p; p = p->next)
+ if (check_field_one(item_value, p))
+ return 1;
+
+ return 0;
+}
+
+static int check_all_fields(struct string_list* values,
+ struct promisor_info *p,
+ int in_list)
+{
+ struct string_list* fields = fields_checked();
+ struct string_list_item *item_checked;
+
+ string_list_sort(values);
+
+ for_each_string_list_item(item_checked, fields) {
+ struct string_list_item *item_value;
+
+ item_value = string_list_lookup(values, item_checked->string);
+ if (!item_value)
+ return 0;
+ if (!check_field(item_value, p, in_list))
+ return 0;
+ }
+
+ return 1;
+}
+
static int should_accept_remote(enum accept_promisor accept,
- const char *remote_name, const char *remote_url,
+ const char *remote_name,
+ const char *remote_url,
+ struct string_list* values,
struct promisor_info *info_list)
{
struct promisor_info *p;
const char *local_url;
if (accept == ACCEPT_ALL)
- return 1;
+ return check_all_fields(values, info_list, 1);
p = remote_nick_find(info_list, remote_name);
@@ -550,7 +614,7 @@ static int should_accept_remote(enum accept_promisor accept,
return 0;
if (accept == ACCEPT_KNOWN_NAME)
- return 1;
+ return check_all_fields(values, p, 0);
if (accept != ACCEPT_KNOWN_URL)
BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
@@ -568,7 +632,7 @@ static int should_accept_remote(enum accept_promisor accept,
local_url = p->fields.items[1].util;
if (!strcmp(local_url, remote_url))
- return 1;
+ return check_all_fields(values, p, 0);
warning(_("known remote named '%s' but with URL '%s' instead of '%s'"),
remote_name, local_url, remote_url);
@@ -602,9 +666,6 @@ static void filter_promisor_remote(struct repository *repo,
if (accept == ACCEPT_NONE)
return;
- if (accept != ACCEPT_ALL)
- info_list = promisor_info_list(repo, NULL);
-
/* Parse remote info received */
remotes = strbuf_split_str(info, ';', 0);
@@ -615,14 +676,29 @@ static void filter_promisor_remote(struct repository *repo,
const char *remote_url = NULL;
char *decoded_name = NULL;
char *decoded_url = NULL;
+ struct string_list field_values = STRING_LIST_INIT_NODUP;
+
+ field_values.cmp = strcasecmp;
strbuf_strip_suffix(remotes[i], ";");
elems = strbuf_split(remotes[i], ',');
for (size_t j = 0; elems[j]; j++) {
+ char *p;
+
strbuf_strip_suffix(elems[j], ",");
- if (!skip_prefix(elems[j]->buf, "name=", &remote_name))
- skip_prefix(elems[j]->buf, "url=", &remote_url);
+ if (skip_prefix(elems[j]->buf, "name=", &remote_name) ||
+ skip_prefix(elems[j]->buf, "url=", &remote_url))
+ continue;
+
+ p = strchr(elems[j]->buf, '=');
+ if (p) {
+ *p = '\0';
+ string_list_append(&field_values, elems[j]->buf)->util = p + 1;
+ } else {
+ warning(_("invalid element '%s' from remote info"),
+ elems[j]->buf);
+ }
}
if (remote_name)
@@ -630,9 +706,16 @@ static void filter_promisor_remote(struct repository *repo,
if (remote_url)
decoded_url = url_percent_decode(remote_url);
- if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, info_list))
- strvec_push(accepted, decoded_name);
+ if (decoded_name) {
+ if (!info_list)
+ info_list = promisor_info_list(repo, fields_checked());
+
+ if (should_accept_remote(accept, decoded_name, decoded_url,
+ &field_values, info_list))
+ strvec_push(accepted, decoded_name);
+ }
+ string_list_clear(&field_values, 0);
strbuf_list_free(elems);
free(decoded_name);
free(decoded_url);
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index 4038c076f1..2b54474ff8 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -321,6 +321,41 @@ test_expect_success "clone with promisor.sendFields" '
check_missing_objects server 1 "$oid"
'
+test_expect_success "clone with promisor.checkFields" '
+ git -C server config promisor.advertise true &&
+ test_when_finished "rm -rf client" &&
+
+ git -C server remote add otherLop "https://invalid.invalid"; &&
+ git -C server config remote.otherLop.token "fooBar" &&
+ git -C server config remote.otherLop.stuff "baz" &&
+ git -C server config remote.otherLop.partialCloneFilter "blob:limit=10k" &&
+ test_when_finished "git -C server remote remove otherLop" &&
+ git -C server config promisor.sendFields "token, partialCloneFilter" &&
+ test_when_finished "git -C server config unset promisor.sendFields" &&
+ test_when_finished "rm trace" &&
+
+ # Clone from server to create a client
+ GIT_TRACE_PACKET="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \
+ -c remote.lop.promisor=true \
+ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+ -c remote.lop.url="file://$(pwd)/lop" \
+ -c remote.lop.partialCloneFilter="blob:none" \
+ -c promisor.acceptfromserver=All \
+ -c promisor.checkFields=partialcloneFilter \
+ --no-local --filter="blob:limit=5k" server client &&
+
+ # Check that fields are properly transmitted
+ ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") &&
+ PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" &&
+ PR2="name=otherLop,url=https://invalid.invalid,token=fooBar,partialCloneFilter=blob:limit=10k"; &&
+ test_grep "clone< promisor-remote=$PR1;$PR2" trace &&
+ test_grep "clone> promisor-remote=lop" trace &&
+ test_grep ! "clone> promisor-remote=lop;otherLop" trace &&
+
+ # Check that the largest object is still missing on the server
+ check_missing_objects server 1 "$oid"
+'
+
test_expect_success "clone with promisor.advertise set to 'true' but don't delete the client" '
git -C server config promisor.advertise true &&
--
2.49.0.157.g09af0369a6
