From: Bagas Sanjaya <bagasdotme@gmail.com>
Date: Wed, 22 Sep 2021 02:32:41 GMT
Subject: Re: [PATCH] http: match headers case-insensitively when redacting
Message-ID: <afd7bd6b-52bf-7fd8-d13e-6dcd660c4100@gmail.com>
In-Reply-To: <YUonS1uoZlZEt+Yd@coredump.intra.peff.net>

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