http, imap-send: stop using CURLOPT_VERBOSE

Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if
GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting
CURLOPT_VERBOSE.

This is to prevent inadvertent revelation of sensitive data. In
particular, GIT_CURL_VERBOSE redacts neither the "Authorization" header
nor any cookies specified by GIT_REDACT_COOKIES.

Unifying the tracing mechanism also has the future benefit that any
improvements to the tracing mechanism will benefit both users of
GIT_CURL_VERBOSE and GIT_TRACE_CURL, and we do not need to remember to
implement any improvement twice.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 9d6769e95ab1d356e6c8b07671ac11446a2acc96..427ea707014958fc31ea714eab47e851b0558fcf 100644 (file)
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -721,8 +721,6 @@ of clones and fetches.
        Enables a curl full trace dump of all incoming and outgoing data,
        including descriptive information, of the git transport protocol.
        This is similar to doing curl `--trace-ascii` on the command line.
-       This option overrides setting the `GIT_CURL_VERBOSE` environment
-       variable.
        See `GIT_TRACE` for available trace output options.
 
 `GIT_TRACE_CURL_NO_DATA`::

diff --git a/http.c b/http.c
index 62aa995245324dc30fec699ed8c570af7b152b8f..4882c9f5b268970c571628f5a284e46943b86535 100644 (file)
--- a/http.c
+++ b/http.c
@@ -804,6 +804,12 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,
        return 0;
 }
 
+void http_trace_curl_no_data(void)
+{
+       trace_override_envvar(&trace_curl, "1");
+       trace_curl_data = 0;
+}
+
 void setup_curl_trace(CURL *handle)
 {
        if (!trace_want(&trace_curl))
@@ -993,7 +999,7 @@ static CURL *get_curl_handle(void)
        warning(_("Protocol restrictions not supported with cURL < 7.19.4"));
 #endif
        if (getenv("GIT_CURL_VERBOSE"))
-               curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
+               http_trace_curl_no_data();
        setup_curl_trace(result);
        if (getenv("GIT_TRACE_CURL_NO_DATA"))
                trace_curl_data = 0;

diff --git a/http.h b/http.h
index 5e0ad724f92f3c708d2b330bb689f6eef6bb8de4..faf8cbb0d10e44e036542a15aa6684a5eb7416cc 100644 (file)
--- a/http.h
+++ b/http.h
@@ -252,6 +252,13 @@ int finish_http_object_request(struct http_object_request *freq);
 void abort_http_object_request(struct http_object_request *freq);
 void release_http_object_request(struct http_object_request *freq);
 
+/*
+ * Instead of using environment variables to determine if curl tracing happens,
+ * behave as if GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set. Call this
+ * before calling setup_curl_trace().
+ */
+void http_trace_curl_no_data(void);
+
 /* setup routine for curl_easy_setopt CURLOPT_DEBUGFUNCTION */
 void setup_curl_trace(CURL *handle);
 #endif /* HTTP_H */

diff --git a/imap-send.c b/imap-send.c
index 6c54d8c29d64c0ea89943494986513c1054065f4..52737546f38b65f1cba1043dbda349d6680b78b6 100644 (file)
--- a/imap-send.c
+++ b/imap-send.c
@@ -1464,7 +1464,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
        curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);
 
        if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
-               curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
+               http_trace_curl_no_data();
        setup_curl_trace(curl);
 
        return curl;

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index acc8473a72a8e7fb01635a825bf98bbc592c6bb8..be01cf7bb24516bc706b06a0cd8b6572c0901bdb 100755 (executable)
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -197,6 +197,18 @@ test_expect_success 'GIT_TRACE_CURL redacts auth details' '
        grep "Authorization: Basic <redacted>" trace
 '
 
+test_expect_success 'GIT_CURL_VERBOSE redacts auth details' '
+       rm -rf redact-auth trace &&
+       set_askpass user@host pass@host &&
+       GIT_CURL_VERBOSE=1 git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth 2>trace &&
+       expect_askpass both user@host &&
+
+       # Ensure that there is no "Basic" followed by a base64 string, but that
+       # the auth details are redacted
+       ! grep "Authorization: Basic [0-9a-zA-Z+/]" trace &&
+       grep "Authorization: Basic <redacted>" trace
+'
+
 test_expect_success 'disable dumb http on server' '
        git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
                config http.getanyfile false
@@ -454,6 +466,18 @@ test_expect_success 'GIT_REDACT_COOKIES redacts cookies' '
        ! grep "Cookie:.*Bar=2" err
 '
 
+test_expect_success 'GIT_REDACT_COOKIES redacts cookies when GIT_CURL_VERBOSE=1' '
+       rm -rf clone &&
+       echo "Set-Cookie: Foo=1" >cookies &&
+       echo "Set-Cookie: Bar=2" >>cookies &&
+       GIT_CURL_VERBOSE=1 GIT_REDACT_COOKIES=Bar,Baz \
+               git -c "http.cookieFile=$(pwd)/cookies" clone \
+               $HTTPD_URL/smart/repo.git clone 2>err &&
+       grep "Cookie:.*Foo=1" err &&
+       grep "Cookie:.*Bar=<redacted>" err &&
+       ! grep "Cookie:.*Bar=2" err
+'
+
 test_expect_success 'GIT_REDACT_COOKIES handles empty values' '
        rm -rf clone &&
        echo "Set-Cookie: Foo=" >cookies &&

diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh
index 5129b0724f703890c80c8dcbe7bd4347792963e3..927aad082098254d0ca31e96ba22310d044d8e3b 100755 (executable)
--- a/t/t5581-http-curl-verbose.sh
+++ b/t/t5581-http-curl-verbose.sh
@@ -20,7 +20,7 @@ test_expect_success 'failure in git-upload-pack is shown' '
        test_might_fail env GIT_CURL_VERBOSE=1 \
                git clone "$HTTPD_URL/error_git_upload_pack/smart/repo.git" \
                2>curl_log &&
-       grep "< HTTP/1.1 500 Intentional Breakage" curl_log
+       grep "<= Recv header: HTTP/1.1 500 Intentional Breakage" curl_log
 '
 
 test_done

diff --git a/trace.c b/trace.c
index b3ef0e627f8cec18433c17ffb080575894882758..f726686fd92f0b9f388b7dddeca55edc2ea8d8a8 100644 (file)
--- a/trace.c
+++ b/trace.c
@@ -29,7 +29,7 @@ struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);
 struct trace_key trace_setup_key = TRACE_KEY_INIT(SETUP);
 
 /* Get a trace file descriptor from "key" env variable. */
-static int get_trace_fd(struct trace_key *key)
+static int get_trace_fd(struct trace_key *key, const char *override_envvar)
 {
        const char *trace;
 
@@ -37,7 +37,7 @@ static int get_trace_fd(struct trace_key *key)
        if (key->initialized)
                return key->fd;
 
-       trace = getenv(key->key);
+       trace = override_envvar ? override_envvar : getenv(key->key);
 
        if (!trace || !strcmp(trace, "") ||
            !strcmp(trace, "0") || !strcasecmp(trace, "false"))
@@ -68,6 +68,18 @@ static int get_trace_fd(struct trace_key *key)
        return key->fd;
 }
 
+void trace_override_envvar(struct trace_key *key, const char *value)
+{
+       trace_disable(key);
+       key->initialized = 0;
+
+       /*
+        * Invoke get_trace_fd() to initialize key using the given value
+        * instead of the value of the environment variable.
+        */
+       get_trace_fd(key, value);
+}
+
 void trace_disable(struct trace_key *key)
 {
        if (key->need_close)
@@ -112,7 +124,7 @@ static int prepare_trace_line(const char *file, int line,
 
 static void trace_write(struct trace_key *key, const void *buf, unsigned len)
 {
-       if (write_in_full(get_trace_fd(key), buf, len) < 0) {
+       if (write_in_full(get_trace_fd(key, NULL), buf, len) < 0) {
                warning("unable to write trace for %s: %s",
                        key->key, strerror(errno));
                trace_disable(key);
@@ -383,7 +395,7 @@ void trace_repo_setup(const char *prefix)
 
 int trace_want(struct trace_key *key)
 {
-       return !!get_trace_fd(key);
+       return !!get_trace_fd(key, NULL);
 }
 
 #if defined(HAVE_CLOCK_GETTIME) && defined(HAVE_CLOCK_MONOTONIC)

diff --git a/trace.h b/trace.h
index 9826618b331af6574dd81387f6210eba803636e0..0dbbad0e41cb074e669f58fc3ed347e16faeb45c 100644 (file)
--- a/trace.h
+++ b/trace.h
@@ -101,6 +101,12 @@ void trace_repo_setup(const char *prefix);
  */
 int trace_want(struct trace_key *key);
 
+/**
+ * Enables or disables tracing for the specified key, as if the environment
+ * variable was set to the given value.
+ */
+void trace_override_envvar(struct trace_key *key, const char *value);
+
 /**
  * Disables tracing for the specified key, even if the environment variable
  * was set.