Re: [PATCH] http: preserve wwwauth_headers across redirects

[Junio C Hamano <gitster@pobox.com>]

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?