Bug with URL-decoding

Bug with URL-decoding

[Dave Abrahams]

$ cd /tmp
$ mkdir x+y
$ cd x+y
$ git init
$ cd ..
$ git clone file:///tmp/x%2By zz  # <==== ERROR
$ git clone file:///tmp/x+y zz # <====== OK

Funny characters do come up in names, especially in test suites

Re: Bug with URL-decoding

[Jeff King]

On Thu, May 06, 2010 at 10:06:50PM +0000, Dave Abrahams wrote:

> $ cd /tmp
> $ mkdir x+y
> $ cd x+y
> $ git init
> $ cd ..
> $ git clone file:///tmp/x%2By zz  # <==== ERROR
> $ git clone file:///tmp/x+y zz # <====== OK
> 
> Funny characters do come up in names, especially in test suites

Yeah, at this point file:// is more or less just an alias for a straight
path (and ssh:// an alias for "host:path" syntax). We correctly
percent-decode http:// URLs because we just hand them to curl, which
does the right thing. We should be decoding these ones when we pick
apart the URL components.

Patch series to follow:

  [1/2]: make url-related functions reusable
  [2/2]: decode file:// and ssh:// URLs

With these patches, you can do:

  git clone file:///tmp/x%2By

or

  git clone /tmp/x+y

The former will clone into "x%2By". Which I'm not even sure is wrong, so
I left it as-is.

-Peff

[PATCH 1/2] make url-related functions reusable

[Jeff King]

The is_url function and url percent-decoding functions were
static, but are generally useful. Let's make them available
to other parts of the code.

Signed-off-by: Jeff King <peff@peff.net>
---
There was some minor tweaking of the url_decode functions to be more
generally useful.

 Makefile       |    1 +
 http-backend.c |   59 +--------------------------
 transport.c    |   51 +-----------------------
 url.c          |  118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 url.h          |   10 +++++
 5 files changed, 134 insertions(+), 105 deletions(-)
 create mode 100644 url.c
 create mode 100644 url.h

diff --git a/Makefile b/Makefile
index 07cab8f..4785f3f 100644
--- a/Makefile
+++ b/Makefile
@@ -627,6 +627,7 @@ LIB_OBJS += tree-diff.o
 LIB_OBJS += tree.o
 LIB_OBJS += tree-walk.o
 LIB_OBJS += unpack-trees.o
+LIB_OBJS += url.o
 LIB_OBJS += usage.o
 LIB_OBJS += userdiff.o
 LIB_OBJS += utf8.o
diff --git a/http-backend.c b/http-backend.c
index d1e83d0..f0e787e 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -6,6 +6,7 @@
 #include "exec_cmd.h"
 #include "run-command.h"
 #include "string-list.h"
+#include "url.h"
 
 static const char content_type[] = "Content-Type";
 static const char content_length[] = "Content-Length";
@@ -25,60 +26,6 @@ static struct rpc_service rpc_service[] = {
 	{ "receive-pack", "receivepack", -1 },
 };
 
-static int decode_char(const char *q)
-{
-	int i;
-	unsigned char val = 0;
-	for (i = 0; i < 2; i++) {
-		unsigned char c = *q++;
-		val <<= 4;
-		if (c >= '0' && c <= '9')
-			val += c - '0';
-		else if (c >= 'a' && c <= 'f')
-			val += c - 'a' + 10;
-		else if (c >= 'A' && c <= 'F')
-			val += c - 'A' + 10;
-		else
-			return -1;
-	}
-	return val;
-}
-
-static char *decode_parameter(const char **query, int is_name)
-{
-	const char *q = *query;
-	struct strbuf out;
-
-	strbuf_init(&out, 16);
-	do {
-		unsigned char c = *q;
-
-		if (!c)
-			break;
-		if (c == '&' || (is_name && c == '=')) {
-			q++;
-			break;
-		}
-
-		if (c == '%') {
-			int val = decode_char(q + 1);
-			if (0 <= val) {
-				strbuf_addch(&out, val);
-				q += 3;
-				continue;
-			}
-		}
-
-		if (c == '+')
-			strbuf_addch(&out, ' ');
-		else
-			strbuf_addch(&out, c);
-		q++;
-	} while (1);
-	*query = q;
-	return strbuf_detach(&out, NULL);
-}
-
 static struct string_list *get_parameters(void)
 {
 	if (!query_params) {
@@ -86,8 +33,8 @@ static struct string_list *get_parameters(void)
 
 		query_params = xcalloc(1, sizeof(*query_params));
 		while (query && *query) {
-			char *name = decode_parameter(&query, 1);
-			char *value = decode_parameter(&query, 0);
+			char *name = url_decode_parameter_name(&query);
+			char *value = url_decode_parameter_value(&query);
 			struct string_list_item *i;
 
 			i = string_list_lookup(name, query_params);
diff --git a/transport.c b/transport.c
index 8ce3936..4dba6f8 100644
--- a/transport.c
+++ b/transport.c
@@ -9,6 +9,7 @@
 #include "dir.h"
 #include "refs.h"
 #include "branch.h"
+#include "url.h"
 
 /* rsync support */
 
@@ -871,54 +872,6 @@ static int is_file(const char *url)
 	return S_ISREG(buf.st_mode);
 }
 
-static int isurlschemechar(int first_flag, int ch)
-{
-	/*
-	 * The set of valid URL schemes, as per STD66 (RFC3986) is
-	 * '[A-Za-z][A-Za-z0-9+.-]*'. But use sightly looser check
-	 * of '[A-Za-z0-9][A-Za-z0-9+.-]*' because earlier version
-	 * of check used '[A-Za-z0-9]+' so not to break any remote
-	 * helpers.
-	 */
-	int alphanumeric, special;
-	alphanumeric = ch > 0 && isalnum(ch);
-	special = ch == '+' || ch == '-' || ch == '.';
-	return alphanumeric || (!first_flag && special);
-}
-
-static int is_url(const char *url)
-{
-	const char *url2, *first_slash;
-
-	if (!url)
-		return 0;
-	url2 = url;
-	first_slash = strchr(url, '/');
-
-	/* Input with no slash at all or slash first can't be URL. */
-	if (!first_slash || first_slash == url)
-		return 0;
-	/* Character before must be : and next must be /. */
-	if (first_slash[-1] != ':' || first_slash[1] != '/')
-		return 0;
-	/* There must be something before the :// */
-	if (first_slash == url + 1)
-		return 0;
-	/*
-	 * Check all characters up to first slash - 1. Only alphanum
-	 * is allowed.
-	 */
-	url2 = url;
-	while (url2 < first_slash - 1) {
-		if (!isurlschemechar(url2 == url, (unsigned char)*url2))
-			return 0;
-		url2++;
-	}
-
-	/* Valid enough. */
-	return 1;
-}
-
 static int external_specification_len(const char *url)
 {
 	return strchr(url, ':') - url;
@@ -946,7 +899,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 	if (url) {
 		const char *p = url;
 
-		while (isurlschemechar(p == url, *p))
+		while (is_urlschemechar(p == url, *p))
 			p++;
 		if (!prefixcmp(p, "::"))
 			helper = xstrndup(url, p - url);
diff --git a/url.c b/url.c
new file mode 100644
index 0000000..cd32b92
--- /dev/null
+++ b/url.c
@@ -0,0 +1,118 @@
+#include "cache.h"
+
+int is_urlschemechar(int first_flag, int ch)
+{
+	/*
+	 * The set of valid URL schemes, as per STD66 (RFC3986) is
+	 * '[A-Za-z][A-Za-z0-9+.-]*'. But use sightly looser check
+	 * of '[A-Za-z0-9][A-Za-z0-9+.-]*' because earlier version
+	 * of check used '[A-Za-z0-9]+' so not to break any remote
+	 * helpers.
+	 */
+	int alphanumeric, special;
+	alphanumeric = ch > 0 && isalnum(ch);
+	special = ch == '+' || ch == '-' || ch == '.';
+	return alphanumeric || (!first_flag && special);
+}
+
+int is_url(const char *url)
+{
+	const char *url2, *first_slash;
+
+	if (!url)
+		return 0;
+	url2 = url;
+	first_slash = strchr(url, '/');
+
+	/* Input with no slash at all or slash first can't be URL. */
+	if (!first_slash || first_slash == url)
+		return 0;
+	/* Character before must be : and next must be /. */
+	if (first_slash[-1] != ':' || first_slash[1] != '/')
+		return 0;
+	/* There must be something before the :// */
+	if (first_slash == url + 1)
+		return 0;
+	/*
+	 * Check all characters up to first slash - 1. Only alphanum
+	 * is allowed.
+	 */
+	url2 = url;
+	while (url2 < first_slash - 1) {
+		if (!is_urlschemechar(url2 == url, (unsigned char)*url2))
+			return 0;
+		url2++;
+	}
+
+	/* Valid enough. */
+	return 1;
+}
+
+static int url_decode_char(const char *q)
+{
+	int i;
+	unsigned char val = 0;
+	for (i = 0; i < 2; i++) {
+		unsigned char c = *q++;
+		val <<= 4;
+		if (c >= '0' && c <= '9')
+			val += c - '0';
+		else if (c >= 'a' && c <= 'f')
+			val += c - 'a' + 10;
+		else if (c >= 'A' && c <= 'F')
+			val += c - 'A' + 10;
+		else
+			return -1;
+	}
+	return val;
+}
+
+static char *url_decode_internal(const char **query, const char *stop_at)
+{
+	const char *q = *query;
+	struct strbuf out;
+
+	strbuf_init(&out, 16);
+	do {
+		unsigned char c = *q;
+
+		if (!c)
+			break;
+		if (stop_at && strchr(stop_at, c)) {
+			q++;
+			break;
+		}
+
+		if (c == '%') {
+			int val = url_decode_char(q + 1);
+			if (0 <= val) {
+				strbuf_addch(&out, val);
+				q += 3;
+				continue;
+			}
+		}
+
+		if (c == '+')
+			strbuf_addch(&out, ' ');
+		else
+			strbuf_addch(&out, c);
+		q++;
+	} while (1);
+	*query = q;
+	return strbuf_detach(&out, NULL);
+}
+
+char *url_decode(const char *url)
+{
+	return url_decode_internal(&url, NULL);
+}
+
+char *url_decode_parameter_name(const char **query)
+{
+	return url_decode_internal(query, "&=");
+}
+
+char *url_decode_parameter_value(const char **query)
+{
+	return url_decode_internal(query, "&");
+}
diff --git a/url.h b/url.h
new file mode 100644
index 0000000..15817f8
--- /dev/null
+++ b/url.h
@@ -0,0 +1,10 @@
+#ifndef URL_H
+#define URL_H
+
+extern int is_url(const char *url);
+extern int is_urlschemechar(int first_flag, int ch);
+extern char *url_decode(const char *url);
+extern char *url_decode_parameter_name(const char **query);
+extern char *url_decode_parameter_value(const char **query);
+
+#endif /* URL_H */
-- 
1.7.1.356.gc7d3.dirty

[PATCH 2/2] decode file:// and ssh:// URLs

[Jeff King]

We generally treat these as equivalent to "/path/to/repo"
and "host:path_to_repo" respectively. However, they are URLs
and as such may be percent-encoded. The current code simply
uses them as-is without any decoding.

With this patch, we will now percent-decode any file:// or
ssh:// url (or ssh+git, git+ssh, etc) at the transport
layer. We continue to treat plain paths and "host:path"
syntax literally.

Signed-off-by: Jeff King <peff@peff.net>
---
I think this also impacts git:// URLs. Which I is probably a good thing,
but I haven't looked extensively for unexpected fallouts.

 connect.c        |    8 +++++++-
 t/t5601-clone.sh |   12 ++++++++++++
 2 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/connect.c b/connect.c
index 9ae991a..0119bd3 100644
--- a/connect.c
+++ b/connect.c
@@ -5,6 +5,7 @@
 #include "refs.h"
 #include "run-command.h"
 #include "remote.h"
+#include "url.h"
 
 static char *server_capabilities;
 
@@ -450,7 +451,7 @@ static struct child_process no_fork;
 struct child_process *git_connect(int fd[2], const char *url_orig,
 				  const char *prog, int flags)
 {
-	char *url = xstrdup(url_orig);
+	char *url;
 	char *host, *path;
 	char *end;
 	int c;
@@ -466,6 +467,11 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 	 */
 	signal(SIGCHLD, SIG_DFL);
 
+	if (is_url(url_orig))
+		url = url_decode(url_orig);
+	else
+		url = xstrdup(url_orig);
+
 	host = strstr(url, "://");
 	if (host) {
 		*host = '\0';
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 678cee5..8abb71a 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -176,4 +176,16 @@ test_expect_success 'clone respects global branch.autosetuprebase' '
 	)
 '
 
+test_expect_success 'respect url-encoding of file://' '
+	git init x+y &&
+	test_must_fail git clone "file://$PWD/x+y" xy-url &&
+	git clone "file://$PWD/x%2By" xy-url
+'
+
+test_expect_success 'do not respect url-encoding of non-url path' '
+	git init x+y &&
+	test_must_fail git clone x%2By xy-regular &&
+	git clone x+y xy-regular
+'
+
 test_done
-- 
1.7.1.356.gc7d3.dirty

Re: [PATCH 2/2] decode file:// and ssh:// URLs

[Ilari Liusvaara]

On Sun, May 23, 2010 at 05:19:44AM -0400, Jeff King wrote:
> We generally treat these as equivalent to "/path/to/repo"
> and "host:path_to_repo" respectively. However, they are URLs
> and as such may be percent-encoded. The current code simply
> uses them as-is without any decoding.
> 
> With this patch, we will now percent-decode any file:// or
> ssh:// url (or ssh+git, git+ssh, etc) at the transport
> layer. We continue to treat plain paths and "host:path"
> syntax literally.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I think this also impacts git:// URLs. Which I is probably a good thing,
> but I haven't looked extensively for unexpected fallouts.

One possible fallout: IPv6 scope syntax uses literal '%' in host
part. The relevant RFC indicates it should be escaped, but in the past
connect would fail if it was... But then, who uses that syntax...

-Ilari

Re: [PATCH 2/2] decode file:// and ssh:// URLs

[Jeff King]

On Thu, May 27, 2010 at 01:50:15PM +0300, Ilari Liusvaara wrote:

> > With this patch, we will now percent-decode any file:// or
> > ssh:// url (or ssh+git, git+ssh, etc) at the transport
> > layer. We continue to treat plain paths and "host:path"
> > syntax literally.
> 
> One possible fallout: IPv6 scope syntax uses literal '%' in host
> part. The relevant RFC indicates it should be escaped, but in the past
> connect would fail if it was... But then, who uses that syntax...

Bleh. I am not happy about breaking a syntax people might be using, but
the current behavior is broken, which will cause problems for some other
people. Short of doing some context-sensitive "is this probably a
literal % in the host" heuristic (which I really think is a bad idea), I
think we will have to break one or the other.

-Peff