On Thu, Nov 10, 2022 at 02:53:54PM -0800, Glen Choo wrote:
> > 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
