Migrate redirect URL handling to common utility

src/netops.c

@@ -573,6 +573,81 @@ int gitno_select_in(gitno_buffer *buf, long int sec, long int usec)
 	return select((int)buf->socket->socket + 1, &fds, NULL, NULL, &tv);
 }
 
+static const char *prefix_http = "http://";
+static const char *prefix_https = "https://";
+
+int gitno_connection_data_from_url(
+		gitno_connection_data *data,
+		const char *url,
+		const char *service_suffix,
+		const char *original_host,
+		bool original_use_ssl)
+{
+	int error = 0;
+	const char *default_port = NULL;
+
+	/* service_suffix is optional */
+	assert(data && url);
+
+	if (!git__prefixcmp(url, prefix_http)) {
+		url = url + strlen(prefix_http);
+		default_port = "80";
+
+		if (data->use_ssl) {
+			giterr_set(GITERR_NET, "Redirect from HTTPS to HTTP is not allowed");
+			return -1;
+		}
+	}
+
+	if (!git__prefixcmp(url, prefix_https)) {
+		url += strlen(prefix_https);
+		default_port = "443";
+		data->use_ssl = true;
+	}
+
+	if (!default_port) {
+		giterr_set(GITERR_NET, "Unrecognized URL prefix");
+		return -1;
+	}
+
+	error = gitno_extract_url_parts(
+		&data->host, &data->port, &data->user, &data->pass,
+		url, default_port);
+
+	if (!error) {
+		const char *path = strchr(url, '/');
+		size_t pathlen = strlen(path);
+		size_t suffixlen = service_suffix ? strlen(service_suffix) : 0;
+
+		if (suffixlen &&
+			!memcmp(path + pathlen - suffixlen, service_suffix, suffixlen))
+			data->path = git__strndup(path, pathlen - suffixlen);
+		else
+			data->path = git__strdup(path);
+
+		/* Check for errors in the resulting data */
+		if (original_use_ssl && !data->use_ssl) {
+			giterr_set(GITERR_NET, "Redirect from HTTPS to HTTP not allowed");
+			error = -1;
+		}
+		if (original_host && url[0] != '/' && strcmp(original_host, data->host)) {
+			giterr_set(GITERR_NET, "Cross host redirect not allowed");
+			error = -1;
+		}
+	}
+
+	return error;
+}
+
+void gitno_connection_data_free_ptrs(gitno_connection_data *d)
+{
+	git__free(d->host); d->host = NULL;
+	git__free(d->port); d->port = NULL;
+	git__free(d->path); d->path = NULL;
+	git__free(d->user); d->user = NULL;
+	git__free(d->pass); d->pass = NULL;
+}
+
 int gitno_extract_url_parts(
 		char **host,
 		char **port,

src/netops.h

@@ -66,6 +66,31 @@ int gitno_send(gitno_socket *socket, const char *msg, size_t len, int flags);
 int gitno_close(gitno_socket *s);
 int gitno_select_in(gitno_buffer *buf, long int sec, long int usec);
 
+typedef struct gitno_connection_data {
+	char *host;
+	char *port;
+	char *path;
+	char *user;
+	char *pass;
+	bool use_ssl;
+} gitno_connection_data;
+
+/*
+ * This replaces all the pointers in `data` with freshly-allocated strings,
+ * that the caller is responsible for freeing.
+ * `gitno_connection_data_free_ptrs` is good for this.
+ */
+
+int gitno_connection_data_from_url(
+		gitno_connection_data *data,
+		const char *url,
+		const char *service_suffix,
+		const char *original_host,
+		bool original_use_ssl);
+
+/* This frees all the pointers IN the struct, but not the struct itself. */
+void gitno_connection_data_free_ptrs(gitno_connection_data *data);
+
 int gitno_extract_url_parts(
 		char **host,
 		char **port,

src/transports/http.c

@@ -67,8 +67,8 @@ typedef struct {
 	git_cred *cred;
 	git_cred *url_cred;
 	http_authmechanism_t auth_mechanism;
-	unsigned connected : 1,
-		use_ssl : 1;
+	bool connected,
+		  use_ssl;
 
 	/* Parser structures */
 	http_parser parser;
@@ -277,70 +277,6 @@ static void free_connection_data(http_subtransport *t)
 	}
 }
 
-static int set_connection_data_from_url(
-	http_subtransport *t, const char *url, const char *service_suffix)
-{
-	int error = 0;
-	const char *default_port = NULL;
-	char *original_host = NULL;
-
-	if (!git__prefixcmp(url, prefix_http)) {
-		url = url + strlen(prefix_http);
-		default_port = "80";
-
-		if (t->use_ssl) {
-			giterr_set(GITERR_NET, "Redirect from HTTPS to HTTP not allowed");
-			return -1;
-		}
-	}
-
-	if (!git__prefixcmp(url, prefix_https)) {
-		url += strlen(prefix_https);
-		default_port = "443";
-		t->use_ssl = 1;
-	}
-
-	if (!default_port) {
-		giterr_set(GITERR_NET, "Unrecognized URL prefix");
-		return -1;
-	}
-
-	/* preserve original host name for checking */
-	original_host = t->host;
-	t->host = NULL;
-
-	free_connection_data(t);
-
-	error = gitno_extract_url_parts(
-		&t->host, &t->port, &t->user_from_url, &t->pass_from_url,
-		url, default_port);
-
-	if (!error) {
-		const char *path = strchr(url, '/');
-		size_t pathlen = strlen(path);
-		size_t suffixlen = service_suffix ? strlen(service_suffix) : 0;
-
-		if (suffixlen &&
-			!memcmp(path + pathlen - suffixlen, service_suffix, suffixlen))
-			t->path = git__strndup(path, pathlen - suffixlen);
-		else
-			t->path = git__strdup(path);
-
-		/* Allow '/'-led urls, or a change of protocol */
-		if (original_host != NULL) {
-			if (strcmp(original_host, t->host) && t->location[0] != '/') {
-				giterr_set(GITERR_NET, "Cross host redirect not allowed");
-				error = -1;
-			}
-
-			git__free(original_host);
-		}
-	}
-
-	return error;
-}
-
-
 static int on_headers_complete(http_parser *parser)
 {
 	parser_context *ctx = (parser_context *) parser->data;
@@ -384,18 +320,30 @@ static int on_headers_complete(http_parser *parser)
 	/* Check for a redirect.
 	 * Right now we only permit a redirect to the same hostname. */
 	if ((parser->status_code == 301 ||
-		 parser->status_code == 302 ||
-		 (parser->status_code == 303 && get_verb == s->verb) ||
-		 parser->status_code == 307) &&
-		t->location) {
+	     parser->status_code == 302 ||
+	     (parser->status_code == 303 && get_verb == s->verb) ||
+	     parser->status_code == 307) &&
+	    t->location) {
+		gitno_connection_data connection_data = {0};
 
 		if (s->redirect_count >= 7) {
 			giterr_set(GITERR_NET, "Too many redirects");
 			return t->parse_error = PARSE_ERROR_GENERIC;
 		}
 
-		if (set_connection_data_from_url(t, t->location, s->service_url) < 0)
+		if (gitno_connection_data_from_url(&connection_data, t->location,
+					s->service_url, t->host, t->use_ssl) < 0) {
+			gitno_connection_data_free_ptrs(&connection_data);
 			return t->parse_error = PARSE_ERROR_GENERIC;
+		}
+
+		free_connection_data(t);
+		t->host = connection_data.host;
+		t->port = connection_data.port;
+		t->path = connection_data.path;
+		t->user_from_url = connection_data.user;
+		t->pass_from_url = connection_data.pass;
+		t->use_ssl = connection_data.use_ssl;
 
 		/* Set the redirect URL on the stream. This is a transfer of
 		 * ownership of the memory. */
@@ -912,8 +860,18 @@ static int http_action(
 		return -1;
 
 	if (!t->host || !t->port || !t->path) {
-		if ((ret = set_connection_data_from_url(t, url, NULL)) < 0)
+		gitno_connection_data data = {0};
+		if ((ret = gitno_connection_data_from_url(&data,
+						url, NULL, NULL, false)) < 0) {
+			gitno_connection_data_free_ptrs(&data);
 			return ret;
+		}
+		t->host = data.host;
+		t->port = data.port;
+		t->path = data.path;
+		t->user_from_url = data.user;
+		t->pass_from_url = data.pass;
+		t->use_ssl = data.use_ssl;
 	}
 
 	if (http_connect(t) < 0)

tests-clar/network/urlparse.c

@@ -2,10 +2,12 @@
 #include "netops.h"
 
 char *host, *port, *user, *pass;
+gitno_connection_data conndata;
 
 void test_network_urlparse__initialize(void)
 {
 	host = port = user = pass = NULL;
+	memset(&conndata, 0, sizeof(conndata));
 }
 
 void test_network_urlparse__cleanup(void)
@@ -15,6 +17,8 @@ void test_network_urlparse__cleanup(void)
 	FREE_AND_NULL(port);
 	FREE_AND_NULL(user);
 	FREE_AND_NULL(pass);
+
+	gitno_connection_data_free_ptrs(&conndata);
 }
 
 void test_network_urlparse__trivial(void)
@@ -80,3 +84,41 @@ void test_network_urlparse__user_pass_port(void)
 	cl_assert_equal_s(user, "user");
 	cl_assert_equal_s(pass, "pass");
 }
+
+void test_network_urlparse__connection_data_http(void)
+{
+	cl_git_pass(gitno_connection_data_from_url(&conndata,
+				"http://example.com/foo/bar/baz", "bar/baz", NULL, false));
+	cl_assert_equal_s(conndata.host, "example.com");
+	cl_assert_equal_s(conndata.port, "80");
+	cl_assert_equal_s(conndata.path, "/foo/");
+	cl_assert_equal_p(conndata.user, NULL);
+	cl_assert_equal_p(conndata.pass, NULL);
+	cl_assert_equal_i(conndata.use_ssl, false);
+}
+
+void test_network_urlparse__connection_data_ssl(void)
+{
+	cl_git_pass(gitno_connection_data_from_url(&conndata,
+				"https://example.com/foo/bar/baz", "bar/baz", NULL, false));
+	cl_assert_equal_s(conndata.host, "example.com");
+	cl_assert_equal_s(conndata.port, "443");
+	cl_assert_equal_s(conndata.path, "/foo/");
+	cl_assert_equal_p(conndata.user, NULL);
+	cl_assert_equal_p(conndata.pass, NULL);
+	cl_assert_equal_i(conndata.use_ssl, true);
+}
+
+void test_network_urlparse__connection_data_cross_host_redirect(void)
+{
+	cl_git_fail_with(gitno_connection_data_from_url(&conndata,
+				"https://foo.com/bar/baz", NULL, "bar.com", true),
+			-1);
+}
+
+void test_network_urlparse__connection_data_http_downgrade(void)
+{
+	cl_git_fail_with(gitno_connection_data_from_url(&conndata,
+				"http://foo.com/bar/baz", NULL, NULL, true),
+			-1);
+}