Re: [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo"
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Sep 15, 2021, 00:27 UTC
- Message-ID
- <87wnnin0ix.fsf@evledraar.gmail.com>
- In-Reply-To
- <YUE1ym1dALRQLztq@coredump.intra.peff.net>
On Tue, Sep 14 2021, Jeff King wrote:
Show 10 quoted lines
> 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 we use the same parser that checks > for regular capabilities like "object-format=sha256". And so we'll > accept "ls-refs=foo", even though everything after the equals is bogus, > and simply ignored. > > This isn't really hurting anything, but the request does violate the > spec. Let's tighten it up to prevent any surprising behavior.
Doesn't need a re-roll, but just for my own sanity:
By violating the spec you mean it doesn't coform to the "key" in the "Capability Advertisement" section of protocol-v2.txt, one might skim this and think values with "=" in them are OK, but a command=WHATEVER has a WHATEVER as a "key", not a "value", that's for other parts of the dialog.
But you could also have meant that it violates the spec because there's no such command as "ls-refs=whatever", just like there isn't "foobar", but I don't think that's what you mean...
Show 40 quoted lines
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> 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 5ea6c915cb..63ee1be7ff 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 3bc96ebcde..ab15078bc0 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 &&
> + grep invalid.command.*ls-refs=whatever err
> +'
> +
> test_expect_success 'wrong object-format' '
> test-tool pkt-line pack >in <<-EOF &&
> command=fetch