Commit 5ce6f51f authored by Junio C Hamano's avatar Junio C Hamano

Merge branch 'jk/http-walker-limit-redirect' into maint

Update the error messages from the dumb-http client when it fails
to obtain loose objects; we used to give sensible error message
only upon 404 but we now forbid unexpected redirects that needs to
be reported with something sensible.

* jk/http-walker-limit-redirect:
  http-walker: complain about non-404 loose object errors
  http: treat http-alternates like redirects
  http: make redirects more obvious
  remote-curl: rename shadowed options variable
  http: always update the base URL for redirects
  http: simplify update_url_from_redirect
parents 7479ca4b 3680f16f
......@@ -1891,6 +1891,16 @@ http.userAgent::
of common USER_AGENT strings (but not including those like git/1.7.1).
Can be overridden by the `GIT_HTTP_USER_AGENT` environment variable.
Whether git should follow HTTP redirects. If set to `true`, git
will transparently follow any redirect issued by a server it
encounters. If set to `false`, git will treat all redirects as
errors. If set to `initial`, git will follow redirects only for
the initial request to a remote, but not for subsequent
follow-up HTTP requests. Since git uses the redirected URL as
the base for the follow-up requests, this is generally
sufficient. The default is `initial`.
Any of the http.* options above can be applied selectively to some URLs.
For a config key to match a URL, each element of the config key is
......@@ -274,9 +274,8 @@ static void process_alternates_response(void *callback_data)
struct strbuf target = STRBUF_INIT;
strbuf_add(&target, base, serverlen);
strbuf_add(&target, data + i, posn - i - 7);
if (walker->get_verbosely)
fprintf(stderr, "Also look at %s\n",
warning("adding alternate object store: %s",
newalt = xmalloc(sizeof(*newalt));
newalt->next = NULL;
newalt->base = strbuf_detach(&target, NULL);
......@@ -302,6 +301,9 @@ static void fetch_alternates(struct walker *walker, const char *base)
struct alternates_request alt_req;
struct walker_data *cdata = walker->data;
if (http_follow_config != HTTP_FOLLOW_ALWAYS)
* If another request has already started fetching alternates,
* wait for them to arrive and return to processing this request's
......@@ -480,10 +482,13 @@ static int fetch_object(struct walker *walker, unsigned char *sha1)
* we turned off CURLOPT_FAILONERROR to avoid losing a
* persistent connection and got CURLE_OK.
if (req->http_code == 404 && req->curl_result == CURLE_OK &&
if (req->http_code >= 300 && req->curl_result == CURLE_OK &&
(starts_with(req->url, "http://") ||
starts_with(req->url, "https://")))
starts_with(req->url, "https://"))) {
req->curl_result = CURLE_HTTP_RETURNED_ERROR;
xsnprintf(req->errorstr, sizeof(req->errorstr),
"HTTP request failed");
if (obj_req->state == ABORTED) {
ret = error("Request for %s aborted", hex);
......@@ -111,6 +111,8 @@ static int http_proactive_auth;
static const char *user_agent;
static int curl_empty_auth;
enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
#if LIBCURL_VERSION_NUM >= 0x071700
#elif LIBCURL_VERSION_NUM >= 0x070903
......@@ -366,6 +368,16 @@ static int http_options(const char *var, const char *value, void *cb)
return 0;
if (!strcmp("http.followredirects", var)) {
if (value && !strcmp(value, "initial"))
http_follow_config = HTTP_FOLLOW_INITIAL;
else if (git_config_bool(var, value))
http_follow_config = HTTP_FOLLOW_ALWAYS;
http_follow_config = HTTP_FOLLOW_NONE;
return 0;
/* Fall back on the default ones */
return git_default_config(var, value, cb);
......@@ -717,7 +729,6 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_FOLLOWLOCATION, 1);
curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
#if LIBCURL_VERSION_NUM >= 0x071301
curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
......@@ -734,6 +745,7 @@ static CURL *get_curl_handle(void)
if (is_transport_allowed("ftps"))
allowed_protocols |= CURLPROTO_FTPS;
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
curl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);
if (transport_restrict_protocols())
warning("protocol restrictions not applied to curl redirects because\n"
......@@ -1044,6 +1056,16 @@ struct active_request_slot *get_active_slot(void)
curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);
* Default following to off unless "ALWAYS" is configured; this gives
* callers a sane starting point, and they can tweak for individual
* HTTP_FOLLOW_* cases themselves.
if (http_follow_config == HTTP_FOLLOW_ALWAYS)
curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 0);
#if LIBCURL_VERSION_NUM >= 0x070a08
curl_easy_setopt(slot->curl, CURLOPT_IPRESOLVE, git_curl_ipresolve);
......@@ -1286,9 +1308,12 @@ static int handle_curl_result(struct slot_results *results)
* If we see a failing http code with CURLE_OK, we have turned off
* FAILONERROR (to keep the server's custom error response), and should
* translate the code into failure here.
* Likewise, if we see a redirect (30x code), that means we turned off
* redirect-following, and we should treat the result as an error.
if (results->curl_result == CURLE_OK &&
results->http_code >= 400) {
results->http_code >= 300) {
results->curl_result = CURLE_HTTP_RETURNED_ERROR;
* Normally curl will already have put the "reason phrase"
......@@ -1607,6 +1632,9 @@ static int http_request(const char *url,
strbuf_addstr(&buf, " no-cache");
if (options && options->keep_error)
curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
if (options && options->initial_request &&
http_follow_config == HTTP_FOLLOW_INITIAL)
curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
headers = curl_slist_append(headers, buf.buf);
......@@ -1655,16 +1683,16 @@ static int http_request(const char *url,
* 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).
* "info/refs". In such a case we die. There's not much we can do, such a
* scheme is unlikely to represent a real git repository, and failing to
* rewrite the base opens options for malicious redirects to do funny things.
static int update_url_from_redirect(struct strbuf *base,
const char *asked,
const struct strbuf *got)
const char *tail;
size_t tail_len;
size_t new_len;
if (!strcmp(asked, got->buf))
return 0;
......@@ -1673,14 +1701,16 @@ static int update_url_from_redirect(struct strbuf *base,
die("BUG: update_url_from_redirect: %s is not a superset of %s",
asked, base->buf);
tail_len = strlen(tail);
if (got->len < tail_len ||
strcmp(tail, got->buf + got->len - tail_len))
return 0; /* insane redirect scheme */
new_len = got->len;
if (!strip_suffix_mem(got->buf, &new_len, tail))
die(_("unable to update url base from redirection:\n"
" asked for: %s\n"
" redirect: %s"),
asked, got->buf);
strbuf_add(base, got->buf, got->len - tail_len);
strbuf_add(base, got->buf, new_len);
return 1;
......@@ -2028,7 +2058,7 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,
if (c != CURLE_OK)
die("BUG: curl_easy_getinfo for HTTP code failed: %s",
if (slot->http_code >= 400)
if (slot->http_code >= 300)
return size;
......@@ -116,6 +116,13 @@ extern struct credential http_auth;
extern char curl_errorstr[CURL_ERROR_SIZE];
enum http_follow_config {
extern enum http_follow_config http_follow_config;
static inline int missing__target(int code, int result)
return /* file:// URL -- do we ever use one??? */
......@@ -139,7 +146,8 @@ extern char *get_remote_object_url(const char *url, const char *hex,
/* Options for http_get_*() */
struct http_get_options {
unsigned no_cache:1,
/* If non-NULL, returns the content-type of the response. */
struct strbuf *content_type;
......@@ -274,7 +274,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
struct strbuf effective_url = STRBUF_INIT;
struct discovery *last = last_discovery;
int http_ret, maybe_smart = 0;
struct http_get_options options;
struct http_get_options http_options;
if (last && !strcmp(service, last->service))
return last;
......@@ -291,15 +291,16 @@ static struct discovery *discover_refs(const char *service, int for_push)
strbuf_addf(&refs_url, "service=%s", service);
memset(&options, 0, sizeof(options));
options.content_type = &type;
options.charset = &charset;
options.effective_url = &effective_url;
options.base_url = &url;
options.no_cache = 1;
options.keep_error = 1;
memset(&http_options, 0, sizeof(http_options));
http_options.content_type = &type;
http_options.charset = &charset;
http_options.effective_url = &effective_url;
http_options.base_url = &url;
http_options.initial_request = 1;
http_options.no_cache = 1;
http_options.keep_error = 1;
http_ret = http_get_strbuf(refs_url.buf, &buffer, &options);
http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);
switch (http_ret) {
case HTTP_OK:
......@@ -314,6 +315,9 @@ static struct discovery *discover_refs(const char *service, int for_push)
die("unable to access '%s': %s", url.buf, curl_errorstr);
if (options.verbosity && !starts_with(refs_url.buf, url.buf))
warning(_("redirecting to %s"), url.buf);
last= xcalloc(1, sizeof(*last_discovery));
last->service = service;
last->buf_alloc = strbuf_detach(&buffer, &last->len);
......@@ -123,6 +123,7 @@ ScriptAlias /error/
RewriteEngine on
RewriteRule ^/dumb-redir/(.*)$ /dumb/$1 [R=301]
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]
......@@ -132,6 +133,19 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]
RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302]
RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
# The first rule issues a client-side redirect to something
# that _doesn't_ look like a git repo. The second rule is a
# server-side rewrite, so that it turns out the odd-looking
# thing _is_ a git repo. The "[PT]" tells Apache to match
# the usual ScriptAlias rules for /smart.
RewriteRule ^/insane-redir/(.*)$ /intern-redir/$1/foo [R=301]
RewriteRule ^/intern-redir/(.*)/foo$ /smart/$1 [PT]
# Serve info/refs internally without redirecting, but
# issue a redirect for any object requests.
RewriteRule ^/redir-objects/(.*/info/refs)$ /dumb/$1 [PT]
RewriteRule ^/redir-objects/(.*/objects/.*)$ /dumb/$1 [R=301]
# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
# And as RewriteCond does not allow testing for non-matches, we match
# the desired case first (one has abra, two has cadabra), and let it
......@@ -307,5 +307,66 @@ test_expect_success 'remote-http complains cleanly about malformed urls' '
test_must_fail git remote-http http::/
test_expect_success 'redirects can be forbidden/allowed' '
test_must_fail git -c http.followRedirects=false \
clone $HTTPD_URL/dumb-redir/repo.git dumb-redir &&
git -c http.followRedirects=true \
clone $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr
test_expect_success 'redirects are reported to stderr' '
# just look for a snippet of the redirected-to URL
test_i18ngrep /dumb/ stderr
test_expect_success 'non-initial redirects can be forbidden' '
test_must_fail git -c http.followRedirects=initial \
clone $HTTPD_URL/redir-objects/repo.git redir-objects &&
git -c http.followRedirects=true \
clone $HTTPD_URL/redir-objects/repo.git redir-objects
test_expect_success 'http.followRedirects defaults to "initial"' '
test_must_fail git clone $HTTPD_URL/redir-objects/repo.git default
# The goal is for a clone of the "evil" repository, which has no objects
# itself, to cause the client to fetch objects from the "victim" repository.
test_expect_success 'set up evil alternates scheme' '
victim=$HTTPD_DOCUMENT_ROOT_PATH/victim.git &&
git init --bare "$victim" &&
git -C "$victim" --work-tree=. commit --allow-empty -m secret &&
git -C "$victim" repack -ad &&
git -C "$victim" update-server-info &&
sha1=$(git -C "$victim" rev-parse HEAD) &&
evil=$HTTPD_DOCUMENT_ROOT_PATH/evil.git &&
git init --bare "$evil" &&
# do this by hand to avoid object existence check
printf "%s\\t%s\\n" $sha1 refs/heads/master >"$evil/info/refs"
# Here we'll just redirect via HTTP. In a real-world attack these would be on
# different servers, but we should reject it either way.
test_expect_success 'http-alternates is a non-initial redirect' '
echo "$HTTPD_URL/dumb/victim.git/objects" \
>"$evil/objects/info/http-alternates" &&
test_must_fail git -c http.followRedirects=initial \
clone $HTTPD_URL/dumb/evil.git evil-initial &&
git -c http.followRedirects=true \
clone $HTTPD_URL/dumb/evil.git evil-initial
# Curl supports a lot of protocols that we'd prefer not to allow
# http-alternates to use, but it's hard to test whether curl has
# accessed, say, the SMTP protocol, because we are not running an SMTP server.
# But we can check that it does not allow access to file://, which would
# otherwise allow this clone to complete.
test_expect_success 'http-alternates cannot point at funny protocols' '
echo "file://$victim/objects" >"$evil/objects/info/http-alternates" &&
test_must_fail git -c http.followRedirects=true \
clone "$HTTPD_URL/dumb/evil.git" evil-file
......@@ -119,6 +119,10 @@ test_expect_success 'redirects re-root further requests' '
git clone $HTTPD_URL/smart-redir-limited/repo.git repo-redir-limited
test_expect_success 're-rooting dies on insane schemes' '
test_must_fail git clone $HTTPD_URL/insane-redir/repo.git insane
test_expect_success 'clone from password-protected repository' '
echo two >expect &&
set_askpass user@host pass@host &&
......@@ -18,6 +18,7 @@ test_proto "smart http" http "$HTTPD_URL/smart/repo.git"
test_expect_success 'curl redirects respect whitelist' '
test_must_fail env GIT_ALLOW_PROTOCOL=http:https \
git clone "$HTTPD_URL/ftp-redir/repo.git" 2>stderr &&
test_i18ngrep "ftp.*disabled" stderr ||
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment