Skip to content

Commit f3bdcc5

Browse files
lunnyCopilot
andauthored
Fix OAuth2 authorization code expiry and reuse handling (go-gitea#36797)
- set OAuth2 authorization code `ValidUntil` on creation and add expiry checks during exchange - return a specific error when codes are invalidated twice to prevent concurrent reuse - add unit tests covering validity timestamps, expiration, and double invalidation --- Generate by a coding agent with Codex 5.2 --------- Signed-off-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 57b5ed3 commit f3bdcc5

3 files changed

Lines changed: 73 additions & 4 deletions

File tree

models/auth/oauth2.go

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"net/url"
1515
"slices"
1616
"strings"
17+
"time"
1718

1819
"code.gitea.io/gitea/models/db"
1920
"code.gitea.io/gitea/modules/container"
@@ -27,6 +28,11 @@ import (
2728
"xorm.io/xorm"
2829
)
2930

31+
// Authorization codes should expire within 10 minutes per https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2
32+
const oauth2AuthorizationCodeValidity = 10 * time.Minute
33+
34+
var ErrOAuth2AuthorizationCodeInvalidated = errors.New("oauth2 authorization code already invalidated")
35+
3036
// OAuth2Application represents an OAuth2 client (RFC 6749)
3137
type OAuth2Application struct {
3238
ID int64 `xorm:"pk autoincr"`
@@ -386,6 +392,14 @@ func (code *OAuth2AuthorizationCode) TableName() string {
386392
return "oauth2_authorization_code"
387393
}
388394

395+
// IsExpired reports whether the authorization code is expired.
396+
func (code *OAuth2AuthorizationCode) IsExpired() bool {
397+
if code.ValidUntil.IsZero() {
398+
return true
399+
}
400+
return code.ValidUntil <= timeutil.TimeStampNow()
401+
}
402+
389403
// GenerateRedirectURI generates a redirect URI for a successful authorization request. State will be used if not empty.
390404
func (code *OAuth2AuthorizationCode) GenerateRedirectURI(state string) (*url.URL, error) {
391405
redirect, err := url.Parse(code.RedirectURI)
@@ -403,8 +417,14 @@ func (code *OAuth2AuthorizationCode) GenerateRedirectURI(state string) (*url.URL
403417

404418
// Invalidate deletes the auth code from the database to invalidate this code
405419
func (code *OAuth2AuthorizationCode) Invalidate(ctx context.Context) error {
406-
_, err := db.GetEngine(ctx).ID(code.ID).NoAutoCondition().Delete(code)
407-
return err
420+
affected, err := db.GetEngine(ctx).ID(code.ID).NoAutoCondition().Delete(code)
421+
if err != nil {
422+
return err
423+
}
424+
if affected == 0 {
425+
return ErrOAuth2AuthorizationCodeInvalidated
426+
}
427+
return nil
408428
}
409429

410430
// ValidateCodeChallenge validates the given verifier against the saved code challenge. This is part of the PKCE implementation.
@@ -472,13 +492,15 @@ func (grant *OAuth2Grant) GenerateNewAuthorizationCode(ctx context.Context, redi
472492
// for code scanners to grab sensitive tokens.
473493
codeSecret := "gta_" + base32Lower.EncodeToString(rBytes)
474494

495+
validUntil := time.Now().Add(oauth2AuthorizationCodeValidity)
475496
code = &OAuth2AuthorizationCode{
476497
Grant: grant,
477498
GrantID: grant.ID,
478499
RedirectURI: redirectURI,
479500
Code: codeSecret,
480501
CodeChallenge: codeChallenge,
481502
CodeChallengeMethod: codeChallengeMethod,
503+
ValidUntil: timeutil.TimeStamp(validUntil.Unix()),
482504
}
483505
if err := db.Insert(ctx, code); err != nil {
484506
return nil, err

models/auth/oauth2_test.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,45 @@ package auth_test
55

66
import (
77
"testing"
8+
"time"
89

910
auth_model "code.gitea.io/gitea/models/auth"
1011
"code.gitea.io/gitea/models/unittest"
12+
"code.gitea.io/gitea/modules/timeutil"
1113

1214
"github.com/stretchr/testify/assert"
1315
)
1416

17+
func TestOAuth2AuthorizationCodeValidity(t *testing.T) {
18+
assert.NoError(t, unittest.PrepareTestDatabase())
19+
20+
t.Run("GenerateSetsValidUntil", func(t *testing.T) {
21+
grant := unittest.AssertExistsAndLoadBean(t, &auth_model.OAuth2Grant{ID: 1})
22+
expectedValidUntil := timeutil.TimeStamp(time.Now().Unix() + 600)
23+
code, err := grant.GenerateNewAuthorizationCode(t.Context(), "http://127.0.0.1/", "", "")
24+
assert.NoError(t, err)
25+
assert.Equal(t, expectedValidUntil, code.ValidUntil)
26+
assert.False(t, code.IsExpired())
27+
assert.NoError(t, code.Invalidate(t.Context()))
28+
})
29+
30+
t.Run("Expired", func(t *testing.T) {
31+
defer timeutil.MockSet(time.Unix(2, 0).UTC())()
32+
33+
code := &auth_model.OAuth2AuthorizationCode{ValidUntil: timeutil.TimeStamp(1)}
34+
assert.True(t, code.IsExpired())
35+
})
36+
37+
t.Run("InvalidateTwice", func(t *testing.T) {
38+
code, err := auth_model.GetOAuth2AuthorizationByCode(t.Context(), "authcode")
39+
assert.NoError(t, err)
40+
if assert.NotNil(t, code) {
41+
assert.NoError(t, code.Invalidate(t.Context()))
42+
assert.ErrorIs(t, code.Invalidate(t.Context()), auth_model.ErrOAuth2AuthorizationCodeInvalidated)
43+
}
44+
})
45+
}
46+
1547
func TestOAuth2Application_GenerateClientSecret(t *testing.T) {
1648
assert.NoError(t, unittest.PrepareTestDatabase())
1749
app := unittest.AssertExistsAndLoadBean(t, &auth_model.OAuth2Application{ID: 1})

routers/web/auth/oauth2_provider.go

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package auth
55

66
import (
7+
"errors"
78
"fmt"
89
"html"
910
"html/template"
@@ -613,6 +614,14 @@ func handleAuthorizationCode(ctx *context.Context, form forms.AccessTokenForm, s
613614
})
614615
return
615616
}
617+
if authorizationCode.IsExpired() {
618+
_ = authorizationCode.Invalidate(ctx)
619+
handleAccessTokenError(ctx, oauth2_provider.AccessTokenError{
620+
ErrorCode: oauth2_provider.AccessTokenErrorCodeInvalidGrant,
621+
ErrorDescription: "authorization code expired",
622+
})
623+
return
624+
}
616625
// check if code verifier authorizes the client, PKCE support
617626
if !authorizationCode.ValidateCodeChallenge(form.CodeVerifier) {
618627
handleAccessTokenError(ctx, oauth2_provider.AccessTokenError{
@@ -631,9 +640,15 @@ func handleAuthorizationCode(ctx *context.Context, form forms.AccessTokenForm, s
631640
}
632641
// remove token from database to deny duplicate usage
633642
if err := authorizationCode.Invalidate(ctx); err != nil {
643+
errDescription := "cannot process your request"
644+
errCode := oauth2_provider.AccessTokenErrorCodeInvalidRequest
645+
if errors.Is(err, auth.ErrOAuth2AuthorizationCodeInvalidated) {
646+
errDescription = "authorization code already used"
647+
errCode = oauth2_provider.AccessTokenErrorCodeInvalidGrant
648+
}
634649
handleAccessTokenError(ctx, oauth2_provider.AccessTokenError{
635-
ErrorCode: oauth2_provider.AccessTokenErrorCodeInvalidRequest,
636-
ErrorDescription: "cannot proceed your request",
650+
ErrorCode: errCode,
651+
ErrorDescription: errDescription,
637652
})
638653
return
639654
}

0 commit comments

Comments
 (0)