Re: [PATCH] Verify Content-Type from smart HTTP servers

Jeff King [off-list ref] writes:

Should this be "From:" Shawn? The tone of the message and the S-O-B
order makes it look like it.

Yes. I should have left that line when edited the format-patch
output in my MUA to say I was resending something that vger rejected
and people did not see after tweaking the patch to slip their taboo
list.

quoted

@@ -133,16 +135,19 @@ static struct discovery* discover_refs(const char *service)
 	last->buf = last->buf_alloc;
 
 	if (maybe_smart && 5 <= last->len && last->buf[4] == '#') {
-		/* smart HTTP response; validate that the service
+		/*
+		 * smart HTTP response; validate that the service
 		 * pkt-line matches our request.
 		 */
-		struct strbuf exp = STRBUF_INIT;
-
+		strbuf_addf(&exp, "application/x-%s-advertisement", service);
+		if (strbuf_cmp(&exp, &type))
+			die("invalid content-type %s", type.buf);

Hmm. I wondered if it is possible for a non-smart server to send us down
this code path, which would now complain of the bogus content-type.
Something like an info/refs file with:

  # 1
  # the comment above is meaningless, but puts a "#" at position 4.

But I note that we would already die in the next line:

quoted

 		if (packet_get_line(&buffer, &last->buf, &last->len) <= 0)
 			die("%s has invalid packet header", refs_url);

so I do not think the patch makes anything worse. However, should we
take this opportunity to make the "did we get a smart response" test
more robust? That is, should we actually be checking the content-type
in the outer conditional, and going down the smart code-path if it is
application/x-%s-advertisement, and otherwise treating the result as
dumb?

Does the outer caller that switches between dumb and smart actually
know what service type it is requesting (I am not familiar with the
callchain involved)?  Even if it doesn't, it may still make sense.

As a side note, should we (can we) care about the content-type for dumb
http? It should probably be text/plain or application/octet-stream, but
I would not be surprised if we get a variety of random junk in the real
world, though.

The design objective of dumb http protocol was to allow working with
any dumb bit transfer thing, so I'd prefer to keep it lenient and
allow application/x-git-loose-object-file and somesuch.

Thanks.

On Fri, Feb 01, 2013 at 10:09:26AM -0800, Junio C Hamano wrote:

quoted

so I do not think the patch makes anything worse. However, should we
take this opportunity to make the "did we get a smart response" test
more robust? That is, should we actually be checking the content-type
in the outer conditional, and going down the smart code-path if it is
application/x-%s-advertisement, and otherwise treating the result as
dumb?

Does the outer caller that switches between dumb and smart actually
know what service type it is requesting (I am not familiar with the
callchain involved)?  Even if it doesn't, it may still make sense.

I was specifically thinking of this (on top of your patch):

diff --git a/remote-curl.c b/remote-curl.c
index e6f3b63..63680a8 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -134,14 +134,12 @@ static struct discovery* discover_refs(const char *service)
 	last->buf_alloc = strbuf_detach(&buffer, &last->len);
 	last->buf = last->buf_alloc;
 
-	if (maybe_smart && 5 <= last->len && last->buf[4] == '#') {
+	strbuf_addf(&exp, "application/x-%s-advertisement", service);
+	if (maybe_smart && !strbuf_cmp(&exp, &type)) {
 		/*
 		 * smart HTTP response; validate that the service
 		 * pkt-line matches our request.
 		 */
-		strbuf_addf(&exp, "application/x-%s-advertisement", service);
-		if (strbuf_cmp(&exp, &type))
-			die("invalid content-type %s", type.buf);
 		if (packet_get_line(&buffer, &last->buf, &last->len) <= 0)
 			die("%s has invalid packet header", refs_url);
 		if (buffer.len && buffer.buf[buffer.len - 1] == '\n')

To just follow the dumb path if we don't get the content-type we expect.
We may want to keep the '#' format check in addition (packet_get_line
will check it and die, anyway, but we may want to drop back to
considering it dumb, just to protect against a badly configured dumb
server which uses our mime type, but I do not think it likely).

quoted

As a side note, should we (can we) care about the content-type for dumb
http? It should probably be text/plain or application/octet-stream, but
I would not be surprised if we get a variety of random junk in the real
world, though.

The design objective of dumb http protocol was to allow working with
any dumb bit transfer thing, so I'd prefer to keep it lenient and
allow application/x-git-loose-object-file and somesuch.

Yeah, I do not think it really buys us anything in practice, and we have
no way of knowing what kind of crap is in the wild. Not worth it.

-Peff