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

models/auth/oauth2.go

@@ -5,9 +5,8 @@ package auth
 
 import (
 	"context"
-	"crypto/sha256"
+	"crypto/subtle"
 	"encoding/base32"
-	"encoding/base64"
 	"errors"
 	"fmt"
 	"net"
@@ -24,14 +23,18 @@ import (
 
 	uuid "github.com/google/uuid"
 	"golang.org/x/crypto/bcrypt"
+	"golang.org/x/oauth2"
 	"xorm.io/builder"
 	"xorm.io/xorm"
 )
 
 // 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 {
-		s = strings.TrimSuffix(strings.ToLower(s), "/")
-		for _, u := range app.RedirectURIs {
-			if strings.TrimSuffix(strings.ToLower(u), "/") == s {
-				return true
-			}
+	redirectCandidates := []string{redirectURI}
+	if !app.ConfidentialClient {
+		loopbackRedirect, ok := normalizePublicClientRedirectURI(redirectURI)
+		if ok {
+			redirectCandidates = append(redirectCandidates, loopbackRedirect)
 		}
-		return false
 	}
-	if !app.ConfidentialClient {
-		uri, err := url.Parse(redirectURI)
-		// 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())
-			if ip != nil && ip.IsLoopback() {
-				// strip port
-				uri.Host = uri.Hostname()
-				if contains(uri.String()) {
-					return true
-				}
+
+	for _, candidate := range redirectCandidates {
+		normalizedCandidate := normalizeRedirectURIForComparison(candidate)
+		for _, registeredURI := range app.RedirectURIs {
+			if normalizeRedirectURIForComparison(registeredURI) == normalizedCandidate {
+				return true
 			}
 		}
 	}
-	return contains(redirectURI)
+
+	return false
+}
+
+func normalizeRedirectURIForComparison(redirectURI string) string {
+	return strings.TrimSuffix(util.ToLowerASCII(redirectURI), "/")
+}
+
+func normalizePublicClientRedirectURI(redirectURI string) (string, bool) {
+	parsedURI, err := url.Parse(redirectURI)
+	if err != nil || parsedURI.Scheme != "http" || parsedURI.Port() == "" {
+		return "", false
+	}
+	if ip := net.ParseIP(parsedURI.Hostname()); ip == nil || !ip.IsLoopback() {
+		return "", false
+	}
+	parsedURI.Host = parsedURI.Hostname()
+	return parsedURI.String(), true
 }
 
 // Base32 characters, but lowercased.
@@ -424,22 +437,34 @@ func (code *OAuth2AuthorizationCode) Invalidate(ctx context.Context) error {
 	return nil
 }
 
-// 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 {
+func (code *OAuth2AuthorizationCode) requiresCodeVerifier() bool {
+	return code.CodeChallengeMethod != "" || code.CodeChallenge != ""
+}
+
+func deriveCodeChallenge(method, verifier string) (string, bool) {
+	switch method {
 	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
+		return oauth2.S256ChallengeFromVerifier(verifier), true
 	case "plain":
-		return verifier == code.CodeChallenge
-	case "":
-		return true
+		return verifier, true
 	default:
-		// unsupported method -> return false
+		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 {
+	if !code.requiresCodeVerifier() {
+		return true
+	}
+	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
@@ -504,15 +529,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 err != nil {
-		return err
+	if affected == 0 {
+		return ErrOAuth2GrantStaleCounter
 	}
-	grant.Counter = updatedGrant.Counter
+	grant.Counter++
 	return nil
 }
 

models/auth/oauth2_test.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/oauth2"
 )
 
 func TestOAuth2AuthorizationCode(t *testing.T) {
@@ -116,6 +117,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})
@@ -193,6 +235,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"})
@@ -237,35 +289,38 @@ func TestRevokeOAuth2Grant(t *testing.T) {
 //////////////////// Authorization Code
 
 func TestOAuth2AuthorizationCode_ValidateCodeChallenge(t *testing.T) {
-	// test plain
-	code := &auth_model.OAuth2AuthorizationCode{
-		CodeChallengeMethod: "plain",
-		CodeChallenge:       "test123",
-	}
-	assert.True(t, code.ValidateCodeChallenge("test123"))
-	assert.False(t, code.ValidateCodeChallenge("ierwgjoergjio"))
-
-	// test S256
-	code = &auth_model.OAuth2AuthorizationCode{
-		CodeChallengeMethod: "S256",
-		CodeChallenge:       "CjvyTLSdR47G5zYenDA-eDWW4lRrO8yvjcWwbD_deOg",
-	}
-	assert.True(t, code.ValidateCodeChallenge("N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt"))
-	assert.False(t, code.ValidateCodeChallenge("wiogjerogorewngoenrgoiuenorg"))
-
-	// test unknown
-	code = &auth_model.OAuth2AuthorizationCode{
-		CodeChallengeMethod: "monkey",
-		CodeChallenge:       "foiwgjioriogeiogjerger",
+	s256Verifier := "s256-verifier"
+	s256Challenge := oauth2.S256ChallengeFromVerifier(s256Verifier)
+	missingVerifierChallenge := oauth2.S256ChallengeFromVerifier("verifier-not-supplied")
+
+	testCases := []struct {
+		name      string
+		method    string
+		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.False(t, code.ValidateCodeChallenge("foiwgjioriogeiogjerger"))
 
-	// test no code challenge
-	code = &auth_model.OAuth2AuthorizationCode{
-		CodeChallengeMethod: "",
-		CodeChallenge:       "foierjiogerogerg",
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			code := &auth_model.OAuth2AuthorizationCode{
+				CodeChallengeMethod: tc.method,
+				CodeChallenge:       tc.challenge,
+			}
+			assert.Equal(t, tc.valid, code.ValidateCodeChallenge(tc.verifier))
+		})
 	}
-	assert.True(t, code.ValidateCodeChallenge(""))
 }
 
 func TestOAuth2AuthorizationCode_GenerateRedirectURI(t *testing.T) {