threads / patch / 35039

patch, 9 partsfollowing http redirects

Subject: [PATCH 0/9] following http redirects

## tl;dr

17 messages between Sep 28, 2013 and Oct 18, 2013. Diffs are folded; open one to read it.

replies: 16people: 4as markdown or json

Jeff King· Sep 28, 2013, 08:29 UTC · lore

Git's http handling takes care of redirects by setting CURLOPT_FOLLOWLOCATION, and just assuming curl will handle the details. For the most part, this works, but there are a few downsides:

  1. We do extra round-trips for subsequent requests, as we
     always feed curl the original URL, and it then has to go hit the
     redirect.
  2. Some redirects are to upgrade http->https. In this case, we end up
     sending the credential over unencrypted http. We do advertise the
     proper unencrypted URL to the credential helpers (and to the user
     when we prompt them), so to some degree this is the user's fault.
     Still, there's no reason not to be friendly and avoid exposing the
     password in plaintext if we know that the server is just going to
     redirect us to an encrypted connection anyway.
  3. Cross-server redirects do not work at all with authentication. If
     we tell curl to go to host X, and it redirects to host Y, it will
     drop the auth credentials to avoid revealing them to Y. This is a
     good thing, because we prompted the user for host X, and curl is
     being safe. However, it means that we have no way to feed curl the
     right password (and the user does not even get a useful error
     message; simply a 401 from Y, since we didn't give it any
     credential).

None of these is fixable at the curl layer; they depend on information we have outside of curl (e.g., the notion that if info/refs is redirected, so should the rest of the requests be).

This series gets the redirect information from curl (after the fact), and uses it update the base URL for the repository for the remainder of the program run.

Kyle, I didn't call into the urlmatch code at all; we might want another patch on top to re-check the http.$url.* config after following such a redirect.

The patches are:
  [1/9]: http_get_file: style fixes
  [2/9]: http_request: factor out curlinfo_strbuf
  [3/9]: http: refactor options to http_get_*
  [4/9]: http: hoist credential request out of handle_curl_result
  [5/9]: http: provide effective url to callers
  [6/9]: http: update base URLs when we see redirects
  [7/9]: remote-curl: make refs_url a strbuf
  [8/9]: remote-curl: store url as a strbuf
  [9/9]: remote-curl: rewrite base url from info/refs redirects

Here is sample output from running a clone against a private repo on github, which redirects from http to https. Generated with:

  GIT_CURL_VERBOSE=1 git clone http://github.com/github/github 2>&1 |
  egrep '^(< HTTP|< Location|> GET|> POST|^Authorization|\* Connected to)' |
  sed -e 's/\(Authorization\): .*/\1: **redacted**/'

Here is the output with stock git (I've added numbering for each connection):

1 * Connected to github.com (192.30.252.129) port 80 (#0)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    < HTTP/1.1 301 Moved Permanently
    < Location: https://github.com/github/github/info/refs?service=git-upload-pack
2 * Connected to github.com (192.30.252.129) port 443 (#1)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    < HTTP/1.1 401 Authorization Required
3 * Connected to github.com (192.30.252.129) port 80 (#2)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    < HTTP/1.1 301 Moved Permanently
    < Location: https://github.com/github/github/info/refs?service=git-upload-pack
4 * Connected to github.com (192.30.252.129) port 443 (#1)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    < HTTP/1.1 401 Authorization Required
5 * Connected to github.com (192.30.252.129) port 443 (#1)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    Authorization: **redacted**
    < HTTP/1.1 200 OK
6 * Connected to github.com (192.30.252.129) port 80 (#3)
    > POST /github/github/git-upload-pack HTTP/1.1
    Authorization: **redacted**
    < HTTP/1.1 301 Moved Permanently
    < Location: https://github.com/github/github/git-upload-pack
7 * Connected to github.com (192.30.252.129) port 443 (#4)
    [... POST actually happens, and I hit ^C before it finishes ...]

You can see that connections 3 and 6 are extraneous round-trips, and that we reveal the credential over cleartext in step 6. Curiously, even though we have fed the credential to curl after step 2, curl does not share it until the server sends another 401 in step 4. I'm not sure why that is the case.

Here's the output with these patches applied:
1 * Connected to github.com (192.30.252.129) port 80 (#0)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    < HTTP/1.1 301 Moved Permanently
    < Location: https://github.com/github/github/info/refs?service=git-upload-pack
2 * Connected to github.com (192.30.252.129) port 443 (#1)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    < HTTP/1.1 401 Authorization Required
3 * Connected to github.com (192.30.252.129) port 443 (#1)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    < HTTP/1.1 401 Authorization Required
4 * Connected to github.com (192.30.252.129) port 443 (#1)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    Authorization: **redacted**
    < HTTP/1.1 200 OK
5 * Connected to github.com (192.30.252.129) port 443 (#2)
    [... POST actually happens ...]

We do 2 fewer round-trips. That isn't all that exciting, but keep in mind that it saves one per request after the initial contact. So if we had more ref negotiation, we'd see more savings. Similarly, dumb http would see many more savings (since it makes individual requests for each loose object and pack).

We also go straight to https in steps 3 and 4, meaning the credential never gets passed in plaintext to the old URL.

I also tested cross-server redirects using a hacky "nc -l" setup as the server. I confirmed that it does not work at all with stock git (you are prompted for the password for "localhost", which curl then drops after following the redirect), and that it does work after the patches.

The one downside I can think of is that we are making the assumption that if info/refs redirects, the rest of the repo will, too. So you could not have "info/refs" redirect to another server to get the ref advertisement, but then expect to get the actual pack from the original location. I'm OK with making that assumption; I can't imagine the insanity of the setup that would be required to break it.

-Peff
Jeff King· Sep 28, 2013, 08:31 UTC · re: Jeff King · lore

[PATCH 1/9] http_get_file: style fixes

Besides being ugly, the extra parentheses are idiomatic for suppressing compiler warnings when we are assigning within a conditional. We aren't doing that here, and they just confuse the reader.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to http.c +2 −2
diff --git a/http.c b/http.c
index f3e1439..a985c40 100644
--- a/http.c
+++ b/http.c
@@ -958,7 +958,7 @@ static int http_get_file(const char *url, const char *filename, int options)
 
 	strbuf_addf(&tmpfile, "%s.temp", filename);
 	result = fopen(tmpfile.buf, "a");
-	if (! result) {
+	if (!result) {
 		error("Unable to open local file %s", tmpfile.buf);
 		ret = HTTP_ERROR;
 		goto cleanup;
@@ -967,7 +967,7 @@ static int http_get_file(const char *url, const char *filename, int options)
 	ret = http_request_reauth(url, NULL, result, HTTP_REQUEST_FILE, options);
 	fclose(result);
 
-	if ((ret == HTTP_OK) && move_temp_to_file(tmpfile.buf, filename))
+	if (ret == HTTP_OK && move_temp_to_file(tmpfile.buf, filename))
 		ret = HTTP_ERROR;
 cleanup:
 	strbuf_release(&tmpfile);

-- 
1.8.4.rc3.19.g9da5bf6
Jeff King· Sep 28, 2013, 08:31 UTC · re: Jeff King · lore

[PATCH 2/9] http_request: factor out curlinfo_strbuf

When we retrieve the content-type of an http response, curl gives us a pointer to internal storage, which we then copy into a strbuf. Let's factor out the get-and-copy routine, which can be used for getting other curl info.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)
Show changes to http.c +14 −7
diff --git a/http.c b/http.c
index a985c40..d325669 100644
--- a/http.c
+++ b/http.c
@@ -837,6 +837,18 @@ int handle_curl_result(struct slot_results *results)
 	}
 }
 
+static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
+{
+	char *ptr;
+	CURLcode ret;
+
+	strbuf_reset(buf);
+	ret = curl_easy_getinfo(curl, info, &ptr);
+	if (!ret && ptr)
+		strbuf_addstr(buf, ptr);
+	return ret;
+}
+
 /* http_request() targets */
 #define HTTP_REQUEST_STRBUF	0
 #define HTTP_REQUEST_FILE	1
@@ -895,13 +907,8 @@ static int http_request(const char *url, struct strbuf *type,
 		ret = HTTP_START_FAILED;
 	}
 
-	if (type) {
-		char *t;
-		strbuf_reset(type);
-		curl_easy_getinfo(slot->curl, CURLINFO_CONTENT_TYPE, &t);
-		if (t)
-			strbuf_addstr(type, t);
-	}
+	if (type)
+		curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE, type);
 
 	curl_slist_free_all(headers);
 	strbuf_release(&buf);

-- 
1.8.4.rc3.19.g9da5bf6
Jeff King· Sep 28, 2013, 08:31 UTC · re: Jeff King · lore

[PATCH 3/9] http: refactor options to http_get_*

Over time, the http_get_strbuf function has grown several optional parameters. We now have a bitfield with multiple boolean options, as well as an optional strbuf for returning the content-type of the response. And a future patch in this series is going to add another strbuf option.

Treating these as separate arguments has a few downsides:
  1. Most call sites need to add extra NULLs and 0s for the
     options they aren't interested in.
  2. The http_get_* functions are actually wrappers around
     2 layers of low-level implementation functions. We have
     to pass these options through individually.
  3. The http_get_strbuf wrapper learned these options, but
     nobody bothered to do so for http_get_file, even though
     it is backed by the same function that does understand
     the options.

Let's consolidate the options into a single struct. For the common case of the default options, we'll allow callers to simply pass a NULL for the options struct.

The resulting code is often a few lines longer, but it ends up being easier to read (and to change as we add new options, since we do not need to update each call site).

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c   |  4 ++--
 http.c        | 44 +++++++++++++++++++++++++-------------------
 http.h        | 15 ++++++++++-----
 remote-curl.c |  9 +++++++--
 4 files changed, 44 insertions(+), 28 deletions(-)
Show changes to 4 files +44 −28

http-push.c, http.c, http.h, remote-curl.c

diff --git a/http-push.c b/http-push.c
index 69200ba..34cb70f 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1542,7 +1542,7 @@ static int remote_exists(const char *path)
 
 	sprintf(url, "%s%s", repo->url, path);
 
-	switch (http_get_strbuf(url, NULL, NULL, 0)) {
+	switch (http_get_strbuf(url, NULL, NULL)) {
 	case HTTP_OK:
 		ret = 1;
 		break;
@@ -1566,7 +1566,7 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)
 	url = xmalloc(strlen(repo->url) + strlen(path) + 1);
 	sprintf(url, "%s%s", repo->url, path);
 
-	if (http_get_strbuf(url, NULL, &buffer, 0) != HTTP_OK)
+	if (http_get_strbuf(url, &buffer, NULL) != HTTP_OK)
 		die("Couldn't get %s for remote symref\n%s", url,
 		    curl_errorstr);
 	free(url);
diff --git a/http.c b/http.c
index d325669..41ee7aa 100644
--- a/http.c
+++ b/http.c
@@ -853,8 +853,9 @@ static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
 #define HTTP_REQUEST_STRBUF	0
 #define HTTP_REQUEST_FILE	1
 
-static int http_request(const char *url, struct strbuf *type,
-			void *result, int target, int options)
+static int http_request(const char *url,
+			void *result, int target,
+			const struct http_get_options *options)
 {
 	struct active_request_slot *slot;
 	struct slot_results results;
@@ -887,9 +888,9 @@ static int http_request(const char *url, struct strbuf *type,
 	}
 
 	strbuf_addstr(&buf, "Pragma:");
-	if (options & HTTP_NO_CACHE)
+	if (options && options->no_cache)
 		strbuf_addstr(&buf, " no-cache");
-	if (options & HTTP_KEEP_ERROR)
+	if (options && options->keep_error)
 		curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
 
 	headers = curl_slist_append(headers, buf.buf);
@@ -907,8 +908,9 @@ static int http_request(const char *url, struct strbuf *type,
 		ret = HTTP_START_FAILED;
 	}
 
-	if (type)
-		curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE, type);
+	if (options && options->content_type)
+		curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE,
+				options->content_type);
 
 	curl_slist_free_all(headers);
 	strbuf_release(&buf);
@@ -917,11 +919,10 @@ static int http_request_reauth(const char *url,
 }
 
 static int http_request_reauth(const char *url,
-			       struct strbuf *type,
 			       void *result, int target,
-			       int options)
+			       struct http_get_options *options)
 {
-	int ret = http_request(url, type, result, target, options);
+	int ret = http_request(url, result, target, options);
 	if (ret != HTTP_REAUTH)
 		return ret;
 
@@ -931,7 +932,7 @@ static int http_request_reauth(const char *url,
 	 * making our next request. We only know how to do this for
 	 * the strbuf case, but that is enough to satisfy current callers.
 	 */
-	if (options & HTTP_KEEP_ERROR) {
+	if (options && options->keep_error) {
 		switch (target) {
 		case HTTP_REQUEST_STRBUF:
 			strbuf_reset(result);
@@ -940,15 +941,14 @@ int http_get_strbuf(const char *url,
 			die("BUG: HTTP_KEEP_ERROR is only supported with strbufs");
 		}
 	}
-	return http_request(url, type, result, target, options);
+	return http_request(url, result, target, options);
 }
 
 int http_get_strbuf(const char *url,
-		    struct strbuf *type,
-		    struct strbuf *result, int options)
+		    struct strbuf *result,
+		    struct http_get_options *options)
 {
-	return http_request_reauth(url, type, result,
-				   HTTP_REQUEST_STRBUF, options);
+	return http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);
 }
 
 /*
@@ -957,7 +957,8 @@ int http_get_strbuf(const char *url,
  * If a previous interrupted download is detected (i.e. a previous temporary
  * file is still around) the download is resumed.
  */
-static int http_get_file(const char *url, const char *filename, int options)
+static int http_get_file(const char *url, const char *filename,
+			 struct http_get_options *options)
 {
 	int ret;
 	struct strbuf tmpfile = STRBUF_INIT;
@@ -971,7 +972,7 @@ static int http_get_file(const char *url, const char *filename, int options)
 		goto cleanup;
 	}
 
-	ret = http_request_reauth(url, NULL, result, HTTP_REQUEST_FILE, options);
+	ret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options);
 	fclose(result);
 
 	if (ret == HTTP_OK && move_temp_to_file(tmpfile.buf, filename))
@@ -983,12 +984,15 @@ int http_fetch_ref(const char *base, struct ref *ref)
 
 int http_fetch_ref(const char *base, struct ref *ref)
 {
+	struct http_get_options options = {0};
 	char *url;
 	struct strbuf buffer = STRBUF_INIT;
 	int ret = -1;
 
+	options.no_cache = 1;
+
 	url = quote_ref_url(base, ref->name);
-	if (http_get_strbuf(url, NULL, &buffer, HTTP_NO_CACHE) == HTTP_OK) {
+	if (http_get_strbuf(url, &buffer, &options) == HTTP_OK) {
 		strbuf_rtrim(&buffer);
 		if (buffer.len == 40)
 			ret = get_sha1_hex(buffer.buf, ref->old_sha1);
@@ -1072,6 +1076,7 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)
 
 int http_get_info_packs(const char *base_url, struct packed_git **packs_head)
 {
+	struct http_get_options options = {0};
 	int ret = 0, i = 0;
 	char *url, *data;
 	struct strbuf buf = STRBUF_INIT;
@@ -1081,7 +1086,8 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)
 	strbuf_addstr(&buf, "objects/info/packs");
 	url = strbuf_detach(&buf, NULL);
 
-	ret = http_get_strbuf(url, NULL, &buf, HTTP_NO_CACHE);
+	options.no_cache = 1;
+	ret = http_get_strbuf(url, &buf, &options);
 	if (ret != HTTP_OK)
 		goto cleanup;
 
diff --git a/http.h b/http.h
index d77c1b5..40404b4 100644
--- a/http.h
+++ b/http.h
@@ -125,11 +125,16 @@ extern char *get_remote_object_url(const char *url, const char *hex,
 extern char *get_remote_object_url(const char *url, const char *hex,
 				   int only_two_digit_prefix);
 
-/* Options for http_request_*() */
-#define HTTP_NO_CACHE		1
-#define HTTP_KEEP_ERROR		2
+/* Options for http_get_*() */
+struct http_get_options {
+	unsigned no_cache:1,
+		 keep_error:1;
 
-/* Return values for http_request_*() */
+	/* If non-NULL, returns the content-type of the response. */
+	struct strbuf *content_type;
+};
+
+/* Return values for http_get_*() */
 #define HTTP_OK			0
 #define HTTP_MISSING_TARGET	1
 #define HTTP_ERROR		2
@@ -142,7 +147,7 @@ extern char *get_remote_object_url(const char *url, const char *hex,
  *
  * If the result pointer is NULL, a HTTP HEAD request is made instead of GET.
  */
-int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf *result, int options);
+int http_get_strbuf(const char *url, struct strbuf *result, struct http_get_options *options);
 
 extern int http_fetch_ref(const char *base, struct ref *ref);
 
diff --git a/remote-curl.c b/remote-curl.c
index b5ebe01..66705bb 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -206,6 +206,7 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	struct discovery *last = last_discovery;
 	char *refs_url;
 	int http_ret, maybe_smart = 0;
+	struct http_get_options options;
 
 	if (last && !strcmp(service, last->service))
 		return last;
@@ -223,8 +224,12 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	}
 	refs_url = strbuf_detach(&buffer, NULL);
 
-	http_ret = http_get_strbuf(refs_url, &type, &buffer,
-				   HTTP_NO_CACHE | HTTP_KEEP_ERROR);
+	memset(&options, 0, sizeof(options));
+	options.content_type = &type;
+	options.no_cache = 1;
+	options.keep_error = 1;
+
+	http_ret = http_get_strbuf(refs_url, &buffer, &options);
 	switch (http_ret) {
 	case HTTP_OK:
 		break;

-- 
1.8.4.rc3.19.g9da5bf6
Jeff King· Sep 28, 2013, 08:31 UTC · re: Jeff King · lore

[PATCH 4/9] http: hoist credential request out of handle_curl_result

When we are handling a curl response code in http_request or in the remote-curl RPC code, we use the handle_curl_result helper to translate curl's response into an easy-to-use code. When we see an HTTP 401, we do one of two things:

  1. If we already had a filled-in credential, we mark it as
     rejected, and then return HTTP_NOAUTH to indicate to
     the caller that we failed.
  2. If we didn't, then we ask for a new credential and tell
     the caller HTTP_REAUTH to indicate that they may want
     to try again.

Rejecting in the first case makes sense; it is the natural result of the request we just made. However, prompting for more credentials in the second step does not always make sense. We do not know for sure that the caller is going to make a second request, and nor are we sure that it will be to the same URL. Logically, the prompt belongs not to the request we just finished, but to the request we are (maybe) about to make.

In practice, it is very hard to trigger any bad behavior. Currently, if we make a second request, it will always be to the same URL (even in the face of redirects, because curl handles the redirects internally). And we almost always retry on HTTP_REAUTH these days. The one exception is if we are streaming a large RPC request to the server (e.g., a pushed packfile), in which case we cannot restart. It's extremely unlikely to see a 401 response at this stage, though, as we would typically have seen it when we sent a probe request, before streaming the data.

This patch drops the automatic prompt out of case 2, and instead requires the caller to do it. This is a few extra lines of code, and the bug it fixes is unlikely to come up in practice. But it is conceptually cleaner, and paves the way for better handling of credentials across redirects.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c        | 6 ++++--
 http.h        | 1 +
 remote-curl.c | 7 ++++++-
 3 files changed, 11 insertions(+), 3 deletions(-)
Show changes to 3 files +11 −3

http.c, http.h, remote-curl.c

diff --git a/http.c b/http.c
index 41ee7aa..5504b2c 100644
--- a/http.c
+++ b/http.c
@@ -47,7 +47,7 @@ static int curl_save_cookies;
 static const char *curl_http_proxy;
 static const char *curl_cookie_file;
 static int curl_save_cookies;
-static struct credential http_auth = CREDENTIAL_INIT;
+struct credential http_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
 static const char *user_agent;
 
@@ -823,7 +823,6 @@ int handle_curl_result(struct slot_results *results)
 			credential_reject(&http_auth);
 			return HTTP_NOAUTH;
 		} else {
-			credential_fill(&http_auth);
 			return HTTP_REAUTH;
 		}
 	} else {
@@ -941,6 +940,9 @@ static int http_request_reauth(const char *url,
 			die("BUG: HTTP_KEEP_ERROR is only supported with strbufs");
 		}
 	}
+
+	credential_fill(&http_auth);
+
 	return http_request(url, result, target, options);
 }
 
diff --git a/http.h b/http.h
index 40404b4..17116ab 100644
--- a/http.h
+++ b/http.h
@@ -102,6 +102,7 @@ extern size_t http_post_buffer;
 extern int active_requests;
 extern int http_is_verbose;
 extern size_t http_post_buffer;
+extern struct credential http_auth;
 
 extern char curl_errorstr[CURL_ERROR_SIZE];
 
diff --git a/remote-curl.c b/remote-curl.c
index 66705bb..d524462 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -9,6 +9,7 @@
 #include "string-list.h"
 #include "sideband.h"
 #include "argv-array.h"
+#include "credential.h"
 
 static struct remote *remote;
 static const char *url; /* always ends with a trailing slash */
@@ -468,6 +469,8 @@ static int post_rpc(struct rpc_state *rpc)
 	if (large_request) {
 		do {
 			err = probe_rpc(rpc);
+			if (err == HTTP_REAUTH)
+				credential_fill(&http_auth);
 		} while (err == HTTP_REAUTH);
 		if (err != HTTP_OK)
 			return -1;
@@ -567,8 +570,10 @@ retry:
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
 
 	err = run_slot(slot);
-	if (err == HTTP_REAUTH && !large_request)
+	if (err == HTTP_REAUTH && !large_request) {
+		credential_fill(&http_auth);
 		goto retry;
+	}
 	if (err != HTTP_OK)
 		err = -1;
 

-- 
1.8.4.rc3.19.g9da5bf6
Jeff King· Sep 28, 2013, 08:32 UTC · re: Jeff King · lore

[PATCH 5/9] http: provide effective url to callers

When we ask curl to access a URL, it may follow one or more redirects to reach the final location. We have no idea this has happened, as curl takes care of the details and simply returns the final content to us.

The final URL that we ended up with can be accessed via CURLINFO_EFFECTIVE_URL. Let's make that optionally available to callers of http_get_*, so that they can make further decisions based on the redirection.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c | 4 ++++
 http.h | 6 ++++++
 2 files changed, 10 insertions(+)
Show changes to 2 files +10 −0

http.c, http.h

diff --git a/http.c b/http.c
index 5504b2c..65a0048 100644
--- a/http.c
+++ b/http.c
@@ -911,6 +911,10 @@ static int http_request(const char *url,
 		curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE,
 				options->content_type);
 
+	if (options && options->effective_url)
+		curlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL,
+				options->effective_url);
+
 	curl_slist_free_all(headers);
 	strbuf_release(&buf);
 
diff --git a/http.h b/http.h
index 17116ab..974ede7 100644
--- a/http.h
+++ b/http.h
@@ -133,6 +133,12 @@ struct http_get_options {
 
 	/* If non-NULL, returns the content-type of the response. */
 	struct strbuf *content_type;
+
+	/*
+	 * If non-NULL, returns the URL we ended up at, including any
+	 * redirects we followed.
+	 */
+	struct strbuf *effective_url;
 };
 
 /* Return values for http_get_*() */

-- 
1.8.4.rc3.19.g9da5bf6
Jeff King· Sep 28, 2013, 08:34 UTC · re: Jeff King · lore

[PATCH 6/9] http: update base URLs when we see redirects

If a caller asks the http_get_* functions to go to a particular URL and we end up elsewhere due to a redirect, the effective_url field can tell us where we went.

It would be nice to remember this redirect and short-cut further requests for two reasons:

  1. It's more efficient. Otherwise we spend an extra http
     round-trip to the server for each subsequent request,
     just to get redirected.
  2. If we end up with an http 401 and are going to ask for
     credentials, it is to feed them to the redirect target.
     If the redirect is an http->https upgrade, this means
     our credentials may be provided on the http leg, just
     to end up redirected to https. And if the redirect
     crosses server boundaries, then curl will drop the
     credentials entirely as it follows the redirect.

However, it, it is not enough to simply record the effective URL we saw and use that for subsequent requests. We were originally fed a "base" url like:

   http://example.com/foo.git

and we want to figure out what the new base is, even though the URLs we see may be:

     original: http://example.com/foo.git/info/refs
    effective: http://example.com/bar.git/info/refs

Subsequent requests will not be for "info/refs", but for other paths relative to the base. We must ask the caller to pass in the original base, and we must pass the redirected base back to the caller (so that it can generte more URLs from it). Furthermore, we need to feed the new base to the credential code, so that requests to credential helpers (or to the user) match the URL we will be requesting.

This patch teaches http_request_reauth to do this munging. Since it is the caller who cares about making more URLs, it seems at first glance that callers could simply check effective_url themselves and handle it. However, since we need to update the credential struct before the second re-auth request, we have to do it inside http_request_reauth.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 http.h |  8 ++++++++
 2 files changed, 68 insertions(+)
Show changes to 2 files +68 −0

http.c, http.h

diff --git a/http.c b/http.c
index 65a0048..8775b5c 100644
--- a/http.c
+++ b/http.c
@@ -921,11 +921,71 @@ static int http_request_reauth(const char *url,
 	return ret;
 }
 
+/*
+ * Update the "base" url to a more appropriate value, as deduced by
+ * redirects seen when requesting a URL starting with "url".
+ *
+ * The "asked" parameter is a URL that we asked curl to access, and must begin
+ * with "base".
+ *
+ * The "got" parameter is the URL that curl reported to us as where we ended
+ * up.
+ *
+ * Returns 1 if we updated the base url, 0 otherwise.
+ *
+ * Our basic strategy is to compare "base" and "asked" to find the bits
+ * specific to our request. We then strip those bits off of "got" to yield the
+ * new base. So for example, if our base is "http://example.com/foo.git",
+ * and we ask for "http://example.com/foo.git/info/refs", we might end up
+ * with "https://other.example.com/foo.git/info/refs". We would want the
+ * new URL to become "https://other.example.com/foo.git".
+ *
+ * Note that this assumes a sane redirect scheme. It's entirely possible
+ * in the example above to end up at a URL that does not even end in
+ * "info/refs".  In such a case we simply punt, as there is not much we can
+ * do (and such a scheme is unlikely to represent a real git repository,
+ * which means we are likely about to abort anyway).
+ */
+static int update_url_from_redirect(struct strbuf *base,
+				    const char *asked,
+				    const struct strbuf *got)
+{
+	const char *tail;
+	size_t tail_len;
+
+	if (!strcmp(asked, got->buf))
+		return 0;
+
+	if (strncmp(asked, base->buf, base->len))
+		die("BUG: update_url_from_redirect: %s is not a superset of %s",
+		    asked, base->buf);
+
+	tail = asked + base->len;
+	tail_len = strlen(tail);
+
+	if (got->len < tail_len ||
+	    strcmp(tail, got->buf + got->len - tail_len))
+		return 0; /* insane redirect scheme */
+
+	strbuf_reset(base);
+	strbuf_add(base, got->buf, got->len - tail_len);
+	return 1;
+}
+
 static int http_request_reauth(const char *url,
 			       void *result, int target,
 			       struct http_get_options *options)
 {
 	int ret = http_request(url, result, target, options);
+
+	if (options && options->effective_url && options->base_url) {
+		if (update_url_from_redirect(options->base_url,
+					     url, options->effective_url)) {
+			credential_from_url(&http_auth, options->base_url->buf);
+			url = options->effective_url->buf;
+		}
+	}
+
 	if (ret != HTTP_REAUTH)
 		return ret;
 
diff --git a/http.h b/http.h
index 974ede7..12ca5c8 100644
--- a/http.h
+++ b/http.h
@@ -139,6 +139,14 @@ struct http_get_options {
 	 * redirects we followed.
 	 */
 	struct strbuf *effective_url;
+
+	/*
+	 * If both base_url and effective_url are non-NULL, the base URL will
+	 * be munged to reflect any redirections going from the requested url
+	 * to effective_url. See the definition of update_url_from_redirect
+	 * for details.
+	 */
+	struct strbuf *base_url;
 };
 
 /* Return values for http_get_*() */

-- 
1.8.4.rc3.19.g9da5bf6
brian m. carlson· Sep 29, 2013, 18:54 UTC · re: Jeff King · lore

Re: [PATCH 6/9] http: update base URLs when we see redirects

On Sat, Sep 28, 2013 at 04:34:05AM -0400, Jeff King wrote:
> Subsequent requests will not be for "info/refs", but for
> other paths relative to the base. We must ask the caller to
> pass in the original base, and we must pass the redirected
> base back to the caller (so that it can generte more URLs
s/generte/generate/
> from it). Furthermore, we need to feed the new base to the
> credential code, so that requests to credential helpers (or
> to the user) match the URL we will be requesting.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187
Eric Sunshine· Sep 29, 2013, 19:26 UTC · re: Jeff King · lore

Re: [PATCH 6/9] http: update base URLs when we see redirects

On Sat, Sep 28, 2013 at 4:34 AM, Jeff King <peff@peff.net> wrote:
Show 18 quoted lines
> diff --git a/http.c b/http.c
> index 65a0048..8775b5c 100644
> --- a/http.c
> +++ b/http.c
> @@ -921,11 +921,71 @@ static int http_request_reauth(const char *url,
> +static int update_url_from_redirect(struct strbuf *base,
> +                                   const char *asked,
> +                                   const struct strbuf *got)
> +{
> +       const char *tail;
> +       size_t tail_len;
> +
> +       if (!strcmp(asked, got->buf))
> +               return 0;
> +
> +       if (strncmp(asked, base->buf, base->len))
> +               die("BUG: update_url_from_redirect: %s is not a superset of %s",
> +                   asked, base->buf);

Is there something non-obvious going on here? die(...,base->buf) takes advantage of the terminating NUL promised by strbuf, but then strncmp(...,base->buf,base->len) is used rather than the simpler strcmp(...,base->buf).

Show 11 quoted lines
> +       tail = asked + base->len;
> +       tail_len = strlen(tail);
> +
> +       if (got->len < tail_len ||
> +           strcmp(tail, got->buf + got->len - tail_len))
> +               return 0; /* insane redirect scheme */
> +
> +       strbuf_reset(base);
> +       strbuf_add(base, got->buf, got->len - tail_len);
> +       return 1;
> +}
Jeff King· Sep 29, 2013, 19:32 UTC · re: Eric Sunshine · lore

Re: [PATCH 6/9] http: update base URLs when we see redirects

On Sun, Sep 29, 2013 at 03:26:45PM -0400, Eric Sunshine wrote:
Show 24 quoted lines
> On Sat, Sep 28, 2013 at 4:34 AM, Jeff King <peff@peff.net> wrote:
> > diff --git a/http.c b/http.c
> > index 65a0048..8775b5c 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -921,11 +921,71 @@ static int http_request_reauth(const char *url,
> > +static int update_url_from_redirect(struct strbuf *base,
> > +                                   const char *asked,
> > +                                   const struct strbuf *got)
> > +{
> > +       const char *tail;
> > +       size_t tail_len;
> > +
> > +       if (!strcmp(asked, got->buf))
> > +               return 0;
> > +
> > +       if (strncmp(asked, base->buf, base->len))
> > +               die("BUG: update_url_from_redirect: %s is not a superset of %s",
> > +                   asked, base->buf);
> 
> Is there something non-obvious going on here? die(...,base->buf) takes
> advantage of the terminating NUL promised by strbuf, but then
> strncmp(...,base->buf,base->len) is used rather than the simpler
> strcmp(...,base->buf).

Yes, we are not checking for equality, but rather making sure that "asked" begins with "base->buf". It might be more clearly written as:

  if (prefixcmp(asked, base->buf))

I was trying to take advantage of the fact that we know base->len already, but this it not a particularly performance-critical code path. We can afford the extra strlen that prefixcmp will do.

-Peff
Eric Sunshine· Sep 29, 2013, 19:50 UTC · re: Jeff King · lore

Re: [PATCH 6/9] http: update base URLs when we see redirects

On Sun, Sep 29, 2013 at 3:32 PM, Jeff King <peff@peff.net> wrote:
Show 29 quoted lines
> On Sun, Sep 29, 2013 at 03:26:45PM -0400, Eric Sunshine wrote:
>
>> On Sat, Sep 28, 2013 at 4:34 AM, Jeff King <peff@peff.net> wrote:
>> > diff --git a/http.c b/http.c
>> > index 65a0048..8775b5c 100644
>> > --- a/http.c
>> > +++ b/http.c
>> > @@ -921,11 +921,71 @@ static int http_request_reauth(const char *url,
>> > +static int update_url_from_redirect(struct strbuf *base,
>> > +                                   const char *asked,
>> > +                                   const struct strbuf *got)
>> > +{
>> > +       const char *tail;
>> > +       size_t tail_len;
>> > +
>> > +       if (!strcmp(asked, got->buf))
>> > +               return 0;
>> > +
>> > +       if (strncmp(asked, base->buf, base->len))
>> > +               die("BUG: update_url_from_redirect: %s is not a superset of %s",
>> > +                   asked, base->buf);
>>
>> Is there something non-obvious going on here? die(...,base->buf) takes
>> advantage of the terminating NUL promised by strbuf, but then
>> strncmp(...,base->buf,base->len) is used rather than the simpler
>> strcmp(...,base->buf).
>
> Yes, we are not checking for equality, but rather making sure that
> "asked" begins with "base->buf". It might be more clearly written as:

Ah right, I knew that that was the intention but had a synapse misfire. Sorry for the noise.

Show 8 quoted lines
>
>   if (prefixcmp(asked, base->buf))
>
> I was trying to take advantage of the fact that we know base->len
> already, but this it not a particularly performance-critical code path.
> We can afford the extra strlen that prefixcmp will do.
>
> -Peff
Junio C Hamano· Oct 18, 2013, 18:25 UTC · re: Jeff King · lore

Re: [PATCH 6/9] http: update base URLs when we see redirects

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> + * Our basic strategy is to compare "base" and "asked" to find the bits
> + * specific to our request. We then strip those bits off of "got" to yield the
> + * new base. So for example, if our base is "http://example.com/foo.git",
> + * and we ask for "http://example.com/foo.git/info/refs", we might end up
> + * with "https://other.example.com/foo.git/info/refs". We would want the
> + * new URL to become "https://other.example.com/foo.git".
Not "https://other.example.com/foo.git/info/refs"?
Jeff King· Oct 18, 2013, 18:42 UTC · re: Junio C Hamano · lore

Re: [PATCH 6/9] http: update base URLs when we see redirects

On Fri, Oct 18, 2013 at 11:25:13AM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > + * Our basic strategy is to compare "base" and "asked" to find the bits
> > + * specific to our request. We then strip those bits off of "got" to yield the
> > + * new base. So for example, if our base is "http://example.com/foo.git",
> > + * and we ask for "http://example.com/foo.git/info/refs", we might end up
> > + * with "https://other.example.com/foo.git/info/refs". We would want the
> > + * new URL to become "https://other.example.com/foo.git".
> 
> Not "https://other.example.com/foo.git/info/refs"?

I think my use of "the new URL" is ambiguous. I meant "the new base", from which one could then construct the new refs URL as you suggest.

-Peff
Junio C Hamano· Oct 18, 2013, 19:20 UTC · re: Jeff King · lore

Re: [PATCH 6/9] http: update base URLs when we see redirects

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> On Fri, Oct 18, 2013 at 11:25:13AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > + * Our basic strategy is to compare "base" and "asked" to find the bits
>> > + * specific to our request. We then strip those bits off of "got" to yield the
>> > + * new base. So for example, if our base is "http://example.com/foo.git",
>> > + * and we ask for "http://example.com/foo.git/info/refs", we might end up
>> > + * with "https://other.example.com/foo.git/info/refs". We would want the
>> > + * new URL to become "https://other.example.com/foo.git".
>> 
>> Not "https://other.example.com/foo.git/info/refs"?
>
> I think my use of "the new URL" is ambiguous. I meant "the new base",
> from which one could then construct the new refs URL as you suggest.

Ahh, there is nothing we need to do to make the new URL to "https://.../info/refs"; we already have it in "got". And the function resets "base" and then adds "got" excluding its tail part to compute the new base. So "s/new URL/new base/" would be all we need to do, I think.

Thanks.
Jeff King· Sep 28, 2013, 08:35 UTC · re: Jeff King · lore

[PATCH 7/9] remote-curl: make refs_url a strbuf

In the discover_refs function, we use a strbuf named "buffer" for multiple purposes. First we build the info/refs URL in it, and then detach that to a bare pointer. Then, we use the same strbuf to store the result of fetching the refs.

Let's instead keep a separate refs_url strbuf. This is less confusing, as the "buffer" strbuf is now used for only one thing.

Signed-off-by: Jeff King <peff@peff.net>
---
This one is not technically required.  In my original draft, having it
as a strbuf was helpful, but that didn't make it to the final version.
However, I think it's a worthwhile cleanup on its own, so I left it in.
 remote-curl.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
Show changes to remote-curl.c +7 −8
diff --git a/remote-curl.c b/remote-curl.c
index d524462..7fb092f 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -204,8 +204,8 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	struct strbuf exp = STRBUF_INIT;
 	struct strbuf type = STRBUF_INIT;
 	struct strbuf buffer = STRBUF_INIT;
+	struct strbuf refs_url = STRBUF_INIT;
 	struct discovery *last = last_discovery;
-	char *refs_url;
 	int http_ret, maybe_smart = 0;
 	struct http_get_options options;
 
@@ -213,24 +213,23 @@ static struct discovery* discover_refs(const char *service, int for_push)
 		return last;
 	free_discovery(last);
 
-	strbuf_addf(&buffer, "%sinfo/refs", url);
+	strbuf_addf(&refs_url, "%sinfo/refs", url);
 	if ((!prefixcmp(url, "http://") || !prefixcmp(url, "https://")) &&
 	     git_env_bool("GIT_SMART_HTTP", 1)) {
 		maybe_smart = 1;
 		if (!strchr(url, '?'))
-			strbuf_addch(&buffer, '?');
+			strbuf_addch(&refs_url, '?');
 		else
-			strbuf_addch(&buffer, '&');
-		strbuf_addf(&buffer, "service=%s", service);
+			strbuf_addch(&refs_url, '&');
+		strbuf_addf(&refs_url, "service=%s", service);
 	}
-	refs_url = strbuf_detach(&buffer, NULL);
 
 	memset(&options, 0, sizeof(options));
 	options.content_type = &type;
 	options.no_cache = 1;
 	options.keep_error = 1;
 
-	http_ret = http_get_strbuf(refs_url, &buffer, &options);
+	http_ret = http_get_strbuf(refs_url.buf, &buffer, &options);
 	switch (http_ret) {
 	case HTTP_OK:
 		break;
@@ -283,7 +282,7 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	else
 		last->refs = parse_info_refs(last);
 
-	free(refs_url);
+	strbuf_release(&refs_url);
 	strbuf_release(&exp);
 	strbuf_release(&type);
 	strbuf_release(&buffer);

-- 
1.8.4.rc3.19.g9da5bf6
Jeff King· Sep 28, 2013, 08:35 UTC · re: Jeff King · lore

[PATCH 8/9] remote-curl: store url as a strbuf

We use a strbuf to generate the string containing the remote URL, but then detach it to a bare pointer. This makes it harder to later manipulate the URL, as we have forgotten the length (and the allocation semantics are not as clear).

Let's instead keep the strbuf around. As a bonus, this eliminates a confusing double-use of the "buf" strbuf in main(). Prior to this, it was used both for constructing the url, and for reading commands from stdin.

The downside is that we have to update each call site to refer to "url.buf" rather than just "url" when they want the C string.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote-curl.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)
Show changes to remote-curl.c +19 −19
diff --git a/remote-curl.c b/remote-curl.c
index 7fb092f..5add2cb 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -12,7 +12,8 @@ static struct remote *remote;
 #include "credential.h"
 
 static struct remote *remote;
-static const char *url; /* always ends with a trailing slash */
+/* always ends with a trailing slash */
+static struct strbuf url = STRBUF_INIT;
 
 struct options {
 	int verbosity;
@@ -131,7 +132,8 @@ static struct ref *parse_info_refs(struct discovery *heads)
 			mid = &data[i];
 		if (data[i] == '\n') {
 			if (mid - start != 40)
-				die("%sinfo/refs not valid: is this a git repository?", url);
+				die("%sinfo/refs not valid: is this a git repository?",
+				    url.buf);
 			data[i] = 0;
 			ref_name = mid + 1;
 			ref = xmalloc(sizeof(struct ref) +
@@ -150,7 +152,7 @@ static struct ref *parse_info_refs(struct discovery *heads)
 	}
 
 	ref = alloc_ref("HEAD");
-	if (!http_fetch_ref(url, ref) &&
+	if (!http_fetch_ref(url.buf, ref) &&
 	    !resolve_remote_symref(ref, refs)) {
 		ref->next = refs;
 		refs = ref;
@@ -213,11 +215,11 @@ static struct discovery* discover_refs(const char *service, int for_push)
 		return last;
 	free_discovery(last);
 
-	strbuf_addf(&refs_url, "%sinfo/refs", url);
-	if ((!prefixcmp(url, "http://") || !prefixcmp(url, "https://")) &&
+	strbuf_addf(&refs_url, "%sinfo/refs", url.buf);
+	if ((!prefixcmp(url.buf, "http://") || !prefixcmp(url.buf, "https://")) &&
 	     git_env_bool("GIT_SMART_HTTP", 1)) {
 		maybe_smart = 1;
-		if (!strchr(url, '?'))
+		if (!strchr(url.buf, '?'))
 			strbuf_addch(&refs_url, '?');
 		else
 			strbuf_addch(&refs_url, '&');
@@ -235,13 +237,13 @@ static struct discovery* discover_refs(const char *service, int for_push)
 		break;
 	case HTTP_MISSING_TARGET:
 		show_http_message(&type, &buffer);
-		die("repository '%s' not found", url);
+		die("repository '%s' not found", url.buf);
 	case HTTP_NOAUTH:
 		show_http_message(&type, &buffer);
-		die("Authentication failed for '%s'", url);
+		die("Authentication failed for '%s'", url.buf);
 	default:
 		show_http_message(&type, &buffer);
-		die("unable to access '%s': %s", url, curl_errorstr);
+		die("unable to access '%s': %s", url.buf, curl_errorstr);
 	}
 
 	last= xcalloc(1, sizeof(*last_discovery));
@@ -607,7 +609,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.buf, svc);
 	rpc->service_url = strbuf_detach(&buf, NULL);
 
 	strbuf_addf(&buf, "Content-Type: application/x-%s-request", svc);
@@ -659,7 +661,7 @@ static int fetch_dumb(int nr_heads, struct ref **to_fetch)
 	for (i = 0; i < nr_heads; i++)
 		targets[i] = xstrdup(sha1_to_hex(to_fetch[i]->old_sha1));
 
-	walker = get_http_walker(url);
+	walker = get_http_walker(url.buf);
 	walker->get_all = 1;
 	walker->get_tree = 1;
 	walker->get_history = 1;
@@ -706,7 +708,7 @@ static int fetch_git(struct discovery *heads,
 		depth_arg = strbuf_detach(&buf, NULL);
 		argv[argc++] = depth_arg;
 	}
-	argv[argc++] = url;
+	argv[argc++] = url.buf;
 	argv[argc++] = NULL;
 
 	for (i = 0; i < nr_heads; i++) {
@@ -804,7 +806,7 @@ static int push_dav(int nr_spec, char **specs)
 		argv[argc++] = "--dry-run";
 	if (options.verbosity > 1)
 		argv[argc++] = "--verbose";
-	argv[argc++] = url;
+	argv[argc++] = url.buf;
 	for (i = 0; i < nr_spec; i++)
 		argv[argc++] = specs[i];
 	argv[argc++] = NULL;
@@ -837,7 +839,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
 	argv_array_push(&args, options.progress ? "--progress" : "--no-progress");
 	for_each_string_list_item(cas_option, &cas_options)
 		argv_array_push(&args, cas_option->string);
-	argv_array_push(&args, url);
+	argv_array_push(&args, url.buf);
 	for (i = 0; i < nr_spec; i++)
 		argv_array_push(&args, specs[i]);
 
@@ -918,14 +920,12 @@ int main(int argc, const char **argv)
 	remote = remote_get(argv[1]);
 
 	if (argc > 2) {
-		end_url_with_slash(&buf, argv[2]);
+		end_url_with_slash(&url, argv[2]);
 	} else {
-		end_url_with_slash(&buf, remote->url[0]);
+		end_url_with_slash(&url, remote->url[0]);
 	}
 
-	url = strbuf_detach(&buf, NULL);
-
-	http_init(remote, url, 0);
+	http_init(remote, url.buf, 0);
 
 	do {
 		if (strbuf_getline(&buf, stdin, '\n') == EOF) {

-- 
1.8.4.rc3.19.g9da5bf6
Jeff King· Sep 28, 2013, 08:35 UTC · re: Jeff King · lore

[PATCH 9/9] remote-curl: rewrite base url from info/refs redirects

For efficiency and security reasons, an earlier commit in this series taught http_get_* to re-write the base url based on redirections we saw while making a specific request.

This commit wires that option into the info/refs request, meaning that a redirect from

    http://example.com/foo.git/info/refs
to
    https://example.com/bar.git/info/refs

will behave as if "https://example.com/bar.git" had been provided to git in the first place.

The tests bear some explanation. We introduce two new hierearchies into the httpd test config:

  1. Requests to /smart-redir-limited will work only for the
     initial info/refs request, but not any subsequent
     requests. As a result, we can confirm whether the
     client is re-rooting its requests after the initial
     contact, since otherwise it will fail (it will ask for
     "repo.git/git-upload-pack", which is not redirected).
  2. Requests to smart-redir-auth will redirect, and require
     auth after the redirection. Since we are using the
     redirected base for further requests, we also update
     the credential struct, in order not to mislead the user
     (or credential helpers) about which credential is
     needed. We can therefore check the GIT_ASKPASS prompts
     to make sure we are prompting for the new location.
     Because we have neither multiple servers nor https
     support in our test setup, we can only redirect between
     paths, meaning we need to turn on
     credential.useHttpPath to see the difference.
Signed-off-by: Jeff King <peff@peff.net>
---
 remote-curl.c           |  4 ++++
 t/lib-httpd.sh          |  3 ++-
 t/lib-httpd/apache.conf |  2 ++
 t/t5551-http-fetch.sh   | 11 +++++++++++
 4 files changed, 19 insertions(+), 1 deletion(-)
Show changes to 4 files +19 −1

remote-curl.c, t/lib-httpd.sh, t/lib-httpd/apache.conf, t/t5551-http-fetch.sh

diff --git a/remote-curl.c b/remote-curl.c
index 5add2cb..c9b891a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -207,6 +207,7 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	struct strbuf type = STRBUF_INIT;
 	struct strbuf buffer = STRBUF_INIT;
 	struct strbuf refs_url = STRBUF_INIT;
+	struct strbuf effective_url = STRBUF_INIT;
 	struct discovery *last = last_discovery;
 	int http_ret, maybe_smart = 0;
 	struct http_get_options options;
@@ -228,6 +229,8 @@ static struct discovery* discover_refs(const char *service, int for_push)
 
 	memset(&options, 0, sizeof(options));
 	options.content_type = &type;
+	options.effective_url = &effective_url;
+	options.base_url = &url;
 	options.no_cache = 1;
 	options.keep_error = 1;
 
@@ -287,6 +290,7 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	strbuf_release(&refs_url);
 	strbuf_release(&exp);
 	strbuf_release(&type);
+	strbuf_release(&effective_url);
 	strbuf_release(&buffer);
 	last_discovery = last;
 	return last;
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 54dbbfe..ad8f1ef 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -204,7 +204,8 @@ expect_askpass() {
 }
 
 expect_askpass() {
-	dest=$HTTPD_DEST
+	dest=$HTTPD_DEST${3+/$3}
+
 	{
 		case "$1" in
 		none)
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 397c480..3a03e82 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -110,6 +110,8 @@ RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]
 RewriteEngine on
 RewriteRule ^/smart-redir-perm/(.*)$ /smart/$1 [R=301]
 RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]
+RewriteRule ^/smart-redir-auth/(.*)$ /auth/smart/$1 [R=301]
+RewriteRule ^/smart-redir-limited/(.*)/info/refs$ /smart/$1/info/refs [R=301]
 
 <IfDefine SSL>
 LoadModule ssl_module modules/mod_ssl.so
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index 8196af1..fb16833 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -113,6 +113,10 @@ test_expect_success 'follow redirects (302)' '
 	git clone $HTTPD_URL/smart-redir-temp/repo.git --quiet repo-t
 '
 
+test_expect_success 'redirects re-root further requests' '
+	git clone $HTTPD_URL/smart-redir-limited/repo.git repo-redir-limited
+'
+
 test_expect_success 'clone from password-protected repository' '
 	echo two >expect &&
 	set_askpass user@host &&
@@ -146,6 +150,13 @@ test_expect_success 'no-op half-auth fetch does not require a password' '
 	expect_askpass none
 '
 
+test_expect_success 'redirects send auth to new location' '
+	set_askpass user@host &&
+	git -c credential.useHttpPath=true \
+	  clone $HTTPD_URL/smart-redir-auth/repo.git repo-redir-auth &&
+	expect_askpass both user@host auth/smart/repo.git
+'
+
 test_expect_success 'disable dumb http on server' '
 	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
 		config http.getanyfile false

-- 
1.8.4.rc3.19.g9da5bf6

← back to recent threads