Re: [PATCH 3/3] t5563: relax whitespace assumptions for unfolded headers

On 2025-12-18 12:22, Jeff King wrote:

 > In t5563 we check the handling of WWW-Authenticate headers that have
 > been folded (i.e., where a continuation line starts with extra
 > whitespace). Traditionally curl handed each line to us individually, but
 > in the upcoming v8.18.0, it hands us full lines that have been unfolded.
 > But it doesn't produce exactly the same unfolding that we did!
 >
 > In particular, two of the tests send an extra blank continuation line.
 > Something like this:
 >
 >    printf 'WWW-Authenticate: foo param1="value1"\r\n'
 >    printf ' \r\n'
 >    printf ' param2="value2"\r\n"
 >
 > We unfold that into:
 >
 >    WWW-Authenticate: foo param1="value1" param2="value2"
 >
 > But curl will give us a string with an extra space:
 >
 >    WWW-Authenticate: foo param1="value1"  param2="value2"
 >
 > I think curl is actually correct here. RFC 7230 says:
 >
 >     A user agent that receives an obs-fold in a response message that is
 >     not within a message/http container MUST replace each received
 >     obs-fold with one or more SP octets prior to interpreting the field
 >     value.
 >
 > So each folded instance turns the initial whitespace into "one or more"
 > spaces, and the "blank" line becomes a single space. Whereas Git's
 > unfolding code explicitly avoids this, with the comment "Do not bother
 > appending the new value if this continuation header is itself empty." in
 > fwrite_wwwauth().
 >
 > I think it's mostly academic at this point. These folded continuations
 > have been deprecated entirely since RFC 7230 came out in 2014, and
 > there's very little reason for a server to add a blank continuation line
 > at all. And anybody parsing the unfolded header contents should skip
 > past the extra whitespace (which is allowed to be present according to
 > the RFC).
 >
 > But our tests do a byte-wise comparison, so they care about the
 > difference between the two outputs. We have two options here:
 >
 >    1. We can modify Git's unfolding code to behave like modern curl.
 >
 >    2. We can relax the tests to be happy with either output.
 >
 > I picked (2) here, just because it seemed less risky to touch only the
 > tests and not the code (though if any real-world systems _do_ care about
 > the distinction, they will eventually run into problems when libcurl is
 > upgraded).

I think that is a fair choice.

 > There is one further curiosity here. There's a second test which mixes
 > tabs and spaces for continuation, like this:
 >
 >    printf 'WWW-Authenticate: foo param1="value1"\r\n'
 >    printf '\t\r\n'
 >    printf ' param2="value2"\r\n"
 >
 >  From the snippet of RFC quoted above, I believe this should produce the
 > exact same output (the continuation whitespace is replaced with one or
 > more spaces, even though it is a tab here). But curl retains the tab
 > instead!

Header continuations are just a nightmare :-(

 > So to implement the "relaxed whitespace" mode in the test, we just
 > convert any run of multiple whitespace characters to a single space.
 > This is a bit hacky and over-zealous, but it's easy to do and good
 > enough for our purposes here. We only enable the relaxed mode for the
 > two tests which trigger this issue.
 >
 > Signed-off-by: Jeff King <peff@peff.net>
 > ---
 > Note that when built against this new version of curl, Git's unfolding
 > code should never trigger at all. In the long run we should be able to
 > rip it out, but we probably need to wait a decade or so before we can
 > bump the minimum libcurl version to 8.18.0.
 >
 > I guess we could make it a conditional in the code (which would help us
 > remember to eventually rip it out), but it felt weird to start adding
 > version conditionals for a version that isn't even released yet. ;)
 >
 >   t/t5563-simple-http-auth.sh | 11 +++++++++--
 >   1 file changed, 9 insertions(+), 2 deletions(-)
 >
 > diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh
 > index c1febbae9d..0967cd501c 100755
 > --- a/t/t5563-simple-http-auth.sh
 > +++ b/t/t5563-simple-http-auth.sh
 > @@ -47,6 +47,13 @@ set_credential_reply () {
 >   expect_credential_query () {
 >   	local suffix="$(test -n "$2" && echo "-$2")"
 >   	cat >"$TRASH_DIRECTORY/$1-expect$suffix.cred" &&
 > +	if $(test "$3" = "--relax-whitespace")
 > +	then
 > +		HT='	' &&
 > +		sed "s/[ $HT][ $HT]*/ /g" \
 > +			<"$TRASH_DIRECTORY/$1-query$suffix.cred" >tmp &&
 > +		mv tmp "$TRASH_DIRECTORY/$1-query$suffix.cred"
 > +	fi &&
 >   	test_cmp "$TRASH_DIRECTORY/$1-expect$suffix.cred" \
 >   		 "$TRASH_DIRECTORY/$1-query$suffix.cred"
 >   }
 > @@ -451,7 +458,7 @@ test_expect_success 'access using basic auth with 
wwwauth header empty continuat
 >   	test_config_global credential.helper test-helper &&
 >   	git ls-remote "$HTTPD_URL/custom_auth/repo.git" &&
 >
 > -	expect_credential_query get <<-EOF &&
 > +	expect_credential_query get "" --relax-whitespace <<-EOF &&
 >   	capability[]=authtype
 >   	capability[]=state
 >   	protocol=http
 > @@ -495,7 +502,7 @@ test_expect_success 'access using basic auth with 
wwwauth header mixed continuat
 >   	test_config_global credential.helper test-helper &&
 >   	git ls-remote "$HTTPD_URL/custom_auth/repo.git" &&
 >
 > -	expect_credential_query get <<-EOF &&
 > +	expect_credential_query get "" --relax-whitespace <<-EOF &&
 >   	capability[]=authtype
 >   	capability[]=state
 >   	protocol=http