[PATCH] http: inform about alternates-as-redirects behavior

[PATCH] http: inform about alternates-as-redirects behavior

[Eric Wong]

It is disconcerting for users to not notice the behavior
change in handling alternates from commit cb4d2d35c4622ec2
("http: treat http-alternates like redirects")

Give the user a hint about the config option so they can
see the URL and decide whether or not they want to enable
http.followRedirects in their config.

Signed-off-by: Eric Wong <e@80x24.org>
---
 http-walker.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/http-walker.c b/http-walker.c
index b34b6ace7..626badfe6 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -168,6 +168,12 @@ static int is_alternate_allowed(const char *url)
 	};
 	int i;
 
+	if (http_follow_config != HTTP_FOLLOW_ALWAYS) {
+		warning("alternate disabled by http.followRedirects!=true: %s",
+			url);
+		return 0;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(protocols); i++) {
 		const char *end;
 		if (skip_prefix(url, protocols[i], &end) &&
@@ -331,9 +337,6 @@ static void fetch_alternates(struct walker *walker, const char *base)
 	struct alternates_request alt_req;
 	struct walker_data *cdata = walker->data;
 
-	if (http_follow_config != HTTP_FOLLOW_ALWAYS)
-		return;
-
 	/*
 	 * If another request has already started fetching alternates,
 	 * wait for them to arrive and return to processing this request's
-- 
EW

Re: [PATCH] http: inform about alternates-as-redirects behavior

[Jeff King]

On Sat, Mar 04, 2017 at 01:35:04AM +0000, Eric Wong wrote:

> It is disconcerting for users to not notice the behavior
> change in handling alternates from commit cb4d2d35c4622ec2
> ("http: treat http-alternates like redirects")
> 
> Give the user a hint about the config option so they can
> see the URL and decide whether or not they want to enable
> http.followRedirects in their config.

Yeah, I agree it makes sense to notify the user.

> diff --git a/http-walker.c b/http-walker.c
> index b34b6ace7..626badfe6 100644
> --- a/http-walker.c
> +++ b/http-walker.c
> @@ -168,6 +168,12 @@ static int is_alternate_allowed(const char *url)
>  	};
>  	int i;
>  
> +	if (http_follow_config != HTTP_FOLLOW_ALWAYS) {
> +		warning("alternate disabled by http.followRedirects!=true: %s",
> +			url);
> +		return 0;
> +	}
> +
>  	for (i = 0; i < ARRAY_SIZE(protocols); i++) {
>  		const char *end;
>  		if (skip_prefix(url, protocols[i], &end) &&
> @@ -331,9 +337,6 @@ static void fetch_alternates(struct walker *walker, const char *base)
>  	struct alternates_request alt_req;
>  	struct walker_data *cdata = walker->data;
>  
> -	if (http_follow_config != HTTP_FOLLOW_ALWAYS)
> -		return;
> -

I was surprised from the description to see not just the addition of a
warning, but a movement of the enforcement code.

I think it's necessary because the original did not bother even fetching
http-alternates if we were not going to respect it. Whereas the new code
will fetch and parse it, and warn only if we actually found something in
it. Which seems reasonable.

The warning itself:

> +		warning("alternate disabled by http.followRedirects!=true: %s",

feels like it could use some whitespace around the "!=", but maybe
that's just me.

-Peff

Re: [PATCH] http: inform about alternates-as-redirects behavior

[Jeff King]

On Fri, Mar 03, 2017 at 10:13:14PM -0500, Jeff King wrote:

> > -	if (http_follow_config != HTTP_FOLLOW_ALWAYS)
> > -		return;
> > -
> 
> I was surprised from the description to see not just the addition of a
> warning, but a movement of the enforcement code.
> 
> I think it's necessary because the original did not bother even fetching
> http-alternates if we were not going to respect it. Whereas the new code
> will fetch and parse it, and warn only if we actually found something in
> it. Which seems reasonable.

One side effect of this is that it exposes[1] the http-alternates
parsing code to the server's input, even if we aren't planning on using
the result. That code is not very well audited. Just looking at the
context from your patches, I noticed one obvious memory access problem.
The fix is below.

-Peff

[1] Obviously this same code was exposed prior to the redirect-limiting
    patches, so it's not like there aren't tons of older clients that
    exhibit the same behavior. But IMHO one of the beneficial side
    effects of the redirect-limiting was that it avoided this largely
    unused and untested code entirely.

-- >8 --
Subject: http-walker: fix buffer underflow processing remote alternates

If we parse a remote alternates (or http-alternates) file,
we expect relative lines like:

  ../../foo.git/objects

which we convert into "$URL/../foo.git/" (and then use that
as a base for fetching more objects).

But if the remote feeds us nonsense like just:

  ../

we will try to blindly strip the last 7 characters, assuming
they contain the string "objects". Since we don't _have_ 7
characters at all, this results in feeding a small negative
value to strbuf_add(), which converts it to a size_t,
resulting in a big positive value. This should consistently
fail (since we can't generally allocate the max size_t minus
7 bytes), so there shouldn't be any security implications
(and even if we did allocate, we'd just copy in gigabytes of
garbage, not overflow a buffer).

Let's fix this by using strbuf_strip_suffix() to drop the
characters we want. As a bonus this lets us handle names
that do not end in "objects" (all git repos do, but there is
nothing to say that an alternate object store needs to be a
git repo).

And while we're here, we can add a few other parsing
niceties, like dropping trailing whitespace, and handling
names that do not end in "/".

Signed-off-by: Jeff King <peff@peff.net>
---
 http-walker.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/http-walker.c b/http-walker.c
index b34b6ace7..d62ca8953 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -296,11 +296,13 @@ static void process_alternates_response(void *callback_data)
 					okay = 1;
 				}
 			}
-			/* skip "objects\n" at end */
 			if (okay) {
 				struct strbuf target = STRBUF_INIT;
 				strbuf_add(&target, base, serverlen);
-				strbuf_add(&target, data + i, posn - i - 7);
+				strbuf_add(&target, data + i, posn - i);
+				strbuf_rtrim(&target);
+				strbuf_strip_suffix(&target, "objects");
+				strbuf_complete(&target, '/');
 
 				if (is_alternate_allowed(target.buf)) {
 					warning("adding alternate object store: %s",
-- 
2.12.0.404.g442e75cca

Re: [PATCH] http: inform about alternates-as-redirects behavior

[Eric Wong]

Jeff King <peff@peff.net> wrote:
> The warning itself:
> 
> > +		warning("alternate disabled by http.followRedirects!=true: %s",
> 
> feels like it could use some whitespace around the "!=", but maybe
> that's just me.

Yeah, I kinda wanted to emulate the command-line syntax.

Maybe rewording it a bit and showing how to enable it will
make more sense:

		warning("alternate: %s", url);
		warning(" may be enabled by -c http.followRedirects=true");

As well as keeping individual lines shorter and hopefully
easier-to-read.

Re: [PATCH] http: inform about alternates-as-redirects behavior

[Jeff King]

On Sat, Mar 04, 2017 at 06:55:48AM +0000, Eric Wong wrote:

> Jeff King <peff@peff.net> wrote:
> > The warning itself:
> > 
> > > +		warning("alternate disabled by http.followRedirects!=true: %s",
> > 
> > feels like it could use some whitespace around the "!=", but maybe
> > that's just me.
> 
> Yeah, I kinda wanted to emulate the command-line syntax.
> 
> Maybe rewording it a bit and showing how to enable it will
> make more sense:
> 
> 		warning("alternate: %s", url);
> 		warning(" may be enabled by -c http.followRedirects=true");

I kind of hoped people would look at the documentation for
followRedirects before blindly enabling it. Though I guess the
documentation doesn't really explain the possible security implications,
so maybe it doesn't matter (and they're pretty subtle anyway).

-Peff

[PATCH v2] http: inform about alternates-as-redirects behavior

[Eric Wong]

Jeff King <peff@peff.net> wrote:
> On Sat, Mar 04, 2017 at 06:55:48AM +0000, Eric Wong wrote:
> > Jeff King <peff@peff.net> wrote:
> > > The warning itself:
> > > 
> > > > +		warning("alternate disabled by http.followRedirects!=true: %s",
> > > 
> > > feels like it could use some whitespace around the "!=", but maybe
> > > that's just me.
> > 
> > Yeah, I kinda wanted to emulate the command-line syntax.
> > 
> > Maybe rewording it a bit and showing how to enable it will
> > make more sense:
> > 
> > 		warning("alternate: %s", url);
> > 		warning(" may be enabled by -c http.followRedirects=true");
> 
> I kind of hoped people would look at the documentation for
> followRedirects before blindly enabling it. Though I guess the
> documentation doesn't really explain the possible security implications,
> so maybe it doesn't matter (and they're pretty subtle anyway).

You bring up a good point, perhaps just mentioning the config
key is enough to convince somebody to (v2 below).


I also think the security implications for relative alternates
on the same host would not matter, since the smart HTTP will
take them into account on the server side.

Perhaps we give http_follow_config ORable flags:

	HTTP_FOLLOW_NONE = 0,
	HTTP_FOLLOW_INITIAL = 0x1,
	HTTP_FOLLOW_RELATIVE = 0x2,
	HTTP_FOLLOW_ABSOLUTE = 0x4,
	HTTP_FOLLOW_ALWAYS = 0x7,

With the default would being: HTTP_FOLLOW_INITIAL|HTTP_FOLLOW_RELATIVE
(but I suppose that's a patch for another time)

----------8<-----------
From: Eric Wong <e@80x24.org>
Subject: [PATCH] http: inform about alternates-as-redirects behavior

It is disconcerting for users to not notice the behavior
change in handling alternates from commit cb4d2d35c4622ec2
("http: treat http-alternates like redirects")

Give the user a hint about the config option so they can
see the URL and decide whether or not they want to enable
http.followRedirects in their config.

Signed-off-by: Eric Wong <e@80x24.org>
---
 http-walker.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/http-walker.c b/http-walker.c
index b34b6ace7..6396cebe5 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -168,6 +168,11 @@ static int is_alternate_allowed(const char *url)
 	};
 	int i;
 
+	if (http_follow_config != HTTP_FOLLOW_ALWAYS) {
+		warning("alternate disabled by http.followRedirects: %s", url);
+		return 0;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(protocols); i++) {
 		const char *end;
 		if (skip_prefix(url, protocols[i], &end) &&
@@ -331,9 +336,6 @@ static void fetch_alternates(struct walker *walker, const char *base)
 	struct alternates_request alt_req;
 	struct walker_data *cdata = walker->data;
 
-	if (http_follow_config != HTTP_FOLLOW_ALWAYS)
-		return;
-
 	/*
 	 * If another request has already started fetching alternates,
 	 * wait for them to arrive and return to processing this request's
-- 
EW

Re: [PATCH v2] http: inform about alternates-as-redirects behavior

[Jeff King]

On Sat, Mar 04, 2017 at 08:36:45AM +0000, Eric Wong wrote:

> I also think the security implications for relative alternates
> on the same host would not matter, since the smart HTTP will
> take them into account on the server side.

It depends on the host whether all of the repos on it have the same
security domain or not. A site like github.com hosts both public and
private repositories, and you do not want a public repo redirecting to
the private one to get objects.

Of course, that depends on untrusted users being able to configure
server-side alternates, which GitHub certainly would not let you do. I
would hope other multi-user hosting sites behave similarly (most hosting
sites do not seem to allow dumb http at all).

> Perhaps we give http_follow_config ORable flags:
> 
> 	HTTP_FOLLOW_NONE = 0,
> 	HTTP_FOLLOW_INITIAL = 0x1,
> 	HTTP_FOLLOW_RELATIVE = 0x2,
> 	HTTP_FOLLOW_ABSOLUTE = 0x4,
> 	HTTP_FOLLOW_ALWAYS = 0x7,
> 
> With the default would being: HTTP_FOLLOW_INITIAL|HTTP_FOLLOW_RELATIVE
> (but I suppose that's a patch for another time)

I don't have a real problem with breaking it down that way, if somebody
wants to make a patch. Mostly the reason I didn't do so is that I don't
think http-alternates are in common use these days, since smart-http is
much more powerful.

> ----------8<-----------
> From: Eric Wong <e@80x24.org>
> Subject: [PATCH] http: inform about alternates-as-redirects behavior

This v2 looks fine to me.

-Peff

Re: [PATCH v2] http: inform about alternates-as-redirects behavior

[Brandon Williams]

On 03/04, Jeff King wrote:
> On Sat, Mar 04, 2017 at 08:36:45AM +0000, Eric Wong wrote:
> 
> > I also think the security implications for relative alternates
> > on the same host would not matter, since the smart HTTP will
> > take them into account on the server side.
> 
> It depends on the host whether all of the repos on it have the same
> security domain or not. A site like github.com hosts both public and
> private repositories, and you do not want a public repo redirecting to
> the private one to get objects.
> 
> Of course, that depends on untrusted users being able to configure
> server-side alternates, which GitHub certainly would not let you do. I
> would hope other multi-user hosting sites behave similarly (most hosting
> sites do not seem to allow dumb http at all).
> 
> > Perhaps we give http_follow_config ORable flags:
> > 
> > 	HTTP_FOLLOW_NONE = 0,
> > 	HTTP_FOLLOW_INITIAL = 0x1,
> > 	HTTP_FOLLOW_RELATIVE = 0x2,
> > 	HTTP_FOLLOW_ABSOLUTE = 0x4,
> > 	HTTP_FOLLOW_ALWAYS = 0x7,
> > 
> > With the default would being: HTTP_FOLLOW_INITIAL|HTTP_FOLLOW_RELATIVE
> > (but I suppose that's a patch for another time)
> 
> I don't have a real problem with breaking it down that way, if somebody
> wants to make a patch. Mostly the reason I didn't do so is that I don't
> think http-alternates are in common use these days, since smart-http is
> much more powerful.
> 
> > ----------8<-----------
> > From: Eric Wong <e@80x24.org>
> > Subject: [PATCH] http: inform about alternates-as-redirects behavior
> 
> This v2 looks fine to me.
> 
> -Peff

I know I'm a little late to the party but v2 looks good to me too.  I
like the change from v1 that only mentions the config option as opposed
to listing a value it should be set to.

-- 
Brandon Williams

Re: [PATCH] http: inform about alternates-as-redirects behavior

[Ramsay Jones]

On 04/03/17 03:13, Jeff King wrote:
> On Sat, Mar 04, 2017 at 01:35:04AM +0000, Eric Wong wrote:
[snip]
> 
> The warning itself:
> 
>> +		warning("alternate disabled by http.followRedirects!=true: %s",
> 
> feels like it could use some whitespace around the "!=", but maybe
> that's just me.

It's probably just me (too), but I think it would read better
without having '!=true' in the message at all. ;-)

ATB,
Ramsay Jones