From b11d991d1ebf04eba3e86824e3dfdd45ca4f241c Mon Sep 17 00:00:00 2001 From: Daniel Bankmann <204984+dbankmann@noreply.gitea.com> Date: Sat, 20 Jun 2026 20:03:34 +0000 Subject: [PATCH] fix(oauth): pass resolved redirect_uri to token exchange (#1019) When --redirect-url is omitted, the local callback listener binds to a free port and opts.RedirectURL is rewritten with it. oauth2Config.RedirectURL was never updated, so Exchange() sent the stale http://127.0.0.1:0 while the authorize step had sent the real port. RFC-6749-compliant servers (Gitea >= #37704, current Forgejo) reject the mismatch. Propagate the resolved URL back into oauth2Config before Exchange. Add a regression test using httptest that drives the flow end-to-end and asserts the redirect_uri values match. --------- Co-authored-by: dbankmann <204984+dbankmann@users.noreply.gitea.com> Reviewed-on: https://gitea.com/gitea/tea/pulls/1019 Reviewed-by: Lunny Xiao Co-authored-by: Daniel Bankmann <204984+dbankmann@noreply.gitea.com> Co-committed-by: Daniel Bankmann <204984+dbankmann@noreply.gitea.com> --- modules/auth/oauth.go | 21 ++++++--- modules/auth/oauth_test.go | 91 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 7 deletions(-) create mode 100644 modules/auth/oauth_test.go diff --git a/modules/auth/oauth.go b/modules/auth/oauth.go index 48bff99d..9c9ada40 100644 --- a/modules/auth/oauth.go +++ b/modules/auth/oauth.go @@ -159,8 +159,12 @@ func performBrowserOAuthFlow(ctx context.Context, opts OAuthOptions) (serverURL // Get the authorization URL authCodeURL := oauth2Config.AuthCodeURL(state, authCodeOpts...) - // Start a local server to receive the callback - code, receivedState, err := startLocalServerAndOpenBrowser(authCodeURL, state, opts) + // Start a local server to receive the callback. When opts.Port == 0, + // this resolves opts.RedirectURL to the actual listener port; mirror + // that into oauth2Config so Exchange sends the same redirect_uri the + // authorize step advertised (RFC 6749 §4.1.3). + code, receivedState, err := startLocalServerAndOpenBrowser(authCodeURL, state, &opts) + oauth2Config.RedirectURL = opts.RedirectURL if err != nil { // Check for redirect URI errors if strings.Contains(err.Error(), "no authorization code") || @@ -218,9 +222,11 @@ func generateCodeChallenge(codeVerifier string) string { return base64.RawURLEncoding.EncodeToString(hash[:]) } -// startLocalServerAndOpenBrowser starts a local HTTP server to receive the OAuth callback -// and opens the browser to the authorization URL -func startLocalServerAndOpenBrowser(authURL, expectedState string, opts OAuthOptions) (string, string, error) { +// startLocalServerAndOpenBrowser starts a local HTTP server to receive the +// OAuth callback and opens the browser to the authorization URL. If +// opts.Port is 0, the listener picks a free port and opts.RedirectURL is +// rewritten in place with that port. +func startLocalServerAndOpenBrowser(authURL, expectedState string, opts *OAuthOptions) (string, string, error) { // Channel to receive the authorization code codeChan := make(chan string, 1) stateChan := make(chan string, 1) @@ -357,8 +363,9 @@ func startLocalServerAndOpenBrowser(authURL, expectedState string, opts OAuthOpt } } -// openBrowser opens the default browser to the specified URL -func openBrowser(url string) error { +// openBrowser opens the default browser to the specified URL. +// It is a variable to allow tests to inject a fake browser. +var openBrowser = func(url string) error { fmt.Printf("Please authorize the application by visiting this URL in your browser:\n%s\n", url) return open.Run(url) diff --git a/modules/auth/oauth_test.go b/modules/auth/oauth_test.go new file mode 100644 index 00000000..6ce82086 --- /dev/null +++ b/modules/auth/oauth_test.go @@ -0,0 +1,91 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package auth + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Regression test for the redirect_uri propagation bug: with --redirect-url +// omitted, the callback listener picks a free port and opts.RedirectURL is +// rewritten in place. The OAuth2 config must see that rewritten URL so the +// token exchange sends the same redirect_uri the authorize step advertised +// (RFC 6749 §4.1.3). +func TestPerformBrowserOAuthFlow_RedirectURIMatchesAcrossAuthorizeAndExchange(t *testing.T) { + var ( + mu sync.Mutex + authorizeRedirectURI string + exchangeRedirectURI string + ) + + idp := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/login/oauth/authorize": + mu.Lock() + authorizeRedirectURI = r.URL.Query().Get("redirect_uri") + mu.Unlock() + cb, err := url.Parse(r.URL.Query().Get("redirect_uri")) + require.NoError(t, err) + q := cb.Query() + q.Set("code", "test-code") + q.Set("state", r.URL.Query().Get("state")) + cb.RawQuery = q.Encode() + http.Redirect(w, r, cb.String(), http.StatusFound) + + case "/login/oauth/access_token": + require.NoError(t, r.ParseForm()) + mu.Lock() + exchangeRedirectURI = r.PostForm.Get("redirect_uri") + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "access_token": "test-token", + "token_type": "Bearer", + "expires_in": 3600, + }) + + default: + http.NotFound(w, r) + } + })) + t.Cleanup(idp.Close) + + // Replace the real browser opener with a goroutine that fetches the + // authorize URL, just like a real browser would. + origOpenBrowser := openBrowser + t.Cleanup(func() { openBrowser = origOpenBrowser }) + openBrowser = func(authURL string) error { + go func() { + resp, err := http.Get(authURL) + if err == nil { + resp.Body.Close() + } + }() + return nil + } + + // Leave RedirectURL empty so tea picks a random port. + _, token, err := performBrowserOAuthFlow(context.Background(), OAuthOptions{ + URL: idp.URL, + ClientID: "test-client-id", + }) + require.NoError(t, err) + require.Equal(t, "test-token", token.AccessToken) + + mu.Lock() + defer mu.Unlock() + require.NotEmpty(t, authorizeRedirectURI) + require.NotEmpty(t, exchangeRedirectURI) + assert.Equal(t, authorizeRedirectURI, exchangeRedirectURI, + "redirect_uri must match between authorize and token exchange (RFC 6749 §4.1.3)") +}