fix(oauth): strengthen PKCE validation and refresh token replay protection (#37706) (#37738)

Backport #37706 

This PR tightens several OAuth validation paths related to PKCE
handling, redirect URI normalization, and refresh-token replay safety.

What it changes:

- switch redirect URI comparison to ASCII-only normalization for
exact-match checks, avoiding Unicode case-folding surprises
- harden PKCE verification by:
  - allowing PKCE omission only when no challenge data was stored
  - rejecting exchanges with a missing verifier when PKCE was used
- rejecting malformed challenge state where a challenge exists without a
valid method
  - comparing derived challenges with constant-time string matching
- make refresh-token invalidation counter updates conditional on the
previously observed counter value, so stale refresh state cannot be
accepted after the grant changes

Why:

These checks close gaps where:
- redirect URI comparisons could rely on broader Unicode normalization
than intended
- malformed or incomplete PKCE state could be treated too permissively
- concurrent or stale refresh-token use could advance the same grant
more than once

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: Claude (Opus 4.7) <noreply@anthropic.com>
Co-authored-by: Nicolas <bircni@icloud.com>

models/auth/oauth2.go

@@ -5,9 +5,8 @@ package auth
 
 import (
 	"context"
-	"crypto/sha256"
+	"crypto/subtle"
 	"encoding/base32"
-	"encoding/base64"
 	"errors"
 	"fmt"
 	"net"
@@ -24,6 +23,7 @@ import (
 
 	uuid "github.com/google/uuid"
 	"golang.org/x/crypto/bcrypt"
+	"golang.org/x/oauth2"
 	"xorm.io/builder"
 	"xorm.io/xorm"
 )
@@ -31,7 +31,10 @@ import (
 // Authorization codes should expire within 10 minutes per https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2
 const oauth2AuthorizationCodeValidity = 10 * time.Minute
 
-var ErrOAuth2AuthorizationCodeInvalidated = errors.New("oauth2 authorization code already invalidated")
+var (
+	ErrOAuth2AuthorizationCodeInvalidated = errors.New("oauth2 authorization code already invalidated")
+	ErrOAuth2GrantStaleCounter            = errors.New("oauth2 grant state changed during token refresh")
+)
 
 // OAuth2Application represents an OAuth2 client (RFC 6749)
 type OAuth2Application struct {
@@ -151,30 +154,40 @@ func (app *OAuth2Application) ContainsRedirectURI(redirectURI string) bool {
 	// https://www.rfc-editor.org/rfc/rfc6819#section-5.2.3.3
 	// https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest
 	// https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-12#section-3.1
-	contains := func(s string) bool {
+	redirectCandidates := []string{redirectURI}
-		s = strings.TrimSuffix(strings.ToLower(s), "/")
+	if !app.ConfidentialClient {
-		for _, u := range app.RedirectURIs {
+		loopbackRedirect, ok := normalizePublicClientRedirectURI(redirectURI)
-			if strings.TrimSuffix(strings.ToLower(u), "/") == s {
+		if ok {
+			redirectCandidates = append(redirectCandidates, loopbackRedirect)
+		}
+	}
+
+	for _, candidate := range redirectCandidates {
+		normalizedCandidate := normalizeRedirectURIForComparison(candidate)
+		for _, registeredURI := range app.RedirectURIs {
+			if normalizeRedirectURIForComparison(registeredURI) == normalizedCandidate {
 				return true
 			}
 		}
-		return false
 	}
-	if !app.ConfidentialClient {
+
-		uri, err := url.Parse(redirectURI)
+	return false
-		// ignore port for http loopback uris following https://datatracker.ietf.org/doc/html/rfc8252#section-7.3
+}
-		if err == nil && uri.Scheme == "http" && uri.Port() != "" {
+
-			ip := net.ParseIP(uri.Hostname())
+func normalizeRedirectURIForComparison(redirectURI string) string {
-			if ip != nil && ip.IsLoopback() {
+	return strings.TrimSuffix(util.ToLowerASCII(redirectURI), "/")
-				// strip port
+}
-				uri.Host = uri.Hostname()
+
-				if contains(uri.String()) {
+func normalizePublicClientRedirectURI(redirectURI string) (string, bool) {
-					return true
+	parsedURI, err := url.Parse(redirectURI)
-				}
+	if err != nil || parsedURI.Scheme != "http" || parsedURI.Port() == "" {
-			}
+		return "", false
-		}
 	}
-	return contains(redirectURI)
+	if ip := net.ParseIP(parsedURI.Hostname()); ip == nil || !ip.IsLoopback() {
+		return "", false
+	}
+	parsedURI.Host = parsedURI.Hostname()
+	return parsedURI.String(), true
 }
 
 // Base32 characters, but lowercased.
@@ -427,22 +440,34 @@ func (code *OAuth2AuthorizationCode) Invalidate(ctx context.Context) error {
 	return nil
 }
 
+func (code *OAuth2AuthorizationCode) requiresCodeVerifier() bool {
+	return code.CodeChallengeMethod != "" || code.CodeChallenge != ""
+}
+
+func deriveCodeChallenge(method, verifier string) (string, bool) {
+	switch method {
+	case "S256":
+		return oauth2.S256ChallengeFromVerifier(verifier), true
+	case "plain":
+		return verifier, true
+	default:
+		return "", false
+	}
+}
+
 // ValidateCodeChallenge validates the given verifier against the saved code challenge. This is part of the PKCE implementation.
 func (code *OAuth2AuthorizationCode) ValidateCodeChallenge(verifier string) bool {
-	switch code.CodeChallengeMethod {
+	if !code.requiresCodeVerifier() {
-	case "S256":
-		// base64url(SHA256(verifier)) see https://tools.ietf.org/html/rfc7636#section-4.6
-		h := sha256.Sum256([]byte(verifier))
-		hashedVerifier := base64.RawURLEncoding.EncodeToString(h[:])
-		return hashedVerifier == code.CodeChallenge
-	case "plain":
-		return verifier == code.CodeChallenge
-	case "":
 		return true
-	default:
+	}
-		// unsupported method -> return false
+	if verifier == "" || code.CodeChallengeMethod == "" {
 		return false
 	}
+	expectedChallenge, ok := deriveCodeChallenge(code.CodeChallengeMethod, verifier)
+	if !ok {
+		return false
+	}
+	return subtle.ConstantTimeCompare([]byte(expectedChallenge), []byte(code.CodeChallenge)) == 1
 }
 
 // GetOAuth2AuthorizationByCode returns an authorization by its code
@@ -510,15 +535,18 @@ func (grant *OAuth2Grant) GenerateNewAuthorizationCode(ctx context.Context, redi
 
 // IncreaseCounter increases the counter and updates the grant
 func (grant *OAuth2Grant) IncreaseCounter(ctx context.Context) error {
-	_, err := db.GetEngine(ctx).ID(grant.ID).Incr("counter").Update(new(OAuth2Grant))
+	affected, err := db.GetEngine(ctx).
+		Where("id = ?", grant.ID).
+		And("counter = ?", grant.Counter).
+		Incr("counter").
+		Update(new(OAuth2Grant))
 	if err != nil {
 		return err
 	}
-	updatedGrant, err := GetOAuth2GrantByID(ctx, grant.ID)
+	if affected == 0 {
-	if err != nil {
+		return ErrOAuth2GrantStaleCounter
-		return err
 	}
-	grant.Counter = updatedGrant.Counter
+	grant.Counter++
 	return nil
 }
 

models/auth/oauth2_test.go

@@ -12,6 +12,7 @@ import (
 	"code.gitea.io/gitea/modules/timeutil"
 
 	"github.com/stretchr/testify/assert"
+	"golang.org/x/oauth2"
 )
 
 func TestOAuth2AuthorizationCodeValidity(t *testing.T) {
@@ -104,6 +105,47 @@ func TestOAuth2Application_ContainsRedirect_Slash(t *testing.T) {
 	assert.False(t, app.ContainsRedirectURI("http://127.0.0.1/other"))
 }
 
+func TestOAuth2Application_ContainsRedirectURI_ASCIIOnlyNormalization(t *testing.T) {
+	testCases := []struct {
+		name        string
+		registered  []string
+		redirectURI string
+		allowed     bool
+	}{
+		{
+			name:        "exact-match",
+			registered:  []string{"https://signin.example.test/callback"},
+			redirectURI: "https://signin.example.test/callback",
+			allowed:     true,
+		},
+		{
+			name:        "ascii-case-insensitive",
+			registered:  []string{"https://signin.example.test/callback"},
+			redirectURI: "https://signIN.example.test/callback",
+			allowed:     true,
+		},
+		{
+			name:        "non-ascii-not-folded",
+			registered:  []string{"https://signin.example.test/callback"},
+			redirectURI: "https://signİn.example.test/callback",
+			allowed:     false,
+		},
+		{
+			name:        "loopback-strips-port",
+			registered:  []string{"http://127.0.0.1/callback"},
+			redirectURI: "http://127.0.0.1:12345/callback",
+			allowed:     true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			app := &auth_model.OAuth2Application{RedirectURIs: tc.registered}
+			assert.Equal(t, tc.allowed, app.ContainsRedirectURI(tc.redirectURI))
+		})
+	}
+}
+
 func TestOAuth2Application_ValidateClientSecret(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 	app := unittest.AssertExistsAndLoadBean(t, &auth_model.OAuth2Application{ID: 1})
@@ -181,6 +223,16 @@ func TestOAuth2Grant_IncreaseCounter(t *testing.T) {
 	unittest.AssertExistsAndLoadBean(t, &auth_model.OAuth2Grant{ID: 1, Counter: 2})
 }
 
+func TestOAuth2Grant_IncreaseCounterRejectsStaleCounter(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	grant := unittest.AssertExistsAndLoadBean(t, &auth_model.OAuth2Grant{ID: 1, Counter: 1})
+	stale := *grant
+
+	assert.NoError(t, grant.IncreaseCounter(t.Context()))
+	err := stale.IncreaseCounter(t.Context())
+	assert.ErrorIs(t, err, auth_model.ErrOAuth2GrantStaleCounter)
+}
+
 func TestOAuth2Grant_ScopeContains(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 	grant := unittest.AssertExistsAndLoadBean(t, &auth_model.OAuth2Grant{ID: 1, Scope: "openid profile"})
@@ -238,35 +290,38 @@ func TestGetOAuth2AuthorizationByCode(t *testing.T) {
 }
 
 func TestOAuth2AuthorizationCode_ValidateCodeChallenge(t *testing.T) {
-	// test plain
+	s256Verifier := "s256-verifier"
-	code := &auth_model.OAuth2AuthorizationCode{
+	s256Challenge := oauth2.S256ChallengeFromVerifier(s256Verifier)
-		CodeChallengeMethod: "plain",
+	missingVerifierChallenge := oauth2.S256ChallengeFromVerifier("verifier-not-supplied")
-		CodeChallenge:       "test123",
-	}
-	assert.True(t, code.ValidateCodeChallenge("test123"))
-	assert.False(t, code.ValidateCodeChallenge("ierwgjoergjio"))
 
-	// test S256
+	testCases := []struct {
-	code = &auth_model.OAuth2AuthorizationCode{
+		name      string
-		CodeChallengeMethod: "S256",
+		method    string
-		CodeChallenge:       "CjvyTLSdR47G5zYenDA-eDWW4lRrO8yvjcWwbD_deOg",
+		challenge string
+		verifier  string
+		valid     bool
+	}{
+		{"plain-success", "plain", "plain-secret", "plain-secret", true},
+		{"plain-failure", "plain", "plain-secret", "ierwgjoergjio", false},
+		{"s256-success", "S256", s256Challenge, s256Verifier, true},
+		{"s256-failure", "S256", s256Challenge, "wiogjerogorewngoenrgoiuenorg", false},
+		{"unsupported-method", "monkey", "foiwgjioriogeiogjerger", "foiwgjioriogeiogjerger", false},
+		{"no-pkce-configured", "", "", "", true},
+		{"s256-missing-verifier", "S256", missingVerifierChallenge, "", false},
+		{"plain-missing-verifier", "plain", "plain-secret", "", false},
+		{"missing-method-with-challenge", "", "foierjiogerogerg", "", false},
+		{"missing-method-rejects-even-matching-verifier", "", "foierjiogerogerg", "foierjiogerogerg", false},
 	}
-	assert.True(t, code.ValidateCodeChallenge("N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt"))
-	assert.False(t, code.ValidateCodeChallenge("wiogjerogorewngoenrgoiuenorg"))
 
-	// test unknown
+	for _, tc := range testCases {
-	code = &auth_model.OAuth2AuthorizationCode{
+		t.Run(tc.name, func(t *testing.T) {
-		CodeChallengeMethod: "monkey",
+			code := &auth_model.OAuth2AuthorizationCode{
-		CodeChallenge:       "foiwgjioriogeiogjerger",
+				CodeChallengeMethod: tc.method,
+				CodeChallenge:       tc.challenge,
+			}
+			assert.Equal(t, tc.valid, code.ValidateCodeChallenge(tc.verifier))
+		})
 	}
-	assert.False(t, code.ValidateCodeChallenge("foiwgjioriogeiogjerger"))
-
-	// test no code challenge
-	code = &auth_model.OAuth2AuthorizationCode{
-		CodeChallengeMethod: "",
-		CodeChallenge:       "foierjiogerogerg",
-	}
-	assert.True(t, code.ValidateCodeChallenge(""))
 }
 
 func TestOAuth2AuthorizationCode_GenerateRedirectURI(t *testing.T) {