From 3d1fb769b26dbf278a0ef0cf7427aac86a161bb0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:31:00 -0400 Subject: [PATCH 01/10] 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 Signed-off-by: Jonathan Nieder --- http.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/http.c b/http.c index 2d086aedfa..9b1015881a 100644 --- a/http.c +++ b/http.c @@ -941,7 +941,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; @@ -950,7 +950,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); From 132b70a2ed6d952e8142981474b41884ff93b780 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:31:11 -0400 Subject: [PATCH 02/10] 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 Signed-off-by: Jonathan Nieder --- http.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/http.c b/http.c index 9b1015881a..202468a980 100644 --- a/http.c +++ b/http.c @@ -820,6 +820,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 @@ -878,13 +890,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); From 1bbcc224ccfdccd99b1221d83150a8ffb8059ffd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:31:23 -0400 Subject: [PATCH 03/10] 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 Signed-off-by: Jonathan Nieder --- http-push.c | 4 ++-- http.c | 44 +++++++++++++++++++++++++------------------- http.h | 15 ++++++++++----- remote-curl.c | 9 +++++++-- 4 files changed, 44 insertions(+), 28 deletions(-) diff --git a/http-push.c b/http-push.c index 6dad188b5f..09ebb8f48c 100644 --- a/http-push.c +++ b/http-push.c @@ -1543,7 +1543,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; @@ -1567,7 +1567,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 202468a980..6bb2e456ff 100644 --- a/http.c +++ b/http.c @@ -836,8 +836,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; @@ -870,9 +871,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); @@ -890,8 +891,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); @@ -900,11 +902,10 @@ static int http_request(const char *url, struct strbuf *type, } 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; @@ -914,7 +915,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); @@ -923,15 +924,14 @@ static int http_request_reauth(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); } /* @@ -940,7 +940,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; @@ -954,7 +955,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)) @@ -966,12 +967,15 @@ cleanup: 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); @@ -1055,6 +1059,7 @@ add_pack: 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; @@ -1064,7 +1069,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 d77c1b54f2..40404b4836 100644 --- a/http.h +++ b/http.h @@ -125,11 +125,16 @@ extern void append_remote_object_url(struct strbuf *buf, const char *url, 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 5b3ce9eed2..ed1499b62c 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -187,6 +187,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; @@ -204,8 +205,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; From 2501aff8b7516115c409cb34cc50305cdde40a47 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:31:45 -0400 Subject: [PATCH 04/10] 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 Signed-off-by: Jonathan Nieder --- http.c | 6 ++++-- http.h | 1 + remote-curl.c | 7 ++++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/http.c b/http.c index 6bb2e456ff..5703074d95 100644 --- a/http.c +++ b/http.c @@ -45,7 +45,7 @@ static long curl_low_speed_time = -1; static int curl_ftp_no_epsv; static const char *curl_http_proxy; static const char *curl_cookie_file; -static struct credential http_auth = CREDENTIAL_INIT; +struct credential http_auth = CREDENTIAL_INIT; static int http_proactive_auth; static const char *user_agent; @@ -806,7 +806,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 { @@ -924,6 +923,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 40404b4836..17116abc57 100644 --- a/http.h +++ b/http.h @@ -102,6 +102,7 @@ extern void http_cleanup(void); 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 ed1499b62c..8ffd7ff7a7 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -8,6 +8,7 @@ #include "pkt-line.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 */ @@ -449,6 +450,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; @@ -548,8 +551,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; From 78868962c03e5bdddad5c5d02c1d5a0c72a7ac26 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:32:02 -0400 Subject: [PATCH 05/10] 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 Signed-off-by: Jonathan Nieder --- http.c | 4 ++++ http.h | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/http.c b/http.c index 5703074d95..1569c56108 100644 --- a/http.c +++ b/http.c @@ -894,6 +894,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 17116abc57..974ede7736 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_*() */ From c93c92f30977adb2eb385a851f9f5e9975da7d5e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:34:05 -0400 Subject: [PATCH 06/10] 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 generate 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 Signed-off-by: Jonathan Nieder --- http.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ http.h | 8 ++++++++ 2 files changed, 68 insertions(+) diff --git a/http.c b/http.c index 1569c56108..5199e602be 100644 --- a/http.c +++ b/http.c @@ -904,11 +904,71 @@ static int http_request(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 (prefixcmp(asked, base->buf)) + 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 974ede7736..12ca5c892d 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_*() */ From c65d5692cd3434133c2e8f1a20074a261e0267f0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:35:10 -0400 Subject: [PATCH 07/10] 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 Signed-off-by: Jonathan Nieder --- remote-curl.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/remote-curl.c b/remote-curl.c index 8ffd7ff7a7..af087267d6 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -185,8 +185,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; @@ -194,24 +194,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; @@ -264,7 +263,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); From b227bbc43a568b282b5f8cb35e563d00d60b272d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:35:25 -0400 Subject: [PATCH 08/10] 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 Signed-off-by: Jonathan Nieder --- remote-curl.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/remote-curl.c b/remote-curl.c index af087267d6..345fea8898 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -11,7 +11,8 @@ #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; @@ -112,7 +113,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) + @@ -131,7 +133,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; @@ -194,11 +196,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, '&'); @@ -216,13 +218,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)); @@ -588,7 +590,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); @@ -640,7 +642,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; @@ -685,7 +687,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++) { @@ -783,7 +785,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; @@ -813,7 +815,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs) else if (options.verbosity > 1) argv_array_push(&args, "--verbose"); argv_array_push(&args, options.progress ? "--progress" : "--no-progress"); - argv_array_push(&args, url); + argv_array_push(&args, url.buf); for (i = 0; i < nr_spec; i++) argv_array_push(&args, specs[i]); @@ -894,14 +896,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) { From 050ef3655c8ea1dc7a2b3b843ca7c45dd94d9c88 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:35:35 -0400 Subject: [PATCH 09/10] 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 Signed-off-by: Jonathan Nieder --- 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(-) diff --git a/remote-curl.c b/remote-curl.c index 345fea8898..ef1684b9df 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -188,6 +188,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; @@ -209,6 +210,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; @@ -268,6 +271,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 895b9258b0..7059cc6c21 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -187,7 +187,8 @@ set_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 dd17e3a09d..4a261f13f5 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -102,6 +102,8 @@ ScriptAlias /broken_smart/ broken-smart-http.sh/ 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] LoadModule ssl_module modules/mod_ssl.so diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh index 55a866af80..1b71bb5156 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 From 70900eda4a0ed473ab5a933940285ac3dda698c0 Mon Sep 17 00:00:00 2001 From: Ramsay Jones Date: Thu, 24 Oct 2013 21:17:19 +0100 Subject: [PATCH 10/10] http.c: Spell the null pointer as NULL Commit 1bbcc224 ("http: refactor options to http_get_*", 28-09-2013) changed the type of final 'options' argument of the http_get_file() function from an int to an 'struct http_get_options' pointer. However, it neglected to update the (single) call site. Since this call was passing '0' to that argument, it was (correctly) being interpreted as a null pointer. Change to argument to NULL. Noticed by sparse. ("Using plain integer as NULL pointer") Signed-off-by: Ramsay Jones Acked-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- http.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http.c b/http.c index 5199e602be..3447945ee4 100644 --- a/http.c +++ b/http.c @@ -1072,7 +1072,7 @@ static char *fetch_pack_index(unsigned char *sha1, const char *base_url) strbuf_addf(&buf, "%s.temp", sha1_pack_index_name(sha1)); tmp = strbuf_detach(&buf, NULL); - if (http_get_file(url, tmp, 0) != HTTP_OK) { + if (http_get_file(url, tmp, NULL) != HTTP_OK) { error("Unable to get pack index %s", url); free(tmp); tmp = NULL;