From 4c2a896edc781df64427292b893dbf9f7cfa0556 Mon Sep 17 00:00:00 2001 From: Olivier Mouren Date: Mon, 2 Nov 2026 15:56:11 +0100 Subject: [PATCH] fix: Prevent mutation of global OAuth client's redirect URL Previously, dynamic selection of the OAuth callback URL for multiple domains would modify the global OAuth client configuration. This caused race conditions when processing concurrent requests for different domains. Now, a request-scoped OAuth client is created for each login flow, ensuring the correct callback URL is used without side-effecting the shared configuration. 75741a --- CHANGELOG.md | 2 + handlers/login.go | 24 +----- handlers/login_test.go | 44 ++++++++++ pkg/providers/common/common.go | 33 ++++++- pkg/providers/common/common_test.go | 129 ++++++++++++++++++++++++++++ 5 files changed, 210 insertions(+), 22 deletions(-) create mode 100644 pkg/providers/common/common_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ca81f77a..c7a3c246 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- fix OAuth `redirect_uri` race with multiple `callback_urls` + Coming soon! Please document any work in progress here as part of your PR. It will be moved to the next tag when released. ## v0.50.0 diff --git a/handlers/login.go b/handlers/login.go index 34f26f58..d2297dd1 100644 --- a/handlers/login.go +++ b/handlers/login.go @@ -24,6 +24,7 @@ import ( "github.com/vouch/vouch-proxy/pkg/cfg" "github.com/vouch/vouch-proxy/pkg/cookie" "github.com/vouch/vouch-proxy/pkg/domains" + "github.com/vouch/vouch-proxy/pkg/providers/common" "github.com/vouch/vouch-proxy/pkg/responses" "golang.org/x/oauth2" ) @@ -250,28 +251,11 @@ func oauthLoginURL(r *http.Request, session sessions.Session) string { // See relevant RFC: http://tools.ietf.org/html/rfc6749#section-10.12 var state string = session.Values["state"].(string) opts := []oauth2.AuthCodeOption{} + oauthClient := common.OAuthClientForRequest(r) if cfg.GenOAuth.Provider == cfg.Providers.IndieAuth { - return cfg.OAuthClient.AuthCodeURL(state, oauth2.SetAuthURLParam("response_type", "id")) + return oauthClient.AuthCodeURL(state, oauth2.SetAuthURLParam("response_type", "id")) } - // cfg.OAuthClient.RedirectURL is set in cfg - // this checks the multiple redirect case for multiple matching domains - if len(cfg.GenOAuth.RedirectURLs) > 0 { - found := false - domain := domains.Matches(r.Host) - log.Debugf("/login looking for callback_url matching %s from host %s", domain, r.Host) - for _, v := range cfg.GenOAuth.RedirectURLs { - if strings.Contains(v, domain) { - found = true - log.Debugf("/login callback_url set to %s", v) - cfg.OAuthClient.RedirectURL = v - break - } - } - if !found { - log.Infof("/login no callback_url matched %s (is the `Host` header being passed to Vouch Proxy?)", domain) - } - } // append code challenge and code challenge method query parameters if enabled if cfg.GenOAuth.CodeChallengeMethod != "" { @@ -281,7 +265,7 @@ func oauthLoginURL(r *http.Request, session sessions.Session) string { if cfg.OAuthopts != nil { opts = append(opts, cfg.OAuthopts...) } - return cfg.OAuthClient.AuthCodeURL(state, opts...) + return oauthClient.AuthCodeURL(state, opts...) } var regExJustAlphaNum, _ = regexp.Compile("[^a-zA-Z0-9]+") diff --git a/handlers/login_test.go b/handlers/login_test.go index 71f519a9..09abf2d8 100644 --- a/handlers/login_test.go +++ b/handlers/login_test.go @@ -18,6 +18,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/gorilla/sessions" "github.com/stretchr/testify/assert" "github.com/vouch/vouch-proxy/pkg/cfg" ) @@ -219,6 +220,49 @@ func TestLoginHandler(t *testing.T) { }) } } + +func TestOAuthLoginURLUsesRequestCallbackURLWithoutMutatingGlobalConfig(t *testing.T) { + setUp("/config/testing/handler_login_redirecturls.yml") + startupRedirectURL := cfg.OAuthClient.RedirectURL + + firstRequest := httptest.NewRequest(http.MethodGet, "/login", nil) + firstRequest.Host = "vouch.example.com" + firstURL, err := url.Parse(oauthLoginURL(firstRequest, loginSession("first"))) + assert.NoError(t, err) + assert.Equal(t, "http://vouch.example.com:9090/auth", firstURL.Query().Get("redirect_uri")) + + secondRequest := httptest.NewRequest(http.MethodGet, "/login", nil) + secondRequest.Host = "vouch.other.com" + secondURL, err := url.Parse(oauthLoginURL(secondRequest, loginSession("second"))) + assert.NoError(t, err) + assert.Equal(t, "http://vouch.other.com:9090/auth", secondURL.Query().Get("redirect_uri")) + + assert.Equal(t, "http://vouch.example.com:9090/auth", firstURL.Query().Get("redirect_uri")) + assert.Equal(t, startupRedirectURL, cfg.OAuthClient.RedirectURL) +} + +func TestOAuthLoginURLUsesRequestCallbackURLForIndieAuth(t *testing.T) { + setUp("/config/testing/handler_login_redirecturls.yml") + startupRedirectURL := cfg.OAuthClient.RedirectURL + cfg.GenOAuth.Provider = cfg.Providers.IndieAuth + + request := httptest.NewRequest(http.MethodGet, "/login", nil) + request.Host = "vouch.other.com" + oauthURL, err := url.Parse(oauthLoginURL(request, loginSession("state"))) + + assert.NoError(t, err) + assert.Equal(t, "http://vouch.other.com:9090/auth", oauthURL.Query().Get("redirect_uri")) + assert.Equal(t, "id", oauthURL.Query().Get("response_type")) + assert.Equal(t, startupRedirectURL, cfg.OAuthClient.RedirectURL) +} + +func loginSession(state string) sessions.Session { + return sessions.Session{Values: map[interface{}]interface{}{ + "state": state, + "codeChallenge": "challenge", + }} +} + func TestLoginErrTooManyRedirects(t *testing.T) { handler := http.HandlerFunc(LoginHandler) diff --git a/pkg/providers/common/common.go b/pkg/providers/common/common.go index 7a23e4e7..34dbdd2a 100644 --- a/pkg/providers/common/common.go +++ b/pkg/providers/common/common.go @@ -14,11 +14,13 @@ import ( "context" "encoding/json" "net/http" + "strings" "go.uber.org/zap" "golang.org/x/oauth2" "github.com/vouch/vouch-proxy/pkg/cfg" + "github.com/vouch/vouch-proxy/pkg/domains" "github.com/vouch/vouch-proxy/pkg/structs" ) @@ -29,9 +31,36 @@ func Configure() { log = cfg.Logging.Logger } +// OAuthClientForRequest returns a request-local OAuth config. When several +// callback URLs are configured, it selects the callback associated with the +// request Host without modifying the shared config created at startup. +func OAuthClientForRequest(r *http.Request) *oauth2.Config { + oauthClient := *cfg.OAuthClient + + if len(cfg.GenOAuth.RedirectURLs) == 0 { + return &oauthClient + } + + domain := domains.Matches(r.Host) + log.Debugf("/login looking for callback_url matching %s from host %s", domain, r.Host) + if domain != "" { + for _, callbackURL := range cfg.GenOAuth.RedirectURLs { + if strings.Contains(callbackURL, domain) { + log.Debugf("/login callback_url set to %s", callbackURL) + oauthClient.RedirectURL = callbackURL + return &oauthClient + } + } + } + + log.Infof("/login no callback_url matched %s (is the `Host` header being passed to Vouch Proxy?)", domain) + return &oauthClient +} + // PrepareTokensAndClient setup the client, usually for a UserInfo request func PrepareTokensAndClient(r *http.Request, ptokens *structs.PTokens, setProviderToken bool, opts ...oauth2.AuthCodeOption) (*http.Client, *oauth2.Token, error) { - providerToken, err := cfg.OAuthClient.Exchange(context.TODO(), r.URL.Query().Get("code"), opts...) + oauthClient := OAuthClientForRequest(r) + providerToken, err := oauthClient.Exchange(context.TODO(), r.URL.Query().Get("code"), opts...) if err != nil { return nil, nil, err } @@ -48,7 +77,7 @@ func PrepareTokensAndClient(r *http.Request, ptokens *structs.PTokens, setProvid } log.Debugf("ptokens: accessToken length: %d, IdToken length: %d", len(ptokens.PAccessToken), len(ptokens.PIdToken)) - client := cfg.OAuthClient.Client(context.TODO(), providerToken) + client := oauthClient.Client(context.TODO(), providerToken) return client, providerToken, err } diff --git a/pkg/providers/common/common_test.go b/pkg/providers/common/common_test.go new file mode 100644 index 00000000..4e3ddfae --- /dev/null +++ b/pkg/providers/common/common_test.go @@ -0,0 +1,129 @@ +/* + +Copyright 2020 The Vouch Proxy Authors. +Use of this source code is governed by The MIT License (MIT) that +can be found in the LICENSE file. Software distributed under The +MIT License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES +OR CONDITIONS OF ANY KIND, either express or implied. + +*/ + +package common + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "go.uber.org/zap" + "golang.org/x/oauth2" + + "github.com/vouch/vouch-proxy/pkg/cfg" + "github.com/vouch/vouch-proxy/pkg/domains" + "github.com/vouch/vouch-proxy/pkg/structs" +) + +func setUpOAuthClientForRequestTest(t *testing.T) { + t.Helper() + + previousCfg := cfg.Cfg + previousOAuthClient := cfg.OAuthClient + previousRedirectURLs := cfg.GenOAuth.RedirectURLs + previousLog := log + + cfg.Cfg = &cfg.Config{Domains: []string{"example.com", "other.com"}} + cfg.OAuthClient = &oauth2.Config{RedirectURL: "https://startup.example.com/auth"} + cfg.GenOAuth.RedirectURLs = []string{ + "https://vouch.example.com/auth", + "https://vouch.other.com/auth", + } + log = zap.NewNop().Sugar() + domains.Configure() + + t.Cleanup(func() { + cfg.Cfg = previousCfg + cfg.OAuthClient = previousOAuthClient + cfg.GenOAuth.RedirectURLs = previousRedirectURLs + log = previousLog + }) +} + +func TestOAuthClientForRequest(t *testing.T) { + setUpOAuthClientForRequestTest(t) + + firstRequest := httptest.NewRequest(http.MethodGet, "/login", nil) + firstRequest.Host = "vouch.example.com" + firstClient := OAuthClientForRequest(firstRequest) + assert.Equal(t, "https://vouch.example.com/auth", firstClient.RedirectURL) + + secondRequest := httptest.NewRequest(http.MethodGet, "/login", nil) + secondRequest.Host = "vouch.other.com" + secondClient := OAuthClientForRequest(secondRequest) + assert.Equal(t, "https://vouch.other.com/auth", secondClient.RedirectURL) + + assert.Equal(t, "https://vouch.example.com/auth", firstClient.RedirectURL) + assert.Equal(t, "https://startup.example.com/auth", cfg.OAuthClient.RedirectURL) +} + +func TestOAuthClientForRequestFallsBackToStartupRedirectURL(t *testing.T) { + setUpOAuthClientForRequestTest(t) + + request := httptest.NewRequest(http.MethodGet, "/login", nil) + for _, host := range []string{"", "vouch.unknown.test"} { + request.Host = host + assert.Equal(t, cfg.OAuthClient.RedirectURL, OAuthClientForRequest(request).RedirectURL) + } + + cfg.GenOAuth.RedirectURLs = nil + request.Host = "vouch.example.com" + assert.Equal(t, cfg.OAuthClient.RedirectURL, OAuthClientForRequest(request).RedirectURL) +} + +func TestPrepareTokensAndClientUsesRequestCallbackURL(t *testing.T) { + setUpOAuthClientForRequestTest(t) + + var redirectURIs []string + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if err := r.ParseForm(); err != nil { + t.Fatal(err) + } + redirectURIs = append(redirectURIs, r.Form.Get("redirect_uri")) + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"access_token":"token","token_type":"Bearer"}`) + })) + defer tokenServer.Close() + + cfg.OAuthClient.Endpoint.TokenURL = tokenServer.URL + cfg.OAuthClient.ClientID = "client-id" + cfg.OAuthClient.ClientSecret = "client-secret" + + for _, host := range []string{"vouch.example.com", "vouch.other.com"} { + request := httptest.NewRequest(http.MethodGet, "/auth/state/?code=code", nil) + request.Host = host + _, _, err := PrepareTokensAndClient(request, &structs.PTokens{}, false) + assert.NoError(t, err) + } + + assert.Equal(t, []string{ + "https://vouch.example.com/auth", + "https://vouch.other.com/auth", + }, redirectURIs) +} + +func TestOAuthClientForRequestPreservesSingleCallbackConfiguration(t *testing.T) { + setUpOAuthClientForRequestTest(t) + + singleCallbackURL := "https://vouch.single.example.com/auth" + cfg.OAuthClient.RedirectURL = singleCallbackURL + cfg.GenOAuth.RedirectURLs = nil + + request := httptest.NewRequest(http.MethodGet, "/login", nil) + request.Host = "vouch.example.com" + oauthClient := OAuthClientForRequest(request) + + assert.Equal(t, singleCallbackURL, oauthClient.RedirectURL) + assert.Equal(t, singleCallbackURL, cfg.OAuthClient.RedirectURL) + assert.NotSame(t, cfg.OAuthClient, oauthClient) +}