Re: [PATCH] http: match headers case-insensitively when redacting

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2021, 20:51 UTC
Message-ID: <xmqqk0j85o6c.fsf@gitster.g>
In-Reply-To: <YUuNXOb5blV7iN6P@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 23 quoted lines

> On Wed, Sep 22, 2021 at 12:19:26PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > 	# 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
>> >
>> > gets confused. It sees the "<redacted>" one from the pre-upgrade
>> > HTTP/1.1 request, but fails to see the unredacted HTTP/2 one, because it
>> > does not match the lower-case "authorization".
>> 
>> Neither pattern of the above two will not match the HTTP/2 one, so
>> the first one would report "there is no leakage of Auth with a
>> caplital letter"; the second one may see only one pre-upgrade Auth
>> with a capital letter, but as long as it does find one, it should be
>> happy, no?
>> 
>> I am a bit puzzled how the test gets confused.
>
> The first one matches nothing, because the HTTP/2 one which fails to
> redact has a lower-case "A". The second one _does_ match, because ...

I thought we were talking about the original case sensitive test getting confused when testing the software that is fixed, i.e. HTTP/2 lowercase "authorization" line properly redacted.

> I get (with some extraneous headers omitted):
> ...
>   => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
>   => Send header: Authorization: Basic <redacted>

So, this is what we see in HTTP/1.1 (with capitalization). And then ...

> ...
>   => Send header: POST /auth/smart/repo.git/git-upload-pack HTTP/2
>   => Send header: authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==

this one, once the redaction code is fixed by applying this patch, would show that we redacted it, too, no?

With or without the fix in the code, I agree that neither of the two "grep" patterns without "grep -i" change will match this line. So the end result is that the test finds no unredacted line, and one redacted one (instead of two).

I agree that it is *not* testing what we want to test, and if you said so, I wouldn't have been puzzled. I just wanted to know if there is something _else_ (other than "gee, we are not testing the HTTP/2 case at all") going on that I failed to read in your "... gets confused".

Thanks.