Re: [PATCH] http: redact curl h2h3 headers in info
From: Glen Choo <hidden>
Date: 2022-11-10 22:54:13
Jeff King [off-list ref] writes:
On Thu, Nov 10, 2022 at 09:48:38AM -0800, Glen Choo wrote:quoted
Taylor Blau [off-list ref] writes:quoted
quoted
* How could we set up end-to-end tests to ensure that we're testing this against affected versions of curl? To avoid regressions, I'd also prefer to test against future versions of curl too.Does that necessarily matter? We want to make sure that we don't see sensitive headers from the h2h3 module with any version of cURL, no?It would help, but it might not be worth setting up infrastructure for just this use case alone. Given the various platforms running tests against the Git codebase, we probably get close to a representative sample of the population with enough time. I think it would be more important to have tests against HTTP/2.0. If we did, we probably would have already caught this, e.g. t/t5551-http-fetch-smart.sh:'GIT_TRACE_CURL redacts auth details' and friends.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. - 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. [1] https://lore.kernel.org/git/4947e81546a51883365d0087ce616b6b77e24a63.1667426970.git.gitgitgadget@gmail.com/ (local)
I agree with you that trying various curl versions isn't worth doing. If enough people/platforms run Git's suite, one of them will eventually see the problem. -Peff