{"thread":{"id":"23721","subject":"Bug with URL-decoding","startedAt":"2010-05-06T22:06:50Z","lastAt":"2010-05-30T05:30:02Z","messageCount":6,"participants":["Dave Abrahams","Jeff King","Ilari Liusvaara"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"141090","messageId":"loom.20100507T000406-241@post.gmane.org","threadId":"23721","inReplyTo":null,"subject":"Bug with URL-decoding","fromName":"Dave Abrahams","fromEmail":"dave@boostpro.com","sentAt":"2010-05-06T22:06:50Z","receivedAt":"2010-05-06T22:06:50Z","isPatch":false,"sender":{"key":"dave@boostpro.com","avatar":"https://gravatar.com/avatar/df0921f05114687777894565de21c052fb137ba7c303a399528b43d08833f065?d=mp&s=160"},"body":"$ cd /tmp\n$ mkdir x+y\n$ cd x+y\n$ git init\n$ cd ..\n$ git clone file:///tmp/x%2By zz  # <==== ERROR\n$ git clone file:///tmp/x+y zz # <====== OK\n\nFunny characters do come up in names, especially in test suites\n"},{"id":"142134","messageId":"20100523091612.GB26123@coredump.intra.peff.net","threadId":"23721","inReplyTo":"loom.20100507T000406-241@post.gmane.org","subject":"Re: Bug with URL-decoding","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-05-23T09:16:13Z","receivedAt":"2010-05-23T09:16:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 06, 2010 at 10:06:50PM +0000, Dave Abrahams wrote:\n\n> $ cd /tmp\n> $ mkdir x+y\n> $ cd x+y\n> $ git init\n> $ cd ..\n> $ git clone file:///tmp/x%2By zz  # <==== ERROR\n> $ git clone file:///tmp/x+y zz # <====== OK\n> \n> Funny characters do come up in names, especially in test suites\n\nYeah, at this point file:// is more or less just an alias for a straight\npath (and ssh:// an alias for \"host:path\" syntax). We correctly\npercent-decode http:// URLs because we just hand them to curl, which\ndoes the right thing. We should be decoding these ones when we pick\napart the URL components.\n\nPatch series to follow:\n\n  [1/2]: make url-related functions reusable\n  [2/2]: decode file:// and ssh:// URLs\n\nWith these patches, you can do:\n\n  git clone file:///tmp/x%2By\n\nor\n\n  git clone /tmp/x+y\n\nThe former will clone into \"x%2By\". Which I'm not even sure is wrong, so\nI left it as-is.\n\n-Peff\n"},{"id":"142135","messageId":"20100523091755.GA16520@coredump.intra.peff.net","threadId":"23721","inReplyTo":"20100523091612.GB26123@coredump.intra.peff.net","subject":"[PATCH 1/2] make url-related functions reusable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-05-23T09:17:55Z","receivedAt":"2010-05-23T09:17:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The is_url function and url percent-decoding functions were\nstatic, but are generally useful. Let's make them available\nto other parts of the code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThere was some minor tweaking of the url_decode functions to be more\ngenerally useful.\n\n Makefile       |    1 +\n http-backend.c |   59 +--------------------------\n transport.c    |   51 +-----------------------\n url.c          |  118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n url.h          |   10 +++++\n 5 files changed, 134 insertions(+), 105 deletions(-)\n create mode 100644 url.c\n create mode 100644 url.h\n\ndiff --git a/Makefile b/Makefile\nindex 07cab8f..4785f3f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -627,6 +627,7 @@ LIB_OBJS += tree-diff.o\n LIB_OBJS += tree.o\n LIB_OBJS += tree-walk.o\n LIB_OBJS += unpack-trees.o\n+LIB_OBJS += url.o\n LIB_OBJS += usage.o\n LIB_OBJS += userdiff.o\n LIB_OBJS += utf8.o\ndiff --git a/http-backend.c b/http-backend.c\nindex d1e83d0..f0e787e 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -6,6 +6,7 @@\n #include \"exec_cmd.h\"\n #include \"run-command.h\"\n #include \"string-list.h\"\n+#include \"url.h\"\n \n static const char content_type[] = \"Content-Type\";\n static const char content_length[] = \"Content-Length\";\n@@ -25,60 +26,6 @@ static struct rpc_service rpc_service[] = {\n \t{ \"receive-pack\", \"receivepack\", -1 },\n };\n \n-static int decode_char(const char *q)\n-{\n-\tint i;\n-\tunsigned char val = 0;\n-\tfor (i = 0; i < 2; i++) {\n-\t\tunsigned char c = *q++;\n-\t\tval <<= 4;\n-\t\tif (c >= '0' && c <= '9')\n-\t\t\tval += c - '0';\n-\t\telse if (c >= 'a' && c <= 'f')\n-\t\t\tval += c - 'a' + 10;\n-\t\telse if (c >= 'A' && c <= 'F')\n-\t\t\tval += c - 'A' + 10;\n-\t\telse\n-\t\t\treturn -1;\n-\t}\n-\treturn val;\n-}\n-\n-static char *decode_parameter(const char **query, int is_name)\n-{\n-\tconst char *q = *query;\n-\tstruct strbuf out;\n-\n-\tstrbuf_init(&out, 16);\n-\tdo {\n-\t\tunsigned char c = *q;\n-\n-\t\tif (!c)\n-\t\t\tbreak;\n-\t\tif (c == '&' || (is_name && c == '=')) {\n-\t\t\tq++;\n-\t\t\tbreak;\n-\t\t}\n-\n-\t\tif (c == '%') {\n-\t\t\tint val = decode_char(q + 1);\n-\t\t\tif (0 <= val) {\n-\t\t\t\tstrbuf_addch(&out, val);\n-\t\t\t\tq += 3;\n-\t\t\t\tcontinue;\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\tq++;\n-\t} while (1);\n-\t*query = q;\n-\treturn strbuf_detach(&out, NULL);\n-}\n-\n static struct string_list *get_parameters(void)\n {\n \tif (!query_params) {\n@@ -86,8 +33,8 @@ static struct string_list *get_parameters(void)\n \n \t\tquery_params = xcalloc(1, sizeof(*query_params));\n \t\twhile (query && *query) {\n-\t\t\tchar *name = decode_parameter(&query, 1);\n-\t\t\tchar *value = decode_parameter(&query, 0);\n+\t\t\tchar *name = url_decode_parameter_name(&query);\n+\t\t\tchar *value = url_decode_parameter_value(&query);\n \t\t\tstruct string_list_item *i;\n \n \t\t\ti = string_list_lookup(name, query_params);\ndiff --git a/transport.c b/transport.c\nindex 8ce3936..4dba6f8 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -9,6 +9,7 @@\n #include \"dir.h\"\n #include \"refs.h\"\n #include \"branch.h\"\n+#include \"url.h\"\n \n /* rsync support */\n \n@@ -871,54 +872,6 @@ static int is_file(const char *url)\n \treturn S_ISREG(buf.st_mode);\n }\n \n-static int isurlschemechar(int first_flag, int ch)\n-{\n-\t/*\n-\t * The set of valid URL schemes, as per STD66 (RFC3986) is\n-\t * '[A-Za-z][A-Za-z0-9+.-]*'. But use sightly looser check\n-\t * of '[A-Za-z0-9][A-Za-z0-9+.-]*' because earlier version\n-\t * of check used '[A-Za-z0-9]+' so not to break any remote\n-\t * helpers.\n-\t */\n-\tint alphanumeric, special;\n-\talphanumeric = ch > 0 && isalnum(ch);\n-\tspecial = ch == '+' || ch == '-' || ch == '.';\n-\treturn alphanumeric || (!first_flag && special);\n-}\n-\n-static int is_url(const char *url)\n-{\n-\tconst char *url2, *first_slash;\n-\n-\tif (!url)\n-\t\treturn 0;\n-\turl2 = url;\n-\tfirst_slash = strchr(url, '/');\n-\n-\t/* Input with no slash at all or slash first can't be URL. */\n-\tif (!first_slash || first_slash == url)\n-\t\treturn 0;\n-\t/* Character before must be : and next must be /. */\n-\tif (first_slash[-1] != ':' || first_slash[1] != '/')\n-\t\treturn 0;\n-\t/* There must be something before the :// */\n-\tif (first_slash == url + 1)\n-\t\treturn 0;\n-\t/*\n-\t * Check all characters up to first slash - 1. Only alphanum\n-\t * is allowed.\n-\t */\n-\turl2 = url;\n-\twhile (url2 < first_slash - 1) {\n-\t\tif (!isurlschemechar(url2 == url, (unsigned char)*url2))\n-\t\t\treturn 0;\n-\t\turl2++;\n-\t}\n-\n-\t/* Valid enough. */\n-\treturn 1;\n-}\n-\n static int external_specification_len(const char *url)\n {\n \treturn strchr(url, ':') - url;\n@@ -946,7 +899,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \tif (url) {\n \t\tconst char *p = url;\n \n-\t\twhile (isurlschemechar(p == url, *p))\n+\t\twhile (is_urlschemechar(p == url, *p))\n \t\t\tp++;\n \t\tif (!prefixcmp(p, \"::\"))\n \t\t\thelper = xstrndup(url, p - url);\ndiff --git a/url.c b/url.c\nnew file mode 100644\nindex 0000000..cd32b92\n--- /dev/null\n+++ b/url.c\n@@ -0,0 +1,118 @@\n+#include \"cache.h\"\n+\n+int is_urlschemechar(int first_flag, int ch)\n+{\n+\t/*\n+\t * The set of valid URL schemes, as per STD66 (RFC3986) is\n+\t * '[A-Za-z][A-Za-z0-9+.-]*'. But use sightly looser check\n+\t * of '[A-Za-z0-9][A-Za-z0-9+.-]*' because earlier version\n+\t * of check used '[A-Za-z0-9]+' so not to break any remote\n+\t * helpers.\n+\t */\n+\tint alphanumeric, special;\n+\talphanumeric = ch > 0 && isalnum(ch);\n+\tspecial = ch == '+' || ch == '-' || ch == '.';\n+\treturn alphanumeric || (!first_flag && special);\n+}\n+\n+int is_url(const char *url)\n+{\n+\tconst char *url2, *first_slash;\n+\n+\tif (!url)\n+\t\treturn 0;\n+\turl2 = url;\n+\tfirst_slash = strchr(url, '/');\n+\n+\t/* Input with no slash at all or slash first can't be URL. */\n+\tif (!first_slash || first_slash == url)\n+\t\treturn 0;\n+\t/* Character before must be : and next must be /. */\n+\tif (first_slash[-1] != ':' || first_slash[1] != '/')\n+\t\treturn 0;\n+\t/* There must be something before the :// */\n+\tif (first_slash == url + 1)\n+\t\treturn 0;\n+\t/*\n+\t * Check all characters up to first slash - 1. Only alphanum\n+\t * is allowed.\n+\t */\n+\turl2 = url;\n+\twhile (url2 < first_slash - 1) {\n+\t\tif (!is_urlschemechar(url2 == url, (unsigned char)*url2))\n+\t\t\treturn 0;\n+\t\turl2++;\n+\t}\n+\n+\t/* Valid enough. */\n+\treturn 1;\n+}\n+\n+static int url_decode_char(const char *q)\n+{\n+\tint i;\n+\tunsigned char val = 0;\n+\tfor (i = 0; i < 2; i++) {\n+\t\tunsigned char c = *q++;\n+\t\tval <<= 4;\n+\t\tif (c >= '0' && c <= '9')\n+\t\t\tval += c - '0';\n+\t\telse if (c >= 'a' && c <= 'f')\n+\t\t\tval += c - 'a' + 10;\n+\t\telse if (c >= 'A' && c <= 'F')\n+\t\t\tval += c - 'A' + 10;\n+\t\telse\n+\t\t\treturn -1;\n+\t}\n+\treturn val;\n+}\n+\n+static char *url_decode_internal(const char **query, const char *stop_at)\n+{\n+\tconst char *q = *query;\n+\tstruct strbuf out;\n+\n+\tstrbuf_init(&out, 16);\n+\tdo {\n+\t\tunsigned char c = *q;\n+\n+\t\tif (!c)\n+\t\t\tbreak;\n+\t\tif (stop_at && strchr(stop_at, c)) {\n+\t\t\tq++;\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tif (c == '%') {\n+\t\t\tint val = url_decode_char(q + 1);\n+\t\t\tif (0 <= val) {\n+\t\t\t\tstrbuf_addch(&out, val);\n+\t\t\t\tq += 3;\n+\t\t\t\tcontinue;\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\tq++;\n+\t} while (1);\n+\t*query = q;\n+\treturn strbuf_detach(&out, NULL);\n+}\n+\n+char *url_decode(const char *url)\n+{\n+\treturn url_decode_internal(&url, NULL);\n+}\n+\n+char *url_decode_parameter_name(const char **query)\n+{\n+\treturn url_decode_internal(query, \"&=\");\n+}\n+\n+char *url_decode_parameter_value(const char **query)\n+{\n+\treturn url_decode_internal(query, \"&\");\n+}\ndiff --git a/url.h b/url.h\nnew file mode 100644\nindex 0000000..15817f8\n--- /dev/null\n+++ b/url.h\n@@ -0,0 +1,10 @@\n+#ifndef URL_H\n+#define URL_H\n+\n+extern int is_url(const char *url);\n+extern int is_urlschemechar(int first_flag, int ch);\n+extern char *url_decode(const char *url);\n+extern char *url_decode_parameter_name(const char **query);\n+extern char *url_decode_parameter_value(const char **query);\n+\n+#endif /* URL_H */\n-- \n1.7.1.356.gc7d3.dirty\n"},{"id":"142137","messageId":"20100523091944.GB16520@coredump.intra.peff.net","threadId":"23721","inReplyTo":"20100523091612.GB26123@coredump.intra.peff.net","subject":"[PATCH 2/2] decode file:// and ssh:// URLs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-05-23T09:19:44Z","receivedAt":"2010-05-23T09:19:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We generally treat these as equivalent to \"/path/to/repo\"\nand \"host:path_to_repo\" respectively. However, they are URLs\nand as such may be percent-encoded. The current code simply\nuses them as-is without any decoding.\n\nWith this patch, we will now percent-decode any file:// or\nssh:// url (or ssh+git, git+ssh, etc) at the transport\nlayer. We continue to treat plain paths and \"host:path\"\nsyntax literally.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI think this also impacts git:// URLs. Which I is probably a good thing,\nbut I haven't looked extensively for unexpected fallouts.\n\n connect.c        |    8 +++++++-\n t/t5601-clone.sh |   12 ++++++++++++\n 2 files changed, 19 insertions(+), 1 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 9ae991a..0119bd3 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -5,6 +5,7 @@\n #include \"refs.h\"\n #include \"run-command.h\"\n #include \"remote.h\"\n+#include \"url.h\"\n \n static char *server_capabilities;\n \n@@ -450,7 +451,7 @@ static struct child_process no_fork;\n struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\t\t\t  const char *prog, int flags)\n {\n-\tchar *url = xstrdup(url_orig);\n+\tchar *url;\n \tchar *host, *path;\n \tchar *end;\n \tint c;\n@@ -466,6 +467,11 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t */\n \tsignal(SIGCHLD, SIG_DFL);\n \n+\tif (is_url(url_orig))\n+\t\turl = url_decode(url_orig);\n+\telse\n+\t\turl = xstrdup(url_orig);\n+\n \thost = strstr(url, \"://\");\n \tif (host) {\n \t\t*host = '\\0';\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 678cee5..8abb71a 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -176,4 +176,16 @@ test_expect_success 'clone respects global branch.autosetuprebase' '\n \t)\n '\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+'\n+\n+test_expect_success 'do not respect url-encoding of non-url path' '\n+\tgit init x+y &&\n+\ttest_must_fail git clone x%2By xy-regular &&\n+\tgit clone x+y xy-regular\n+'\n+\n test_done\n-- \n1.7.1.356.gc7d3.dirty\n"},{"id":"142413","messageId":"20100527105014.GA7865@LK-Perkele-V2.elisa-laajakaista.fi","threadId":"23721","inReplyTo":"20100523091944.GB16520@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] decode file:// and ssh:// URLs","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-05-27T10:50:15Z","receivedAt":"2010-05-27T10:50:15Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Sun, May 23, 2010 at 05:19:44AM -0400, Jeff King wrote:\n> We generally treat these as equivalent to \"/path/to/repo\"\n> and \"host:path_to_repo\" respectively. However, they are URLs\n> and as such may be percent-encoded. The current code simply\n> uses them as-is without any decoding.\n> \n> With this patch, we will now percent-decode any file:// or\n> ssh:// url (or ssh+git, git+ssh, etc) at the transport\n> layer. We continue to treat plain paths and \"host:path\"\n> syntax literally.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I think this also impacts git:// URLs. Which I is probably a good thing,\n> but I haven't looked extensively for unexpected fallouts.\n\nOne possible fallout: IPv6 scope syntax uses literal '%' in host\npart. The relevant RFC indicates it should be escaped, but in the past\nconnect would fail if it was... But then, who uses that syntax...\n\n-Ilari\n"},{"id":"142551","messageId":"20100530053002.GA19219@coredump.intra.peff.net","threadId":"23721","inReplyTo":"20100527105014.GA7865@LK-Perkele-V2.elisa-laajakaista.fi","subject":"Re: [PATCH 2/2] decode file:// and ssh:// URLs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-05-30T05:30:02Z","receivedAt":"2010-05-30T05:30:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 27, 2010 at 01:50:15PM +0300, Ilari Liusvaara wrote:\n\n> > With this patch, we will now percent-decode any file:// or\n> > ssh:// url (or ssh+git, git+ssh, etc) at the transport\n> > layer. We continue to treat plain paths and \"host:path\"\n> > syntax literally.\n> \n> One possible fallout: IPv6 scope syntax uses literal '%' in host\n> part. The relevant RFC indicates it should be escaped, but in the past\n> connect would fail if it was... But then, who uses that syntax...\n\nBleh. I am not happy about breaking a syntax people might be using, but\nthe current behavior is broken, which will cause problems for some other\npeople. Short of doing some context-sensitive \"is this probably a\nliteral % in the host\" heuristic (which I really think is a bad idea), I\nthink we will have to break one or the other.\n\n-Peff\n"}]}