net: refactor gitno redirect handling

Move the redirect handling into `git_net_url` for consistency.

src/net.c

@@ -153,6 +153,112 @@ done:
 	return error;
 }
 
+/*
+ * Some servers strip the query parameters from the Location header
+ * when sending a redirect. Others leave it in place.
+ * Check for both, starting with the stripped case first,
+ * since it appears to be more common.
+ */
+static void remove_service_suffix(
+	git_net_url *url,
+	const char *service_suffix)
+{
+	const char *service_query = strchr(service_suffix, '?');
+	size_t full_suffix_len = strlen(service_suffix);
+	size_t suffix_len = service_query ?
+		(size_t)(service_query - service_suffix) : full_suffix_len;
+	size_t path_len = strlen(url->path);
+	ssize_t truncate = -1;
+
+	/*
+	 * Check for a redirect without query parameters,
+	 * like "/newloc/info/refs"'
+	 */
+	if (suffix_len && path_len >= suffix_len) {
+		size_t suffix_offset = path_len - suffix_len;
+
+		if (git__strncmp(url->path + suffix_offset, service_suffix, suffix_len) == 0 &&
+		    (!service_query || git__strcmp(url->query, service_query + 1) == 0)) {
+			truncate = suffix_offset;
+		}
+	}
+
+	/*
+	 * If we haven't already found where to truncate to remove the
+	 * suffix, check for a redirect with query parameters, like
+	 * "/newloc/info/refs?service=git-upload-pack"
+	 */
+	if (truncate < 0 && git__suffixcmp(url->path, service_suffix) == 0)
+		truncate = path_len - full_suffix_len;
+
+	/* Ensure we leave a minimum of '/' as the path */
+	if (truncate == 0)
+		truncate++;
+
+	if (truncate > 0) {
+		url->path[truncate] = '\0';
+
+		git__free(url->query);
+		url->query = NULL;
+	}
+}
+
+int git_net_url_apply_redirect(
+	git_net_url *url,
+	const char *redirect_location,
+	const char *service_suffix)
+{
+	git_net_url tmp = GIT_NET_URL_INIT;
+	int error = 0;
+
+	assert(url && redirect_location);
+
+	if (redirect_location[0] == '/') {
+		git__free(url->path);
+
+		if ((url->path = git__strdup(redirect_location)) == NULL) {
+			error = -1;
+			goto done;
+		}
+	} else {
+		git_net_url *original = url;
+
+		if ((error = git_net_url_parse(&tmp, redirect_location)) < 0)
+			goto done;
+
+		/* Validate that this is a legal redirection */
+
+		if (original->scheme &&
+			strcmp(original->scheme, tmp.scheme) != 0 &&
+			strcmp(tmp.scheme, "https") != 0) {
+			git_error_set(GIT_ERROR_NET, "cannot redirect from '%s' to '%s'",
+				original->scheme, tmp.scheme);
+
+			error = -1;
+			goto done;
+		}
+
+		if (original->host &&
+		    git__strcasecmp(original->host, tmp.host) != 0) {
+			git_error_set(GIT_ERROR_NET, "cannot redirect from '%s' to '%s'",
+				original->host, tmp.host);
+
+			error = -1;
+			goto done;
+		}
+
+		git_net_url_swap(url, &tmp);
+	}
+
+	/* Remove the service suffix if it was given to us */
+	if (service_suffix)
+		remove_service_suffix(url, service_suffix);
+
+done:
+	git_net_url_dispose(&tmp);
+	return error;
+}
+
 bool git_net_url_valid(git_net_url *url)
 {
 	return (url->host && url->port && url->path);

src/net.h

@@ -30,6 +30,12 @@ extern bool git_net_url_valid(git_net_url *url);
 /** Returns nonzero if the URL is on the default port. */
 extern int git_net_url_is_default_port(git_net_url *url);
 
+/* Applies a redirect to the URL with a git-aware service suffix. */
+extern int git_net_url_apply_redirect(
+	git_net_url *url,
+	const char *redirect_location,
+	const char *service_suffix);
+
 /** Swaps the contents of one URL for another.  */
 extern void git_net_url_swap(git_net_url *a, git_net_url *b);
 

src/netops.c

@@ -121,101 +121,3 @@ int gitno__match_host(const char *pattern, const char *host)
 
 	return -1;
 }
-
-int gitno_connection_data_handle_redirect(
-		git_net_url *url,
-		const char *redirect_str,
-		const char *service_suffix)
-{
-	git_net_url tmp = GIT_NET_URL_INIT;
-	int error = 0;
-
-	assert(url && redirect_str);
-
-	if (redirect_str[0] == '/') {
-		git__free(url->path);
-
-		if ((url->path = git__strdup(redirect_str)) == NULL) {
-			error = -1;
-			goto done;
-		}
-	} else {
-		git_net_url *original = url;
-
-		if ((error = git_net_url_parse(&tmp, redirect_str)) < 0)
-			goto done;
-
-		/* Validate that this is a legal redirection */
-
-		if (original->scheme &&
-		    strcmp(original->scheme, tmp.scheme) != 0 &&
-			strcmp(tmp.scheme, "https") != 0) {
-			git_error_set(GIT_ERROR_NET, "cannot redirect from '%s' to '%s'",
-				original->scheme, tmp.scheme);
-
-			error = -1;
-			goto done;
-		}
-
-		if (original->host &&
-		    git__strcasecmp(original->host, tmp.host) != 0) {
-			git_error_set(GIT_ERROR_NET, "cannot redirect from '%s' to '%s'",
-				original->host, tmp.host);
-
-			error = -1;
-			goto done;
-		}
-
-		git_net_url_swap(url, &tmp);
-	}
-
-	/* Remove the service suffix if it was given to us */
-	if (service_suffix) {
-		/*
-		 * Some servers strip the query parameters from the Location header
-		 * when sending a redirect. Others leave it in place.
-		 * Check for both, starting with the stripped case first,
-		 * since it appears to be more common.
-		 */
-		const char *service_query = strchr(service_suffix, '?');
-		size_t full_suffix_len = strlen(service_suffix);
-		size_t suffix_len = service_query ?
-			(size_t)(service_query - service_suffix) : full_suffix_len;
-		size_t path_len = strlen(url->path);
-		ssize_t truncate = -1;
-
-		/* Check for a redirect without query parameters, like "/newloc/info/refs" */
-		if (suffix_len && path_len >= suffix_len) {
-			size_t suffix_offset = path_len - suffix_len;
-
-			if (git__strncmp(url->path + suffix_offset, service_suffix, suffix_len) == 0 &&
-			    (!service_query || git__strcmp(url->query, service_query + 1) == 0)) {
-				truncate = suffix_offset;
-			}
-		}
-
-		/*
-		 * If we haven't already found where to truncate to remove the suffix,
-		 * check for a redirect with query parameters,
-		 * like "/newloc/info/refs?service=git-upload-pack"
-		 */
-		if (truncate == -1 && git__suffixcmp(url->path, service_suffix) == 0) {
-			truncate = path_len - full_suffix_len;
-		}
-
-		if (truncate >= 0) {
-			/* Ensure we leave a minimum of '/' as the path */
-			if (truncate == 0)
-				truncate++;
-			url->path[truncate] = '\0';
-
-			git__free(url->query);
-			url->query = NULL;
-		}
-	}
-
-done:
-	git_net_url_dispose(&tmp);
-	return error;
-}
-

src/netops.h

@@ -65,15 +65,4 @@ int gitno_recv(gitno_buffer *buf);
 void gitno_consume(gitno_buffer *buf, const char *ptr);
 void gitno_consume_n(gitno_buffer *buf, size_t cons);
 
-/*
- * 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_handle_redirect(
-		git_net_url *data,
-		const char *url,
-		const char *service_suffix);
-
 #endif

src/transports/http.c

@@ -609,17 +609,9 @@ static int on_headers_complete(http_parser *parser)
 	     parser->status_code == 308) &&
 	    t->location) {
 
-		if (gitno_connection_data_handle_redirect(&t->server.url, t->location, s->service_url) < 0)
+		if (git_net_url_apply_redirect(&t->server.url, t->location, s->service_url) < 0)
 			return t->parse_error = PARSE_ERROR_GENERIC;
 
-		/* Set the redirect URL on the stream. This is a transfer of
-		 * ownership of the memory. */
-		if (s->redirect_url)
-			git__free(s->redirect_url);
-
-		s->redirect_url = t->location;
-		t->location = NULL;
-
 		t->connected = 0;
 		t->parse_error = PARSE_ERROR_REPLAY;
 		return 0;
@@ -1422,9 +1414,6 @@ static void http_stream_free(git_smart_subtransport_stream *stream)
 	if (s->chunk_buffer)
 		git__free(s->chunk_buffer);
 
-	if (s->redirect_url)
-		git__free(s->redirect_url);
-
 	git__free(s);
 }
 

src/transports/winhttp.c

@@ -1128,7 +1128,7 @@ replay:
 
 			if (!git__prefixcmp_icase(location8, prefix_https)) {
 				/* Upgrade to secure connection; disconnect and start over */
-				if (gitno_connection_data_handle_redirect(&t->server.url, location8, s->service_url) < 0) {
+				if (git_net_url_apply_redirect(&t->server.url, location8, s->service_url) < 0) {
 					git__free(location8);
 					return -1;
 				}

tests/network/redirect.c

@@ -18,7 +18,7 @@ void test_network_redirect__redirect_http(void)
 {
 	cl_git_pass(git_net_url_parse(&conndata,
 				"http://example.com/foo/bar/baz"));
-	cl_git_pass(gitno_connection_data_handle_redirect(&conndata,
+	cl_git_pass(git_net_url_apply_redirect(&conndata,
 				"http://example.com/foo/bar/baz", "bar/baz"));
 	cl_assert_equal_s(conndata.scheme, "http");
 	cl_assert_equal_s(conndata.host, "example.com");
@@ -32,7 +32,7 @@ void test_network_redirect__redirect_ssl(void)
 {
 	cl_git_pass(git_net_url_parse(&conndata,
 				"https://example.com/foo/bar/baz"));
-	cl_git_pass(gitno_connection_data_handle_redirect(&conndata,
+	cl_git_pass(git_net_url_apply_redirect(&conndata,
 				"https://example.com/foo/bar/baz", "bar/baz"));
 	cl_assert_equal_s(conndata.scheme, "https");
 	cl_assert_equal_s(conndata.host, "example.com");
@@ -46,7 +46,7 @@ void test_network_redirect__redirect_leaves_root_path(void)
 {
 	cl_git_pass(git_net_url_parse(&conndata,
 				"https://example.com/foo/bar/baz"));
-	cl_git_pass(gitno_connection_data_handle_redirect(&conndata,
+	cl_git_pass(git_net_url_apply_redirect(&conndata,
 				"https://example.com/foo/bar/baz", "/foo/bar/baz"));
 	cl_assert_equal_s(conndata.scheme, "https");
 	cl_assert_equal_s(conndata.host, "example.com");
@@ -60,7 +60,7 @@ void test_network_redirect__redirect_encoded_username_password(void)
 {
 	cl_git_pass(git_net_url_parse(&conndata,
 				"https://user%2fname:pass%40word%zyx%v@example.com/foo/bar/baz"));
-	cl_git_pass(gitno_connection_data_handle_redirect(&conndata,
+	cl_git_pass(git_net_url_apply_redirect(&conndata,
 				"https://user%2fname:pass%40word%zyx%v@example.com/foo/bar/baz", "bar/baz"));
 	cl_assert_equal_s(conndata.scheme, "https");
 	cl_assert_equal_s(conndata.host, "example.com");
@@ -73,7 +73,7 @@ void test_network_redirect__redirect_encoded_username_password(void)
 void test_network_redirect__redirect_cross_host_denied(void)
 {
 	cl_git_pass(git_net_url_parse(&conndata, "https://bar.com/bar/baz"));
-	cl_git_fail_with(gitno_connection_data_handle_redirect(&conndata,
+	cl_git_fail_with(git_net_url_apply_redirect(&conndata,
 				"https://foo.com/bar/baz", NULL),
 			-1);
 }
@@ -81,7 +81,7 @@ void test_network_redirect__redirect_cross_host_denied(void)
 void test_network_redirect__redirect_http_downgrade_denied(void)
 {
 	cl_git_pass(git_net_url_parse(&conndata, "https://foo.com/bar/baz"));
-	cl_git_fail_with(gitno_connection_data_handle_redirect(&conndata,
+	cl_git_fail_with(git_net_url_apply_redirect(&conndata,
 				"http://foo.com/bar/baz", NULL),
 			-1);
 }
@@ -89,7 +89,7 @@ void test_network_redirect__redirect_http_downgrade_denied(void)
 void test_network_redirect__redirect_relative(void)
 {
 	cl_git_pass(git_net_url_parse(&conndata, "http://foo.com/bar/baz/biff"));
-	cl_git_pass(gitno_connection_data_handle_redirect(&conndata,
+	cl_git_pass(git_net_url_apply_redirect(&conndata,
 				"/zap/baz/biff?bam", NULL));
 	cl_assert_equal_s(conndata.scheme, "http");
 	cl_assert_equal_s(conndata.host, "foo.com");
@@ -102,7 +102,7 @@ void test_network_redirect__redirect_relative(void)
 void test_network_redirect__redirect_relative_ssl(void)
 {
 	cl_git_pass(git_net_url_parse(&conndata, "https://foo.com/bar/baz/biff"));
-	cl_git_pass(gitno_connection_data_handle_redirect(&conndata,
+	cl_git_pass(git_net_url_apply_redirect(&conndata,
 				"/zap/baz/biff?bam", NULL));
 	cl_assert_equal_s(conndata.scheme, "https");
 	cl_assert_equal_s(conndata.host, "foo.com");
@@ -115,7 +115,7 @@ void test_network_redirect__redirect_relative_ssl(void)
 void test_network_redirect__service_query_no_query_params_in_location(void)
 {
 	cl_git_pass(git_net_url_parse(&conndata, "https://foo.com/bar/info/refs?service=git-upload-pack"));
-	cl_git_pass(gitno_connection_data_handle_redirect(&conndata,
+	cl_git_pass(git_net_url_apply_redirect(&conndata,
 				"/baz/info/refs", "/info/refs?service=git-upload-pack"));
 	cl_assert_equal_s(conndata.path, "/baz");
 }
@@ -123,7 +123,7 @@ void test_network_redirect__service_query_no_query_params_in_location(void)
 void test_network_redirect__service_query_with_query_params_in_location(void)
 {
 	cl_git_pass(git_net_url_parse(&conndata, "https://foo.com/bar/info/refs?service=git-upload-pack"));
-	cl_git_pass(gitno_connection_data_handle_redirect(&conndata,
+	cl_git_pass(git_net_url_apply_redirect(&conndata,
 				"/baz/info/refs?service=git-upload-pack", "/info/refs?service=git-upload-pack"));
 	cl_assert_equal_s(conndata.path, "/baz");
 }