Re: [PATCH v2 0/3] Safer GIT_CURL_VERBOSE

From: Jeff King <peff@peff.net>
Date: May 15, 2020, 20:47 UTC
Message-ID: <20200515204729.GA115445@coredump.intra.peff.net>
In-Reply-To: <xmqqlflvtysu.fsf@gitster.c.googlers.com>

On Wed, May 13, 2020 at 12:33:37PM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Jonathan Tan <jonathantanmy@google.com> writes:
> >
> >> Thanks everyone. I went ahead with GIT_REDACT_AUTHORIZATION to match
> >> GIT_REDACT_COOKIES, with the default being true (i.e. you need to set it
> >> to "0" to have behavior change).
> >
> > Hmm, I would actually have expected us to move in the direction of
> > deprecating specific REDACT_BLAH and consolidate them into one,
> > instead of adding another one.  Especially as the primary reason why
> > we redact cookies is to protect those that are used for auth anyway.
> 
> Also I had forgot to grep for anonymi.e to find transport_anonymize_url(),
> which I was hoping that the new environment variable would cover to help
> those who debug.

Yeah, I'd agree with both of these. If Jonathan doesn't feel like working on transport_anonymize_url() now, I don't mind if we leave that for later. But let's come up with a general scheme that we're aiming for, so we minimize changes to things that users are exposed to.

IMHO an option that only impacts the format of the human-readable trace output is not something we need to deprecate. It's an internal detail, and people can't rely on the exact format of the trace anyway. So I'd be fine to just kill off GIT_REDACT_COOKIES completely (especially if the default behavior is the safer "always redact").

That said, a boolean GIT_REDACT doesn't quite do the same thing, because it's actually a list of cookies. My gut feeling is that this is a bit over-engineered. Any cookies that Git uses are likely to be sensitive, so just treating their values like auth (redacting by default, but allowing them to be unblinded when the user asks for it). But maybe Jonathan had a specific tracing case in mind, as the author of the original.

-Peff