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

[Jeff King <peff@peff.net>]

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

> > 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).

> > 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