From 3d1fb769b26dbf278a0ef0cf7427aac86a161bb0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:31:00 -0400 Subject: 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(-) (limited to 'http.c') 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); -- cgit v1.2.1 From 132b70a2ed6d952e8142981474b41884ff93b780 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:31:11 -0400 Subject: 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(-) (limited to 'http.c') 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); -- cgit v1.2.1 From 1bbcc224ccfdccd99b1221d83150a8ffb8059ffd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:31:23 -0400 Subject: 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.c | 44 +++++++++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 19 deletions(-) (limited to 'http.c') 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; -- cgit v1.2.1 From 2501aff8b7516115c409cb34cc50305cdde40a47 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:31:45 -0400 Subject: 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 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'http.c') 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); } -- cgit v1.2.1 From 78868962c03e5bdddad5c5d02c1d5a0c72a7ac26 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:32:02 -0400 Subject: 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 ++++ 1 file changed, 4 insertions(+) (limited to 'http.c') 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); -- cgit v1.2.1 From c93c92f30977adb2eb385a851f9f5e9975da7d5e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:34:05 -0400 Subject: 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 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) (limited to 'http.c') 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; -- cgit v1.2.1 From 70900eda4a0ed473ab5a933940285ac3dda698c0 Mon Sep 17 00:00:00 2001 From: Ramsay Jones Date: Thu, 24 Oct 2013 21:17:19 +0100 Subject: 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(-) (limited to 'http.c') 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; -- cgit v1.2.1