Re: [PATCH] http: preserve wwwauth_headers across redirects

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 3, 2026, 00:15 UTC
Message-ID: <xmqqpl28scll.fsf@gitster.g>
In-Reply-To: <20260602161150.1527493-1-aplattner@nvidia.com>

Aaron Plattner <aplattner@nvidia.com> writes:

Show 26 quoted lines

> 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?