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

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/[email protected]/

Thanks.

Aaron Plattner <[email protected]> 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 <[email protected]>
> ---
> 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/[email protected]/
> ---
> 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/[email protected]
> ---
>  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