netops: handle intact query parameters in service_suffix removal

Some servers leave the query parameters intact in the
Location header when responding with a redirect.
The service_suffix removal check as written assumed
that the server removed them.
Handle both cases.

Along with PR #5325, this fixes #5321.

There are two new tests. The first already passed;
the second previously failed.

2 files changed, 45 insertions, 8 deletions

diff --git a/src/netops.c b/src/netops.c
index f19bae9de..c885d5e89 100644
--- a/src/netops.c
+++ b/src/netops.c
@@ -171,25 +171,46 @@ int gitno_connection_data_handle_redirect(
 
 	/* 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) : strlen(service_suffix);
+			(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)) {
-				/* Ensure we leave a minimum of '/' as the path */
-				if (suffix_offset == 0)
-					suffix_offset++;
+				truncate = suffix_offset;
+			}
+		}
 
-				url->path[suffix_offset] = '\0';
+		/*
+		 * 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;
+		}
 
-				git__free(url->query);
-				url->query = NULL;
-			}
+		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;
 		}
 	}
 
diff --git a/tests/network/redirect.c b/tests/network/redirect.c
index 3fc0b1826..ce0a080dd 100644
--- a/tests/network/redirect.c
+++ b/tests/network/redirect.c
@@ -111,3 +111,19 @@ void test_network_redirect__redirect_relative_ssl(void)
 	cl_assert_equal_p(conndata.username, NULL);
 	cl_assert_equal_p(conndata.password, NULL);
 }
+
+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,
+				"/baz/info/refs", "/info/refs?service=git-upload-pack"));
+	cl_assert_equal_s(conndata.path, "/baz");
+}
+
+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,
+				"/baz/info/refs?service=git-upload-pack", "/info/refs?service=git-upload-pack"));
+	cl_assert_equal_s(conndata.path, "/baz");
+}