Re: [PATCHv5 0/2] Implement the GIT_TRACE_CURL environment variable

From: Jeff King <peff@peff.net>
Date: May 2, 2016, 18:13 UTC
Message-ID: <20160502181347.GB8439@sigill.intra.peff.net>
In-Reply-To: <20160502142813.50868-1-gitter.spiros@gmail.com>

On Mon, May 02, 2016 at 02:28:11PM +0000, Elia Pinto wrote:

Show 7 quoted lines

> -  redo the authorization header skip with a replace of possible sensitive data. 
>    We prefer to print only:
>        09:00:53.238330 http.c:534              => Send header: Authorization:  <redacted>
>    intested of 
>        09:00:53.238330 http.c:534              => Send header: Authorization:  Basic(o other scheme) <redacted>
>    as it was done in the original proposed suggestion by Jeff King. 
>    This is because i think it's better not to print even the authorization scheme.

I'm not sure I agree. If you're debugging curl's auth selection, that's omitting an important piece of data. And unlike the actual credential, I don't think it's particularly secret (and in many cases can be deduced from the "WWW-Authenticate" header the server sends, coupled with curl's code).

>    We add also the (previously missing) proxy-authorization case

Good catch.

Show 5 quoted lines

> In this series i keep the original curl_dump parsing code, even though it is 
> objectively difficult to read. This is because the same code is used internally by curl 
> to do "ascii-trace" and is also reported in the libcurl code examples and test. 
> I think this may make maintenance of code easier in the future (libcurl 
> new dev, new features and so on)

I don't agree with this line of reasoning. The code in question is purely about how we format the output buffer, not about parsing what curl gives us. We _should_ be diverging if we prefer a different output format. And I don't think it's a question just of readability (though I do agree the existing one is hard to read); it also foils the redaction of the authorization header.

-Peff