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

diff --git a/serve.c b/serve.c
index 1a7c8a118f3745b894d314b295cd42ed15ecea25..db5ecfed2dae53c8a1dd32771deb588411393b19 100644 (file)
--- 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 520672f842873646d6de46500f413c2927c69995..2e51886defe3773870b5cd5db7b1926ad438e0a5 100755 (executable)
--- 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