mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <>
To: Glen Choo <>
Cc: Taylor Blau <>,
	Glen Choo via GitGitGadget <>,
Subject: Re: [PATCH] http: redact curl h2h3 headers in info
Date: Thu, 10 Nov 2022 21:29:15 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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; 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.


  reply	other threads:[~2022-11-11  2:29 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-09  0:52 [PATCH] http: redact curl h2h3 headers in info Glen Choo via GitGitGadget
2022-11-10  2:52 ` Taylor Blau
2022-11-10 17:48   ` Glen Choo
2022-11-10 21:50     ` Jeff King
2022-11-10 22:53       ` Glen Choo
2022-11-11  2:29         ` Jeff King [this message]
2022-11-11  2:31           ` Taylor Blau
2022-11-11 14:49           ` [PATCH] t: run t5551 tests with both HTTP and HTTP/2 Jeff King
2022-11-11 15:06             ` Ævar Arnfjörð Bjarmason
2022-11-11 15:19               ` Jeff King
2022-11-11 15:20             ` Jeff King
2022-11-10 21:57 ` [PATCH] http: redact curl h2h3 headers in info Emily Shaffer
2022-11-10 22:14   ` Glen Choo
2022-11-11  2:35     ` Taylor Blau
2022-11-10 22:57 ` [PATCH v2] " Glen Choo via GitGitGadget
2022-11-11  2:36   ` Taylor Blau
2022-11-11  2:38   ` Jeff King
2022-11-11  2:39     ` Taylor Blau
2022-11-11 17:55     ` Glen Choo
2022-11-11 22:35   ` [PATCH v3 0/2] " Glen Choo via GitGitGadget
2022-11-11 22:35     ` [PATCH v3 1/2] t: run t5551 tests with both HTTP and HTTP/2 Jeff King via GitGitGadget
2022-11-11 22:35     ` [PATCH v3 2/2] http: redact curl h2h3 headers in info Glen Choo via GitGitGadget
2022-11-14 22:33     ` [PATCH v3 0/2] " Jeff King
2022-11-14 22:43       ` Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).