[PATCH] http: preserve wwwauth_headers across redirects

[PATCH] http: preserve wwwauth_headers across redirects

[Aaron Plattner]

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.

For example, I have a server that supports HTTP but always redirects to
HTTPS before handling requests. This redirect breaks OAuth
authentication:

  $ git ls-remote http://server/git
  => Send header: GET /git/info/refs?service=git-upload-pack HTTP/1.1
  <= Recv header: HTTP/1.1 302 Found
  <= Recv header: Location: https://server.nvidia.com/git/info/refs?service=git-upload-pack
  == Info: Issue another request to this URL: 'https://server.nvidia.com/git/info/refs?service=git-upload-pack'
  => Send header: GET /git/info/refs?service=git-upload-pack HTTP/1.1
  <= Recv header: HTTP/1.1 401 Unauthorized
  <= Recv header: WWW-Authenticate: Bearer error="invalid_request", error_description="No bearer token found in the request", msal-tenant-id="<tenant>", msal-client-id="<client>"
  trace: run_command: 'git credential-cache --timeout 7200 get'
  trace: start_command: /bin/sh -c 'git credential-cache --timeout 7200 get' 'git credential-cache --timeout 7200 get'
  trace: built-in: git credential-cache --timeout 7200 get
  trace: run_command: 'git credential-msal get'
  trace: start_command: /bin/sh -c 'git credential-msal get' 'git credential-msal get'
  trace: exec: git-credential-msal get
  trace: run_command: git-credential-msal get
  trace: start_command: /usr/bin/git-credential-msal get
  Username for 'https://server.nvidia.com': ^C

When git invokes the credential helper, it doesn't include the wwwauth[]
array, so git-credential-msal doesn't think that OAuth is supported [1].

Fix the problem by preserving the wwwauth_headers strvec across the call
to credential_from_url().

[1] https://github.com/Binary-Eater/git-credential-msal/blob/trunk/src/git_credential_msal/main.py#L69

Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
---
 http.c                      | 14 ++++++++++++
 t/lib-httpd/apache.conf     |  1 +
 t/t5563-simple-http-auth.sh | 45 +++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+)

diff --git a/http.c b/http.c
index ea9b16861b..cac8c9bfc9 100644
--- a/http.c
+++ b/http.c
@@ -2425,7 +2425,21 @@ 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)) {
+			struct strvec wwwauth_headers = STRVEC_INIT;
+
+			/*
+			 * Preserve wwwauth_headers across the call to
+			 * credential_from_url(): if the effective URL doesn't
+			 * specify its own credentials, a credential helper
+			 * might need the wwwauth[] array from the server's
+			 * redirect response in order to authenticate.
+			 */
+			strvec_pushv(&wwwauth_headers,
+				     http_auth.wwwauth_headers.v);
 			credential_from_url(&http_auth, options->base_url->buf);
+			strvec_pushv(&http_auth.wwwauth_headers,
+				     wwwauth_headers.v);
+			strvec_clear(&wwwauth_headers);
 			url = options->effective_url->buf;
 		}
 	}
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 40a690b0bb..664f23fc6c 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -202,6 +202,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" &&
 
-- 
2.54.0

Re: [PATCH] http: preserve wwwauth_headers across redirects

[Junio C Hamano]

Aaron Plattner <aplattner@nvidia.com> writes:

> diff --git a/http.c b/http.c
> index ea9b16861b..cac8c9bfc9 100644
> --- a/http.c
> +++ b/http.c
> @@ -2425,7 +2425,21 @@ 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)) {
> +			struct strvec wwwauth_headers = STRVEC_INIT;
> +
> +			/*
> +			 * Preserve wwwauth_headers across the call to
> +			 * credential_from_url(): if the effective URL doesn't
> +			 * specify its own credentials, a credential helper
> +			 * might need the wwwauth[] array from the server's
> +			 * redirect response in order to authenticate.
> +			 */
> +			strvec_pushv(&wwwauth_headers,
> +				     http_auth.wwwauth_headers.v);
>  			credential_from_url(&http_auth, options->base_url->buf);
> +			strvec_pushv(&http_auth.wwwauth_headers,
> +				     wwwauth_headers.v);
> +			strvec_clear(&wwwauth_headers);
>  			url = options->effective_url->buf;
>  		}
>  	}

As strvec_pushv() makes copies of the strings contained in .v[]
array, the above will

 - make a deep copy of http_auth.wwwauth_headers.v[] and store it away
   in wwwauth_headers.v[];

 - let credential_from_url() get rid of
   http_auth.wwwauth_headers.v[] (the original is freed here, but we
   have a deep copy stashed away safely), and perhaps add some of
   its own there; then

 - add what we stashed away back to http_auth.wwwauth_headers.v[].

So it does not leak and it does not have use-after-free, either,
which is good, even though it may be a bit inefficient having to
copy these strings so many times.

I briefly wondered if it is unconditionally adding back the original
wwwauth_headers always the right thing to do, but I think this is
good.  In the context of http_request_recovorable(), the redirect
has already happened, and the request to the redirect target has
failed with a 401. The wwwauth_headers currently in http_auth were
populated from this 401 response from the redirect target. Since we
are updating http_auth's URL to match this redirect target (in order
to query the helper for the correct host), the headers we currently
have are the active challenges for this new URL. Thus, they must be
preserved and passed to the helper.

A few design questions that came to my mind are:

 - Is wwwauth_headers the _only_ thing that needs to be preserved in
   the existing credential in http_auth?  Will it stay to be the
   only thing, or will we need to rethink what this patch did in the
   future when we add such a new member to "struct credential"?

 - If we need to preserve some other members in "struct credential",
   or if we add such members to the struct in the future, what would
   be the recommended way to extend what this patch does to cover?

If we add new members in the future to store other transient
response-based authentication state (e.g. Authentication-Info
headers, or proxy authentication states), they will be wiped by
credential_from_url() and will need to be preserved the same way,
no?  This observation and thought experiment may hint that the
manual save-and-restore approach is not robust against future
extensions of struct credential.

The current approach of manually saving and restoring
wwwauth_headers in http.c creates a tight coupling between the HTTP
layer and the internals of struct credential. If new transient
fields are added in the future, developers must remember to update
http.c to preserve them, which may be error-prone.

I wonder if it would make the design more robust and future-proof to
encapsulate this logic in credential.c instead.  For example, we
could introduce a helper function:

    void credential_update_url(struct credential *c, const char *url)

that does what the new code added around credential_from_url() by
this patch does, perhaps?

Re: [PATCH] http: preserve wwwauth_headers across redirects

[Aaron Plattner]

On 6/2/26 5:15 PM, Junio C Hamano wrote:
> Aaron Plattner <aplattner@nvidia.com> writes:
> 
>> diff --git a/http.c b/http.c
>> index ea9b16861b..cac8c9bfc9 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -2425,7 +2425,21 @@ 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)) {
>> +			struct strvec wwwauth_headers = STRVEC_INIT;
>> +
>> +			/*
>> +			 * Preserve wwwauth_headers across the call to
>> +			 * credential_from_url(): if the effective URL doesn't
>> +			 * specify its own credentials, a credential helper
>> +			 * might need the wwwauth[] array from the server's
>> +			 * redirect response in order to authenticate.
>> +			 */
>> +			strvec_pushv(&wwwauth_headers,
>> +				     http_auth.wwwauth_headers.v);
>>   			credential_from_url(&http_auth, options->base_url->buf);
>> +			strvec_pushv(&http_auth.wwwauth_headers,
>> +				     wwwauth_headers.v);
>> +			strvec_clear(&wwwauth_headers);
>>   			url = options->effective_url->buf;
>>   		}
>>   	}
> 
> As strvec_pushv() makes copies of the strings contained in .v[]
> array, the above will
> 
>   - make a deep copy of http_auth.wwwauth_headers.v[] and store it away
>     in wwwauth_headers.v[];
> 
>   - let credential_from_url() get rid of
>     http_auth.wwwauth_headers.v[] (the original is freed here, but we
>     have a deep copy stashed away safely), and perhaps add some of
>     its own there; then
> 
>   - add what we stashed away back to http_auth.wwwauth_headers.v[].
> 
> So it does not leak and it does not have use-after-free, either,
> which is good, even though it may be a bit inefficient having to
> copy these strings so many times.

I agree, although in the grand scheme wwwauth_headers is a drop in the 
bucket compared to, say, nearly any git object.

I tried to find an easy way to just not clear it in the first place, but 
that doesn't really match what credential_clear() is intended to do.
> I briefly wondered if it is unconditionally adding back the original
> wwwauth_headers always the right thing to do, but I think this is
> good.  In the context of http_request_recovorable(), the redirect
> has already happened, and the request to the redirect target has
> failed with a 401. The wwwauth_headers currently in http_auth were
> populated from this 401 response from the redirect target. Since we
> are updating http_auth's URL to match this redirect target (in order
> to query the helper for the correct host), the headers we currently
> have are the active challenges for this new URL. Thus, they must be
> preserved and passed to the helper.

This matches my understanding and I think it points to a more 
fundamental design issue: wwwauth_headers aren't really credentials at 
all, and maybe they shouldn't be in struct credential in the first 
place. I wonder if it would make sense to encapsulate it in some other 
http-related structure that lives alongside the credentials, presumably 
along with the protocol, host, and path.

> A few design questions that came to my mind are:
> 
>   - Is wwwauth_headers the _only_ thing that needs to be preserved in
>     the existing credential in http_auth?  Will it stay to be the
>     only thing, or will we need to rethink what this patch did in the
>     future when we add such a new member to "struct credential"?
> 
>   - If we need to preserve some other members in "struct credential",
>     or if we add such members to the struct in the future, what would
>     be the recommended way to extend what this patch does to cover?
> 
> If we add new members in the future to store other transient
> response-based authentication state (e.g. Authentication-Info
> headers, or proxy authentication states), they will be wiped by
> credential_from_url() and will need to be preserved the same way,
> no?  This observation and thought experiment may hint that the
> manual save-and-restore approach is not robust against future
> extensions of struct credential.
> 
> The current approach of manually saving and restoring
> wwwauth_headers in http.c creates a tight coupling between the HTTP
> layer and the internals of struct credential. If new transient
> fields are added in the future, developers must remember to update
> http.c to preserve them, which may be error-prone.
> 
> I wonder if it would make the design more robust and future-proof to
> encapsulate this logic in credential.c instead.  For example, we
> could introduce a helper function:
> 
>      void credential_update_url(struct credential *c, const char *url)
> 
> that does what the new code added around credential_from_url() by
> this patch does, perhaps?

Yeah, maybe. I'll think about this design some more.

-- Aaron