On Wed, Sep 22, 2021 at 12:19:26PM -0700, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> 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 we do issue an HTTP/1.1 request in addition to the HTTP/2 one. We have to in order to probe the server to say "this is HTTP/1.1, but by the way, we support HTTP/2". I am a little surprised that we get as far as sending auth info via HTTP/1.1, since the initial probe that results in a 401 (causing us to send the auth) could in theory let us know the server speaks HTTP/2. But in practice it doesn't. It looks like the server does not do the upgrade for a 401 (perhaps that's true for any non-success code, I don't know). You can see it in action if you use the test changes I mentioned earlier _without_ my patch applied (so neither the "grep -i" fix, nor the actual code change). And then do: ./t5551-http-fetch-smart.sh --run=1-17 --debug egrep 'Send header:|Recv header:' trash*/trace 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: Connection: Upgrade, HTTP2-Settings => Send header: Upgrade: h2c => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA <= Recv header: HTTP/1.1 401 Unauthorized <= Recv header: Date: Wed, 22 Sep 2021 20:03:32 GMT <= Recv header: Server: Apache/2.4.49 (Debian) <= Recv header: WWW-Authenticate: Basic realm="git-auth" => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 => Send header: Authorization: Basic <redacted> => Send header: Connection: Upgrade, HTTP2-Settings => Send header: Upgrade: h2c => Send header: HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA <= Recv header: HTTP/1.1 101 Switching Protocols <= Recv header: Upgrade: h2c <= Recv header: Connection: Upgrade <= Recv header: HTTP/2 200 <= Recv header: content-type: application/x-git-upload-pack-advertisement => Send header: POST /auth/smart/repo.git/git-upload-pack HTTP/2 => Send header: authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA== => Send header: content-type: application/x-git-upload-pack-request <= Recv header: HTTP/2 200 <= Recv header: content-type: application/x-git-upload-pack-result [...and so on...] So you can see both the redacted and unredacted lines in that output. I'm happy to include that in the commit message if it helps; I avoided it earlier because it was already getting quite long. ;) -Peff
