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

On 22/09/21 01.41, Jeff King wrote:

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().

Testing is more complicated, though. We do have a test for the redacting
feature, but we don't hit the problem case because our test Apache setup
does not understand HTTP/2. You can reproduce the issue by applying this
on top of the test change in this patch:

	diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
	index afa91e38b0..19267c7107 100644
	--- a/t/lib-httpd/apache.conf
	+++ b/t/lib-httpd/apache.conf
	@@ -29,6 +29,9 @@ ErrorLog error.log
	 	LoadModule setenvif_module modules/mod_setenvif.so
	 </IfModule>

	+LoadModule http2_module modules/mod_http2.so
	+Protocols h2c
	+
	 <IfVersion < 2.4>
	 LockFile accept.lock
	 </IfVersion>
	@@ -64,8 +67,8 @@ LockFile accept.lock
	 <IfModule !mod_access_compat.c>
	 	LoadModule access_compat_module modules/mod_access_compat.so
	 </IfModule>
	-<IfModule !mod_mpm_prefork.c>
	-	LoadModule mpm_prefork_module modules/mod_mpm_prefork.so
	+<IfModule !mod_mpm_event.c>
	+	LoadModule mpm_event_module modules/mod_mpm_event.so
	 </IfModule>
	 <IfModule !mod_unixd.c>
	 	LoadModule unixd_module modules/mod_unixd.so
	diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
	index 1c2a444ae7..ff74f0ae8a 100755
	--- a/t/t5551-http-fetch-smart.sh
	+++ b/t/t5551-http-fetch-smart.sh
	@@ -24,6 +24,10 @@ test_expect_success 'create http-accessible bare repository' '
	 	git push public main:main
	 '

	+test_expect_success 'prefer http/2' '
	+	git config --global http.version HTTP/2
	+'
	+
	 setup_askpass_helper

	 test_expect_success 'clone http repository' '

but this has a few issues:

   - it's not necessarily portable. The http2 apache module might not be
     available on all systems. Further, the http2 module isn't compatible
     with the prefork mpm, so we have to switch to something else. But we
     don't necessarily know what's available. It would be nice if we
     could have conditional config, but IfModule only tells us if a
     module is already loaded, not whether it is available at all.

     This might be a non-issue. The http tests are already optional, and
     modern-enough systems may just have both of these. But...

   - if we do this, then we'd no longer be testing HTTP/1.1 at all. I'm
     not sure how much that matters since it's all handled by curl under
     the hood, but I'd worry that some detail leaks through. We'd
     probably want two scripts running similar tests, one with HTTP/2 and
     one with HTTP/1.1.

Maybe for httpd config we can say that if mpm_prefork isn't loaded, load 
mpm_event and mod_http2.

And for testing both HTTP/2 and HTTP/1.1 did you mean sharing the same 
test code (with adjustments for each protocol)?

-- 
An old man doll... just what I always wanted! - Clara