Re: [PATCH v2] http: preserve wwwauth_headers across redirects

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> For those of you who are watching from the sidelines, this v2 lacks
> the threading history.
>
> The v1 is at https://lore.kernel.org/git/20260602161150.1527493-1-aplattner@nvidia.com/

Any takers?  It should be quite easy to be more interested and be a
better reviewer in the http transfer codepaths than I am.

Thanks.

> Aaron Plattner <aplattner@nvidia.com> writes:
>
>> When cURL follows a redirect, it calls the CURLOPT_HEADERFUNCTION for
>> each header received including ones from a redirect. http_request() sets
>> fwrite_wwwauth() as the header function, which will record the wwwauth[]
>> entries for the last step in the redirection chain.
>>
>> However, when http_request_recoverable() sees that cURL followed a
>> redirect, it attempts to update the credentials for the request from the
>> new URL using credential_from_url(). The first thing that does is call
>> credential_clear(), which clears everything including wwwauth_headers.
>>
>> If the new URL should use a credential helper rather than credentials
>> embedded in the URL, this loses the list of authentication methods that
>> the server provided in the redirect.
>>
>> The WWW-Authenticate challenge is not derived from the URL; it is
>> populated from the server's response, and after a redirect it describes
>> how to authenticate to the redirect target and it needs to survive the
>> URL update so that credential helpers can know which authentication
>> methods are allowed.
>>
>> Add a new credential_update_url() that wraps credential_from_url() and
>> preserves wwwauth_headers specifically. Use SWAP() to avoid having to
>> copy the whole strbuf.
>>
>> Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
>> ---
>> I decided to come back to this after I noticed that at least one other
>> person had run into the same bug:
>>
>> https://lore.kernel.org/all/CADoNwcscDrx+YcfbcW4YKONDZZQgnPiwEOxL4QYV_C7_=FOFcg@mail.gmail.com/
>>
>> Rather than reworking everything about how credentials are stored, I
>> took your advice in [1] and just moved the code to preserve the wwwauth_headers
>> into credential.c. That way any future credential fields that need to be
>> preserved can be added there without having to hunt down other places
>> like http.c that are reaching into it.
>>
>> [1] https://lore.kernel.org/all/xmqqpl28scll.fsf@gitster.g/
>> ---
>> Changes in v2:
>> - Move strvec preservation into a helper function in credential.c
>> - Use SWAP instead of strvec_pushv() to avoid having to copy the
>>   contents of the strvec.
>> - Link to v1: https://patch.msgid.link/20260602161150.1527493-1-aplattner@nvidia.com
>> ---
>>  credential.c                | 16 ++++++++++++++++
>>  credential.h                |  8 ++++++++
>>  http.c                      |  9 ++++++++-
>>  t/lib-httpd/apache.conf     |  1 +
>>  t/t5563-simple-http-auth.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 78 insertions(+), 1 deletion(-)
>>
>> diff --git a/credential.c b/credential.c
>> index 2594c0c422..035399d7ee 100644
>> --- a/credential.c
>> +++ b/credential.c
>> @@ -708,3 +708,19 @@ void credential_from_url(struct credential *c, const char *url)
>>  	if (credential_from_url_gently(c, url, 0) < 0)
>>  		die(_("credential url cannot be parsed: %s"), url);
>>  }
>> +
>> +void credential_update_url(struct credential *c, const char *url)
>> +{
>> +	struct strvec wwwauth_headers = STRVEC_INIT;
>> +
>> +	/*
>> +	 * credential_from_url() clears the whole credential. Preserve the
>> +	 * WWW-Authenticate list, which is derived from the server's original
>> +	 * response rather than from the URL and is required to authenticate to
>> +	 * the new URL.
>> +	 */
>> +	SWAP(wwwauth_headers, c->wwwauth_headers);
>> +	credential_from_url(c, url);
>> +	SWAP(c->wwwauth_headers, wwwauth_headers);
>> +	strvec_clear(&wwwauth_headers);
>> +}
>> diff --git a/credential.h b/credential.h
>> index c78b72d110..b90f666e33 100644
>> --- a/credential.h
>> +++ b/credential.h
>> @@ -305,6 +305,14 @@ void credential_write(const struct credential *, FILE *,
>>  void credential_from_url(struct credential *, const char *url);
>>  int credential_from_url_gently(struct credential *, const char *url, int quiet);
>>  
>> +/*
>> + * Update the URL-derived fields (protocol, host, path) of an existing
>> + * credential to match a new URL. Unlike credential_from_url(), this function
>> + * preserves state that was derived from a server's HTTP redirect response,
>> + * such as the WWW-Authenticate headers.
>> + */
>> +void credential_update_url(struct credential *c, const char *url);
>> +
>>  int credential_match(const struct credential *want,
>>  		     const struct credential *have, int match_password);
>>  
>> diff --git a/http.c b/http.c
>> index a0d399b274..e8abb9f95a 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -2427,7 +2427,14 @@ static int http_request_recoverable(const char *url,
>>  	if (options->effective_url && options->base_url) {
>>  		if (update_url_from_redirect(options->base_url,
>>  					     url, options->effective_url)) {
>> -			credential_from_url(&http_auth, options->base_url->buf);
>> +			/*
>> +			 * Use credential_update_url() rather than
>> +			 * credential_from_url() so that the WWW-Authenticate
>> +			 * challenge the server sent with the redirect target's
>> +			 * response is preserved and handed to the credential
>> +			 * helper.
>> +			 */
>> +			credential_update_url(&http_auth, options->base_url->buf);
>>  			url = options->effective_url->buf;
>>  		}
>>  	}
>> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
>> index 4149fc1078..0627ef1433 100644
>> --- a/t/lib-httpd/apache.conf
>> +++ b/t/lib-httpd/apache.conf
>> @@ -203,6 +203,7 @@ RewriteRule ^/dumb-redir/(.*)$ /dumb/$1 [R=301]
>>  RewriteRule ^/smart-redir-perm/(.*)$ /smart/$1 [R=301]
>>  RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]
>>  RewriteRule ^/smart-redir-auth/(.*)$ /auth/smart/$1 [R=301]
>> +RewriteRule ^/custom_auth_redir/(.*)$ /custom_auth/$1 [R=302]
>>  RewriteRule ^/smart-redir-limited/(.*)/info/refs$ /smart/$1/info/refs [R=301]
>>  RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]
>>  
>> diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh
>> index a7d475dd68..349ae4ab39 100755
>> --- a/t/t5563-simple-http-auth.sh
>> +++ b/t/t5563-simple-http-auth.sh
>> @@ -557,6 +557,51 @@ test_expect_success 'access using bearer auth' '
>>  	EOF
>>  '
>>  
>> +test_expect_success 'bearer auth after redirect preserves wwwauth headers' '
>> +	test_when_finished "per_test_cleanup" &&
>> +
>> +	set_credential_reply get <<-EOF &&
>> +	capability[]=authtype
>> +	authtype=Bearer
>> +	credential=YS1naXQtdG9rZW4=
>> +	EOF
>> +
>> +	cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF &&
>> +	id=1 creds=Bearer YS1naXQtdG9rZW4=
>> +	EOF
>> +
>> +	cat >"$HTTPD_ROOT_PATH/custom-auth.challenge" <<-EOF &&
>> +	id=1 status=200
>> +	id=default response=WWW-Authenticate: FooBar param1="value1" param2="value2"
>> +	id=default response=WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0
>> +	id=default response=WWW-Authenticate: Basic realm="example.com"
>> +	EOF
>> +
>> +	test_config_global credential.helper test-helper &&
>> +	test_config_global credential.useHttpPath true &&
>> +	git ls-remote "$HTTPD_URL/custom_auth_redir/repo.git" &&
>> +
>> +	expect_credential_query get <<-EOF &&
>> +	capability[]=authtype
>> +	capability[]=state
>> +	protocol=http
>> +	host=$HTTPD_DEST
>> +	path=custom_auth/repo.git
>> +	wwwauth[]=FooBar param1="value1" param2="value2"
>> +	wwwauth[]=Bearer authorize_uri="id.example.com" p=1 q=0
>> +	wwwauth[]=Basic realm="example.com"
>> +	EOF
>> +
>> +	expect_credential_query store <<-EOF
>> +	capability[]=authtype
>> +	authtype=Bearer
>> +	credential=YS1naXQtdG9rZW4=
>> +	protocol=http
>> +	host=$HTTPD_DEST
>> +	path=custom_auth/repo.git
>> +	EOF
>> +'
>> +
>>  test_expect_success 'access using bearer auth with invalid credentials' '
>>  	test_when_finished "per_test_cleanup" &&
>>  
>>
>> ---
>> base-commit: dea0ea3582e6980ddbc1173cc8e3e9f9db91cde0
>> change-id: 20260819-http-preserve-wwwauth-redirect-a3fe4dab6b35