Re: [PATCH] http: redact curl h2h3 headers in info

From: Jeff King <peff@peff.net>
Date: Nov 11, 2022, 02:29 UTC
Message-ID: <Y22ze1m6ayQCv9B5@coredump.intra.peff.net>
In-Reply-To: <kl6lk0422zgd.fsf@chooglen-macbookpro.roam.corp.google.com>

On Thu, Nov 10, 2022 at 02:53:54PM -0800, Glen Choo wrote:

Show 17 quoted lines

> > There's some discussion in b66c77a64e (http: match headers
> > case-insensitively when redacting, 2021-09-22) about testing with
> > HTTP/2. Which ironically is basically this exact same bug in a different
> > form. ;)
> >
> > The short answer is that it's do-able, but probably there are some
> > headaches to make it work portably.
> 
> Argh, what a shame :( Okay, maybe it's not worth trying to use httpd
> then.
> 
> Some other ideas I had were:
> 
> - Create a test-tool that calls the redaction logic directly (without
>   involving about curl), and we pass the strings we want to redacted to
>   it. Way less than ideal, since we'd never be able to proactively catch
>   failures, but better than nothing I suppose.

I don't think this is worth the effort. It's nice to exercise the code a bit, but it wouldn't have actually found this regression, since the unexpected thing here was curl changing.

> - Write our own HTTP/2 server for redaction tests. I assume this won't
>   be trivial, but maybe not prohibitive, e.g. [1] implements its own
>   http server for credential helper tests.

These seems like a lot more work than just setting up HTTP/2 support for Apache. I checked the recipe from b66c77a64e, and it still works. It does indeed find the bug (my curl is 7.86.0) and confirms your fix.

I think a simple path forward could be something like:

  - teach lib-httpd to conditionally use the current set up versus the
    http2 one outlined in b66c77a64e

  - push most of t5551 into a lib-http-fetch.sh; the client-side set up
    from b66c77a64e checks for an HTTP2 prereq. The test that looks for
    chunked encoding (and only works on HTTP1) checks for !HTTP2.

  - t5551 tells lib-httpd to use the usual setup, and then sources
    lib-http-fetch; it behaves as before

  - t5559 (sadly, not contiguous without renumbering intermediate tests)
    tells lib-httpd to use http2, and sets the HTTP2 prereq. It runs the
    same tests but via http2.

We don't get the _whole_ test suite running with http2, but hopefully it gives us a fairly representative sample. And it does find this bug.

I can try to work the above into patch form, but I may not get to it for a day or two.

-Peff