git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 11/11] ls-refs: reject unknown arguments

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Sep 15, 2021, 00:09 UTC
Message-ID
<875yv2ofkq.fsf@evledraar.gmail.com>
In-Reply-To
<YUE2J74PP8TGthOZ@coredump.intra.peff.net>
On Tue, Sep 14 2021, Jeff King wrote:
Show 63 quoted lines
> 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@peff.net>
> ---
>  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@gmail.com>
---
 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
Previous: Jeff KingNext: Jeff King
Message 60 of 77 in “reducing memory allocations for v2 servers”
  1. 0/9 reducing memory allocations for v2 serversJeff King, Sep 14, 2021
  2. 1/9 serve: rename is_command() to parse_command()Jeff King, Sep 14, 2021
  3. 2/9 serve: return capability "value" from get_capability()Jeff King, Sep 14, 2021
  4. 3/9 serve: add "receive" method for v2 capabilities tableJeff King, Sep 14, 2021
  5. 4/9 serve: provide "receive" function for object-format capabilityJeff King, Sep 14, 2021
  6. Martin ÅgrenSep 14, 2021
  7. 5/9 serve: provide "receive" function for session-id capabilityJeff King, Sep 14, 2021
  8. Taylor BlauSep 14, 2021
  9. Jeff KingSep 14, 2021
  10. Taylor BlauSep 14, 2021
  11. Martin ÅgrenSep 14, 2021
  12. Jeff KingSep 14, 2021
  13. 6/9 serve: drop "keys" strvecJeff King, Sep 14, 2021
  14. Taylor BlauSep 14, 2021
  15. Jeff KingSep 14, 2021
  16. 7/9 ls-refs: ignore very long ref-prefix countsJeff King, Sep 14, 2021
  17. Taylor BlauSep 14, 2021
  18. Jeff KingSep 14, 2021
  19. Jeff KingSep 14, 2021
  20. Martin ÅgrenSep 14, 2021
  21. Jeff KingSep 14, 2021
  22. Jeff KingSep 14, 2021
  23. Taylor BlauSep 14, 2021
  24. Jeff KingSep 14, 2021
  25. 8/9 serve: reject bogus v2 "command=ls-refs=foo"Jeff King, Sep 14, 2021
  26. Taylor BlauSep 14, 2021
  27. 9/9 serve: reject commands used as capabilitiesJeff King, Sep 14, 2021
  28. Taylor BlauSep 14, 2021
  29. Junio C HamanoSep 14, 2021
  30. Jeff KingSep 14, 2021
  31. 0/11 limit memory allocations for v2 serversJeff King, Sep 14, 2021
  32. 01/11 serve: rename is_command() to parse_command()Jeff King, Sep 14, 2021
  33. 02/11 serve: return capability "value" from get_capability()Jeff King, Sep 14, 2021
  34. 03/11 serve: add "receive" method for v2 capabilities tableJeff King, Sep 14, 2021
  35. Ævar Arnfjörð BjarmasonSep 15, 2021
  36. Jeff KingSep 15, 2021
  37. Junio C HamanoSep 15, 2021
  38. Jeff KingSep 15, 2021
  39. 04/11 serve: provide "receive" function for object-format capabilityJeff King, Sep 14, 2021
  40. Junio C HamanoSep 15, 2021
  41. 05/11 serve: provide "receive" function for session-id capabilityJeff King, Sep 14, 2021
  42. Junio C HamanoSep 15, 2021
  43. 07/11 ls-refs: ignore very long ref-prefix countsJeff King, Sep 14, 2021
  44. Taylor BlauSep 15, 2021
  45. Jeff KingSep 15, 2021
  46. Eric SunshineSep 15, 2021
  47. Jeff KingSep 15, 2021
  48. 06/11 serve: drop "keys" strvecJeff King, Sep 14, 2021
  49. Junio C HamanoSep 15, 2021
  50. 08/11 docs/protocol-v2: clarify some ls-refs ref-prefix detailsJeff King, Sep 14, 2021
  51. 09/11 serve: reject bogus v2 "command=ls-refs=foo"Jeff King, Sep 14, 2021
  52. Ævar Arnfjörð BjarmasonSep 15, 2021
  53. Jeff KingSep 15, 2021
  54. Eric SunshineSep 15, 2021
  55. Jeff KingSep 15, 2021
  56. Junio C HamanoSep 15, 2021
  57. Jeff KingSep 15, 2021
  58. 10/11 serve: reject commands used as capabilitiesJeff King, Sep 14, 2021
  59. 11/11 ls-refs: reject unknown argumentsJeff King, Sep 14, 2021
  60. Ævar Arnfjörð BjarmasonSep 15, 2021
  61. Jeff KingSep 15, 2021
  62. Taylor BlauSep 15, 2021
  63. Jeff KingSep 15, 2021
  64. 0/11 limit memory allocations for v2 serversJeff King, Sep 15, 2021
  65. 01/11 serve: rename is_command() to parse_command()Jeff King, Sep 15, 2021
  66. 02/11 serve: return capability "value" from get_capability()Jeff King, Sep 15, 2021
  67. 03/11 serve: add "receive" method for v2 capabilities tableJeff King, Sep 15, 2021
  68. 04/11 serve: provide "receive" function for object-format capabilityJeff King, Sep 15, 2021
  69. 05/11 serve: provide "receive" function for session-id capabilityJeff King, Sep 15, 2021
  70. 06/11 serve: drop "keys" strvecJeff King, Sep 15, 2021
  71. 07/11 ls-refs: ignore very long ref-prefix countsJeff King, Sep 15, 2021
  72. 08/11 docs/protocol-v2: clarify some ls-refs ref-prefix detailsJeff King, Sep 15, 2021
  73. 09/11 serve: reject bogus v2 "command=ls-refs=foo"Jeff King, Sep 15, 2021
  74. 10/11 serve: reject commands used as capabilitiesJeff King, Sep 15, 2021
  75. 11/11 ls-refs: reject unknown argumentsJeff King, Sep 15, 2021
  76. Ævar Arnfjörð BjarmasonSep 15, 2021
  77. Jeff KingSep 15, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.