Thread (14 messages) flat view 14 messages, 7 authors, 2021-09-23

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

From: Jeff King <hidden>
Date: 2021-09-21 18:47:29

On Tue, Sep 21, 2021 at 02:41:16PM -0400, Jeff King wrote:
When HTTP/2 is in use, we fail to correctly redact "Authorization" (and
other) headers in our GIT_TRACE_CURL output.

We get the headers in our CURLOPT_DEBUGFUNCTION callback, curl_trace().
It passes them along to curl_dump_header(), which in turn checks
redact_sensitive_header(). We see the headers as a text buffer like:

  Host: ...
  Authorization: Basic ...

After breaking it into lines, we match each header using skip_prefix().
This is case-insensitive, even though HTTP headers are case-insensitive.
This has worked reliably in the past because these headers are generated
by curl itself, which is predictable in what it sends.

But when HTTP/2 is in use, instead we get a lower-case "authorization:"
header, and we fail to match it. The fix is simple: we should match with
skip_iprefix().
Daniel,

I cc'd you here mostly as an FYI. I think Git was doing the wrong thing
in assuming case here (we're only expecting these particular headers
coming from the client, but for response headers, I thnk curl will give
us whatever form the server sent us).

But certainly I found the behavior surprising. :) I'd guess it's because
HTTP/2 is sending some binary goo instead of text headers, and the names
we get are just coming from some lookup table? Or maybe I'm just showing
my ignorance of HTTP/2.

At any rate, I wonder if it would be friendlier for curl to hand strings
to the debug function with the usual capitalization.

-Peff

PS This nit aside, it is totally cool that I have been seamlessly using
   HTTP/2 to talk to github.com without even realizing it. I wonder for
   how long!
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help