remote-curl: die on server-side errors

When a smart HTTP server sends an error message via pkt-line, remote-curl will fail to detect the error (which usually results in incorrectly falling back to dumb-HTTP mode).

This patch adds a check in discover_refs() for server-side error messages, as well as a test case for this issue.

Signed-off-by Josh Steadmon <[email protected]>

When a smart HTTP server sends an error message via pkt-line, remote-curl will fail to detect the error (which usually results in incorrectly falling back to dumb-HTTP mode).

This patch adds a check in discover_refs() for server-side error messages, as well as a test case for this issue.

Signed-off-by Josh Steadmon <[email protected]>

---
 remote-curl.c                   | 4 +++-
 t/lib-httpd.sh                  | 1 +
 t/lib-httpd/apache.conf         | 4 ++++
 t/lib-httpd/error-smart-http.sh | 3 +++
 t/t5551-http-fetch-smart.sh     | 5 +++++
 5 files changed, 16 insertions(+), 1 deletion(-)
 create mode 100644 t/lib-httpd/error-smart-http.sh

remote-curl.c +0 -0

index 762a55a75f..bb3a86505e 100644

t/lib-httpd.sh +1 -0

index a8729f8232..4e946623bb 100644
@@ -131,6 +131,7 @@ prepare_httpd() {
 	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
 	cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
 	install_script broken-smart-http.sh
+	install_script error-smart-http.sh
 	install_script error.sh
 	install_script apply-one-time-sed.sh
 

t/lib-httpd/apache.conf +4 -0

index 581c010d8f..6de2bc775c 100644
@@ -117,6 +117,7 @@ Alias /auth/dumb/ www/auth/dumb/
 </LocationMatch>
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
+ScriptAlias /error_smart/ error-smart-http.sh/
 ScriptAlias /error/ error.sh/
 ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1
 <Directory ${GIT_EXEC_PATH}>

@@ -125,6 +126,9 @@ ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1
 <Files broken-smart-http.sh>
 	Options ExecCGI
 </Files>
+<Files error-smart-http.sh>
+	Options ExecCGI
+</Files>
 <Files error.sh>
   Options ExecCGI
 </Files>

t/lib-httpd/error-smart-http.sh new file +3 -0

new file mode 100644
index 0000000000..0a1af318f7
@@ -0,0 +1,3 @@
+printf "Content-Type: text/%s\n" "html"
+echo
+printf "%s" "0019ERR server-side error"

t/t5551-http-fetch-smart.sh +5 -0

index 8630b0cc39..ba83e567e5 100755
@@ -429,5 +429,10 @@ test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' '
 	! grep "=> Send data" err
 '
 
+test_expect_success 'server-side error detected' '
+	test_must_fail git clone $HTTPD_URL/error_smart/repo.git 2>actual &&
+	grep "server-side error" actual
+'
+
 stop_httpd
 test_done

When a smart HTTP server sends an error message via pkt-line, remote-curl will fail to detect the error (which usually results in incorrectly falling back to dumb-HTTP mode).

This patch adds a check in discover_refs() for server-side error messages, as well as a test case for this issue.

Signed-off-by Josh Steadmon <[email protected]>

---

Quoted 9 lines · [email protected] writes:

When a smart HTTP server sends an error message via pkt-line, remote-curl will fail to detect the error (which usually results in incorrectly falling back to dumb-HTTP mode).

This patch adds a check in discover_refs() for server-side error messages, as well as a test case for this issue.

Signed-off-by Josh Steadmon <[email protected]>

---

Forgot to mention one procedural comment.

As you can see in the To: line of this reply, your MUA is placing only the e-mail address without name on your From: line.

Preferrably I'd like to see the same string as your sign-off on the "From:" line in your messages for a bit of human touch ;-) Can you tweak your MUA to make that happen?

The second preference is to add an in-body header (i.e. as the first line of the body of the message) so that the body of the message starts like this:

From: Josh Steadmon <[email protected]>

When a smart HTTP server sends an error message via pkt-line, remote-curl will fail to detect the error (which usually results in incorrectly falling back to dumb-HTTP mode).

This patch adds a check in discover_refs() for server-side error messages, as well as a test case for this issue.

Signed-off-by Josh Steadmon <[email protected]>

---
remote-curl.c | 4 +++-
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf | 4 ++++

Either way would make sure that the resulting patch's author line will be attributed correctly to the same identity as who is signing it off first as the author.

Thanks.

When a smart HTTP server sends an error message via pkt-line, remote-curl will fail to detect the error (which usually results in incorrectly falling back to dumb-HTTP mode).

This patch adds a check in discover_refs() for server-side error messages, as well as a test case for this issue.

Quoted 6 lines · On Mon, Nov 12, 2018 at 02:44:56PM -0800, [email protected] wrote:

When a smart HTTP server sends an error message via pkt-line, remote-curl will fail to detect the error (which usually results in incorrectly falling back to dumb-HTTP mode).

This patch adds a check in discover_refs() for server-side error messages, as well as a test case for this issue.

Aside from the reformatting of the conditional that Junio mentioned, this seems pretty good to me. But while looking at that, I found a few things, some old and some new. :)

remote-curl.c static struct discovery *discover_refs(const char *service, int for_push) · line 441

Quoted 12 lines

remote-curl.c +3 -1

index 762a55a75f..bb3a86505e 100644
@@ -436,7 +436,9 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	} else if (maybe_smart &&
 		   last->len > 5 && starts_with(last->buf + 4, "version 2")) {
 		last->proto_git = 1;
-	}
+	} else if (maybe_smart && last->len > 5 &&
+		   starts_with(last->buf + 4, "ERR "))
+		die(_("remote error: %s"), last->buf + 8);

The magic "4" here and in the existing "version 2" check is because we are expecting pkt-lines. The original conditional always calls packed_read_line_buf() and will complain if we didn't actually get a pkt-line.

Should we confirm that we got a real packet-line? Or at least that those first four are even plausible hex chars?

I admit that it's pretty unlikely that the server is going to fool us here. It would need something like "foo ERRORS ARE FUN!". And even then we'd report an error (whereas the correct behavior is falling back to dumb http, but we know that won't work anyway because that's not a valid ref advertisement). So I doubt this is really a bug per se, but it might make it easier to understand what's going on if we actually parsed the packet.

Similarly, we seem eager to accept "version 2" even if we are only expecting v0. I know you have another series working in that direction, but I don't think it touches this "proto_git". I guess accepting "version 2" as "we're speaking git protocol" and then barfing later with "wait, I didn't mean to speak v2" is probably OK.

-Peff

Quoted 8 lines

The magic "4" here and in the existing "version 2" check is because we are expecting pkt-lines. The original conditional always calls packed_read_line_buf() and will complain if we didn't actually get a pkt-line.

Should we confirm that we got a real packet-line? Or at least that those first four are even plausible hex chars?

Quoted 7 lines

I admit that it's pretty unlikely that the server is going to fool us here. It would need something like "foo ERRORS ARE FUN!". And even then we'd report an error (whereas the correct behavior is falling back to dumb http, but we know that won't work anyway because that's not a valid ref advertisement). So I doubt this is really a bug per se, but it might make it easier to understand what's going on if we actually parsed the packet.

Unfortunately we can't just directly parse the data in last->buf, because other parts of the code are expecting to see the raw pkt-line data there. I tried adding a duplicate pointer and length variable for this data and parsing that through packet_read_line_buf(), but even without using the results this apparently has side-effects that break all of t5550 (and probably other tests as well). It also fails if I completely duplicate last->buf into a new char* and call packet_readline_buf() on that, so there's clearly some interaction in the pkt-line guts that I'm not properly accounting for.

Yes, the packet_read_line_buf() interface will both advance the buf pointer and decrement the length. So if we want to "peek", we have to do so with a copy (there's a peek function if you use the packet_reader interface, but that might be overkill here).

You can rewrite it like this, which is a pretty faithful conversion and passes the tests (but see below).
[...]

Quoted 8 lines · Jeff King

Yes, the packet_read_line_buf() interface will both advance the buf pointer and decrement the length. So if we want to "peek", we have to do so with a copy (there's a peek function if you use the packet_reader interface, but that might be overkill here).

You can rewrite it like this, which is a pretty faithful conversion and passes the tests (but see below).
[...]

Here's a version which is less faithful, but I think does sensible things in all cases, and is much easier to follow. I get a little nervous just because it tightens some cases, and one never knows if other implementations might be relying on the looseness. E.g.:

- in the current code we'd still drop back to dumb http if the server tells us "application/x-git-upload-pack" but the initial pktline doesn't start with "#" (even though if it _does_ have "#" at position 5 but isn't a valid pktline, we'd complain!)

- right now the v2 protocol does not require the server to say "application/x-git-upload-pack" for the content-type

This patch tightens both of those (I also made a few stylistic tweaks, and added the ERR condition to show where it would go). I dunno. Part of me sees this as a nice cleanup, but maybe it is better to just leave it alone. A lot of these behaviors are just how it happens to work now, and not part of the spec, but we don't know what might be relying on them.

remote-curl.c +55 -34

index 762a55a75f..1adb96311b 100644
@@ -330,9 +330,61 @@ static int get_protocol_http_header(enum protocol_version version,
 	return 0;
 }
 
+static void check_smart_http(struct discovery *d, const char *service,
+			     struct strbuf *type)
+{
+	char *src_buf;
+	size_t src_len;
+	char *line;
+	const char *p;
+
+	if (!skip_prefix(type->buf, "application/x-", &p) ||
+	    !skip_prefix(p, service, &p) ||
+	    strcmp(p, "-advertisement"))
+		return;
+
+	/*
+	 * We speculatively try to read a packet, which means we must preserve
+	 * the original buf/len pair in some cases.
+	 */
+	src_buf = d->buf;
+	src_len = d->len;
+	line = packet_read_line_buf(&src_buf, &src_len, NULL);
+	if (!line)
+		die("invalid server response; expected service, got flush packet");
+
+	if (skip_prefix(line, "# service=", &p) && !strcmp(p, service)) {
+		/*
+		 * The header can include additional metadata lines, up
+		 * until a packet flush marker.  Ignore these now, but
+		 * in the future we might start to scan them.
+		 */
+		while (packet_read_line_buf(&src_buf, &src_len, NULL))
+			;
+
+		/*
+		 * v0 smart http; callers expect us to soak up the
+		 * service and header packets
+		 */
+		d->buf = src_buf;
+		d->len = src_len;
+		d->proto_git = 1;
+
+	} else if (starts_with(line, "version 2")) { /* should be strcmp()? */
+		/*
+		 * v2 smart http; do not consume version packet, which will
+		 * be handled elsewhere.
+		 */
+		d->proto_git = 1;
+	} else if (skip_prefix(line, "ERR ", &p)) {
+		die(_("remote error: %s"), p);
+	} else {
+		die("invalid server response; got '%s'", line);
+	}
+}
+
 static struct discovery *discover_refs(const char *service, int for_push)
 {
-	struct strbuf exp = STRBUF_INIT;
 	struct strbuf type = STRBUF_INIT;
 	struct strbuf charset = STRBUF_INIT;
 	struct strbuf buffer = STRBUF_INIT;

@@ -405,38 +457,8 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	last->buf_alloc = strbuf_detach(&buffer, &last->len);
 	last->buf = last->buf_alloc;
 
-	strbuf_addf(&exp, "application/x-%s-advertisement", service);
-	if (maybe_smart &&
-	    (5 <= last->len && last->buf[4] == '#') &&
-	    !strbuf_cmp(&exp, &type)) {
-		char *line;
-
-		/*
-		 * smart HTTP response; validate that the service
-		 * pkt-line matches our request.
-		 */
-		line = packet_read_line_buf(&last->buf, &last->len, NULL);
-		if (!line)
-			die("invalid server response; expected service, got flush packet");
-
-		strbuf_reset(&exp);
-		strbuf_addf(&exp, "# service=%s", service);
-		if (strcmp(line, exp.buf))
-			die("invalid server response; got '%s'", line);
-		strbuf_release(&exp);
-
-		/* The header can include additional metadata lines, up
-		 * until a packet flush marker.  Ignore these now, but
-		 * in the future we might start to scan them.
-		 */
-		while (packet_read_line_buf(&last->buf, &last->len, NULL))
-			;
-
-		last->proto_git = 1;
-	} else if (maybe_smart &&
-		   last->len > 5 && starts_with(last->buf + 4, "version 2")) {
-		last->proto_git = 1;
-	}
+	if (maybe_smart)
+		check_smart_http(last, service, &type);
 
 	if (last->proto_git)
 		last->refs = parse_git_refs(last, for_push);

@@ -444,7 +466,6 @@ static struct discovery *discover_refs(const char *service, int for_push)
 		last->refs = parse_info_refs(last);
 
 	strbuf_release(&refs_url);
-	strbuf_release(&exp);
 	strbuf_release(&type);
 	strbuf_release(&charset);
 	strbuf_release(&effective_url);

Quoted 5 lines

This patch tightens both of those (I also made a few stylistic tweaks, and added the ERR condition to show where it would go). I dunno. Part of me sees this as a nice cleanup, but maybe it is better to just leave it alone. A lot of these behaviors are just how it happens to work now, and not part of the spec, but we don't know what might be relying on them.

At least according to the protocol-v2 and http-protocol docs, the stricter behavior seems correct:

For the first point above, dumb servers should never use an "application/x-git-*" content type (http-protocol.txt line 163-167).

For the second point, the docs require v2 servers to use "application/x-git-*" content types. protocol-v2.txt lines 63-65 state that v2 clients should make a smart http request, while http-protocol.txt lines 247-252 state that a smart server's response type must be "application/x-git-*".

Of course we don't know if other implementations follow the spec, but ISTM that this patch at least doesn't contradict how we've promised the protocols should work.

If no one has any objections, I'll include the diff below in v2. Thanks for the help Jeff!

After making initial contact with an http server, we have to decide if the server supports smart-http, and if so, which version. Our rules are a bit inconsistent:

1. For v0, we require that the content-type indicates a smart-http response. We also require the response to look vaguely like a pkt-line starting with "#". If one of those does not match, we fall back to dumb-http.

But according to our http protocol spec[1]:

Dumb servers MUST NOT return a return type starting with application/x-git-.

If we see the expected content-type, we should consider it smart-http. At that point we can parse the pkt-line for real, and complain if it is not syntactically valid.

2. For v2, we do not actually check the content-type. Our v2 protocol spec says[2]:

When using the http:// or https:// transport a client makes a "smart" info/refs request as described in http-protocol.txt[...]

and the http spec is clear that for a smart-http[3]:

The Content-Type MUST be application/x-$servicename-advertisement.

So it is required according to the spec.

These inconsistencies were easy to miss because of the way the original code was written as an inline conditional. Let's pull it out into its own function for readability, and improve a few things:

- we now predicate the smart/dumb decision entirely on the presence of the correct content-type

- we do a real pkt-line parse before deciding how to proceed (and die if it isn't valid)

- use skip_prefix() for comparing service strings, instead of constructing expected output in a strbuf; this avoids dealing with memory cleanup

Note that this _is_ tightening what the client will allow. It's all according to the spec, but it's possible that other implementations might violate these. However, violating these particular rules seems like an odd choice for a server to make.

[1] Documentation/technical/http-protocol.txt, l. 166-167
[2] Documentation/technical/protocol-v2.txt, l. 63-64
[3] Documentation/technical/http-protocol.txt, l. 247

Helped-by Josh Steadmon <[email protected]>

Signed-off-by Jeff King <[email protected]>

---
 remote-curl.c | 93 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 34 deletions(-)

remote-curl.c +59 -34

index 762a55a75f..dd9bc41aa1 100644
@@ -330,9 +330,65 @@ static int get_protocol_http_header(enum protocol_version version,
 	return 0;
 }
 
+static void check_smart_http(struct discovery *d, const char *service,
+			     struct strbuf *type)
+{
+	char *src_buf;
+	size_t src_len;
+	char *line;
+	const char *p;
+
+	/*
+	 * If we don't see x-$service-advertisement, then it's not smart-http.
+	 * But once we do, we commit to it and assume any other protocol
+	 * violations are hard errors.
+	 */
+	if (!skip_prefix(type->buf, "application/x-", &p) ||
+	    !skip_prefix(p, service, &p) ||
+	    strcmp(p, "-advertisement"))
+		return;
+
+	/*
+	 * "Peek" at the first packet by using a separate buf/len pair; some
+	 * cases below require us leaving the originals intact.
+	 */
+	src_buf = d->buf;
+	src_len = d->len;
+	line = packet_read_line_buf(&src_buf, &src_len, NULL);
+	if (!line)
+		die("invalid server response; expected service, got flush packet");
+
+	if (skip_prefix(line, "# service=", &p) && !strcmp(p, service)) {
+		/*
+		 * The header can include additional metadata lines, up
+		 * until a packet flush marker.  Ignore these now, but
+		 * in the future we might start to scan them.
+		 */
+		while (packet_read_line_buf(&src_buf, &src_len, NULL))
+			;
+
+		/*
+		 * v0 smart http; callers expect us to soak up the
+		 * service and header packets
+		 */
+		d->buf = src_buf;
+		d->len = src_len;
+		d->proto_git = 1;
+
+	} else if (starts_with(line, "version 2")) {
+		/*
+		 * v2 smart http; do not consume version packet, which will
+		 * be handled elsewhere.
+		 */
+		d->proto_git = 1;
+
+	} else {
+		die("invalid server response; got '%s'", line);
+	}
+}
+
 static struct discovery *discover_refs(const char *service, int for_push)
 {
-	struct strbuf exp = STRBUF_INIT;
 	struct strbuf type = STRBUF_INIT;
 	struct strbuf charset = STRBUF_INIT;
 	struct strbuf buffer = STRBUF_INIT;

@@ -405,38 +461,8 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	last->buf_alloc = strbuf_detach(&buffer, &last->len);
 	last->buf = last->buf_alloc;
 
-	strbuf_addf(&exp, "application/x-%s-advertisement", service);
-	if (maybe_smart &&
-	    (5 <= last->len && last->buf[4] == '#') &&
-	    !strbuf_cmp(&exp, &type)) {
-		char *line;
-
-		/*
-		 * smart HTTP response; validate that the service
-		 * pkt-line matches our request.
-		 */
-		line = packet_read_line_buf(&last->buf, &last->len, NULL);
-		if (!line)
-			die("invalid server response; expected service, got flush packet");
-
-		strbuf_reset(&exp);
-		strbuf_addf(&exp, "# service=%s", service);
-		if (strcmp(line, exp.buf))
-			die("invalid server response; got '%s'", line);
-		strbuf_release(&exp);
-
-		/* The header can include additional metadata lines, up
-		 * until a packet flush marker.  Ignore these now, but
-		 * in the future we might start to scan them.
-		 */
-		while (packet_read_line_buf(&last->buf, &last->len, NULL))
-			;
-
-		last->proto_git = 1;
-	} else if (maybe_smart &&
-		   last->len > 5 && starts_with(last->buf + 4, "version 2")) {
-		last->proto_git = 1;
-	}
+	if (maybe_smart)
+		check_smart_http(last, service, &type);
 
 	if (last->proto_git)
 		last->refs = parse_git_refs(last, for_push);

@@ -444,7 +470,6 @@ static struct discovery *discover_refs(const char *service, int for_push)
 		last->refs = parse_info_refs(last);
 
 	strbuf_release(&refs_url);
-	strbuf_release(&exp);
 	strbuf_release(&type);
 	strbuf_release(&charset);
 	strbuf_release(&effective_url);

Signature · 1 line

2.19.1.1636.gc7a073d580

After making initial contact with an http server, we have to decide if the server supports smart-http, and if so, which version. Our rules are a bit inconsistent:
...

- we now predicate the smart/dumb decision entirely on the presence of the correct content-type

- we do a real pkt-line parse before deciding how to proceed (and die if it isn't valid)

- use skip_prefix() for comparing service strings, instead of constructing expected output in a strbuf; this avoids dealing with memory cleanup

Note that this _is_ tightening what the client will allow. It's all according to the spec, but it's possible that other implementations might violate these. However, violating these particular rules seems like an odd choice for a server to make.

[1] Documentation/technical/http-protocol.txt, l. 166-167
[2] Documentation/technical/protocol-v2.txt, l. 63-64
[3] Documentation/technical/http-protocol.txt, l. 247

Helped-by Josh Steadmon <[email protected]>

Signed-off-by Jeff King <[email protected]>

---
 remote-curl.c | 93 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 34 deletions(-)

Quoted 24 lines · Jeff King

After making initial contact with an http server, we have to decide if the server supports smart-http, and if so, which version. Our rules are a bit inconsistent:
...

- we now predicate the smart/dumb decision entirely on the presence of the correct content-type

- we do a real pkt-line parse before deciding how to proceed (and die if it isn't valid)

- use skip_prefix() for comparing service strings, instead of constructing expected output in a strbuf; this avoids dealing with memory cleanup

Note that this _is_ tightening what the client will allow. It's all according to the spec, but it's possible that other implementations might violate these. However, violating these particular rules seems like an odd choice for a server to make.

[1] Documentation/technical/http-protocol.txt, l. 166-167
[2] Documentation/technical/protocol-v2.txt, l. 63-64
[3] Documentation/technical/http-protocol.txt, l. 247

Quoted 5 lines

Helped-by Josh Steadmon <[email protected]>

Signed-off-by Jeff King <[email protected]>

---
 remote-curl.c | 93 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 34 deletions(-)

It turns out that this has interactions with 01f9ec64 ("Use packet_reader instead of packet_read_line", 2018-12-29) on the ms/packet-err-check branch. Can we get this rebased on top of a more recent 'master'?

After making initial contact with an http server, we have to decide if the server supports smart-http, and if so, which version. Our rules are a bit inconsistent:

1. For v0, we require that the content-type indicates a smart-http response. We also require the response to look vaguely like a pkt-line starting with "#". If one of those does not match, we fall back to dumb-http.

But according to our http protocol spec[1]:

Dumb servers MUST NOT return a return type starting with application/x-git-.

If we see the expected content-type, we should consider it smart-http. At that point we can parse the pkt-line for real, and complain if it is not syntactically valid.

2. For v2, we do not actually check the content-type. Our v2 protocol spec says[2]:

When using the http:// or https:// transport a client makes a "smart" info/refs request as described in http-protocol.txt[...]

and the http spec is clear that for a smart-http response[3]:

The Content-Type MUST be application/x-$servicename-advertisement.

So it is required according to the spec.

These inconsistencies were easy to miss because of the way the original code was written as an inline conditional. Let's pull it out into its own function for readability, and improve a few things:

- we now predicate the smart/dumb decision entirely on the presence of the correct content-type

- we do a real pkt-line parse before deciding how to proceed (and die if it isn't valid)

- use skip_prefix() for comparing service strings, instead of constructing expected output in a strbuf; this avoids dealing with memory cleanup

Note that this _is_ tightening what the client will allow. It's all according to the spec, but it's possible that other implementations might violate these. However, violating these particular rules seems like an odd choice for a server to make.

[1] Documentation/technical/http-protocol.txt, l. 166-167
[2] Documentation/technical/protocol-v2.txt, l. 63-64
[3] Documentation/technical/http-protocol.txt, l. 247

Helped-by Josh Steadmon <[email protected]>

Signed-off-by Jeff King <[email protected]>

---
 remote-curl.c | 100 ++++++++++++++++++++++++++++----------------------
 1 file changed, 57 insertions(+), 43 deletions(-)

remote-curl.c +11 -0

index 2e04d53ac8..c78ba83744 100644
@@ -331,9 +331,63 @@ static int get_protocol_http_header(enum protocol_version version,
 	return 0;
 }
 
+static void check_smart_http(struct discovery *d, const char *service,
+			     struct strbuf *type)
+{
+	const char *p;
+	struct packet_reader reader;
+
+	/*
+	 * If we don't see x-$service-advertisement, then it's not smart-http.
+	 * But once we do, we commit to it and assume any other protocol
+	 * violations are hard errors.
+	 */

remote-curl.c +23 -43

+
+		/*
+		 * v0 smart http; callers expect us to soak up the
+		 * service and header packets
+		 */
+		d->buf = reader.src_buffer;
+		d->len = reader.src_len;
+		d->proto_git = 1;
+
+	} else if (starts_with(reader.line, "version 2")) {
+		/*
+		 * v2 smart http; do not consume version packet, which will
+		 * be handled elsewhere.
+		 */
+		d->proto_git = 1;
+
+	} else {
+		die("invalid server response; got '%s'", reader.line);
+	}
+}
+
 static struct discovery *discover_refs(const char *service, int for_push)
 {
-	struct strbuf exp = STRBUF_INIT;
 	struct strbuf type = STRBUF_INIT;
 	struct strbuf charset = STRBUF_INIT;
 	struct strbuf buffer = STRBUF_INIT;

@@ -405,47 +459,8 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	last->buf_alloc = strbuf_detach(&buffer, &last->len);
 	last->buf = last->buf_alloc;
 
-	strbuf_addf(&exp, "application/x-%s-advertisement", service);
-	if (maybe_smart &&
-	    (5 <= last->len && last->buf[4] == '#') &&
-	    !strbuf_cmp(&exp, &type)) {
-		struct packet_reader reader;
-		packet_reader_init(&reader, -1, last->buf, last->len,
-				   PACKET_READ_CHOMP_NEWLINE |
-				   PACKET_READ_DIE_ON_ERR_PACKET);
-
-		/*
-		 * smart HTTP response; validate that the service
-		 * pkt-line matches our request.
-		 */
-		if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
-			die("invalid server response; expected service, got flush packet");
-
-		strbuf_reset(&exp);
-		strbuf_addf(&exp, "# service=%s", service);
-		if (strcmp(reader.line, exp.buf))
-			die("invalid server response; got '%s'", reader.line);
-		strbuf_release(&exp);
-
-		/* The header can include additional metadata lines, up
-		 * until a packet flush marker.  Ignore these now, but
-		 * in the future we might start to scan them.
-		 */
-		for (;;) {
-			packet_reader_read(&reader);
-			if (reader.pktlen <= 0) {
-				break;
-			}
-		}
-
-		last->buf = reader.src_buffer;
-		last->len = reader.src_len;
-
-		last->proto_git = 1;
-	} else if (maybe_smart &&
-		   last->len > 5 && starts_with(last->buf + 4, "version 2")) {
-		last->proto_git = 1;
-	}
+	if (maybe_smart)
+		check_smart_http(last, service, &type);
 
 	if (last->proto_git)
 		last->refs = parse_git_refs(last, for_push);

@@ -453,7 +468,6 @@ static struct discovery *discover_refs(const char *service, int for_push)
 		last->refs = parse_info_refs(last);
 
 	strbuf_release(&refs_url);
-	strbuf_release(&exp);
 	strbuf_release(&type);
 	strbuf_release(&charset);
 	strbuf_release(&effective_url);

Signature · 1 line

2.20.1.1122.g2972e48916

remote-curl.c static int get_protocol_http_header(enum protocol_version version, · line 354

Quoted 5 lines

remote-curl.c +5 -0

+	packet_reader_init(&reader, -1, d->buf, d->len,
+			   PACKET_READ_CHOMP_NEWLINE |
+			   PACKET_READ_DIE_ON_ERR_PACKET);
+	if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
+		die("invalid server response; expected service, got flush packet");

This can also trigger on an EOF or a delim packet, should we clarify the error message?

remote-curl.c static int get_protocol_http_header(enum protocol_version version, · line 367

Quoted 13 lines

remote-curl.c +13 -0

+
+	if (skip_prefix(reader.line, "# service=", &p) && !strcmp(p, service)) {
+		/*
+		 * The header can include additional metadata lines, up
+		 * until a packet flush marker.  Ignore these now, but
+		 * in the future we might start to scan them.
+		 */
+		for (;;) {
+			packet_reader_read(&reader);
+			if (reader.pktlen <= 0) {
+				break;
+			}
+		}

Could we make this loop cleaner as:

while (packet_reader_read(&reader) != PACKET_READ_NORMAL)
;

remote-curl.c static int get_protocol_http_header(enum protocol_version version, · line 354

Quoted 5 lines

remote-curl.c +5 -0

+	packet_reader_init(&reader, -1, d->buf, d->len,
+			   PACKET_READ_CHOMP_NEWLINE |
+			   PACKET_READ_DIE_ON_ERR_PACKET);
+	if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
+		die("invalid server response; expected service, got flush packet");

This can also trigger on an EOF or a delim packet, should we clarify the error message?

remote-curl.c static int get_protocol_http_header(enum protocol_version version, · line 367

Quoted 12 lines

remote-curl.c +12 -0

+	if (skip_prefix(reader.line, "# service=", &p) && !strcmp(p, service)) {
+		/*
+		 * The header can include additional metadata lines, up
+		 * until a packet flush marker.  Ignore these now, but
+		 * in the future we might start to scan them.
+		 */
+		for (;;) {
+			packet_reader_read(&reader);
+			if (reader.pktlen <= 0) {
+				break;
+			}
+		}

Could we make this loop cleaner as:

while (packet_reader_read(&reader) != PACKET_READ_NORMAL)
;

Yep. Here it is.

Rather than a range-diff, which is quite large due to the code movement, I'll include below the interesting hunk of a diff between the two endpoints (i.e., what we would have seen applying the packet-err-check changes on top of my code movement, which is more or less what I did to generate it).

In a v2 smart-http conversation, the server should reply to our initial request with a pkt-line saying "version 2" (this is the start of the "capabilities advertisement"). We check for the string using starts_with(), but that's overly permissive (it would match "version 20", for example).

Let's tighten this check to use strcmp(). Note that we don't need to worry about a trailing newline here, because the ptk-line code will have chomped it for us already.

Signed-off-by Jeff King <[email protected]>

---
 remote-curl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

remote-curl.c +1 -1

index dd9bc41aa1..3c9c4a07c3 100644
@@ -375,7 +375,7 @@ static void check_smart_http(struct discovery *d, const char *service,
 		d->len = src_len;
 		d->proto_git = 1;
 
-	} else if (starts_with(line, "version 2")) {
+	} else if (!strcmp(line, "version 2")) {
 		/*
 		 * v2 smart http; do not consume version packet, which will
 		 * be handled elsewhere.

Signature · 1 line

2.19.1.1636.gc7a073d580

Amusingly, this does break the test you just added, because it tries to issue an ERR after claiming "text/html" (and after my patch, we correctly fall back to dumb-http).

From: Masaya Suzuki <[email protected]>

In the Git pack protocol definition, an error packet may appear only in a certain context. However, servers can face a runtime error (e.g. I/O error) at an arbitrary timing. This patch changes the protocol to allow an error packet to be sent instead of any packet.

Following this protocol spec change, the error packet handling code is moved to pkt-line.c.

pkt-line.c +0 -0

index 04d10bbd03..ce9e42d10e 100644

Quoted 7 lines · Josh Steadmon

From: Masaya Suzuki <[email protected]>

In the Git pack protocol definition, an error packet may appear only in a certain context. However, servers can face a runtime error (e.g. I/O error) at an arbitrary timing. This patch changes the protocol to allow an error packet to be sent instead of any packet.

Quoted 2 lines

Following this protocol spec change, the error packet handling code is moved to pkt-line.c.

This is a change in the spec with an accompanying change in the code, which raises the question: what do other implementations do with this change (both older Git, and implementations like JGit, libgit2, etc)?

I think the answer for older Git is "hang up unceremoniously", which is probably OK given the semantics of the change. And I'd suspect most other implementations would do the same. I just wonder if anybody tested it with other implementations.

Documentation/technical/pack-protocol.txt protocol-common.txt. When the grammar indicate `PKT-LINE(...)`, unless · line 33

Quoted 10 lines

Documentation/technical/pack-protocol.txt +9 -0

+An error packet is a special pkt-line that contains an error string.
+
+----
+  error-line     =  PKT-LINE("ERR" SP explanation-text)
+----
+
+Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY
+be sent. Once this packet is sent by a client or a server, the data transfer
+process defined in this protocol is terminated.

The packfile data is typically packetized, too, and contains arbitrary data (that could have "ERR" in it). It looks like we don't specifically say PKT-LINE() in that part of the protocol spec, though, so I think this is OK.

Likewise, in the implementation:

Quoted 5 lines

pkt-line.c +0 -0

index 04d10bbd03..ce9e42d10e 100644

pkt-line.c enum packet_read_status packet_read_with_status(int fd, char **src_buffer, · line 355

Quoted 10 lines

pkt-line.c +4 -0

@@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
              return PACKET_READ_EOF;
      }
 
+     if (starts_with(buffer, "ERR ")) {
+             die(_("remote error: %s"), buffer + 4);
+     }
+
      if ((options & PACKET_READ_CHOMP_NEWLINE) &&
          len && buffer[len-1] == '\n')
              len--;

This ERR handling has been moved to a very low level. What happens if we're passing arbitrary data via the packet_read() code? Could we erroneously trigger an error if a packfile happens to have the bytes "ERR " at a packet boundary?

For packfiles via upload-pack, I _think_ we're OK, because we only packetize it when a sideband is in use. In which case this would never match, because we'd have "\1" in the first byte slot.

But are there are other cases we need to worry about? Just brainstorming, I can think of:

1. We also pass packetized packfiles between git-remote-https and the stateless-rpc mode of fetch-pack/send-pack. And I don't think we use sidebands there.

2. The packet code is used for long-lived clean/smudge filters these days, which also pass arbitrary data.

So I think it's probably not a good idea to unconditionally have callers of packet_read_with_status() handle this. We'd need a flag like PACKET_READ_RESPECT_ERR, and to trigger it from the appropriate callers.

-Peff

Quoted 3 lines

This is a change in the spec with an accompanying change in the code, which raises the question: what do other implementations do with this change (both older Git, and implementations like JGit, libgit2, etc)?

JGit is similar to Git. It parses "ERR " in limited places. When it sees an ERR packet in an unexpected place, it'll fail somewhere in the parsing code.

https://github.com/eclipse/jgit/blob/30c6c7542190c149e2aee792f992a312a5fc5793/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java#L145-L147
https://github.com/eclipse/jgit/blob/f40b39345cd9b54473ee871bff401fe3d394ffe3/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java#L208

I'm not familiar with libgit2 code, but it seems it handles this at a lower level. An error type packet is parsed out at a low level, and the error handling is done by the callers of the packet parser.

https://github.com/libgit2/libgit2/blob/bea65980c7a42e34edfafbdc40b199ba7b2a564e/src/transports/smart_pkt.c#L482-L483

I cannot find an ERR packet handling in go-git. It seems to me that if an ERR packet appears it treats it as a parsing error.

https://github.com/src-d/go-git/blob/master/plumbing/protocol/packp/common.go#L60-L62

Quoted 4 lines

I think the answer for older Git is "hang up unceremoniously", which is probably OK given the semantics of the change. And I'd suspect most other implementations would do the same. I just wonder if anybody tested it with other implementations.

I'm thinking aloud here. There would be two aspects of the protocol
compatibility: (1) new clients speak to old servers (2) old clients speak to a new server that speaks the updated protocol.

For (1), I assume that in the Git pack protocol, a packet starting from "ERR " does not appear naturally except for a very special case that the server doesn't support sideband, but using the updated protocol. As you mentioned, at first it looks like this can mistakenly parse the pack file of git-receive-pack as an ERR packet, assuming that git-receive-pack's pack file is packetized. Actually git-receive-pack's pack file is not packetized in the Git pack protocol (https://github.com/git/git/blob/master/builtin/receive-pack.c#L1695).
I recently wrote a Git protocol parser
(https://github.com/google/gitprotocolio), and I confirmed that this is the case at least for the HTTP transport. git-upload-pack's pack file is indeed packetized, but packetized with sideband. Except for the case where sideband is not used, the packfiles wouldn't be considered as an ERR packet accidentally.

For (2), if the old clients see an unexpected ERR packet, they cannot parse it. They would handle this unparsable data as if the server is not speaking Git protocol correctly. Even if the old clients just ignore the packet, due to the nature of the ERR packet, the server won't send further data. The client won't be able to proceed. Overall, the clients anyway face an error, and the only difference would be whether the clients can show an error nicely or not. The new clients will show the errors nicely to users. Old clients will not.

Quoted 3 lines

So I think it's probably not a good idea to unconditionally have callers of packet_read_with_status() handle this. We'd need a flag like PACKET_READ_RESPECT_ERR, and to trigger it from the appropriate callers.

This is outside of the Git pack protocol so having a separate parsing mode makes sense to me.

Quoted 19 lines · Jeff King

This ERR handling has been moved to a very low level. What happens if we're passing arbitrary data via the packet_read() code? Could we erroneously trigger an error if a packfile happens to have the bytes "ERR " at a packet boundary?

For packfiles via upload-pack, I _think_ we're OK, because we only packetize it when a sideband is in use. In which case this would never match, because we'd have "\1" in the first byte slot.

But are there are other cases we need to worry about? Just brainstorming, I can think of:

1. We also pass packetized packfiles between git-remote-https and the stateless-rpc mode of fetch-pack/send-pack. And I don't think we use sidebands there.

2. The packet code is used for long-lived clean/smudge filters these days, which also pass arbitrary data.

Quoted 3 lines

So I think it's probably not a good idea to unconditionally have callers of packet_read_with_status() handle this. We'd need a flag like PACKET_READ_RESPECT_ERR, and to trigger it from the appropriate callers.

This is outside of the Git pack protocol so having a separate parsing mode makes sense to me.

Quoted 24 lines · Masaya Suzuki

Quoted 22 lines · Jeff King

This ERR handling has been moved to a very low level. What happens if we're passing arbitrary data via the packet_read() code? Could we erroneously trigger an error if a packfile happens to have the bytes "ERR " at a packet boundary?

For packfiles via upload-pack, I _think_ we're OK, because we only packetize it when a sideband is in use. In which case this would never match, because we'd have "\1" in the first byte slot.

But are there are other cases we need to worry about? Just brainstorming, I can think of:

1. We also pass packetized packfiles between git-remote-https and the stateless-rpc mode of fetch-pack/send-pack. And I don't think we use sidebands there.

2. The packet code is used for long-lived clean/smudge filters these days, which also pass arbitrary data.

So I think it's probably not a good idea to unconditionally have callers of packet_read_with_status() handle this. We'd need a flag like PACKET_READ_RESPECT_ERR, and to trigger it from the appropriate callers.

Quoted 2 lines

This is outside of the Git pack protocol so having a separate parsing mode makes sense to me.

This sounds like it could be a significant refactoring. Should we go back to V2 of this series, and then work on the new parsing mode separately?

Quoted 28 lines · Josh Steadmon

Quoted 26 lines · Masaya Suzuki

Quoted 16 lines · Jeff King

This ERR handling has been moved to a very low level. What happens if we're passing arbitrary data via the packet_read() code? Could we erroneously trigger an error if a packfile happens to have the bytes "ERR " at a packet boundary?

For packfiles via upload-pack, I _think_ we're OK, because we only packetize it when a sideband is in use. In which case this would never match, because we'd have "\1" in the first byte slot.

But are there are other cases we need to worry about? Just brainstorming, I can think of:

1. We also pass packetized packfiles between git-remote-https and the stateless-rpc mode of fetch-pack/send-pack. And I don't think we use sidebands there.

Quoted 6 lines

2. The packet code is used for long-lived clean/smudge filters these days, which also pass arbitrary data.

So I think it's probably not a good idea to unconditionally have callers of packet_read_with_status() handle this. We'd need a flag like PACKET_READ_RESPECT_ERR, and to trigger it from the appropriate callers.

This is outside of the Git pack protocol so having a separate parsing mode makes sense to me.

Quoted 3 lines

This sounds like it could be a significant refactoring. Should we go back to V2 of this series, and then work on the new parsing mode separately?

Which one is v2? :)

Just the remote-curl cleanups from me, and then your "die on server-side errors" patch?

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

This is outside of the Git pack protocol so having a separate parsing mode makes sense to me.

This sounds like it could be a significant refactoring. Should we go back to V2 of this series, and then work on the new parsing mode separately?

Quoted 4 lines

Which one is v2? :)

Just the remote-curl cleanups from me, and then your "die on server-side errors" patch?

Yes, that one :)

This is a reroll of js/smart-http-detect-remote-error that also includes a fixed version of ms/proto-err-packet-anywhere [1].

The first patch clarifies the use of ERR messages in the pkt-line protocol and unifies error handling in pkt-line.c

The second patch refactors smart-http discovery in remote-curl.c. Among other improvements, it makes more use of the pkt-line functions, which allows us to catch remote errors that were previously ignored.

The third patch makes the version check in remote-curl more strict.

The final patch adds a test to verify that the fix in patch #2 does actually catch remote HTTP errors.

[1]: https://public-inbox.org/git/[email protected]/

Jeff King (2):
remote-curl: refactor smart-http discovery
remote-curl: tighten "version 2" check for smart-http

Josh Steadmon (1):
lib-httpd, t5551: check server-side HTTP errors

Masaya Suzuki (1):
pack-protocol.txt: accept error packets in any context

 Documentation/technical/pack-protocol.txt | 20 ++---
 builtin/archive.c                         |  2 -
 connect.c                                 |  3 -
 fetch-pack.c                              |  2 -
 pkt-line.c                                |  4 +
 remote-curl.c                             | 93 ++++++++++++++---------
 t/lib-httpd.sh                            |  1 +
 t/lib-httpd/apache.conf                   |  4 +
 t/lib-httpd/error-smart-http.sh           |  3 +
 t/t5551-http-fetch-smart.sh               |  5 ++
 t/t5703-upload-pack-ref-in-want.sh        |  4 +-
 11 files changed, 89 insertions(+), 52 deletions(-)
 create mode 100644 t/lib-httpd/error-smart-http.sh

When a smart HTTP server sends an error message via pkt-line, remote-curl will fail to detect the error (which usually results in incorrectly falling back to dumb-HTTP mode).

This patch adds a check in discover_refs() for server-side error messages, as well as a test case for this issue.

Quoted 3 lines · [email protected] writes:

When a smart HTTP server sends an error message via pkt-line, remote-curl will fail to detect the error (which usually results in incorrectly falling back to dumb-HTTP mode).

OK, that is a valid thing to worry about.

Quoted 1 line

Quoted 2 lines

This patch adds a check in discover_refs() for server-side error messages, as well as a test case for this issue.

This makes me wonder if discoer_refs() is the only place where we ought to be checking for ERR packets but we are not. Are there other places that we also need a similar fix?