From: Matthew John Cheetham Date: Thu, 18 Dec 2025 13:45:54 GMT Subject: Re: [PATCH 3/3] t5563: relax whitespace assumptions for unfolded headers Message-ID: In-Reply-To: <20251218122204.GC3758205@coredump.intra.peff.net> 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 > --- > 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