Skip to content

Commit cd57b6a

Browse files
appleboyappleboy
authored
refactor(security): fix info disclosure, TOCTOU race, and reduce parameter sprawl (#146)
- Replace err.Error() with generic messages in device code and auth code exchange error responses to prevent internal detail leakage - Add atomic AuthorizeDeviceCode store method using WHERE authorized=false to prevent TOCTOU race condition in concurrent device code authorization - Introduce CreateAuthorizationCodeParams struct to replace 9 positional parameters in CreateAuthorizationCode - Simplify issueCodeAndRedirect from 8 parameters to 4 by fully populating AuthorizationRequest with CodeChallenge in ValidateAuthorizationRequest - Add explicit error mapping for all authorization code exchange sentinel errors in token handler
1 parent 067737d commit cd57b6a

10 files changed

Lines changed: 246 additions & 105 deletions

File tree

‎internal/core/store.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ type DeviceCodeStore interface {
6262
GetDeviceCodesByID(deviceCodeID string) ([]*models.DeviceCode, error)
6363
GetDeviceCodeByUserCode(userCode string) (*models.DeviceCode, error)
6464
UpdateDeviceCode(dc *models.DeviceCode) error
65+
AuthorizeDeviceCode(id int64, userID string) error
6566
DeleteDeviceCodeByID(id int64) error
6667
}
6768

‎internal/handlers/authorization.go‎

Lines changed: 21 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ func (h *AuthorizationHandler) ShowAuthorizePage(c *gin.Context) {
5656

5757
// Validate the authorization request parameters
5858
req, err := h.authorizationService.ValidateAuthorizationRequest(
59-
clientID, redirectURI, responseType, scope, codeChallengeMethod, nonce,
59+
clientID, redirectURI, responseType, scope, codeChallenge, codeChallengeMethod, nonce,
6060
)
6161
if err != nil {
6262
h.redirectWithError(c, redirectURI, state, oauthErrorCode(err), err.Error())
@@ -77,16 +77,7 @@ func (h *AuthorizationHandler) ShowAuthorizePage(c *gin.Context) {
7777
if h.config.ConsentRemember {
7878
existing, _ := h.authorizationService.GetUserAuthorization(userIDStr, req.Client.ID)
7979
if existing != nil && util.IsScopeSubset(existing.Scopes, req.Scopes) {
80-
h.issueCodeAndRedirect(
81-
c,
82-
req,
83-
userIDStr,
84-
redirectURI,
85-
state,
86-
nonce,
87-
codeChallenge,
88-
codeChallengeMethod,
89-
)
80+
h.issueCodeAndRedirect(c, req, userIDStr, state)
9081
return
9182
}
9283
}
@@ -99,13 +90,13 @@ func (h *AuthorizationHandler) ShowAuthorizePage(c *gin.Context) {
9990
ClientID: req.Client.ClientID,
10091
ClientName: req.Client.ClientName,
10192
ClientDescription: req.Client.Description,
102-
RedirectURI: redirectURI,
93+
RedirectURI: req.RedirectURI,
10394
Scopes: req.Scopes,
10495
ScopeList: strings.Fields(req.Scopes),
10596
State: state,
106-
Nonce: nonce,
107-
CodeChallenge: codeChallenge,
108-
CodeChallengeMethod: codeChallengeMethod,
97+
Nonce: req.Nonce,
98+
CodeChallenge: req.CodeChallenge,
99+
CodeChallengeMethod: req.CodeChallengeMethod,
109100
}))
110101
}
111102

@@ -139,7 +130,7 @@ func (h *AuthorizationHandler) HandleAuthorize(c *gin.Context) {
139130

140131
// Re-validate request on POST to prevent parameter tampering
141132
req, err := h.authorizationService.ValidateAuthorizationRequest(
142-
clientID, redirectURI, "code", scope, codeChallengeMethod, nonce,
133+
clientID, redirectURI, "code", scope, codeChallenge, codeChallengeMethod, nonce,
143134
)
144135
if err != nil {
145136
h.redirectWithError(c, redirectURI, state, oauthErrorCode(err), err.Error())
@@ -160,40 +151,40 @@ func (h *AuthorizationHandler) HandleAuthorize(c *gin.Context) {
160151
return
161152
}
162153

163-
h.issueCodeAndRedirect(
164-
c, req, userIDStr, redirectURI, state, nonce, codeChallenge, codeChallengeMethod,
165-
)
154+
h.issueCodeAndRedirect(c, req, userIDStr, state)
166155
}
167156

168157
// issueCodeAndRedirect generates an authorization code and redirects to the client's redirect_uri.
169158
func (h *AuthorizationHandler) issueCodeAndRedirect(
170159
c *gin.Context,
171160
req *services.AuthorizationRequest,
172-
userID, redirectURI, state, nonce, codeChallenge, codeChallengeMethod string,
161+
userID, state string,
173162
) {
174163
plainCode, _, err := h.authorizationService.CreateAuthorizationCode(
175164
c.Request.Context(),
176-
req.Client.ID,
177-
req.Client.ClientID,
178-
userID,
179-
redirectURI,
180-
req.Scopes,
181-
codeChallenge,
182-
codeChallengeMethod,
183-
nonce,
165+
services.CreateAuthorizationCodeParams{
166+
ApplicationID: req.Client.ID,
167+
ClientID: req.Client.ClientID,
168+
UserID: userID,
169+
RedirectURI: req.RedirectURI,
170+
Scopes: req.Scopes,
171+
CodeChallenge: req.CodeChallenge,
172+
CodeChallengeMethod: req.CodeChallengeMethod,
173+
Nonce: req.Nonce,
174+
},
184175
)
185176
if err != nil {
186177
h.redirectWithError(
187178
c,
188-
redirectURI,
179+
req.RedirectURI,
189180
state,
190181
errServerError,
191182
"Failed to generate authorization code",
192183
)
193184
return
194185
}
195186

196-
u, err := url.Parse(redirectURI)
187+
u, err := url.Parse(req.RedirectURI)
197188
if err != nil {
198189
c.JSON(http.StatusInternalServerError, gin.H{"error": "invalid redirect_uri"})
199190
return

‎internal/handlers/device.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package handlers
22

33
import (
44
"errors"
5+
"log"
56
"net/http"
67

78
"github.com/go-authgate/authgate/internal/config"
@@ -21,6 +22,8 @@ func deviceCodeErrorMessage(err error) string {
2122
return "User code not found"
2223
case errors.Is(err, services.ErrDeviceCodeExpired):
2324
return "Code has expired, please request a new one"
25+
case errors.Is(err, services.ErrDeviceCodeAlreadyAuthorized):
26+
return "This code has already been authorized"
2427
default:
2528
return "Invalid or expired code"
2629
}
@@ -124,7 +127,13 @@ func (h *DeviceHandler) DeviceCodeRequest(c *gin.Context) {
124127
)
125128
return
126129
}
127-
respondOAuthError(c, http.StatusInternalServerError, errServerError, err.Error())
130+
log.Printf("[device] device code generation error: %v", err)
131+
respondOAuthError(
132+
c,
133+
http.StatusInternalServerError,
134+
errServerError,
135+
"An internal error occurred",
136+
)
128137
return
129138
}
130139

‎internal/handlers/token.go‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -491,10 +491,28 @@ func (h *TokenHandler) handleAuthorizationCodeGrant(c *gin.Context) {
491491
)
492492
if err != nil {
493493
errCode := errInvalidGrant
494-
if errors.Is(err, services.ErrUnauthorizedClient) {
494+
var description string
495+
switch {
496+
case errors.Is(err, services.ErrUnauthorizedClient):
495497
errCode = errUnauthorizedClient
498+
description = "Client authentication failed"
499+
case errors.Is(err, services.ErrAuthCodeNotFound):
500+
description = "Authorization code is invalid or expired"
501+
case errors.Is(err, services.ErrAuthCodeExpired):
502+
description = "Authorization code has expired"
503+
case errors.Is(err, services.ErrAuthCodeAlreadyUsed):
504+
description = "Authorization code has already been used"
505+
case errors.Is(err, services.ErrInvalidRedirectURI):
506+
description = "Redirect URI does not match"
507+
case errors.Is(err, services.ErrPKCERequired):
508+
description = "PKCE code_verifier is required for public clients"
509+
case errors.Is(err, services.ErrInvalidCodeVerifier):
510+
description = "PKCE code_verifier validation failed"
511+
default:
512+
log.Printf("[token] authorization code exchange error: %v", err)
513+
description = "An internal error occurred"
496514
}
497-
respondOAuthError(c, http.StatusBadRequest, errCode, err.Error())
515+
respondOAuthError(c, http.StatusBadRequest, errCode, description)
498516
return
499517
}
500518

‎internal/mocks/mock_store.go‎

Lines changed: 28 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/services/authorization.go‎

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ func NewAuthorizationService(
7474
// ValidateAuthorizationRequest validates all parameters of an incoming authorization request.
7575
// Returns the parsed AuthorizationRequest on success.
7676
func (s *AuthorizationService) ValidateAuthorizationRequest(
77-
clientID, redirectURI, responseType, scope, codeChallengeMethod, nonce string,
77+
clientID, redirectURI, responseType, scope, codeChallenge, codeChallengeMethod, nonce string,
7878
) (*AuthorizationRequest, error) {
7979
// 1. response_type must be "code"
8080
if responseType != "code" {
@@ -126,17 +126,29 @@ func (s *AuthorizationService) ValidateAuthorizationRequest(
126126
Client: client,
127127
RedirectURI: redirectURI,
128128
Scopes: scope,
129+
CodeChallenge: codeChallenge,
129130
CodeChallengeMethod: codeChallengeMethod,
130131
Nonce: nonce,
131132
}, nil
132133
}
133134

135+
// CreateAuthorizationCodeParams bundles the inputs for authorization code creation.
136+
type CreateAuthorizationCodeParams struct {
137+
ApplicationID int64
138+
ClientID string
139+
UserID string
140+
RedirectURI string
141+
Scopes string
142+
CodeChallenge string
143+
CodeChallengeMethod string
144+
Nonce string
145+
}
146+
134147
// CreateAuthorizationCode generates a one-time authorization code and saves it to the database.
135148
// Returns the plaintext code (to be sent in the redirect) and the stored record.
136149
func (s *AuthorizationService) CreateAuthorizationCode(
137150
ctx context.Context,
138-
applicationID int64,
139-
clientID, userID, redirectURI, scopes, codeChallenge, codeChallengeMethod, nonce string,
151+
params CreateAuthorizationCodeParams,
140152
) (plainCode string, record *models.AuthorizationCode, err error) {
141153
// Generate 32 cryptographically random bytes (256-bit entropy)
142154
rawBytes, err := util.CryptoRandomBytes(32)
@@ -153,14 +165,14 @@ func (s *AuthorizationService) CreateAuthorizationCode(
153165
UUID: uuid.New().String(),
154166
CodeHash: codeHash,
155167
CodePrefix: codePrefix,
156-
ApplicationID: applicationID,
157-
ClientID: clientID,
158-
UserID: userID,
159-
RedirectURI: redirectURI,
160-
Scopes: scopes,
161-
CodeChallenge: codeChallenge,
162-
CodeChallengeMethod: codeChallengeMethod,
163-
Nonce: nonce,
168+
ApplicationID: params.ApplicationID,
169+
ClientID: params.ClientID,
170+
UserID: params.UserID,
171+
RedirectURI: params.RedirectURI,
172+
Scopes: params.Scopes,
173+
CodeChallenge: params.CodeChallenge,
174+
CodeChallengeMethod: params.CodeChallengeMethod,
175+
Nonce: params.Nonce,
164176
ExpiresAt: time.Now().Add(s.config.AuthCodeExpiration),
165177
}
166178

@@ -172,15 +184,15 @@ func (s *AuthorizationService) CreateAuthorizationCode(
172184
s.auditService.Log(ctx, AuditLogEntry{
173185
EventType: models.EventAuthorizationCodeGenerated,
174186
Severity: models.SeverityInfo,
175-
ActorUserID: userID,
187+
ActorUserID: params.UserID,
176188
ResourceType: models.ResourceAuthorization,
177189
ResourceID: record.UUID,
178190
Action: "Authorization code generated",
179191
Details: models.AuditDetails{
180-
"client_id": clientID,
181-
"scopes": scopes,
182-
"pkce": codeChallenge != "",
183-
"redirect_uri": redirectURI,
192+
"client_id": params.ClientID,
193+
"scopes": params.Scopes,
194+
"pkce": params.CodeChallenge != "",
195+
"redirect_uri": params.RedirectURI,
184196
},
185197
Success: true,
186198
})

0 commit comments

Comments
 (0)