On Tue, Sep 14 2021, Jeff King wrote:
> 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 <peff@xxxxxxxx>
> ---
> 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 18c4f41e87..460ac9b229 100644
> --- a/ls-refs.c
> +++ b/ls-refs.c
> @@ -168,6 +168,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 b027ba9b06..e4d60bc605 100755
> --- a/t/t5701-git-serve.sh
> +++ b/t/t5701-git-serve.sh
> @@ -145,6 +145,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 &&
> + grep unexpected.line.*no-such-arg err
> +'
> +
> test_expect_success 'basic ref-prefixes' '
> test-tool pkt-line pack >in <<-EOF &&
> command=ls-refs
Looks good. For what it's worth some of this series is stuff I've been
meaning to submit after my current in-flight series, so I guess we'll be
playing some rebase ping-pong in this area.
In my version of this I'd led with a change to do changes like these for
all the protocol verbs:
diff --git a/connect.c b/connect.c
index 70b13389ba5..5e991a92279 100644
--- a/connect.c
+++ b/connect.c
@@ -514,6 +514,8 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
packet_write_fmt(fd_out, "symrefs\n");
if (server_supports_feature("ls-refs", "unborn", 0))
packet_write_fmt(fd_out, "unborn\n");
+ if (git_env_bool("GIT_TEST_PROTOCOL_BAD_LS_REFS", 0))
+ packet_write_fmt(fd_out, "test-bad-client\n");
for (i = 0; ref_prefixes && i < ref_prefixes->nr; i++) {
packet_write_fmt(fd_out, "ref-prefix %s\n",
ref_prefixes->v[i]);
So the below patch isn't anywhere near applying, but you can see the
test coverage (those tests are new) & what changes if we instrument a
client to actually send this bad data.
That packet_client_error() function is new, part of what I'm doing there
is converting all of this error emitting from die() to properly sending
ERR packets.
I think a bug in your versio is that you're using _() here, if your
server program happens to be started in Chinese you probably still want
to emit errors to clients in LANG=C.
That's why I'm using N_() in my version, it knows to emit the die()
localized, but the ERR packet in LANG=C, no you'll get the i18n message
in the server terminal, but the client gets LANG=C.
Of course that actually working is subject to various races that I may
or may not be able to untangle...
-- >8 --
protocol v2: correct & adjust "ls-refs" ERR behavior
Change the protocol v2 "ls-refs" to error out on unknown arguments,
before this we'd silently ignore unknown arguments.
This brings us in line with the behavior we've had in
"fetch" (i.e. upload-pack.c) since 3145ea957d2 (upload-pack: introduce
fetch server command, 2018-03-15) (see the "/* ignore unknown lines
maybe? */" comment in "process_args()".
The looser behavior in "ls-refs" seems to have been unintentionally
added in 72d0ea0056b (ls-refs: introduce ls-refs server command,
2018-03-15).
I've also changed the wording of the "expected flush" message in
"ls-refs" to be consistent with "fetch" and "object-info".
In the changes leading up to this we had to grep out the racy "fatal:
the remote end hung up unexpectedly" messages. Now we need to guard
the full test_cmp with a test_expect_failure. I could also write a
looser test, but this makes subsequent changes smaller, and is more
accurate.
Most of the time the test_expect_failure() succeeds, but in some cases
the two messages will be out-of-order, or e.g. only the STDERR one
will make it, and not the ERR message, or the other way
around. I.e. now that we emit both an ERR line and die() on the
remote, we might see ERR before die(), or die() before ERR.
This race does not happen in the t5701-git-serve.sh and other
"test-tool pkt-line" tests, which only use one process.
Ideally we should only emit ERR and not also something on STDERR when
we die() on the server. Subsequent commits will address that, but for
now let's keep that behavior. I.e. we'll now emit two mostly duplicate
errors in some cases from the client, or one where we emitted none (or
one that made no sense) before.
The current client implementation doesn't expect the server to be this
overly helpful in giving us errors both via pkt-line and STDERR, so we
usually get two lines, and in some cases just one (but never
zero).
Subsequent commits will address this. I.e. we can assume that the
client can format the ERR lines, and that we can just exit(128) or
disconnect on the server, not die().
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
---
ls-refs.c | 6 +++++-
t/t5703-protocol-v2-file.sh | 24 +++++++++++++++++-------
t/t5704-protocol-v2-git.sh | 20 +++++++++++++-------
t/t5705-protocol-v2-http.sh | 25 +++++++++++++++++++------
t/t5710-protocol-violations.sh | 2 +-
5 files changed, 55 insertions(+), 22 deletions(-)
diff --git a/ls-refs.c b/ls-refs.c
index 1e50e785665..f9bd577dcd2 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -150,11 +150,15 @@ int ls_refs(struct repository *r, struct packet_reader *request)
strvec_push(&data.prefixes, out);
else if (!strcmp("unborn", arg))
data.unborn = allow_unborn;
+ else
+ packet_client_error(&data.writer,
+ N_("ls-refs: unexpected argument: '%s'"),
+ request->line);
}
if (request->status != PACKET_READ_FLUSH)
packet_client_error(&data.writer,
- N_("expected flush after ls-refs arguments"));
+ N_("ls-refs: expected flush after arguments"));
send_possibly_unborn_head(&data);
if (!data.prefixes.nr)
diff --git a/t/t5703-protocol-v2-file.sh b/t/t5703-protocol-v2-file.sh
index e572767ee00..79457eaf101 100755
--- a/t/t5703-protocol-v2-file.sh
+++ b/t/t5703-protocol-v2-file.sh
@@ -32,18 +32,28 @@ test_expect_success 'list refs with file:// using protocol v2' '
test_expect_success 'ls-remote handling a bad client using file:// protocol v2' '
test_when_finished "rm -f log" &&
- cat >expect <<-EOF &&
- $(git -C file_parent rev-parse refs/heads/main)$(printf "\t")refs/heads/main
+ cat >log.expect <<-\EOF &&
+ packet: upload-pack> ERR ls-refs: unexpected argument: '"'"'test-bad-client'"'"'
+ packet: git< ERR ls-refs: unexpected argument: '"'"'test-bad-client'"'"'
EOF
- env \
+ cat >err.expect <<-\EOF &&
+ fatal: ls-refs: unexpected argument: '"'"'test-bad-client'"'"'
+ fatal: remote error: ls-refs: unexpected argument: '"'"'test-bad-client'"'"'
+ EOF
+ test_must_fail env \
GIT_TRACE_PACKET="$(pwd)/log" \
GIT_TEST_PROTOCOL_BAD_LS_REFS=true \
git -c protocol.version=2 \
- ls-remote "file://$(pwd)/file_parent" main >actual 2>err &&
+ ls-remote "file://$(pwd)/file_parent" main >out 2>err.actual &&
- test_must_be_empty err &&
- test_cmp expect actual &&
- grep "git> test-bad-client$" log
+ grep "unexpected argument.*test-bad-client" err.actual &&
+ test_must_be_empty out &&
+ grep ERR log >log.actual &&
+ test_cmp log.expect log.actual
+'
+
+test_expect_failure 'ls-remote ERR and die() is racy under file:// protocol v2' '
+ test_cmp err.expect err.actual
'
test_expect_success 'ref advertisement is filtered with ls-remote using protocol v2' '
diff --git a/t/t5704-protocol-v2-git.sh b/t/t5704-protocol-v2-git.sh
index c9293bbaad2..123c3f56ef8 100755
--- a/t/t5704-protocol-v2-git.sh
+++ b/t/t5704-protocol-v2-git.sh
@@ -48,19 +48,25 @@ test_expect_success 'ref advertisement is filtered with ls-remote using protocol
test_cmp expect actual
'
-test_expect_success 'ls-remote handling a bad client using git:// protocol v2' '
+test_expect_success 'ls-remote handling a bad client using protocol v2' '
test_when_finished "rm -f log" &&
- git ls-remote "$daemon_parent" >expect &&
- env \
+ cat >err.expect <<-EOF &&
+ fatal: remote error: ls-refs: unexpected argument: '"'"'test-bad-client'"'"'
+ EOF
+ test_must_fail env \
GIT_TRACE_PACKET="$(pwd)/log" \
GIT_TEST_PROTOCOL_BAD_LS_REFS=true \
git -c protocol.version=2 \
- ls-remote "$GIT_DAEMON_URL/parent" >actual 2>err &&
+ ls-remote "$GIT_DAEMON_URL/parent" main >out.actual 2>err.actual &&
- test_must_be_empty err &&
- test_cmp expect actual &&
- grep "git> test-bad-client$" log
+ test_must_be_empty out.actual &&
+ grep "unexpected argument.*test-bad-client" err.actual &&
+ grep "ERR ls-refs: unexpected argument.*test-bad-client" log
+'
+
+test_expect_failure 'ls-remote ERR and die() is racy under file:// protocol v2' '
+ test_cmp err.expect err.actual
'
test_expect_success 'clone with git:// using protocol v2' '
diff --git a/t/t5705-protocol-v2-http.sh b/t/t5705-protocol-v2-http.sh
index 5982c38743e..6f526e0829f 100755
--- a/t/t5705-protocol-v2-http.sh
+++ b/t/t5705-protocol-v2-http.sh
@@ -192,16 +192,29 @@ test_expect_success 'fetch from namespaced repo respects namespaces' '
test_expect_success 'ls-remote handling a bad client using http:// protocol v2' '
test_when_finished "rm -f log" &&
- git ls-remote "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" >expect &&
- env \
+ cat >log.expect <<-\EOF &&
+ packet: upload-pack> ERR ls-refs: unexpected argument: '"'"'test-bad-client'"'"'
+ packet: git< ERR ls-refs: unexpected argument: '"'"'test-bad-client'"'"'
+ EOF
+
+ cat >err.expect <<-\EOF &&
+ fatal: ls-refs: unexpected argument: '"'"'test-bad-client'"'"'
+ fatal: remote error: ls-refs: unexpected argument: '"'"'test-bad-client'"'"'
+ EOF
+ test_must_fail env \
GIT_TRACE_PACKET="$(pwd)/log" \
GIT_TEST_PROTOCOL_BAD_LS_REFS=true \
git -c protocol.version=2 \
- ls-remote "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" >actual 2>err &&
+ ls-remote "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" >out 2>err.actual &&
- test_must_be_empty err &&
- test_cmp expect actual &&
- grep "git> test-bad-client$" log
+ grep "unexpected argument.*test-bad-client" err.actual &&
+ test_must_be_empty out &&
+ grep ERR log >log.actual &&
+ test_cmp log.expect log.actual
+'
+
+test_expect_failure 'ls-remote ERR and die() is racy under http:// protocol v2' '
+ test_cmp err.expect err.actual
'
test_expect_success 'ls-remote with v2 http sends only one POST' '
diff --git a/t/t5710-protocol-violations.sh b/t/t5710-protocol-violations.sh
index 44e2c0d3ded..75a457d39ca 100755
--- a/t/t5710-protocol-violations.sh
+++ b/t/t5710-protocol-violations.sh
@@ -16,7 +16,7 @@ test_expect_success 'extra delim packet in v2 ls-refs args' '
EOF
cat >err.expect <<-\EOF &&
- fatal: expected flush after ls-refs arguments
+ fatal: ls-refs: expected flush after arguments
EOF
test_must_fail env GIT_PROTOCOL=version=2 \
git upload-pack . <input 2>err.actual &&
--
2.33.0.1013.ge8323766266
