{"thread":{"id":"65740","subject":"[PATCH] http: preserve wwwauth_headers across redirects","startedAt":"2026-06-02T16:12:35Z","lastAt":"2026-07-07T22:35:16Z","messageCount":6,"participants":["Aaron Plattner","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"544531","messageId":"20260602161150.1527493-1-aplattner@nvidia.com","threadId":"65740","inReplyTo":null,"subject":"[PATCH] http: preserve wwwauth_headers across redirects","fromName":"Aaron Plattner","fromEmail":"aplattner@nvidia.com","sentAt":"2026-06-02T16:11:48Z","receivedAt":"2026-06-02T16:12:35Z","isPatch":true,"body":"When cURL follows a redirect, it calls the CURLOPT_HEADERFUNCTION for\neach header received including ones from a redirect. http_request() sets\nfwrite_wwwauth() as the header function, which will record the wwwauth[]\nentries for the last step in the redirection chain.\n\nHowever, when http_request_recoverable() sees that cURL followed a\nredirect, it attempts to update the credentials for the request from the\nnew URL using credential_from_url(). The first thing that does is call\ncredential_clear(), which clears everything including wwwauth_headers.\n\nIf the new URL should use a credential helper rather than credentials\nembedded in the URL, this loses the list of authentication methods that\nthe server provided in the redirect.\n\nFor example, I have a server that supports HTTP but always redirects to\nHTTPS before handling requests. This redirect breaks OAuth\nauthentication:\n\n  $ git ls-remote http://server/git\n  => Send header: GET /git/info/refs?service=git-upload-pack HTTP/1.1\n  <= Recv header: HTTP/1.1 302 Found\n  <= Recv header: Location: https://server.nvidia.com/git/info/refs?service=git-upload-pack\n  == Info: Issue another request to this URL: 'https://server.nvidia.com/git/info/refs?service=git-upload-pack'\n  => Send header: GET /git/info/refs?service=git-upload-pack HTTP/1.1\n  <= Recv header: HTTP/1.1 401 Unauthorized\n  <= 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>\"\n  trace: run_command: 'git credential-cache --timeout 7200 get'\n  trace: start_command: /bin/sh -c 'git credential-cache --timeout 7200 get' 'git credential-cache --timeout 7200 get'\n  trace: built-in: git credential-cache --timeout 7200 get\n  trace: run_command: 'git credential-msal get'\n  trace: start_command: /bin/sh -c 'git credential-msal get' 'git credential-msal get'\n  trace: exec: git-credential-msal get\n  trace: run_command: git-credential-msal get\n  trace: start_command: /usr/bin/git-credential-msal get\n  Username for 'https://server.nvidia.com': ^C\n\nWhen git invokes the credential helper, it doesn't include the wwwauth[]\narray, so git-credential-msal doesn't think that OAuth is supported [1].\n\nFix the problem by preserving the wwwauth_headers strvec across the call\nto credential_from_url().\n\n[1] https://github.com/Binary-Eater/git-credential-msal/blob/trunk/src/git_credential_msal/main.py#L69\n\nSigned-off-by: Aaron Plattner <aplattner@nvidia.com>\n---\n http.c                      | 14 ++++++++++++\n t/lib-httpd/apache.conf     |  1 +\n t/t5563-simple-http-auth.sh | 45 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 60 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex ea9b16861b..cac8c9bfc9 100644\n--- a/http.c\n+++ b/http.c\n@@ -2425,7 +2425,21 @@ static int http_request_recoverable(const char *url,\n \tif (options->effective_url && options->base_url) {\n \t\tif (update_url_from_redirect(options->base_url,\n \t\t\t\t\t     url, options->effective_url)) {\n+\t\t\tstruct strvec wwwauth_headers = STRVEC_INIT;\n+\n+\t\t\t/*\n+\t\t\t * Preserve wwwauth_headers across the call to\n+\t\t\t * credential_from_url(): if the effective URL doesn't\n+\t\t\t * specify its own credentials, a credential helper\n+\t\t\t * might need the wwwauth[] array from the server's\n+\t\t\t * redirect response in order to authenticate.\n+\t\t\t */\n+\t\t\tstrvec_pushv(&wwwauth_headers,\n+\t\t\t\t     http_auth.wwwauth_headers.v);\n \t\t\tcredential_from_url(&http_auth, options->base_url->buf);\n+\t\t\tstrvec_pushv(&http_auth.wwwauth_headers,\n+\t\t\t\t     wwwauth_headers.v);\n+\t\t\tstrvec_clear(&wwwauth_headers);\n \t\t\turl = options->effective_url->buf;\n \t\t}\n \t}\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 40a690b0bb..664f23fc6c 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -202,6 +202,7 @@ RewriteRule ^/dumb-redir/(.*)$ /dumb/$1 [R=301]\n RewriteRule ^/smart-redir-perm/(.*)$ /smart/$1 [R=301]\n RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]\n RewriteRule ^/smart-redir-auth/(.*)$ /auth/smart/$1 [R=301]\n+RewriteRule ^/custom_auth_redir/(.*)$ /custom_auth/$1 [R=302]\n RewriteRule ^/smart-redir-limited/(.*)/info/refs$ /smart/$1/info/refs [R=301]\n RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]\n \ndiff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh\nindex a7d475dd68..349ae4ab39 100755\n--- a/t/t5563-simple-http-auth.sh\n+++ b/t/t5563-simple-http-auth.sh\n@@ -557,6 +557,51 @@ test_expect_success 'access using bearer auth' '\n \tEOF\n '\n \n+test_expect_success 'bearer auth after redirect preserves wwwauth headers' '\n+\ttest_when_finished \"per_test_cleanup\" &&\n+\n+\tset_credential_reply get <<-EOF &&\n+\tcapability[]=authtype\n+\tauthtype=Bearer\n+\tcredential=YS1naXQtdG9rZW4=\n+\tEOF\n+\n+\tcat >\"$HTTPD_ROOT_PATH/custom-auth.valid\" <<-EOF &&\n+\tid=1 creds=Bearer YS1naXQtdG9rZW4=\n+\tEOF\n+\n+\tcat >\"$HTTPD_ROOT_PATH/custom-auth.challenge\" <<-EOF &&\n+\tid=1 status=200\n+\tid=default response=WWW-Authenticate: FooBar param1=\"value1\" param2=\"value2\"\n+\tid=default response=WWW-Authenticate: Bearer authorize_uri=\"id.example.com\" p=1 q=0\n+\tid=default response=WWW-Authenticate: Basic realm=\"example.com\"\n+\tEOF\n+\n+\ttest_config_global credential.helper test-helper &&\n+\ttest_config_global credential.useHttpPath true &&\n+\tgit ls-remote \"$HTTPD_URL/custom_auth_redir/repo.git\" &&\n+\n+\texpect_credential_query get <<-EOF &&\n+\tcapability[]=authtype\n+\tcapability[]=state\n+\tprotocol=http\n+\thost=$HTTPD_DEST\n+\tpath=custom_auth/repo.git\n+\twwwauth[]=FooBar param1=\"value1\" param2=\"value2\"\n+\twwwauth[]=Bearer authorize_uri=\"id.example.com\" p=1 q=0\n+\twwwauth[]=Basic realm=\"example.com\"\n+\tEOF\n+\n+\texpect_credential_query store <<-EOF\n+\tcapability[]=authtype\n+\tauthtype=Bearer\n+\tcredential=YS1naXQtdG9rZW4=\n+\tprotocol=http\n+\thost=$HTTPD_DEST\n+\tpath=custom_auth/repo.git\n+\tEOF\n+'\n+\n test_expect_success 'access using bearer auth with invalid credentials' '\n \ttest_when_finished \"per_test_cleanup\" &&\n \n-- \n2.54.0\n\n"},{"id":"544566","messageId":"xmqqpl28scll.fsf@gitster.g","threadId":"65740","inReplyTo":"20260602161150.1527493-1-aplattner@nvidia.com","subject":"Re: [PATCH] http: preserve wwwauth_headers across redirects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-03T00:15:50Z","receivedAt":"2026-06-03T00:15:52Z","isPatch":true,"body":"Aaron Plattner <aplattner@nvidia.com> writes:\n\n> diff --git a/http.c b/http.c\n> index ea9b16861b..cac8c9bfc9 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -2425,7 +2425,21 @@ static int http_request_recoverable(const char *url,\n>  \tif (options->effective_url && options->base_url) {\n>  \t\tif (update_url_from_redirect(options->base_url,\n>  \t\t\t\t\t     url, options->effective_url)) {\n> +\t\t\tstruct strvec wwwauth_headers = STRVEC_INIT;\n> +\n> +\t\t\t/*\n> +\t\t\t * Preserve wwwauth_headers across the call to\n> +\t\t\t * credential_from_url(): if the effective URL doesn't\n> +\t\t\t * specify its own credentials, a credential helper\n> +\t\t\t * might need the wwwauth[] array from the server's\n> +\t\t\t * redirect response in order to authenticate.\n> +\t\t\t */\n> +\t\t\tstrvec_pushv(&wwwauth_headers,\n> +\t\t\t\t     http_auth.wwwauth_headers.v);\n>  \t\t\tcredential_from_url(&http_auth, options->base_url->buf);\n> +\t\t\tstrvec_pushv(&http_auth.wwwauth_headers,\n> +\t\t\t\t     wwwauth_headers.v);\n> +\t\t\tstrvec_clear(&wwwauth_headers);\n>  \t\t\turl = options->effective_url->buf;\n>  \t\t}\n>  \t}\n\nAs strvec_pushv() makes copies of the strings contained in .v[]\narray, the above will\n\n - make a deep copy of http_auth.wwwauth_headers.v[] and store it away\n   in wwwauth_headers.v[];\n\n - let credential_from_url() get rid of\n   http_auth.wwwauth_headers.v[] (the original is freed here, but we\n   have a deep copy stashed away safely), and perhaps add some of\n   its own there; then\n\n - add what we stashed away back to http_auth.wwwauth_headers.v[].\n\nSo it does not leak and it does not have use-after-free, either,\nwhich is good, even though it may be a bit inefficient having to\ncopy these strings so many times.\n\nI briefly wondered if it is unconditionally adding back the original\nwwwauth_headers always the right thing to do, but I think this is\ngood.  In the context of http_request_recovorable(), the redirect\nhas already happened, and the request to the redirect target has\nfailed with a 401. The wwwauth_headers currently in http_auth were\npopulated from this 401 response from the redirect target. Since we\nare updating http_auth's URL to match this redirect target (in order\nto query the helper for the correct host), the headers we currently\nhave are the active challenges for this new URL. Thus, they must be\npreserved and passed to the helper.\n\nA few design questions that came to my mind are:\n\n - Is wwwauth_headers the _only_ thing that needs to be preserved in\n   the existing credential in http_auth?  Will it stay to be the\n   only thing, or will we need to rethink what this patch did in the\n   future when we add such a new member to \"struct credential\"?\n\n - If we need to preserve some other members in \"struct credential\",\n   or if we add such members to the struct in the future, what would\n   be the recommended way to extend what this patch does to cover?\n\nIf we add new members in the future to store other transient\nresponse-based authentication state (e.g. Authentication-Info\nheaders, or proxy authentication states), they will be wiped by\ncredential_from_url() and will need to be preserved the same way,\nno?  This observation and thought experiment may hint that the\nmanual save-and-restore approach is not robust against future\nextensions of struct credential.\n\nThe current approach of manually saving and restoring\nwwwauth_headers in http.c creates a tight coupling between the HTTP\nlayer and the internals of struct credential. If new transient\nfields are added in the future, developers must remember to update\nhttp.c to preserve them, which may be error-prone.\n\nI wonder if it would make the design more robust and future-proof to\nencapsulate this logic in credential.c instead.  For example, we\ncould introduce a helper function:\n\n    void credential_update_url(struct credential *c, const char *url)\n\nthat does what the new code added around credential_from_url() by\nthis patch does, perhaps?\n"},{"id":"544567","messageId":"5144a29d-a53f-4446-beff-e1f549345bf9@nvidia.com","threadId":"65740","inReplyTo":"xmqqpl28scll.fsf@gitster.g","subject":"Re: [PATCH] http: preserve wwwauth_headers across redirects","fromName":"Aaron Plattner","fromEmail":"aplattner@nvidia.com","sentAt":"2026-06-03T00:37:48Z","receivedAt":"2026-06-03T00:37:53Z","isPatch":true,"body":"On 6/2/26 5:15 PM, Junio C Hamano wrote:\n> Aaron Plattner <aplattner@nvidia.com> writes:\n> \n>> diff --git a/http.c b/http.c\n>> index ea9b16861b..cac8c9bfc9 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -2425,7 +2425,21 @@ static int http_request_recoverable(const char *url,\n>>   \tif (options->effective_url && options->base_url) {\n>>   \t\tif (update_url_from_redirect(options->base_url,\n>>   \t\t\t\t\t     url, options->effective_url)) {\n>> +\t\t\tstruct strvec wwwauth_headers = STRVEC_INIT;\n>> +\n>> +\t\t\t/*\n>> +\t\t\t * Preserve wwwauth_headers across the call to\n>> +\t\t\t * credential_from_url(): if the effective URL doesn't\n>> +\t\t\t * specify its own credentials, a credential helper\n>> +\t\t\t * might need the wwwauth[] array from the server's\n>> +\t\t\t * redirect response in order to authenticate.\n>> +\t\t\t */\n>> +\t\t\tstrvec_pushv(&wwwauth_headers,\n>> +\t\t\t\t     http_auth.wwwauth_headers.v);\n>>   \t\t\tcredential_from_url(&http_auth, options->base_url->buf);\n>> +\t\t\tstrvec_pushv(&http_auth.wwwauth_headers,\n>> +\t\t\t\t     wwwauth_headers.v);\n>> +\t\t\tstrvec_clear(&wwwauth_headers);\n>>   \t\t\turl = options->effective_url->buf;\n>>   \t\t}\n>>   \t}\n> \n> As strvec_pushv() makes copies of the strings contained in .v[]\n> array, the above will\n> \n>   - make a deep copy of http_auth.wwwauth_headers.v[] and store it away\n>     in wwwauth_headers.v[];\n> \n>   - let credential_from_url() get rid of\n>     http_auth.wwwauth_headers.v[] (the original is freed here, but we\n>     have a deep copy stashed away safely), and perhaps add some of\n>     its own there; then\n> \n>   - add what we stashed away back to http_auth.wwwauth_headers.v[].\n> \n> So it does not leak and it does not have use-after-free, either,\n> which is good, even though it may be a bit inefficient having to\n> copy these strings so many times.\n\nI agree, although in the grand scheme wwwauth_headers is a drop in the \nbucket compared to, say, nearly any git object.\n\nI tried to find an easy way to just not clear it in the first place, but \nthat doesn't really match what credential_clear() is intended to do.\n> I briefly wondered if it is unconditionally adding back the original\n> wwwauth_headers always the right thing to do, but I think this is\n> good.  In the context of http_request_recovorable(), the redirect\n> has already happened, and the request to the redirect target has\n> failed with a 401. The wwwauth_headers currently in http_auth were\n> populated from this 401 response from the redirect target. Since we\n> are updating http_auth's URL to match this redirect target (in order\n> to query the helper for the correct host), the headers we currently\n> have are the active challenges for this new URL. Thus, they must be\n> preserved and passed to the helper.\n\nThis matches my understanding and I think it points to a more \nfundamental design issue: wwwauth_headers aren't really credentials at \nall, and maybe they shouldn't be in struct credential in the first \nplace. I wonder if it would make sense to encapsulate it in some other \nhttp-related structure that lives alongside the credentials, presumably \nalong with the protocol, host, and path.\n\n> A few design questions that came to my mind are:\n> \n>   - Is wwwauth_headers the _only_ thing that needs to be preserved in\n>     the existing credential in http_auth?  Will it stay to be the\n>     only thing, or will we need to rethink what this patch did in the\n>     future when we add such a new member to \"struct credential\"?\n> \n>   - If we need to preserve some other members in \"struct credential\",\n>     or if we add such members to the struct in the future, what would\n>     be the recommended way to extend what this patch does to cover?\n> \n> If we add new members in the future to store other transient\n> response-based authentication state (e.g. Authentication-Info\n> headers, or proxy authentication states), they will be wiped by\n> credential_from_url() and will need to be preserved the same way,\n> no?  This observation and thought experiment may hint that the\n> manual save-and-restore approach is not robust against future\n> extensions of struct credential.\n> \n> The current approach of manually saving and restoring\n> wwwauth_headers in http.c creates a tight coupling between the HTTP\n> layer and the internals of struct credential. If new transient\n> fields are added in the future, developers must remember to update\n> http.c to preserve them, which may be error-prone.\n> \n> I wonder if it would make the design more robust and future-proof to\n> encapsulate this logic in credential.c instead.  For example, we\n> could introduce a helper function:\n> \n>      void credential_update_url(struct credential *c, const char *url)\n> \n> that does what the new code added around credential_from_url() by\n> this patch does, perhaps?\n\nYeah, maybe. I'll think about this design some more.\n\n-- Aaron\n"},{"id":"547386","messageId":"xmqqo6gi3905.fsf@gitster.g","threadId":"65740","inReplyTo":"5144a29d-a53f-4446-beff-e1f549345bf9@nvidia.com","subject":"Re: [PATCH] http: preserve wwwauth_headers across redirects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-07T19:16:58Z","receivedAt":"2026-07-07T19:17:00Z","isPatch":true,"body":"Aaron Plattner <aplattner@nvidia.com> writes:\n\n>> I wonder if it would make the design more robust and future-proof to\n>> encapsulate this logic in credential.c instead.  For example, we\n>> could introduce a helper function:\n>> \n>>      void credential_update_url(struct credential *c, const char *url)\n>> \n>> that does what the new code added around credential_from_url() by\n>> this patch does, perhaps?\n>\n> Yeah, maybe. I'll think about this design some more.\n\nSorry, I lost track.\n\nDid anything come of that discussion?  No rush, since this change\nfixes an immediate issue and the helper suggestion is for long-term\nfuture-proofing.  We can treat them as separate steps.\n\nThanks. \n"},{"id":"547388","messageId":"68c2b88f-8976-474b-8965-97733eba5a99@nvidia.com","threadId":"65740","inReplyTo":"xmqqo6gi3905.fsf@gitster.g","subject":"Re: [PATCH] http: preserve wwwauth_headers across redirects","fromName":"Aaron Plattner","fromEmail":"aplattner@nvidia.com","sentAt":"2026-07-07T19:21:27Z","receivedAt":"2026-07-07T19:21:33Z","isPatch":true,"body":"On 7/7/26 12:16 PM, Junio C Hamano wrote:\n> Aaron Plattner <aplattner@nvidia.com> writes:\n> \n>>> I wonder if it would make the design more robust and future-proof to\n>>> encapsulate this logic in credential.c instead.  For example, we\n>>> could introduce a helper function:\n>>>\n>>>       void credential_update_url(struct credential *c, const char *url)\n>>>\n>>> that does what the new code added around credential_from_url() by\n>>> this patch does, perhaps?\n>>\n>> Yeah, maybe. I'll think about this design some more.\n> \n> Sorry, I lost track.\n> \n> Did anything come of that discussion?  No rush, since this change\n> fixes an immediate issue and the helper suggestion is for long-term\n> future-proofing.  We can treat them as separate steps.\n> \n> Thanks.\n\nNo, I got sidetracked with other work and didn't get a chance to get \nback to this, sorry. It's not directly impacting my users since I can \njust tell them they have to use my server's FQDN, so fine with me to \ntreat this as a low-priority issue.\n\n-- Aaron\n"},{"id":"547410","messageId":"xmqqmrw2zavx.fsf@gitster.g","threadId":"65740","inReplyTo":"68c2b88f-8976-474b-8965-97733eba5a99@nvidia.com","subject":"Re: [PATCH] http: preserve wwwauth_headers across redirects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-07T22:35:14Z","receivedAt":"2026-07-07T22:35:16Z","isPatch":true,"body":"Aaron Plattner <aplattner@nvidia.com> writes:\n\n>> Did anything come of that discussion?  No rush, since this change\n>> fixes an immediate issue and the helper suggestion is for long-term\n>> future-proofing.  We can treat them as separate steps.\n>> \n>> Thanks.\n>\n> No, I got sidetracked with other work and didn't get a chance to get \n> back to this, sorry. It's not directly impacting my users since I can \n> just tell them they have to use my server's FQDN, so fine with me to \n> treat this as a low-priority issue.\n\nUnderstood.\n\nI hate to leave a topic backburnered for too long.  As this topic\nunfortunately has not seen enough attention by reviewers, between\ntwo easy approach available to me to deal with such a topic, namely,\nmerging it to 'next' and discarding it (with invitation to resubmit\nonce the author can spend enough time on the topic again), I'd\nprobably choose the latter.\n\nUnless somebody else steps up and promises to usher the topic\nforward in its current shape, that is.\n\nThanks.\n"}]}