{"thread":{"id":"24481","subject":"URL decoding changed semantics of + in URLs","startedAt":"2010-07-23T13:18:30Z","lastAt":"2010-08-06T10:46:42Z","messageCount":18,"participants":["Thomas Rast","Ævar Arnfjörð Bjarmason","Jasper St. Pierre","Junio C Hamano","Jeff King","Matthieu Moy","Ralf Ebert"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"146067","messageId":"201007231518.31071.trast@student.ethz.ch","threadId":"24481","inReplyTo":null,"subject":"URL decoding changed semantics of + in URLs","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-07-23T13:18:30Z","receivedAt":"2010-07-23T13:18:30Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Hi Jeff\n\nAs pointed out by Jasper St. Pierre on #git, it is no longer possible\nto clone\n\n git://git.gnome.org/gtk+\n\nbecause your 9d2e942 (decode file:// and ssh:// URLs, 2010-05-23)\ndecodes + characters in URLs to spaces in the http style. It was\nlater fixed by ce83eda (url.c: \"://\" part at the beginning\nshould not be URL decoded, 2010-06-23) but the later part of the url\nstill decodes + as space.\n\nThe tests that go along with the commit make it clear that it was an\nintended change. But the interesting thing is, I cannot find any\nreference in any RFC that + must have this meaning. In particular,\n\n http://www.ietf.org/rfc/rfc2396.txt\n\ndoesn't say much about + and the only escaping defined is the usual\n%xx style. So is there a standard that mandates this, or was it just\na well-meaning but unnecessary backwards incompatible change?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"146068","messageId":"201007231521.52750.trast@student.ethz.ch","threadId":"24481","inReplyTo":"201007231518.31071.trast@student.ethz.ch","subject":"Re: URL decoding changed semantics of + in URLs","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-07-23T13:21:52Z","receivedAt":"2010-07-23T13:21:52Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Thomas Rast wrote:\n> Hi Jeff\n> \n> As pointed out by Jasper St. Pierre on #git, it is no longer possible\n> to clone\n> \n> git://git.gnome.org/gtk+\n\nWell, that reads wrong now that I'm seeing it again.\n\nI of course meant to say that the above URL no longer works and must\nbe spelled\n\n git://git.gnome.org/gtk%2B\n\ninstead.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"146070","messageId":"AANLkTinsixPihZRtduuB_0puX_ucC0HYqHPU0UJMX2e-@mail.gmail.com","threadId":"24481","inReplyTo":"201007231518.31071.trast@student.ethz.ch","subject":"Re: URL decoding changed semantics of + in URLs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-23T14:10:36Z","receivedAt":"2010-07-23T14:10:36Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jul 23, 2010 at 13:18, Thomas Rast wrote:\n\n> doesn't say much about + and the only escaping defined is the usual\n> %xx style.  So is there a standard that mandates this, or was it just\n> a well-meaning but unnecessary backwards incompatible change?\n\n+ and %20 are as far as I know only interchangable in *query strings*,\nso having to clone 'git://git.gnome.org/gtk%2B' where you could\npreviously clone 'git://git.gnome.org/gtk+' is a bug. Git shouldn't be\nchanging that + to a %20.\n\nI haven't followed why we need to escape + to %20 at all, even in the\nquery string. E.g. curl(1) doesn't do that before sending requests to\nApache, which can handle either one. The + v.s. %20 duality is always\nhandled at the server AFAIK.\n"},{"id":"146072","messageId":"AANLkTikRFCmE8BeB6G107cz6ZPxWtq=j-PmmX59pR5zj@mail.gmail.com","threadId":"24481","inReplyTo":"AANLkTinsixPihZRtduuB_0puX_ucC0HYqHPU0UJMX2e-@mail.gmail.com","subject":"Re: URL decoding changed semantics of + in URLs","fromName":"Jasper St. Pierre","fromEmail":"jstpierre@mecheye.net","sentAt":"2010-07-23T14:25:57Z","receivedAt":"2010-07-23T14:25:57Z","isPatch":false,"sender":{"key":"jstpierre@mecheye.net","avatar":"https://gravatar.com/avatar/f217edd3d2ae0103760aa317c58d2f38f2fdedd40520803d67396286eacd53ba?d=mp&s=160"},"body":"Yep. http://www.ietf.org/rfc/rfc2396.txt defines '+' as a reserved character,\nbut doesn't give a purpose for it. www-form-encoded replaces space with '+'\nbut in a URL it can mean anything it wants.\n\nOn Fri, Jul 23, 2010 at 10:10 AM, Ævar Arnfjörð Bjarmason\n wrote:\n> On Fri, Jul 23, 2010 at 13:18, Thomas Rast wrote:\n>\n>> doesn't say much about + and the only escaping defined is the usual\n>> %xx style.  So is there a standard that mandates this, or was it just\n>> a well-meaning but unnecessary backwards incompatible change?\n>\n> + and %20 are as far as I know only interchangable in *query strings*,\n> so having to clone 'git://git.gnome.org/gtk%2B' where you could\n> previously clone 'git://git.gnome.org/gtk+' is a bug. Git shouldn't be\n> changing that + to a %20.\n>\n> I haven't followed why we need to escape + to %20 at all, even in the\n> query string. E.g. curl(1) doesn't do that before sending requests to\n> Apache, which can handle either one. The + v.s. %20 duality is always\n> handled at the server AFAIK.\n>\n"},{"id":"146113","messageId":"db9c97908966fa332be07b2a9f5215679e35b9e0.1279920066.git.trast@student.ethz.ch","threadId":"24481","inReplyTo":"AANLkTinsixPihZRtduuB_0puX_ucC0HYqHPU0UJMX2e-@mail.gmail.com","subject":"[PATCH] Do not unquote + into ' ' in URLs","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-07-23T21:23:31Z","receivedAt":"2010-07-23T21:23:31Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Since 9d2e942 (decode file:// and ssh:// URLs, 2010-05-23) the URL\nlogic unquotes escaped URLs. For the %2B type of escape, this is\nconformant with RFC 2396. However, it also unquotes + into a space\ncharacter, which is only appropriate for the query strings in HTTP.\nThis notably broke fetching from the gtk+ repository.\n\nRemove the corresponding bit of code.\n\nReported-by: Jasper St. Pierre \nSigned-off-by: Thomas Rast \n---\n\nJasper St. Pierre wrote:\n> Yep. http://www.ietf.org/rfc/rfc2396.txt defines '+' as a reserved character,\n> but doesn't give a purpose for it. www-form-encoded replaces space with '+'\n> but in a URL it can mean anything it wants.\n\nSo let's do this then, instead?\n\nBased on the discussion, I would consider this a bugfix that should go\nin 1.7.2.1.\n\n\n t/t5601-clone.sh | 10 ++++++++--\n url.c | 5 +----\n 2 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 8abb71a..4431dfd 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -178,8 +178,14 @@ test_expect_success 'clone respects global branch.autosetuprebase' '\n \n test_expect_success 'respect url-encoding of file://' '\n \tgit init x+y &&\n-\ttest_must_fail git clone \"file://$PWD/x+y\" xy-url &&\n-\tgit clone \"file://$PWD/x%2By\" xy-url\n+\tgit clone \"file://$PWD/x+y\" xy-url-1 &&\n+\tgit clone \"file://$PWD/x%2By\" xy-url-2\n+'\n+\n+test_expect_success 'do not query-string-decode + in URLs' '\n+\trm -rf x+y &&\n+\tgit init \"x y\" &&\n+\ttest_must_fail git clone \"file://$PWD/x+y\" xy-no-plus\n '\n \n test_expect_success 'do not respect url-encoding of non-url path' '\ndiff --git a/url.c b/url.c\nindex 2306236..fa4b8d4 100644\n--- a/url.c\n+++ b/url.c\n@@ -90,10 +90,7 @@ static char *url_decode_internal(const char **query, const char *stop_at, struct\n \t\t\t}\n \t\t}\n \n-\t\tif (c == '+')\n-\t\t\tstrbuf_addch(out, ' ');\n-\t\telse\n-\t\t\tstrbuf_addch(out, c);\n+\t\tstrbuf_addch(out, c);\n \t\tq++;\n \t} while (1);\n \t*query = q;\n-- \n1.7.2.rc3.335.g26d7d\n"},{"id":"146115","messageId":"AANLkTil6sD71n1aDrUQf4ATnlE_L7ltJa500cI6bH6X8@mail.gmail.com","threadId":"24481","inReplyTo":"db9c97908966fa332be07b2a9f5215679e35b9e0.1279920066.git.trast@student.ethz.ch","subject":"Re: [PATCH] Do not unquote + into ' ' in URLs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-23T22:20:25Z","receivedAt":"2010-07-23T22:20:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jul 23, 2010 at 21:23, Thomas Rast wrote:\n> Since 9d2e942 (decode file:// and ssh:// URLs, 2010-05-23) the URL\n> logic unquotes escaped URLs.  For the %2B type of escape, this is\n> conformant with RFC 2396.  However, it also unquotes + into a space\n> character, which is only appropriate for the query strings in HTTP.\n> This notably broke fetching from the gtk+ repository.\n>\n> Remove the corresponding bit of code.\n>\n> Reported-by: Jasper St. Pierre \n> Signed-off-by: Thomas Rast \n> ---\n>\n> Jasper St. Pierre wrote:\n>> Yep. http://www.ietf.org/rfc/rfc2396.txt defines '+' as a reserved character,\n>> but doesn't give a purpose for it. www-form-encoded replaces space with '+'\n>> but in a URL it can mean anything it wants.\n>\n> So let's do this then, instead?\n>\n> Based on the discussion, I would consider this a bugfix that should go\n> in 1.7.2.1.\n>\n>\n>  t/t5601-clone.sh |   10 ++++++++--\n>  url.c            |    5 +----\n>  2 files changed, 9 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 8abb71a..4431dfd 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -178,8 +178,14 @@ test_expect_success 'clone respects global branch.autosetuprebase' '\n>\n>  test_expect_success 'respect url-encoding of file://' '\n>        git init x+y &&\n> -       test_must_fail git clone \"file://$PWD/x+y\" xy-url &&\n> -       git clone \"file://$PWD/x%2By\" xy-url\n> +       git clone \"file://$PWD/x+y\" xy-url-1 &&\n> +       git clone \"file://$PWD/x%2By\" xy-url-2\n> +'\n> +\n> +test_expect_success 'do not query-string-decode + in URLs' '\n> +       rm -rf x+y &&\n> +       git init \"x y\" &&\n> +       test_must_fail git clone \"file://$PWD/x+y\" xy-no-plus\n>  '\n>\n>  test_expect_success 'do not respect url-encoding of non-url path' '\n> diff --git a/url.c b/url.c\n> index 2306236..fa4b8d4 100644\n> --- a/url.c\n> +++ b/url.c\n> @@ -90,10 +90,7 @@ static char *url_decode_internal(const char **query, const char *stop_at, struct\n>                        }\n>                }\n>\n> -               if (c == '+')\n> -                       strbuf_addch(out, ' ');\n> -               else\n> -                       strbuf_addch(out, c);\n> +               strbuf_addch(out, c);\n>                q++;\n>        } while (1);\n>        *query = q;\n> --\n> 1.7.2.rc3.335.g26d7d\n>\n\nThis looks good, Ack.\n\nBut as icing, it'd be nice to extend these tests to create files /\nclone repositories with the rest of the reserved characters:\nhttp://en.wikipedia.org/wiki/Percent-encoding#Percent-encoding_reserved_characters\n\nOf course that would have to be wrapped in something that skips the\ntests if those paths can't be created. E.g. \"/\" is a no-no on Unix,\nand some of the others will probably cause troubles on other systems.\n"},{"id":"146116","messageId":"7viq45eswt.fsf@alter.siamese.dyndns.org","threadId":"24481","inReplyTo":"db9c97908966fa332be07b2a9f5215679e35b9e0.1279920066.git.trast@student.ethz.ch","subject":"Re: [PATCH] Do not unquote + into ' ' in URLs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-23T22:26:42Z","receivedAt":"2010-07-23T22:26:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Since 9d2e942 (decode file:// and ssh:// URLs, 2010-05-23) the URL\n> logic unquotes escaped URLs. For the %2B type of escape, this is\n> conformant with RFC 2396. However, it also unquotes + into a space\n> character, which is only appropriate for the query strings in HTTP.\n> This notably broke fetching from the gtk+ repository.\n\nWait a minute.\n\n> Based on the discussion, I would consider this a bugfix that should go\n> in 1.7.2.1.\n\nSome form of this may need to be applied to help the client side, but what\nwill happen to\n\n http-backend.c::get_info_refs()\n -> http-backend.c::get_parameter()\n -> http-backend.c::get_parameters()\n -> url.c::url_decode_parameter_value()\n -> url.c::url_decode_internal()\n\ncodepath, which is the server-side handing of query strings?\n"},{"id":"146122","messageId":"201007240104.25341.trast@student.ethz.ch","threadId":"24481","inReplyTo":"7viq45eswt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Do not unquote + into ' ' in URLs","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-07-23T23:04:24Z","receivedAt":"2010-07-23T23:04:24Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano wrote:\n> Thomas Rast writes:\n> \n> > Since 9d2e942 (decode file:// and ssh:// URLs, 2010-05-23) the URL\n> > logic unquotes escaped URLs. For the %2B type of escape, this is\n> > conformant with RFC 2396. However, it also unquotes + into a space\n> > character, which is only appropriate for the query strings in HTTP.\n> > This notably broke fetching from the gtk+ repository.\n> \n> Wait a minute.\n> \n> > Based on the discussion, I would consider this a bugfix that should go\n> > in 1.7.2.1.\n> \n> Some form of this may need to be applied to help the client side, but what\n> will happen to\n> \n> http-backend.c::get_info_refs()\n> -> http-backend.c::get_parameter()\n> -> http-backend.c::get_parameters()\n> -> url.c::url_decode_parameter_value()\n> -> url.c::url_decode_internal()\n> \n> codepath, which is the server-side handing of query strings?\n\nYou're right, I forgot about those. I imagine it would be one of two\ncases:\n\n* It should never have dequoted + before the ? query-delimiter, so we\n need to fix that too;\n\n* It should never have dequoted + before the ? query-delimiter, but\n it's too late to change that now.\n\nShawn, can you help with this?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"146169","messageId":"ed2d311355fca478f97b82f8d955494509d6b9de.1279982471.git.trast@student.ethz.ch","threadId":"24481","inReplyTo":"201007240104.25341.trast@student.ethz.ch","subject":"[PATCH v2] Do not unquote + into ' ' in URLs","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-07-24T14:49:04Z","receivedAt":"2010-07-24T14:49:04Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Since 9d2e942 (decode file:// and ssh:// URLs, 2010-05-23) the URL\nlogic unquotes escaped URLs. For the %2B type of escape, this is\nconformant with RFC 2396. However, it also unquotes + into a space\ncharacter, which is only appropriate for the query strings in HTTP.\nThis notably broke fetching from the gtk+ repository.\n\nWe cannot just remove the corresponding code since the same\nurl_decode_internal() is also used by the HTTP backend to decode query\nparameters. Introduce a new argument that controls whether the +\ndecoding happens, and use it only in the (client-side) url_decode().\n\nReported-by: Jasper St. Pierre \nSigned-off-by: Thomas Rast \n---\n\nI wrote:\n> Junio C Hamano wrote:\n> > \n> > http-backend.c::get_info_refs()\n> > -> http-backend.c::get_parameter()\n> > -> http-backend.c::get_parameters()\n> > -> url.c::url_decode_parameter_value()\n> > -> url.c::url_decode_internal()\n> \n> You're right, I forgot about those. I imagine it would be one of two\n> cases:\n[...]\n> Shawn, can you help with this?\n\nThe third case, of course, is:\n\n* It only uses these functions for parameter decoding, which of course\n was correct to begin with.\n\nSo after hopefully drinking enough coffee, I made this one. The catch\nis that I'm not entirely clear whether *not* decoding the +\nclient-side anywhere in the URL is correct for http:// URLs? If the\nclient decodes and re-encodes the URL, then the + would be turned into\na %2B on the re-encoding. Then again maybe UI-facing URLs should\nnever have a query part at all?\n\n\n t/t5601-clone.sh | 10 ++++++++--\n url.c | 11 ++++++-----\n 2 files changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 8abb71a..4431dfd 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -178,8 +178,14 @@ test_expect_success 'clone respects global branch.autosetuprebase' '\n \n test_expect_success 'respect url-encoding of file://' '\n \tgit init x+y &&\n-\ttest_must_fail git clone \"file://$PWD/x+y\" xy-url &&\n-\tgit clone \"file://$PWD/x%2By\" xy-url\n+\tgit clone \"file://$PWD/x+y\" xy-url-1 &&\n+\tgit clone \"file://$PWD/x%2By\" xy-url-2\n+'\n+\n+test_expect_success 'do not query-string-decode + in URLs' '\n+\trm -rf x+y &&\n+\tgit init \"x y\" &&\n+\ttest_must_fail git clone \"file://$PWD/x+y\" xy-no-plus\n '\n \n test_expect_success 'do not respect url-encoding of non-url path' '\ndiff --git a/url.c b/url.c\nindex 2306236..cd8f74f 100644\n--- a/url.c\n+++ b/url.c\n@@ -67,7 +67,8 @@ static int url_decode_char(const char *q)\n \treturn val;\n }\n \n-static char *url_decode_internal(const char **query, const char *stop_at, struct strbuf *out)\n+static char *url_decode_internal(const char **query, const char *stop_at,\n+\t\t\t\t struct strbuf *out, int decode_plus)\n {\n \tconst char *q = *query;\n \n@@ -90,7 +91,7 @@ static char *url_decode_internal(const char **query, const char *stop_at, struct\n \t\t\t}\n \t\t}\n \n-\t\tif (c == '+')\n+\t\tif (decode_plus && c == '+')\n \t\t\tstrbuf_addch(out, ' ');\n \t\telse\n \t\t\tstrbuf_addch(out, c);\n@@ -110,17 +111,17 @@ char *url_decode(const char *url)\n \t\tstrbuf_add(&out, url, colon - url);\n \t\turl = colon;\n \t}\n-\treturn url_decode_internal(&url, NULL, &out);\n+\treturn url_decode_internal(&url, NULL, &out, 0);\n }\n \n char *url_decode_parameter_name(const char **query)\n {\n \tstruct strbuf out = STRBUF_INIT;\n-\treturn url_decode_internal(query, \"&=\", &out);\n+\treturn url_decode_internal(query, \"&=\", &out, 1);\n }\n \n char *url_decode_parameter_value(const char **query)\n {\n \tstruct strbuf out = STRBUF_INIT;\n-\treturn url_decode_internal(query, \"&\", &out);\n+\treturn url_decode_internal(query, \"&\", &out, 1);\n }\n-- \n1.7.2.278.g76edd.dirty\n"},{"id":"146386","messageId":"20100726154041.GA18762@coredump.intra.peff.net","threadId":"24481","inReplyTo":"201007231518.31071.trast@student.ethz.ch","subject":"Re: URL decoding changed semantics of + in URLs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-07-26T15:40:42Z","receivedAt":"2010-07-26T15:40:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 23, 2010 at 03:18:30PM +0200, Thomas Rast wrote:\n\n> As pointed out by Jasper St. Pierre on #git, it is no longer possible\n> to clone\n> \n> git://git.gnome.org/gtk+\n> \n> because your 9d2e942 (decode file:// and ssh:// URLs, 2010-05-23)\n> decodes + characters in URLs to spaces in the http style. It was\n> later fixed by ce83eda (url.c: \"://\" part at the beginning\n> should not be URL decoded, 2010-06-23) but the later part of the url\n> still decodes + as space.\n> \n> The tests that go along with the commit make it clear that it was an\n> intended change. But the interesting thing is, I cannot find any\n> reference in any RFC that + must have this meaning. In particular,\n> \n> http://www.ietf.org/rfc/rfc2396.txt\n> \n> doesn't say much about + and the only escaping defined is the usual\n> %xx style. So is there a standard that mandates this, or was it just\n> a well-meaning but unnecessary backwards incompatible change?\n\nSorry for the slow reply. I am in the middle of moving and just got a\nusable machine running.\n\nI think it is well-meaning but unnecessary. That is, I think %-decoding\nis probably still a good idea, and I just dragged '+'-decoding along out\nof cluelessness. I thought I cross-checked with what curl was doing to\nhttp URLs, but now I can't even get it to do anything but pass the path\ncomponent literally to the webserver. So I'm really not sure what I\ntested before.\n\nAs Jasper noted, \"+\" is a reserved character, which means \"gtk+\"\nprobably _should_ be escaped. But clearly it doesn't happen in practice,\nand I am more interested in not breaking current use than in nitpicking\nwith a standard.\n\nSo I agree with the spirit of your patches, and reading over your v2, it\nlooks fine to me, but I didn't do any extensive testing.\n\n-Peff\n"},{"id":"146402","messageId":"AANLkTikmFVHeMVEgj_G5h8VMNaw0zIm0Ol-vC1ffc45v@mail.gmail.com","threadId":"24481","inReplyTo":"20100726154041.GA18762@coredump.intra.peff.net","subject":"Re: URL decoding changed semantics of + in URLs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-26T17:57:48Z","receivedAt":"2010-07-26T17:57:48Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Jul 26, 2010 at 15:40, Jeff King wrote:\n\n> As Jasper noted, \"+\" is a reserved character, which means \"gtk+\"\n> probably _should_ be escaped. But clearly it doesn't happen in practice,\n> and I am more interested in not breaking current use than in nitpicking\n> with a standard.\n\nReserved characters only have to be escaped in certain contexts, from\nRFC 2396:\n\n Many URI include components consisting of or delimited by, certain\n special characters. These characters are called \"reserved\", since\n their usage within the URI component is limited to their reserved\n purpose. If the data for a URI component would conflict with the\n reserved purpose, then the conflicting data must be escaped before\n forming the URI.\n\n reserved = \";\" | \"/\" | \"?\" | \":\" | \"@\" | \"&\" | \"=\" | \"+\" |\n \"$\" | \",\"\n\nE.g. @ is special in the hostname part (http://user@example.org), but\nwriting http://example.org/Git@Big%20companies:%20A%20Study is just\nfine.\n\nWhich is why curl passes it along literally, it *can* escape them, and\nreal webservers like Apache handle reserved characters equivalently\n(in their unreserved contexts) whether they're escaped or not, but the\ngit-daemon at git.gnome.org evidently doesn't implement RFC 2396\ncarefully enough.\n\nSo we shouldn't escape + for backwards compatibility and because it's\nnot necessary, but we should probably also fix git-daemon to accept\nboth forms if that hasn't been done already.\n"},{"id":"146406","messageId":"AANLkTimRrpiv7cu=j598K3x4h3UGW7-Hik6-6jA8R_5J@mail.gmail.com","threadId":"24481","inReplyTo":"AANLkTikmFVHeMVEgj_G5h8VMNaw0zIm0Ol-vC1ffc45v@mail.gmail.com","subject":"Re: URL decoding changed semantics of + in URLs","fromName":"Jasper St. Pierre","fromEmail":"jstpierre@mecheye.net","sentAt":"2010-07-26T18:22:42Z","receivedAt":"2010-07-26T18:22:42Z","isPatch":false,"sender":{"key":"jstpierre@mecheye.net","avatar":"https://gravatar.com/avatar/f217edd3d2ae0103760aa317c58d2f38f2fdedd40520803d67396286eacd53ba?d=mp&s=160"},"body":"Where is '+' used in the URL context? I don't see that it's a\nreplacement for '+'\naside from formencoded, which is from CGI, not HTTP or the URI spec.\n\nI also can't access something called \"test 2.txt\" from Apache with\n\"http://localhost/test+2.txt\", so I don't think it's unescaping the '+'.\n\nI don't think we should do anything about the '+' case, except where used in\nformencoded parameters (aka the \"query string\"), where it is used.\n\nOn Mon, Jul 26, 2010 at 1:57 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> On Mon, Jul 26, 2010 at 15:40, Jeff King wrote:\n>\n>> As Jasper noted, \"+\" is a reserved character, which means \"gtk+\"\n>> probably _should_ be escaped. But clearly it doesn't happen in practice,\n>> and I am more interested in not breaking current use than in nitpicking\n>> with a standard.\n>\n> Reserved characters only have to be escaped in certain contexts, from\n> RFC 2396:\n>\n>   Many URI include components consisting of or delimited by, certain\n>   special characters.  These characters are called \"reserved\", since\n>   their usage within the URI component is limited to their reserved\n>   purpose.  If the data for a URI component would conflict with the\n>   reserved purpose, then the conflicting data must be escaped before\n>   forming the URI.\n>\n>      reserved    = \";\" | \"/\" | \"?\" | \":\" | \"@\" | \"&\" | \"=\" | \"+\" |\n>                    \"$\" | \",\"\n>\n> E.g. @ is special in the hostname part (http://user@example.org), but\n> writing http://example.org/Git@Big%20companies:%20A%20Study is just\n> fine.\n>\n> Which is why curl passes it along literally, it *can* escape them, and\n> real webservers like Apache handle reserved characters equivalently\n> (in their unreserved contexts) whether they're escaped or not, but the\n> git-daemon at git.gnome.org evidently doesn't implement RFC 2396\n> carefully enough.\n>\n> So we shouldn't escape + for backwards compatibility and because it's\n> not necessary, but we should probably also fix git-daemon to accept\n> both forms if that hasn't been done already.\n>\n"},{"id":"146412","messageId":"vpq7hkif64y.fsf@bauges.imag.fr","threadId":"24481","inReplyTo":"AANLkTimRrpiv7cu=j598K3x4h3UGW7-Hik6-6jA8R_5J@mail.gmail.com","subject":"Re: URL decoding changed semantics of + in URLs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-07-26T18:30:05Z","receivedAt":"2010-07-26T18:30:05Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"\"Jasper St. Pierre\" writes:\n\n> I don't think we should do anything about the '+' case, except where used in\n> formencoded parameters (aka the \"query string\"), where it is used.\n\nThis is what the latest version of the patch does, AIUI, yes.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"146413","messageId":"AANLkTilDKIwcQ0OmXAihq8AkS7v31HseEI3BGTUyzl9v@mail.gmail.com","threadId":"24481","inReplyTo":"AANLkTimRrpiv7cu=j598K3x4h3UGW7-Hik6-6jA8R_5J@mail.gmail.com","subject":"Re: URL decoding changed semantics of + in URLs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-26T18:35:27Z","receivedAt":"2010-07-26T18:35:27Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Jul 26, 2010 at 18:22, Jasper St. Pierre wrote:\n> Where is '+' used in the URL context? I don't see that it's a\n> replacement for '+'\n> aside from formencoded, which is from CGI, not HTTP or the URI spec.\n\nIn my example? Nowhere, but I used : and @, which are also reserved\ncharacters.\n\n> I also can't access something called \"test 2.txt\" from Apache with\n> \"http://localhost/test+2.txt\", so I don't think it's unescaping the '+'.\n\nYes, that's not supposed to work. But you should be able to access\n\"test+2.txt\" using /test+2.txt and /test%2B.txt and\n/%74%65%73%74%2B2%2E%74%78%74 for that matter.\n\ngit-daemon only seems to handle the first form. Which is probably a\nbug, maybe it doesn't *have to* URI unescape its arguments, but it's\nprobably a good idea anyway. E.g. some systems that handle URIs will\nconvert : to %3A automatically when passing them through. That would\nbreak a git URL.\n\n> I don't think we should do anything about the '+' case, except where used in\n> formencoded parameters (aka the \"query string\"), where it is used.\n\nI don't really have an opinion on what we should do. It's not a\nproblem for me, I'm just noting how it could break, and that maybe we\nshould try harder and support URI escaping where we handle URLs.\n"},{"id":"146415","messageId":"AANLkTinmwZiqdhrzSnF=3nADwoFbor8XUAfw1LDjWoHR@mail.gmail.com","threadId":"24481","inReplyTo":"AANLkTilDKIwcQ0OmXAihq8AkS7v31HseEI3BGTUyzl9v@mail.gmail.com","subject":"Re: URL decoding changed semantics of + in URLs","fromName":"Jasper St. Pierre","fromEmail":"jstpierre@mecheye.net","sentAt":"2010-07-26T18:44:30Z","receivedAt":"2010-07-26T18:44:30Z","isPatch":false,"sender":{"key":"jstpierre@mecheye.net","avatar":"https://gravatar.com/avatar/f217edd3d2ae0103760aa317c58d2f38f2fdedd40520803d67396286eacd53ba?d=mp&s=160"},"body":"Hm. I've always wondered about the percent double-encoding problem,\nwhere the \"%\"\nin \"%2B\" double-encodes to \"%252B\", which might require the mandatory server\nimplementation then.\n\nI do think that because \"git://git.gnome.org/git+\" was working,\nsomething on the server\nwas going right.\n\nOn Mon, Jul 26, 2010 at 2:35 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> On Mon, Jul 26, 2010 at 18:22, Jasper St. Pierre wrote:\n>> Where is '+' used in the URL context? I don't see that it's a\n>> replacement for '+'\n>> aside from formencoded, which is from CGI, not HTTP or the URI spec.\n>\n> In my example? Nowhere, but I used : and @, which are also reserved\n> characters.\n>\n>> I also can't access something called \"test 2.txt\" from Apache with\n>> \"http://localhost/test+2.txt\", so I don't think it's unescaping the '+'.\n>\n> Yes, that's not supposed to work. But you should be able to access\n> \"test+2.txt\" using /test+2.txt and /test%2B.txt and\n> /%74%65%73%74%2B2%2E%74%78%74 for that matter.\n>\n> git-daemon only seems to handle the first form. Which is probably a\n> bug, maybe it doesn't *have to* URI unescape its arguments, but it's\n> probably a good idea anyway. E.g. some systems that handle URIs will\n> convert : to %3A automatically when passing them through. That would\n> break a git URL.\n>\n>> I don't think we should do anything about the '+' case, except where used in\n>> formencoded parameters (aka the \"query string\"), where it is used.\n>\n> I don't really have an opinion on what we should do. It's not a\n> problem for me, I'm just noting how it could break, and that maybe we\n> should try harder and support URI escaping where we handle URLs.\n>\n"},{"id":"146874","messageId":"AANLkTikY_ayRzdbQ2Qsf6wus+=yg99aTLDTahVXUw0eK@mail.gmail.com","threadId":"24481","inReplyTo":"ed2d311355fca478f97b82f8d955494509d6b9de.1279982471.git.trast@student.ethz.ch","subject":"Re: [PATCH v2] Do not unquote + into ' ' in URLs","fromName":"Jasper St. Pierre","fromEmail":"jstpierre@mecheye.net","sentAt":"2010-07-31T21:18:28Z","receivedAt":"2010-07-31T21:18:28Z","isPatch":true,"sender":{"key":"jstpierre@mecheye.net","avatar":"https://gravatar.com/avatar/f217edd3d2ae0103760aa317c58d2f38f2fdedd40520803d67396286eacd53ba?d=mp&s=160"},"body":"Hmm.. could we get a follow-up on this patch? It's annoying because it's both\nbackwards and forwards incompatible: the %2b workaround doesn't work in\nolder versions.\n\nOn Sat, Jul 24, 2010 at 10:49 AM, Thomas Rast wrote:\n> Since 9d2e942 (decode file:// and ssh:// URLs, 2010-05-23) the URL\n> logic unquotes escaped URLs.  For the %2B type of escape, this is\n> conformant with RFC 2396.  However, it also unquotes + into a space\n> character, which is only appropriate for the query strings in HTTP.\n> This notably broke fetching from the gtk+ repository.\n>\n> We cannot just remove the corresponding code since the same\n> url_decode_internal() is also used by the HTTP backend to decode query\n> parameters.  Introduce a new argument that controls whether the +\n> decoding happens, and use it only in the (client-side) url_decode().\n>\n> Reported-by: Jasper St. Pierre \n> Signed-off-by: Thomas Rast \n> ---\n>\n> I wrote:\n>> Junio C Hamano wrote:\n>> >\n>> >   http-backend.c::get_info_refs()\n>> >    -> http-backend.c::get_parameter()\n>> >      -> http-backend.c::get_parameters()\n>> >        -> url.c::url_decode_parameter_value()\n>> >          -> url.c::url_decode_internal()\n>>\n>> You're right, I forgot about those.  I imagine it would be one of two\n>> cases:\n> [...]\n>> Shawn, can you help with this?\n>\n> The third case, of course, is:\n>\n> * It only uses these functions for parameter decoding, which of course\n>  was correct to begin with.\n>\n> So after hopefully drinking enough coffee, I made this one.  The catch\n> is that I'm not entirely clear whether *not* decoding the +\n> client-side anywhere in the URL is correct for http:// URLs?  If the\n> client decodes and re-encodes the URL, then the + would be turned into\n> a %2B on the re-encoding.  Then again maybe UI-facing URLs should\n> never have a query part at all?\n>\n>\n>  t/t5601-clone.sh |   10 ++++++++--\n>  url.c            |   11 ++++++-----\n>  2 files changed, 14 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 8abb71a..4431dfd 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -178,8 +178,14 @@ test_expect_success 'clone respects global branch.autosetuprebase' '\n>\n>  test_expect_success 'respect url-encoding of file://' '\n>        git init x+y &&\n> -       test_must_fail git clone \"file://$PWD/x+y\" xy-url &&\n> -       git clone \"file://$PWD/x%2By\" xy-url\n> +       git clone \"file://$PWD/x+y\" xy-url-1 &&\n> +       git clone \"file://$PWD/x%2By\" xy-url-2\n> +'\n> +\n> +test_expect_success 'do not query-string-decode + in URLs' '\n> +       rm -rf x+y &&\n> +       git init \"x y\" &&\n> +       test_must_fail git clone \"file://$PWD/x+y\" xy-no-plus\n>  '\n>\n>  test_expect_success 'do not respect url-encoding of non-url path' '\n> diff --git a/url.c b/url.c\n> index 2306236..cd8f74f 100644\n> --- a/url.c\n> +++ b/url.c\n> @@ -67,7 +67,8 @@ static int url_decode_char(const char *q)\n>        return val;\n>  }\n>\n> -static char *url_decode_internal(const char **query, const char *stop_at, struct strbuf *out)\n> +static char *url_decode_internal(const char **query, const char *stop_at,\n> +                                struct strbuf *out, int decode_plus)\n>  {\n>        const char *q = *query;\n>\n> @@ -90,7 +91,7 @@ static char *url_decode_internal(const char **query, const char *stop_at, struct\n>                        }\n>                }\n>\n> -               if (c == '+')\n> +               if (decode_plus && c == '+')\n>                        strbuf_addch(out, ' ');\n>                else\n>                        strbuf_addch(out, c);\n> @@ -110,17 +111,17 @@ char *url_decode(const char *url)\n>                strbuf_add(&out, url, colon - url);\n>                url = colon;\n>        }\n> -       return url_decode_internal(&url, NULL, &out);\n> +       return url_decode_internal(&url, NULL, &out, 0);\n>  }\n>\n>  char *url_decode_parameter_name(const char **query)\n>  {\n>        struct strbuf out = STRBUF_INIT;\n> -       return url_decode_internal(query, \"&=\", &out);\n> +       return url_decode_internal(query, \"&=\", &out, 1);\n>  }\n>\n>  char *url_decode_parameter_value(const char **query)\n>  {\n>        struct strbuf out = STRBUF_INIT;\n> -       return url_decode_internal(query, \"&\", &out);\n> +       return url_decode_internal(query, \"&\", &out, 1);\n>  }\n> --\n> 1.7.2.278.g76edd.dirty\n>\n>\n"},{"id":"146875","messageId":"201007312333.38471.trast@student.ethz.ch","threadId":"24481","inReplyTo":"AANLkTikY_ayRzdbQ2Qsf6wus+=yg99aTLDTahVXUw0eK@mail.gmail.com","subject":"Re: [PATCH v2] Do not unquote + into ' ' in URLs","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-07-31T21:33:38Z","receivedAt":"2010-07-31T21:33:38Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jasper St. Pierre wrote:\n> Hmm.. could we get a follow-up on this patch? It's annoying because it's both\n> backwards and forwards incompatible: the %2b workaround doesn't work in\n> older versions.\n\nI was hoping for an Ack from someone who knows the whole HTTP\nbusiness, because I don't feel confident enough to say it won't break\nanything there.\n\nTo be precise, if the client ever attempts to decode URLs with query\nstrings (for whatever reasons) then it would break. Probably there\nare no such URLs, but I don't know for sure.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"147306","messageId":"i3gp6i$qap$1@dough.gmane.org","threadId":"24481","inReplyTo":"201007312333.38471.trast@student.ethz.ch","subject":"Re: [PATCH v2] Do not unquote + into ' ' in URLs","fromName":"Ralf Ebert","fromEmail":"info@ralfebert.de","sentAt":"2010-08-06T10:46:42Z","receivedAt":"2010-08-06T10:46:42Z","isPatch":true,"sender":{"key":"info@ralfebert.de","avatar":"https://gravatar.com/avatar/4cd7ff4e970e3882f949fb8f42b39141307e5e504d199129f1377c9c1aea0cb9?d=mp&s=160"},"body":"Hi,\n\n> To be precise, if the client ever attempts to decode URLs with query\n> strings (for whatever reasons) then it would break. Probably there\n> are no such URLs, but I don't know for sure.\n\nurl_decode seems to be called only from connect.c to support \npercent-encoding for git/ssh-URLs, so at least as of now, there should \nbe no problem here. Since applying the patch, git clone \n'git://git.gnome.org/gtk+' works ok again for me.\n\nGreetings,\n\nRalf\n"}]}