From: Aaron Plattner Date: Wed, 03 Jun 2026 00:37:48 GMT Subject: Re: [PATCH] http: preserve wwwauth_headers across redirects Message-ID: <5144a29d-a53f-4446-beff-e1f549345bf9@nvidia.com> In-Reply-To: On 6/2/26 5:15 PM, Junio C Hamano wrote: > Aaron Plattner 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