mirror of
https://gitea.com/gitea/tea.git
synced 2026-06-25 22:27:39 +02:00
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 <xiaolunwen@gmail.com> Co-authored-by: Daniel Bankmann <204984+dbankmann@noreply.gitea.com> Co-committed-by: Daniel Bankmann <204984+dbankmann@noreply.gitea.com>
This commit is contained in:
committed by
Lunny Xiao
parent
4b209d68de
commit
b11d991d1e
+14
-7
@@ -159,8 +159,12 @@ func performBrowserOAuthFlow(ctx context.Context, opts OAuthOptions) (serverURL
|
|||||||
// Get the authorization URL
|
// Get the authorization URL
|
||||||
authCodeURL := oauth2Config.AuthCodeURL(state, authCodeOpts...)
|
authCodeURL := oauth2Config.AuthCodeURL(state, authCodeOpts...)
|
||||||
|
|
||||||
// Start a local server to receive the callback
|
// Start a local server to receive the callback. When opts.Port == 0,
|
||||||
code, receivedState, err := startLocalServerAndOpenBrowser(authCodeURL, state, opts)
|
// 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 {
|
if err != nil {
|
||||||
// Check for redirect URI errors
|
// Check for redirect URI errors
|
||||||
if strings.Contains(err.Error(), "no authorization code") ||
|
if strings.Contains(err.Error(), "no authorization code") ||
|
||||||
@@ -218,9 +222,11 @@ func generateCodeChallenge(codeVerifier string) string {
|
|||||||
return base64.RawURLEncoding.EncodeToString(hash[:])
|
return base64.RawURLEncoding.EncodeToString(hash[:])
|
||||||
}
|
}
|
||||||
|
|
||||||
// startLocalServerAndOpenBrowser starts a local HTTP server to receive the OAuth callback
|
// startLocalServerAndOpenBrowser starts a local HTTP server to receive the
|
||||||
// and opens the browser to the authorization URL
|
// OAuth callback and opens the browser to the authorization URL. If
|
||||||
func startLocalServerAndOpenBrowser(authURL, expectedState string, opts OAuthOptions) (string, string, error) {
|
// 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
|
// Channel to receive the authorization code
|
||||||
codeChan := make(chan string, 1)
|
codeChan := make(chan string, 1)
|
||||||
stateChan := 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
|
// openBrowser opens the default browser to the specified URL.
|
||||||
func openBrowser(url string) error {
|
// 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)
|
fmt.Printf("Please authorize the application by visiting this URL in your browser:\n%s\n", url)
|
||||||
|
|
||||||
return open.Run(url)
|
return open.Run(url)
|
||||||
|
|||||||
@@ -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)")
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user