Re: [PATCH] remote-curl: ensure that URLs do not have a trailing slash

8 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH] remote-curl: ensure that URLs do not have a trailing slash

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:34

Tay Ray Chuan [off-list ref] writes:

quoted hunk

Signed-off-by: Tay Ray Chuan <redacted>
---
 remote-curl.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 0782756..0f21f8a 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -101,7 +101,8 @@ static struct discovery* discover_refs(const char *service)
 		return last;
 	free_discovery(last);
 
-	strbuf_addf(&buffer, "%s/info/refs", url);
+	end_url_with_slash(&buffer, url);
+	strbuf_addstr(&buffer, "info/refs");
 	if (!prefixcmp(url, "http://") || !prefixcmp(url, "https://")) {
 		is_http = 1;
 		if (!strchr(url, '?'))

@@ -120,7 +121,8 @@ static struct discovery* discover_refs(const char *service)
 		strbuf_reset(&buffer);
 
 		proto_git_candidate = 0;
-		strbuf_addf(&buffer, "%s/info/refs", url);
+		end_url_with_slash(&buffer, url);
+		strbuf_addstr(&buffer, "info/refs");
 		refs_url = strbuf_detach(&buffer, NULL);
 
 		http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);

@@ -511,7 +513,8 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
 	rpc->out = client.out;
 	strbuf_init(&rpc->result, 0);
 
-	strbuf_addf(&buf, "%s/%s", url, svc);
+	end_url_with_slash(&buf, url)
+	strbuf_addf(&buf, "%s", svc);
 	rpc->service_url = strbuf_detach(&buf, NULL);
 
 	strbuf_addf(&buf, "Content-Type: application/x-%s-request", svc);

The code does not look like it is making sure things do not have a
trailing slash.  For example, if svc has trailing slash, either %s/%s or
strbuf_addf() would have the last character of svc in rpc->service_url.

You are fixing something else.

The new code avoids duplicated slashes in the middle if "url" can but does
not necessarily end with a slash.  Is that what you are trying to fix?  I
wonder if it makes more sense to make sure "url" always ends with a slash
at the calling sites where it is obtained from either the end user or from
the running environment...

Re: [PATCH] remote-curl: ensure that URLs do not have a trailing slash

From: Tay Ray Chuan <hidden>
Date: 2016-06-15 22:48:34

Hi,

On Wed, Apr 7, 2010 at 11:12 PM, Junio C Hamano [off-list ref] wrote:

The code does not look like it is making sure things do not have a
trailing slash.  For example, if svc has trailing slash, either %s/%s or
strbuf_addf() would have the last character of svc in rpc->service_url.

You are fixing something else.

The new code avoids duplicated slashes in the middle if "url" can but does
not necessarily end with a slash.  Is that what you are trying to fix?

the commit message is misleading, yes. I'll have another go at it.

 I
wonder if it makes more sense to make sure "url" always ends with a slash
at the calling sites where it is obtained from either the end user or from
the running environment...

Good idea.

-- 
Cheers,
Ray Chuan

[PATCH v2 1/2] http: make end_url_with_slash() public

From: Tay Ray Chuan <hidden>
Date: 2016-06-15 22:48:34

---
 http.c |    2 +-
 http.h |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index 51253e1..07a03fd 100644
--- a/http.c
+++ b/http.c

@@ -720,7 +720,7 @@ static inline int hex(int v)
 		return 'A' + v - 10;
 }
 
-static void end_url_with_slash(struct strbuf *buf, const char *url)
+void end_url_with_slash(struct strbuf *buf, const char *url)
 {
 	strbuf_addstr(buf, url);
 	if (buf->len && buf->buf[buf->len - 1] != '/')

diff --git a/http.h b/http.h
index 2dd03e8..37a6a6a 100644
--- a/http.h
+++ b/http.h

@@ -117,6 +117,7 @@ extern void append_remote_object_url(struct strbuf *buf, const char *url,
 				     int only_two_digit_prefix);
 extern char *get_remote_object_url(const char *url, const char *hex,
 				   int only_two_digit_prefix);
+extern void end_url_with_slash(struct strbuf *buf, const char *url);
 
 /* Options for http_request_*() */
 #define HTTP_NO_CACHE		1

-- 
1.6.6.1368.g82eeb


>From 38c2aba1e77591d9cd57790d9a993f59659496ae Mon Sep 17 00:00:00 2001
From: Tay Ray Chuan <rctay89@gmail.com>
Date: Wed, 7 Apr 2010 23:44:01 +0800
Subject: [PATCH v2 2/2] remote-curl: ensure that URLs have a trailing slash

---
 remote-curl.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 0782756..ae14137 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -9,6 +9,10 @@
 #include "sideband.h"
 
 static struct remote *remote;
+
+/* At assignment-time, we append a trailing slash; always use this as
+ * if it ends with a slash.
+ */
 static const char *url;
 
 struct options {
@@ -101,7 +105,7 @@ static struct discovery* discover_refs(const char *service)
 		return last;
 	free_discovery(last);
 
-	strbuf_addf(&buffer, "%s/info/refs", url);
+	strbuf_addf(&buffer, "%sinfo/refs", url);
 	if (!prefixcmp(url, "http://") || !prefixcmp(url, "https://")) {
 		is_http = 1;
 		if (!strchr(url, '?'))
@@ -120,7 +124,7 @@ static struct discovery* discover_refs(const char *service)
 		strbuf_reset(&buffer);
 
 		proto_git_candidate = 0;
-		strbuf_addf(&buffer, "%s/info/refs", url);
+		strbuf_addf(&buffer, "%sinfo/refs", url);
 		refs_url = strbuf_detach(&buffer, NULL);
 
 		http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);
@@ -511,7 +515,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
 	rpc->out = client.out;
 	strbuf_init(&rpc->result, 0);
 
-	strbuf_addf(&buf, "%s/%s", url, svc);
+	strbuf_addf(&buf, "%s%s", url, svc);
 	rpc->service_url = strbuf_detach(&buf, NULL);
 
 	strbuf_addf(&buf, "Content-Type: application/x-%s-request", svc);
@@ -800,11 +804,13 @@ int main(int argc, const char **argv)
 	remote = remote_get(argv[1]);
 
 	if (argc > 2) {
-		url = argv[2];
+		end_url_with_slash(&buf, argv[2]);
 	} else {
-		url = remote->url[0];
+		end_url_with_slash(&buf, remote->url[0]);
 	}
 
+	url = strbuf_detach(&buf, NULL);
+
 	http_init(remote);
 
 	do {
-- 
1.6.6.1368.g82eeb

[PATCH v2 resend 1/2] http: make end_url_with_slash() public

From: Tay Ray Chuan <hidden>
Date: 2016-06-15 22:48:34

---
 http.c |    2 +-
 http.h |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index 51253e1..07a03fd 100644
--- a/http.c
+++ b/http.c

@@ -720,7 +720,7 @@ static inline int hex(int v)
 		return 'A' + v - 10;
 }
 
-static void end_url_with_slash(struct strbuf *buf, const char *url)
+void end_url_with_slash(struct strbuf *buf, const char *url)
 {
 	strbuf_addstr(buf, url);
 	if (buf->len && buf->buf[buf->len - 1] != '/')

diff --git a/http.h b/http.h
index 2dd03e8..37a6a6a 100644
--- a/http.h
+++ b/http.h

@@ -117,6 +117,7 @@ extern void append_remote_object_url(struct strbuf *buf, const char *url,
 				     int only_two_digit_prefix);
 extern char *get_remote_object_url(const char *url, const char *hex,
 				   int only_two_digit_prefix);
+extern void end_url_with_slash(struct strbuf *buf, const char *url);
 
 /* Options for http_request_*() */
 #define HTTP_NO_CACHE		1

-- 
1.6.6.1368.g82eeb

[PATCH v2 resend 2/2] remote-curl: ensure that URLs have a trailing slash

From: Tay Ray Chuan <hidden>
Date: 2016-06-15 22:48:34

---
 remote-curl.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 0782756..ae14137 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -9,6 +9,10 @@
 #include "sideband.h"
 
 static struct remote *remote;
+
+/* At assignment-time, we append a trailing slash; always use this as
+ * if it ends with a slash.
+ */
 static const char *url;
 
 struct options {

@@ -101,7 +105,7 @@ static struct discovery* discover_refs(const char *service)
 		return last;
 	free_discovery(last);
 
-	strbuf_addf(&buffer, "%s/info/refs", url);
+	strbuf_addf(&buffer, "%sinfo/refs", url);
 	if (!prefixcmp(url, "http://") || !prefixcmp(url, "https://")) {
 		is_http = 1;
 		if (!strchr(url, '?'))

@@ -120,7 +124,7 @@ static struct discovery* discover_refs(const char *service)
 		strbuf_reset(&buffer);
 
 		proto_git_candidate = 0;
-		strbuf_addf(&buffer, "%s/info/refs", url);
+		strbuf_addf(&buffer, "%sinfo/refs", url);
 		refs_url = strbuf_detach(&buffer, NULL);
 
 		http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);

@@ -511,7 +515,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
 	rpc->out = client.out;
 	strbuf_init(&rpc->result, 0);
 
-	strbuf_addf(&buf, "%s/%s", url, svc);
+	strbuf_addf(&buf, "%s%s", url, svc);
 	rpc->service_url = strbuf_detach(&buf, NULL);
 
 	strbuf_addf(&buf, "Content-Type: application/x-%s-request", svc);

@@ -800,11 +804,13 @@ int main(int argc, const char **argv)
 	remote = remote_get(argv[1]);
 
 	if (argc > 2) {
-		url = argv[2];
+		end_url_with_slash(&buf, argv[2]);
 	} else {
-		url = remote->url[0];
+		end_url_with_slash(&buf, remote->url[0]);
 	}
 
+	url = strbuf_detach(&buf, NULL);
+
 	http_init(remote);
 
 	do {

-- 
1.6.6.1368.g82eeb

[PATCH v3 1/3] t5541-http-push: add test for URLs with trailing slash

From: Tay Ray Chuan <hidden>
Date: 2016-06-15 22:48:34

Signed-off-by: Tay Ray Chuan <redacted>
---
 t/t5541-http-push.sh |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 795dc2b..e94ac78 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh

@@ -34,8 +34,34 @@ test_expect_success 'setup remote repository' '
 	mv test_repo.git "$HTTPD_DOCUMENT_ROOT_PATH"
 '
 
-test_expect_success 'clone remote repository' '
+cat >exp <<EOF
+GET  /smart/test_repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
+POST /smart/test_repo.git/git-upload-pack HTTP/1.1 200
+EOF
+test_expect_failure 'no empty path components' '
+	# In the URL, add a trailing slash, and see if git appends yet another
+	# slash.
 	cd "$ROOT_PATH" &&
+	git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
+
+	sed -e "
+		s/^.* \"//
+		s/\"//
+		s/ [1-9][0-9]*\$//
+		s/^GET /GET  /
+	" >act <"$HTTPD_ROOT_PATH"/access.log &&
+
+	# Clear the log, so that it does not affect the "used receive-pack
+	# service" test which reads the log too.
+	#
+	# We do this before the actual comparison to ensure the log is cleared.
+	echo > "$HTTPD_ROOT_PATH"/access.log &&
+
+	test_cmp exp act
+'
+
+test_expect_success 'clone remote repository' '
+	rm -rf test_repo_clone &&
 	git clone $HTTPD_URL/smart/test_repo.git test_repo_clone
 '

@@ -68,6 +94,7 @@ test_expect_success 'create and delete remote branch' '
 '
 
 cat >exp <<EOF
+
 GET  /smart/test_repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
 POST /smart/test_repo.git/git-upload-pack HTTP/1.1 200
 GET  /smart/test_repo.git/info/refs?service=git-receive-pack HTTP/1.1 200

-- 
1.6.6.1368.g82eeb

[PATCH v3 3/3] remote-curl: ensure that URLs have a trailing slash

From: Tay Ray Chuan <hidden>
Date: 2016-06-15 22:48:34

Previously, we blindly assumed that URLs passed to the remote-curl
helper did not end with a trailing slash.

Use the convenience function end_url_with_slash() from http.[ch] to
ensure that URLs have a trailing slash on invocation of the remote-curl
helper, and use the URL as one with a trailing slash throughout.

It is possible for users to pass a URL with a trailing slash to
remote-curl, by, say, setting it in remote.<name>.url in their git
config. The resulting requests have an empty path component (//) and may
break implementations of the http git protocol.

Signed-off-by: Tay Ray Chuan <redacted>
---
 remote-curl.c        |   16 +++++++++++-----
 t/t5541-http-push.sh |    2 +-
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 0782756..ae14137 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -9,6 +9,10 @@
 #include "sideband.h"
 
 static struct remote *remote;
+
+/* At assignment-time, we append a trailing slash; always use this as
+ * if it ends with a slash.
+ */
 static const char *url;
 
 struct options {

@@ -101,7 +105,7 @@ static struct discovery* discover_refs(const char *service)
 		return last;
 	free_discovery(last);
 
-	strbuf_addf(&buffer, "%s/info/refs", url);
+	strbuf_addf(&buffer, "%sinfo/refs", url);
 	if (!prefixcmp(url, "http://") || !prefixcmp(url, "https://")) {
 		is_http = 1;
 		if (!strchr(url, '?'))

@@ -120,7 +124,7 @@ static struct discovery* discover_refs(const char *service)
 		strbuf_reset(&buffer);
 
 		proto_git_candidate = 0;
-		strbuf_addf(&buffer, "%s/info/refs", url);
+		strbuf_addf(&buffer, "%sinfo/refs", url);
 		refs_url = strbuf_detach(&buffer, NULL);
 
 		http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);

@@ -511,7 +515,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
 	rpc->out = client.out;
 	strbuf_init(&rpc->result, 0);
 
-	strbuf_addf(&buf, "%s/%s", url, svc);
+	strbuf_addf(&buf, "%s%s", url, svc);
 	rpc->service_url = strbuf_detach(&buf, NULL);
 
 	strbuf_addf(&buf, "Content-Type: application/x-%s-request", svc);

@@ -800,11 +804,13 @@ int main(int argc, const char **argv)
 	remote = remote_get(argv[1]);
 
 	if (argc > 2) {
-		url = argv[2];
+		end_url_with_slash(&buf, argv[2]);
 	} else {
-		url = remote->url[0];
+		end_url_with_slash(&buf, remote->url[0]);
 	}
 
+	url = strbuf_detach(&buf, NULL);
+
 	http_init(remote);
 
 	do {

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index e94ac78..17e1bdc 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh

@@ -38,7 +38,7 @@ cat >exp <<EOF
 GET  /smart/test_repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
 POST /smart/test_repo.git/git-upload-pack HTTP/1.1 200
 EOF
-test_expect_failure 'no empty path components' '
+test_expect_success 'no empty path components' '
 	# In the URL, add a trailing slash, and see if git appends yet another
 	# slash.
 	cd "$ROOT_PATH" &&

-- 
1.6.6.1368.g82eeb

[PATCH v3 2/3] http: make end_url_with_slash() public

From: Tay Ray Chuan <hidden>
Date: 2016-06-15 22:48:34

Signed-off-by: Tay Ray Chuan <redacted>
---
 http.c |    2 +-
 http.h |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index 51253e1..07a03fd 100644
--- a/http.c
+++ b/http.c

@@ -720,7 +720,7 @@ static inline int hex(int v)
 		return 'A' + v - 10;
 }
 
-static void end_url_with_slash(struct strbuf *buf, const char *url)
+void end_url_with_slash(struct strbuf *buf, const char *url)
 {
 	strbuf_addstr(buf, url);
 	if (buf->len && buf->buf[buf->len - 1] != '/')

diff --git a/http.h b/http.h
index 2dd03e8..37a6a6a 100644
--- a/http.h
+++ b/http.h

@@ -117,6 +117,7 @@ extern void append_remote_object_url(struct strbuf *buf, const char *url,
 				     int only_two_digit_prefix);
 extern char *get_remote_object_url(const char *url, const char *hex,
 				   int only_two_digit_prefix);
+extern void end_url_with_slash(struct strbuf *buf, const char *url);
 
 /* Options for http_request_*() */
 #define HTTP_NO_CACHE		1

-- 
1.6.6.1368.g82eeb

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help