On 6/2/26 5:15 PM, Junio C Hamano wrote:

Aaron Plattner [off-list ref] writes:

quoted

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