{"thread":{"id":"45287","subject":"[PATCH] http: inform about alternates-as-redirects behavior","startedAt":"2017-03-04T01:35:12Z","lastAt":"2017-03-06T18:29:53Z","messageCount":9,"participants":["Eric Wong","Jeff King","Ramsay Jones","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"313253","messageId":"20170304013504.GA27183@untitled","threadId":"45287","inReplyTo":null,"subject":"[PATCH] http: inform about alternates-as-redirects behavior","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2017-03-04T01:35:04Z","receivedAt":"2017-03-04T01:35:12Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"It is disconcerting for users to not notice the behavior\nchange in handling alternates from commit cb4d2d35c4622ec2\n(\"http: treat http-alternates like redirects\")\n\nGive the user a hint about the config option so they can\nsee the URL and decide whether or not they want to enable\nhttp.followRedirects in their config.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n http-walker.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex b34b6ace7..626badfe6 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -168,6 +168,12 @@ static int is_alternate_allowed(const char *url)\n \t};\n \tint i;\n \n+\tif (http_follow_config != HTTP_FOLLOW_ALWAYS) {\n+\t\twarning(\"alternate disabled by http.followRedirects!=true: %s\",\n+\t\t\turl);\n+\t\treturn 0;\n+\t}\n+\n \tfor (i = 0; i < ARRAY_SIZE(protocols); i++) {\n \t\tconst char *end;\n \t\tif (skip_prefix(url, protocols[i], &end) &&\n@@ -331,9 +337,6 @@ static void fetch_alternates(struct walker *walker, const char *base)\n \tstruct alternates_request alt_req;\n \tstruct walker_data *cdata = walker->data;\n \n-\tif (http_follow_config != HTTP_FOLLOW_ALWAYS)\n-\t\treturn;\n-\n \t/*\n \t * If another request has already started fetching alternates,\n \t * wait for them to arrive and return to processing this request's\n-- \nEW\n"},{"id":"313256","messageId":"20170304031314.32bta4prahf7pfp7@sigill.intra.peff.net","threadId":"45287","inReplyTo":"20170304013504.GA27183@untitled","subject":"Re: [PATCH] http: inform about alternates-as-redirects behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-04T03:13:14Z","receivedAt":"2017-03-04T03:20:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 04, 2017 at 01:35:04AM +0000, Eric Wong wrote:\n\n> It is disconcerting for users to not notice the behavior\n> change in handling alternates from commit cb4d2d35c4622ec2\n> (\"http: treat http-alternates like redirects\")\n> \n> Give the user a hint about the config option so they can\n> see the URL and decide whether or not they want to enable\n> http.followRedirects in their config.\n\nYeah, I agree it makes sense to notify the user.\n\n> diff --git a/http-walker.c b/http-walker.c\n> index b34b6ace7..626badfe6 100644\n> --- a/http-walker.c\n> +++ b/http-walker.c\n> @@ -168,6 +168,12 @@ static int is_alternate_allowed(const char *url)\n>  \t};\n>  \tint i;\n>  \n> +\tif (http_follow_config != HTTP_FOLLOW_ALWAYS) {\n> +\t\twarning(\"alternate disabled by http.followRedirects!=true: %s\",\n> +\t\t\turl);\n> +\t\treturn 0;\n> +\t}\n> +\n>  \tfor (i = 0; i < ARRAY_SIZE(protocols); i++) {\n>  \t\tconst char *end;\n>  \t\tif (skip_prefix(url, protocols[i], &end) &&\n> @@ -331,9 +337,6 @@ static void fetch_alternates(struct walker *walker, const char *base)\n>  \tstruct alternates_request alt_req;\n>  \tstruct walker_data *cdata = walker->data;\n>  \n> -\tif (http_follow_config != HTTP_FOLLOW_ALWAYS)\n> -\t\treturn;\n> -\n\nI was surprised from the description to see not just the addition of a\nwarning, but a movement of the enforcement code.\n\nI think it's necessary because the original did not bother even fetching\nhttp-alternates if we were not going to respect it. Whereas the new code\nwill fetch and parse it, and warn only if we actually found something in\nit. Which seems reasonable.\n\nThe warning itself:\n\n> +\t\twarning(\"alternate disabled by http.followRedirects!=true: %s\",\n\nfeels like it could use some whitespace around the \"!=\", but maybe\nthat's just me.\n\n-Peff\n"},{"id":"313257","messageId":"20170304034914.cgyvz735lxhe2cno@sigill.intra.peff.net","threadId":"45287","inReplyTo":"20170304031314.32bta4prahf7pfp7@sigill.intra.peff.net","subject":"Re: [PATCH] http: inform about alternates-as-redirects behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-04T03:49:15Z","receivedAt":"2017-03-04T03:50:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 03, 2017 at 10:13:14PM -0500, Jeff King wrote:\n\n> > -\tif (http_follow_config != HTTP_FOLLOW_ALWAYS)\n> > -\t\treturn;\n> > -\n> \n> I was surprised from the description to see not just the addition of a\n> warning, but a movement of the enforcement code.\n> \n> I think it's necessary because the original did not bother even fetching\n> http-alternates if we were not going to respect it. Whereas the new code\n> will fetch and parse it, and warn only if we actually found something in\n> it. Which seems reasonable.\n\nOne side effect of this is that it exposes[1] the http-alternates\nparsing code to the server's input, even if we aren't planning on using\nthe result. That code is not very well audited. Just looking at the\ncontext from your patches, I noticed one obvious memory access problem.\nThe fix is below.\n\n-Peff\n\n[1] Obviously this same code was exposed prior to the redirect-limiting\n    patches, so it's not like there aren't tons of older clients that\n    exhibit the same behavior. But IMHO one of the beneficial side\n    effects of the redirect-limiting was that it avoided this largely\n    unused and untested code entirely.\n\n-- >8 --\nSubject: http-walker: fix buffer underflow processing remote alternates\n\nIf we parse a remote alternates (or http-alternates) file,\nwe expect relative lines like:\n\n  ../../foo.git/objects\n\nwhich we convert into \"$URL/../foo.git/\" (and then use that\nas a base for fetching more objects).\n\nBut if the remote feeds us nonsense like just:\n\n  ../\n\nwe will try to blindly strip the last 7 characters, assuming\nthey contain the string \"objects\". Since we don't _have_ 7\ncharacters at all, this results in feeding a small negative\nvalue to strbuf_add(), which converts it to a size_t,\nresulting in a big positive value. This should consistently\nfail (since we can't generally allocate the max size_t minus\n7 bytes), so there shouldn't be any security implications\n(and even if we did allocate, we'd just copy in gigabytes of\ngarbage, not overflow a buffer).\n\nLet's fix this by using strbuf_strip_suffix() to drop the\ncharacters we want. As a bonus this lets us handle names\nthat do not end in \"objects\" (all git repos do, but there is\nnothing to say that an alternate object store needs to be a\ngit repo).\n\nAnd while we're here, we can add a few other parsing\nniceties, like dropping trailing whitespace, and handling\nnames that do not end in \"/\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http-walker.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex b34b6ace7..d62ca8953 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -296,11 +296,13 @@ static void process_alternates_response(void *callback_data)\n \t\t\t\t\tokay = 1;\n \t\t\t\t}\n \t\t\t}\n-\t\t\t/* skip \"objects\\n\" at end */\n \t\t\tif (okay) {\n \t\t\t\tstruct strbuf target = STRBUF_INIT;\n \t\t\t\tstrbuf_add(&target, base, serverlen);\n-\t\t\t\tstrbuf_add(&target, data + i, posn - i - 7);\n+\t\t\t\tstrbuf_add(&target, data + i, posn - i);\n+\t\t\t\tstrbuf_rtrim(&target);\n+\t\t\t\tstrbuf_strip_suffix(&target, \"objects\");\n+\t\t\t\tstrbuf_complete(&target, '/');\n \n \t\t\t\tif (is_alternate_allowed(target.buf)) {\n \t\t\t\t\twarning(\"adding alternate object store: %s\",\n-- \n2.12.0.404.g442e75cca\n\n"},{"id":"313261","messageId":"20170304065548.GA20734@whir","threadId":"45287","inReplyTo":"20170304031314.32bta4prahf7pfp7@sigill.intra.peff.net","subject":"Re: [PATCH] http: inform about alternates-as-redirects behavior","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2017-03-04T06:55:48Z","receivedAt":"2017-03-04T07:07:25Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King <peff@peff.net> wrote:\n> The warning itself:\n> \n> > +\t\twarning(\"alternate disabled by http.followRedirects!=true: %s\",\n> \n> feels like it could use some whitespace around the \"!=\", but maybe\n> that's just me.\n\nYeah, I kinda wanted to emulate the command-line syntax.\n\nMaybe rewording it a bit and showing how to enable it will\nmake more sense:\n\n\t\twarning(\"alternate: %s\", url);\n\t\twarning(\" may be enabled by -c http.followRedirects=true\");\n\nAs well as keeping individual lines shorter and hopefully\neasier-to-read.\n"},{"id":"313262","messageId":"20170304074140.mzgs27jp2jer4mlv@sigill.intra.peff.net","threadId":"45287","inReplyTo":"20170304065548.GA20734@whir","subject":"Re: [PATCH] http: inform about alternates-as-redirects behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-04T07:41:40Z","receivedAt":"2017-03-04T07:42:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 04, 2017 at 06:55:48AM +0000, Eric Wong wrote:\n\n> Jeff King <peff@peff.net> wrote:\n> > The warning itself:\n> > \n> > > +\t\twarning(\"alternate disabled by http.followRedirects!=true: %s\",\n> > \n> > feels like it could use some whitespace around the \"!=\", but maybe\n> > that's just me.\n> \n> Yeah, I kinda wanted to emulate the command-line syntax.\n> \n> Maybe rewording it a bit and showing how to enable it will\n> make more sense:\n> \n> \t\twarning(\"alternate: %s\", url);\n> \t\twarning(\" may be enabled by -c http.followRedirects=true\");\n\nI kind of hoped people would look at the documentation for\nfollowRedirects before blindly enabling it. Though I guess the\ndocumentation doesn't really explain the possible security implications,\nso maybe it doesn't matter (and they're pretty subtle anyway).\n\n-Peff\n"},{"id":"313264","messageId":"20170304083645.GA24694@whir","threadId":"45287","inReplyTo":"20170304074140.mzgs27jp2jer4mlv@sigill.intra.peff.net","subject":"[PATCH v2] http: inform about alternates-as-redirects behavior","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2017-03-04T08:36:45Z","receivedAt":"2017-03-04T08:37:20Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King <peff@peff.net> wrote:\n> On Sat, Mar 04, 2017 at 06:55:48AM +0000, Eric Wong wrote:\n> > Jeff King <peff@peff.net> wrote:\n> > > The warning itself:\n> > > \n> > > > +\t\twarning(\"alternate disabled by http.followRedirects!=true: %s\",\n> > > \n> > > feels like it could use some whitespace around the \"!=\", but maybe\n> > > that's just me.\n> > \n> > Yeah, I kinda wanted to emulate the command-line syntax.\n> > \n> > Maybe rewording it a bit and showing how to enable it will\n> > make more sense:\n> > \n> > \t\twarning(\"alternate: %s\", url);\n> > \t\twarning(\" may be enabled by -c http.followRedirects=true\");\n> \n> I kind of hoped people would look at the documentation for\n> followRedirects before blindly enabling it. Though I guess the\n> documentation doesn't really explain the possible security implications,\n> so maybe it doesn't matter (and they're pretty subtle anyway).\n\nYou bring up a good point, perhaps just mentioning the config\nkey is enough to convince somebody to (v2 below).\n\n\nI also think the security implications for relative alternates\non the same host would not matter, since the smart HTTP will\ntake them into account on the server side.\n\nPerhaps we give http_follow_config ORable flags:\n\n\tHTTP_FOLLOW_NONE = 0,\n\tHTTP_FOLLOW_INITIAL = 0x1,\n\tHTTP_FOLLOW_RELATIVE = 0x2,\n\tHTTP_FOLLOW_ABSOLUTE = 0x4,\n\tHTTP_FOLLOW_ALWAYS = 0x7,\n\nWith the default would being: HTTP_FOLLOW_INITIAL|HTTP_FOLLOW_RELATIVE\n(but I suppose that's a patch for another time)\n\n----------8<-----------\nFrom: Eric Wong <e@80x24.org>\nSubject: [PATCH] http: inform about alternates-as-redirects behavior\n\nIt is disconcerting for users to not notice the behavior\nchange in handling alternates from commit cb4d2d35c4622ec2\n(\"http: treat http-alternates like redirects\")\n\nGive the user a hint about the config option so they can\nsee the URL and decide whether or not they want to enable\nhttp.followRedirects in their config.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n http-walker.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex b34b6ace7..6396cebe5 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -168,6 +168,11 @@ static int is_alternate_allowed(const char *url)\n \t};\n \tint i;\n \n+\tif (http_follow_config != HTTP_FOLLOW_ALWAYS) {\n+\t\twarning(\"alternate disabled by http.followRedirects: %s\", url);\n+\t\treturn 0;\n+\t}\n+\n \tfor (i = 0; i < ARRAY_SIZE(protocols); i++) {\n \t\tconst char *end;\n \t\tif (skip_prefix(url, protocols[i], &end) &&\n@@ -331,9 +336,6 @@ static void fetch_alternates(struct walker *walker, const char *base)\n \tstruct alternates_request alt_req;\n \tstruct walker_data *cdata = walker->data;\n \n-\tif (http_follow_config != HTTP_FOLLOW_ALWAYS)\n-\t\treturn;\n-\n \t/*\n \t * If another request has already started fetching alternates,\n \t * wait for them to arrive and return to processing this request's\n-- \nEW\n"},{"id":"313265","messageId":"20170304084547.4mg4beudseznaw72@sigill.intra.peff.net","threadId":"45287","inReplyTo":"20170304083645.GA24694@whir","subject":"Re: [PATCH v2] http: inform about alternates-as-redirects behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-04T08:45:47Z","receivedAt":"2017-03-04T08:45:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 04, 2017 at 08:36:45AM +0000, Eric Wong wrote:\n\n> I also think the security implications for relative alternates\n> on the same host would not matter, since the smart HTTP will\n> take them into account on the server side.\n\nIt depends on the host whether all of the repos on it have the same\nsecurity domain or not. A site like github.com hosts both public and\nprivate repositories, and you do not want a public repo redirecting to\nthe private one to get objects.\n\nOf course, that depends on untrusted users being able to configure\nserver-side alternates, which GitHub certainly would not let you do. I\nwould hope other multi-user hosting sites behave similarly (most hosting\nsites do not seem to allow dumb http at all).\n\n> Perhaps we give http_follow_config ORable flags:\n> \n> \tHTTP_FOLLOW_NONE = 0,\n> \tHTTP_FOLLOW_INITIAL = 0x1,\n> \tHTTP_FOLLOW_RELATIVE = 0x2,\n> \tHTTP_FOLLOW_ABSOLUTE = 0x4,\n> \tHTTP_FOLLOW_ALWAYS = 0x7,\n> \n> With the default would being: HTTP_FOLLOW_INITIAL|HTTP_FOLLOW_RELATIVE\n> (but I suppose that's a patch for another time)\n\nI don't have a real problem with breaking it down that way, if somebody\nwants to make a patch. Mostly the reason I didn't do so is that I don't\nthink http-alternates are in common use these days, since smart-http is\nmuch more powerful.\n\n> ----------8<-----------\n> From: Eric Wong <e@80x24.org>\n> Subject: [PATCH] http: inform about alternates-as-redirects behavior\n\nThis v2 looks fine to me.\n\n-Peff\n"},{"id":"313272","messageId":"25225b13-c8c0-b153-4ddd-499370716054@ramsayjones.plus.com","threadId":"45287","inReplyTo":"20170304031314.32bta4prahf7pfp7@sigill.intra.peff.net","subject":"Re: [PATCH] http: inform about alternates-as-redirects behavior","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-03-04T15:06:58Z","receivedAt":"2017-03-04T15:07:30Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 04/03/17 03:13, Jeff King wrote:\n> On Sat, Mar 04, 2017 at 01:35:04AM +0000, Eric Wong wrote:\n[snip]\n> \n> The warning itself:\n> \n>> +\t\twarning(\"alternate disabled by http.followRedirects!=true: %s\",\n> \n> feels like it could use some whitespace around the \"!=\", but maybe\n> that's just me.\n\nIt's probably just me (too), but I think it would read better\nwithout having '!=true' in the message at all. ;-)\n\nATB,\nRamsay Jones\n\n\n"},{"id":"313344","messageId":"20170306180302.GA183239@google.com","threadId":"45287","inReplyTo":"20170304084547.4mg4beudseznaw72@sigill.intra.peff.net","subject":"Re: [PATCH v2] http: inform about alternates-as-redirects behavior","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-06T18:03:02Z","receivedAt":"2017-03-06T18:29:53Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/04, Jeff King wrote:\n> On Sat, Mar 04, 2017 at 08:36:45AM +0000, Eric Wong wrote:\n> \n> > I also think the security implications for relative alternates\n> > on the same host would not matter, since the smart HTTP will\n> > take them into account on the server side.\n> \n> It depends on the host whether all of the repos on it have the same\n> security domain or not. A site like github.com hosts both public and\n> private repositories, and you do not want a public repo redirecting to\n> the private one to get objects.\n> \n> Of course, that depends on untrusted users being able to configure\n> server-side alternates, which GitHub certainly would not let you do. I\n> would hope other multi-user hosting sites behave similarly (most hosting\n> sites do not seem to allow dumb http at all).\n> \n> > Perhaps we give http_follow_config ORable flags:\n> > \n> > \tHTTP_FOLLOW_NONE = 0,\n> > \tHTTP_FOLLOW_INITIAL = 0x1,\n> > \tHTTP_FOLLOW_RELATIVE = 0x2,\n> > \tHTTP_FOLLOW_ABSOLUTE = 0x4,\n> > \tHTTP_FOLLOW_ALWAYS = 0x7,\n> > \n> > With the default would being: HTTP_FOLLOW_INITIAL|HTTP_FOLLOW_RELATIVE\n> > (but I suppose that's a patch for another time)\n> \n> I don't have a real problem with breaking it down that way, if somebody\n> wants to make a patch. Mostly the reason I didn't do so is that I don't\n> think http-alternates are in common use these days, since smart-http is\n> much more powerful.\n> \n> > ----------8<-----------\n> > From: Eric Wong <e@80x24.org>\n> > Subject: [PATCH] http: inform about alternates-as-redirects behavior\n> \n> This v2 looks fine to me.\n> \n> -Peff\n\nI know I'm a little late to the party but v2 looks good to me too.  I\nlike the change from v1 that only mentions the config option as opposed\nto listing a value it should be set to.\n\n-- \nBrandon Williams\n"}]}