Re: [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo"

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Sep 14 2021, Jeff King wrote:

> 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...

> 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