From 76804526f9795f94fb666248653a5f41ed921241 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 14 Sep 2021 11:30:20 -0400 Subject: [PATCH 01/11] serve: rename is_command() to parse_command() The is_command() function not only tells us whether the pktline is a valid command string, but it also parses out the command (and complains if we see a duplicate). Let's rename it to make those extra functions a bit more obvious. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- serve.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/serve.c b/serve.c index 1817edc7f5..fd88b95343 100644 --- a/serve.c +++ b/serve.c @@ -163,7 +163,7 @@ static int is_valid_capability(const char *key) return c && c->advertise(the_repository, NULL); } -static int is_command(const char *key, struct protocol_capability **command) +static int parse_command(const char *key, struct protocol_capability **command) { const char *out; @@ -251,7 +251,7 @@ static int process_request(void) BUG("Should have already died when seeing EOF"); case PACKET_READ_NORMAL: /* collect request; a sequence of keys and values */ - if (is_command(reader.line, &command) || + if (parse_command(reader.line, &command) || is_valid_capability(reader.line)) strvec_push(&keys, reader.line); else From 5ef260d2d124c0fcd799bd4aaf6fa1793a6c74de Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 14 Sep 2021 11:30:50 -0400 Subject: [PATCH 02/11] serve: return capability "value" from get_capability() When the client sends v2 capabilities, they may be simple, like: foo or have a value like: foo=bar (all of the current capabilities actually expect a value, but the protocol allows for boolean ones). We use get_capability() to make sure the client's pktline matches a capability. In doing so, we parse enough to see the "=" and the value (if any), but we immediately forget it. Nobody cares for now, because they end up parsing the values out later using has_capability(). But in preparation for changing that, let's pass back a pointer so the callers know what we found. Note that unlike has_capability(), we'll return NULL for a "simple" capability. Distinguishing these will be useful for some future patches. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- serve.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/serve.c b/serve.c index fd88b95343..78a4e83554 100644 --- a/serve.c +++ b/serve.c @@ -139,7 +139,7 @@ void protocol_v2_advertise_capabilities(void) strbuf_release(&value); } -static struct protocol_capability *get_capability(const char *key) +static struct protocol_capability *get_capability(const char *key, const char **value) { int i; @@ -149,8 +149,16 @@ static struct protocol_capability *get_capability(const char *key) for (i = 0; i < ARRAY_SIZE(capabilities); i++) { struct protocol_capability *c = &capabilities[i]; const char *out; - if (skip_prefix(key, c->name, &out) && (!*out || *out == '=')) + if (!skip_prefix(key, c->name, &out)) + continue; + if (!*out) { + *value = NULL; return c; + } + if (*out++ == '=') { + *value = out; + return c; + } } return NULL; @@ -158,7 +166,8 @@ static struct protocol_capability *get_capability(const char *key) static int is_valid_capability(const char *key) { - const struct protocol_capability *c = get_capability(key); + const char *value; + const struct protocol_capability *c = get_capability(key, &value); return c && c->advertise(the_repository, NULL); } @@ -168,7 +177,8 @@ static int parse_command(const char *key, struct protocol_capability **command) const char *out; if (skip_prefix(key, "command=", &out)) { - struct protocol_capability *cmd = get_capability(out); + const char *value; + struct protocol_capability *cmd = get_capability(out, &value); if (*command) die("command '%s' requested after already requesting command '%s'", From e56e53067fc3d31899e837293a5bf72e2d086b4a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 14 Sep 2021 11:31:07 -0400 Subject: [PATCH 03/11] serve: add "receive" method for v2 capabilities table We have a capabilities table that tells us what we should tell the client we are capable of, and what to do when a client gives us a particular command (e.g., "command=ls-refs"). But it doesn't tell us what to do when the client sends us back a capability (e.g., "object-format=sha256"). We just collect them all in a strvec and hope somebody can use them later. Instead, let's provide a function pointer in the table to act on these. This will eventually help us avoid collecting the strings, which will be more efficient and less prone to mischief. Using the new method is optional, which helps in two ways: - we can move existing capabilities over to this new system gradually in individual commits - some capabilities we don't actually do anything with anyway. For example, the client is free to say "agent=git/1.2.3" to us, but we do not act on the information in any way. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- serve.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/serve.c b/serve.c index 78a4e83554..a161189984 100644 --- a/serve.c +++ b/serve.c @@ -70,6 +70,16 @@ struct protocol_capability { * This field should be NULL for capabilities which are not commands. */ int (*command)(struct repository *r, struct packet_reader *request); + + /* + * Function called when a client requests the capability as a + * non-command. This may be NULL if the capability does nothing. + * + * For a capability of the form "foo=bar", the value string points to + * the content after the "=" (i.e., "bar"). For simple capabilities + * (just "foo"), it is NULL. + */ + void (*receive)(struct repository *r, const char *value); }; static struct protocol_capability capabilities[] = { @@ -164,12 +174,17 @@ static struct protocol_capability *get_capability(const char *key, const char ** return NULL; } -static int is_valid_capability(const char *key) +static int receive_client_capability(const char *key) { const char *value; const struct protocol_capability *c = get_capability(key, &value); - return c && c->advertise(the_repository, NULL); + if (!c || !c->advertise(the_repository, NULL)) + return 0; + + if (c->receive) + c->receive(the_repository, value); + return 1; } static int parse_command(const char *key, struct protocol_capability **command) @@ -262,7 +277,7 @@ static int process_request(void) case PACKET_READ_NORMAL: /* collect request; a sequence of keys and values */ if (parse_command(reader.line, &command) || - is_valid_capability(reader.line)) + receive_client_capability(reader.line)) strvec_push(&keys, reader.line); else die("unknown capability '%s'", reader.line); From c7d3aabd2793ab5772627ff6da95f8f7c28258ab Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 14 Sep 2021 19:51:27 -0400 Subject: [PATCH 04/11] serve: provide "receive" function for object-format capability We get any "object-format" specified by the client by searching for it in the collected list of capabilities the client sent. We can instead just handle it as soon as they send it. This is slightly more efficient, and gets us one step closer to dropping that collected list. Note that we do still have to do our final hash check after receiving all capabilities (because they might not have sent an object-format line at all, and we still have to check that the default matches our repository algorithm). Since the check_algorithm() function would now be down to a single if() statement, I've just inlined it in its only caller. There should be no change of behavior here, except for two broken-protocol cases: - if the client sends multiple conflicting object-format capabilities (which they should not), we'll now choose the last one rather than the first. We could also detect and complain about the duplicates quite easily now, which we could not before, but I didn't do so here. - if the client sends a bogus "object-format" with no equals sign, we'll now say so, rather than "unknown object format: ''" Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- serve.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/serve.c b/serve.c index a161189984..f6ea2953eb 100644 --- a/serve.c +++ b/serve.c @@ -10,6 +10,7 @@ #include "upload-pack.h" static int advertise_sid = -1; +static int client_hash_algo = GIT_HASH_SHA1; static int always_advertise(struct repository *r, struct strbuf *value) @@ -33,6 +34,17 @@ static int object_format_advertise(struct repository *r, return 1; } +static void object_format_receive(struct repository *r, + const char *algo_name) +{ + if (!algo_name) + die("object-format capability requires an argument"); + + client_hash_algo = hash_algo_by_name(algo_name); + if (client_hash_algo == GIT_HASH_UNKNOWN) + die("unknown object format '%s'", algo_name); +} + static int session_id_advertise(struct repository *r, struct strbuf *value) { if (advertise_sid == -1 && @@ -104,6 +116,7 @@ static struct protocol_capability capabilities[] = { { .name = "object-format", .advertise = object_format_advertise, + .receive = object_format_receive, }, { .name = "session-id", @@ -228,22 +241,6 @@ static int has_capability(const struct strvec *keys, const char *capability, return 0; } -static void check_algorithm(struct repository *r, struct strvec *keys) -{ - int client = GIT_HASH_SHA1, server = hash_algo_by_ptr(r->hash_algo); - const char *algo_name; - - if (has_capability(keys, "object-format", &algo_name)) { - client = hash_algo_by_name(algo_name); - if (client == GIT_HASH_UNKNOWN) - die("unknown object format '%s'", algo_name); - } - - if (client != server) - die("mismatched object format: server %s; client %s\n", - r->hash_algo->name, hash_algos[client].name); -} - enum request_state { PROCESS_REQUEST_KEYS, PROCESS_REQUEST_DONE, @@ -317,7 +314,10 @@ static int process_request(void) if (!command) die("no command requested"); - check_algorithm(the_repository, &keys); + if (client_hash_algo != hash_algo_by_ptr(the_repository->hash_algo)) + die("mismatched object format: server %s; client %s\n", + the_repository->hash_algo->name, + hash_algos[client_hash_algo].name); if (has_capability(&keys, "session-id", &client_sid)) trace2_data_string("transfer", NULL, "client-sid", client_sid); From ab539c9094e3e3691f157830f74406042d55f774 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 14 Sep 2021 19:51:30 -0400 Subject: [PATCH 05/11] serve: provide "receive" function for session-id capability Rather than pulling the session-id string from the list of collected capabilities, we can handle it as soon as we receive it. This gets us closer to dropping the collected list entirely. The behavior should be the same, with one exception. Previously if the client sent us multiple session-id lines, we'd report only the first. Now we'll pass each one along to trace2. This shouldn't matter in practice, since clients shouldn't do that (and if they do, it's probably sensible to log them all). As this removes the last caller of the static has_capability(), we can remove it, as well (and in fact we must to avoid -Wunused-function complaining). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- serve.c | 33 +++++++++------------------------ 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/serve.c b/serve.c index f6ea2953eb..6bbf54cbbe 100644 --- a/serve.c +++ b/serve.c @@ -57,6 +57,14 @@ static int session_id_advertise(struct repository *r, struct strbuf *value) return 1; } +static void session_id_receive(struct repository *r, + const char *client_sid) +{ + if (!client_sid) + client_sid = ""; + trace2_data_string("transfer", NULL, "client-sid", client_sid); +} + struct protocol_capability { /* * The name of the capability. The server uses this name when @@ -121,6 +129,7 @@ static struct protocol_capability capabilities[] = { { .name = "session-id", .advertise = session_id_advertise, + .receive = session_id_receive, }, { .name = "object-info", @@ -221,26 +230,6 @@ static int parse_command(const char *key, struct protocol_capability **command) return 0; } -static int has_capability(const struct strvec *keys, const char *capability, - const char **value) -{ - int i; - for (i = 0; i < keys->nr; i++) { - const char *out; - if (skip_prefix(keys->v[i], capability, &out) && - (!*out || *out == '=')) { - if (value) { - if (*out == '=') - out++; - *value = out; - } - return 1; - } - } - - return 0; -} - enum request_state { PROCESS_REQUEST_KEYS, PROCESS_REQUEST_DONE, @@ -252,7 +241,6 @@ static int process_request(void) struct packet_reader reader; struct strvec keys = STRVEC_INIT; struct protocol_capability *command = NULL; - const char *client_sid; packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE | @@ -319,9 +307,6 @@ static int process_request(void) the_repository->hash_algo->name, hash_algos[client_hash_algo].name); - if (has_capability(&keys, "session-id", &client_sid)) - trace2_data_string("transfer", NULL, "client-sid", client_sid); - command->command(the_repository, &reader); strvec_clear(&keys); From f0a35c9ce52ade69311ab3b8cb111e145eb7b875 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 15 Sep 2021 14:35:29 -0400 Subject: [PATCH 06/11] serve: drop "keys" strvec We collect the set of capabilities the client sends us in a strvec. While this is usually small, there's no limit to the number of capabilities the client can send us (e.g., they could just send us "agent" pkt-lines over and over, and we'd keep adding them to the list). Since all code has been converted away from using this list, let's get rid of it. This avoids a potential attack where clients waste our memory. Note that we do have to replace it with a flag, because some of the flush-packet logic checks whether we've seen any valid commands or keys. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- serve.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/serve.c b/serve.c index 6bbf54cbbe..1a7c8a118f 100644 --- a/serve.c +++ b/serve.c @@ -239,7 +239,7 @@ static int process_request(void) { enum request_state state = PROCESS_REQUEST_KEYS; struct packet_reader reader; - struct strvec keys = STRVEC_INIT; + int seen_capability_or_command = 0; struct protocol_capability *command = NULL; packet_reader_init(&reader, 0, NULL, 0, @@ -260,10 +260,9 @@ static int process_request(void) case PACKET_READ_EOF: BUG("Should have already died when seeing EOF"); case PACKET_READ_NORMAL: - /* collect request; a sequence of keys and values */ if (parse_command(reader.line, &command) || receive_client_capability(reader.line)) - strvec_push(&keys, reader.line); + seen_capability_or_command = 1; else die("unknown capability '%s'", reader.line); @@ -275,7 +274,7 @@ static int process_request(void) * If no command and no keys were given then the client * wanted to terminate the connection. */ - if (!keys.nr) + if (!seen_capability_or_command) return 1; /* @@ -309,7 +308,6 @@ static int process_request(void) command->command(the_repository, &reader); - strvec_clear(&keys); return 0; } From 7f0e4f6ac28b7a9494f3affd1336244d4fb0fe38 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 15 Sep 2021 14:35:31 -0400 Subject: [PATCH 07/11] ls-refs: ignore very long ref-prefix counts Because each "ref-prefix" capability from the client comes in its own pkt-line, there's no limit to the number of them that a misbehaving client may send. We read them all into a strvec, which means the client can waste arbitrary amounts of our memory by just sending us "ref-prefix foo" over and over. One possible solution is to just drop the connection when the limit is reached. If we set it high enough, then only misbehaving or malicious clients would hit it. But "high enough" is vague, and it's unfriendly if we guess wrong and a legitimate client hits this. But we can do better. Since supporting the ref-prefix capability is optional anyway, the client has to further cull the response based on their own patterns. So we can simply ignore the patterns once we cross a certain threshold. Note that we have to ignore _all_ patterns, not just the ones past our limit (since otherwise we'd send too little data). The limit here is fairly arbitrary, and probably much higher than anyone would need in practice. It might be worth limiting it further, if only because we check it linearly (so with "m" local refs and "n" patterns, we do "m * n" string comparisons). But if we care about optimizing this, an even better solution may be a more advanced data structure anyway. I didn't bother making the limit configurable, since it's so high and since Git should behave correctly in either case. It wouldn't be too hard to do, but it makes both the code and documentation more complex. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- ls-refs.c | 20 ++++++++++++++++++-- t/t5701-git-serve.sh | 31 +++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/ls-refs.c b/ls-refs.c index df7f167433..07429f3a1c 100644 --- a/ls-refs.c +++ b/ls-refs.c @@ -40,6 +40,12 @@ static void ensure_config_read(void) config_read = 1; } +/* + * If we see this many or more "ref-prefix" lines from the client, we consider + * it "too many" and will avoid using the prefix feature entirely. + */ +#define TOO_MANY_PREFIXES 65536 + /* * Check if one of the prefixes is a prefix of the ref. * If no prefixes were provided, all refs match. @@ -158,8 +164,10 @@ int ls_refs(struct repository *r, struct packet_reader *request) data.peel = 1; else if (!strcmp("symrefs", arg)) data.symrefs = 1; - else if (skip_prefix(arg, "ref-prefix ", &out)) - strvec_push(&data.prefixes, out); + else if (skip_prefix(arg, "ref-prefix ", &out)) { + if (data.prefixes.nr < TOO_MANY_PREFIXES) + strvec_push(&data.prefixes, out); + } else if (!strcmp("unborn", arg)) data.unborn = allow_unborn; } @@ -167,6 +175,14 @@ int ls_refs(struct repository *r, struct packet_reader *request) if (request->status != PACKET_READ_FLUSH) die(_("expected flush after ls-refs arguments")); + /* + * If we saw too many prefixes, we must avoid using them at all; as + * soon as we have any prefix, they are meant to form a comprehensive + * list. + */ + if (data.prefixes.nr >= TOO_MANY_PREFIXES) + strvec_clear(&data.prefixes); + send_possibly_unborn_head(&data); if (!data.prefixes.nr) strvec_push(&data.prefixes, ""); diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh index 930721f053..520672f842 100755 --- a/t/t5701-git-serve.sh +++ b/t/t5701-git-serve.sh @@ -158,6 +158,37 @@ test_expect_success 'refs/heads prefix' ' test_cmp expect actual ' +test_expect_success 'ignore very large set of prefixes' ' + # generate a large number of ref-prefixes that we expect + # to match nothing; the value here exceeds TOO_MANY_PREFIXES + # from ls-refs.c. + { + echo command=ls-refs && + echo object-format=$(test_oid algo) && + echo 0001 && + perl -le "print \"ref-prefix refs/heads/\$_\" for (1..65536)" && + echo 0000 + } | + test-tool pkt-line pack >in && + + # and then confirm that we see unmatched prefixes anyway (i.e., + # that the prefix was not applied). + cat >expect <<-EOF && + $(git rev-parse HEAD) HEAD + $(git rev-parse refs/heads/dev) refs/heads/dev + $(git rev-parse refs/heads/main) refs/heads/main + $(git rev-parse refs/heads/release) refs/heads/release + $(git rev-parse refs/tags/annotated-tag) refs/tags/annotated-tag + $(git rev-parse refs/tags/one) refs/tags/one + $(git rev-parse refs/tags/two) refs/tags/two + 0000 + EOF + + test-tool serve-v2 --stateless-rpc out && + test-tool pkt-line unpack actual && + test_cmp expect actual +' + test_expect_success 'peel parameter' ' test-tool pkt-line pack >in <<-EOF && command=ls-refs From 9db5fb4fb352e79931e50f6de71497e273a3a6ac Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 15 Sep 2021 14:35:34 -0400 Subject: [PATCH 08/11] docs/protocol-v2: clarify some ls-refs ref-prefix details We've never documented the fact that a client can provide multiple ref-prefix capabilities. Let's describe the behavior. We also never discussed the "best effort" nature of the prefixes. The client side of git.git has always treated them this way, filtering the result with local patterns. And indeed any client must do this, because the prefix patterns are not sufficient to express the usual refspecs (and so for "foo" we ask for "refs/heads/foo", "refs/tags/foo", and so on). So this may be considered a change in the spec with respect to client expectations / requirements, but it's mostly codifying existing behavior. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/technical/protocol-v2.txt | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt index 213538f1d0..9347b2ad13 100644 --- a/Documentation/technical/protocol-v2.txt +++ b/Documentation/technical/protocol-v2.txt @@ -193,7 +193,11 @@ ls-refs takes in the following arguments: Show peeled tags. ref-prefix When specified, only references having a prefix matching one of - the provided prefixes are displayed. + the provided prefixes are displayed. Multiple instances may be + given, in which case references matching any prefix will be + shown. Note that this is purely for optimization; a server MAY + show refs not matching the prefix if it chooses, and clients + should filter the result themselves. If the 'unborn' feature is advertised the following argument can be included in the client's request. From 108c265f272d30ffaee423f7cc35885e9ac5d0e5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 15 Sep 2021 14:36:33 -0400 Subject: [PATCH 09/11] serve: reject bogus v2 "command=ls-refs=foo" When we see a line from the client like "command=ls-refs", we parse everything after the equals sign as a capability, which we check against our capabilities table. If we don't recognize the command (e.g., "command=foo"), we'll reject it. But in parse_command(), we use the same get_capability() parser for parsing non-command lines. So if we see "command=ls-refs=foo", we will feed "ls-refs=foo" to get_capability(), which will say "OK, that's ls-refs, with value 'foo'". But then we simply ignore the value entirely. The client is violating the spec here, which says: command = PKT-LINE("command=" key LF) key = 1*(ALPHA | DIGIT | "-_") I.e., the key is not even allowed to have an equals sign in it. Whereas a real non-command capability does allow a value: capability = PKT-LINE(key[=value] LF) So by reusing the same get_capability() parser, we are mixing up the "key" and "capability" tokens. However, since that parser tells us whether it saw an "=", we can still use it; we just need to reject any input that produces a non-NULL value field. The current behavior isn't really hurting anything (the client should never send such a request, and if it does, we just ignore the "value" part). But since it does violate the spec, let's tighten it up to prevent any surprising behavior. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- serve.c | 2 +- t/t5701-git-serve.sh | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/serve.c b/serve.c index 1a7c8a118f..db5ecfed2d 100644 --- a/serve.c +++ b/serve.c @@ -220,7 +220,7 @@ static int parse_command(const char *key, struct protocol_capability **command) if (*command) die("command '%s' requested after already requesting command '%s'", out, (*command)->name); - if (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command) + if (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command || value) die("invalid command '%s'", out); *command = cmd; diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh index 520672f842..2e51886def 100755 --- a/t/t5701-git-serve.sh +++ b/t/t5701-git-serve.sh @@ -72,6 +72,16 @@ test_expect_success 'request invalid command' ' test_i18ngrep "invalid command" err ' +test_expect_success 'requested command is command=value' ' + test-tool pkt-line pack >in <<-EOF && + command=ls-refs=whatever + object-format=$(test_oid algo) + 0000 + EOF + test_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-EOF && command=fetch From 0ab7eeccd9aea668819288c086dcdf57ca14a026 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 15 Sep 2021 14:36:36 -0400 Subject: [PATCH 10/11] serve: reject commands used as capabilities Our table of v2 "capabilities" contains everything we might tell the client we support. But there are differences in how we expect the client to respond. Some of the entries are true capabilities (i.e., we expect the client to say "yes, I support this"), and some are ones we expect them to send as commands (with "command=ls-refs" or similar). When we receive a capability used as a command, we complain about that. But when we receive a command used as a capability (e.g., just "ls-refs" in a pkt-line by itself), we silently ignore it. This isn't really hurting anything (clients shouldn't send it, and we'll ignore it), but we can tighten up the protocol to match what we expect to happen. There are two new tests here. The first one checks a capability used as a command, which already passes. The second tests a command as a capability, which this patch fixes. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- serve.c | 2 +- t/t5701-git-serve.sh | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/serve.c b/serve.c index db5ecfed2d..b3fe9b5126 100644 --- a/serve.c +++ b/serve.c @@ -201,7 +201,7 @@ static int receive_client_capability(const char *key) const char *value; const struct protocol_capability *c = get_capability(key, &value); - if (!c || !c->advertise(the_repository, NULL)) + if (!c || c->command || !c->advertise(the_repository, NULL)) return 0; if (c->receive) diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh index 2e51886def..3928424e1b 100755 --- a/t/t5701-git-serve.sh +++ b/t/t5701-git-serve.sh @@ -72,6 +72,27 @@ test_expect_success 'request invalid command' ' test_i18ngrep "invalid command" err ' +test_expect_success 'request capability as command' ' + test-tool pkt-line pack >in <<-EOF && + command=agent + object-format=$(test_oid algo) + 0000 + EOF + test_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-EOF && + command=ls-refs + object-format=$(test_oid algo) + fetch + 0000 + EOF + test_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-EOF && command=ls-refs=whatever From ccf094788c50c597972ee1fd9c2b554cadc0f14c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 15 Sep 2021 14:36:38 -0400 Subject: [PATCH 11/11] ls-refs: reject unknown arguments The v2 ls-refs command may receive extra arguments from the client, one per pkt-line. The spec is pretty clear that the arguments must come from a specified set, but we silently ignore any unknown entries. For a well-behaved client this doesn't matter, but it makes testing and debugging more confusing. Let's tighten this up to match the spec. In theory this liberal behavior _could_ be useful for extending the protocol. But: - every other part of the protocol requires that the server first indicate that it supports the argument; this includes the fetch and object-info commands, plus the "unborn" capability added to ls-refs itself - it's not a very good extension mechanism anyway; without the server advertising support, clients would have no idea if the argument was silently ignored, or accepted and simply had no effect So we're not really losing anything by tightening this. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- ls-refs.c | 2 ++ t/t5701-git-serve.sh | 13 +++++++++++++ 2 files changed, 15 insertions(+) diff --git a/ls-refs.c b/ls-refs.c index 07429f3a1c..883c0c58b2 100644 --- a/ls-refs.c +++ b/ls-refs.c @@ -170,6 +170,8 @@ int ls_refs(struct repository *r, struct packet_reader *request) } else if (!strcmp("unborn", arg)) data.unborn = allow_unborn; + else + die(_("unexpected line: '%s'"), arg); } if (request->status != PACKET_READ_FLUSH) diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh index 3928424e1b..aa1827d841 100755 --- a/t/t5701-git-serve.sh +++ b/t/t5701-git-serve.sh @@ -147,6 +147,19 @@ test_expect_success 'basics of ls-refs' ' test_cmp expect actual ' +test_expect_success 'ls-refs complains about unknown options' ' + test-tool pkt-line pack >in <<-EOF && + command=ls-refs + object-format=$(test_oid algo) + 0001 + no-such-arg + 0000 + EOF + + test_must_fail test-tool serve-v2 --stateless-rpc 2>err in <<-EOF && command=ls-refs