Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 66 additions & 5 deletions config.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ package oauth

import (
"fmt"
"strconv"
"strings"

"github.com/golang-jwt/jwt/v5"
"github.com/tuannvm/oauth-mcp-proxy/provider"
)

Expand All @@ -18,6 +21,7 @@ type Config struct {
Audience string
ClientID string
ClientSecret string
Scopes []string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add documentation for Scopes field.

The Scopes field lacks documentation explaining its purpose and default behavior. Users need to understand what scopes are used when this field is empty.

-	Scopes       []string
+	// Scopes defines the OAuth2 scopes to request during authentication.
+	// When empty, defaults to ["openid", "profile", "email"] for OIDC providers.
+	Scopes       []string
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Scopes []string
// Scopes defines the OAuth2 scopes to request during authentication.
// When empty, defaults to ["openid", "profile", "email"] for OIDC providers.
Scopes []string
🤖 Prompt for AI Agents
In config.go around line 24, the Scopes []string field is undocumented; add a Go
doc comment above the field explaining what Scopes represent (the OAuth/API
permission scopes the application will request), what happens when the slice is
empty (e.g., default scopes are applied or none are requested), and any
examples/default values or precedence rules; keep the comment concise and follow
Go doc style (starts with "Scopes ...").


// Server configuration
ServerURL string // Full URL of the MCP server
Expand All @@ -31,6 +35,12 @@ type Config struct {
// Implement the Logger interface (Debug, Info, Warn, Error methods) to
// integrate with your application's logging system (e.g., zap, logrus).
Logger Logger

// Token validation configuration
SkipIssuerCheck bool
SkipAudienceCheck bool
SkipExpiryCheck bool
TokenValidationFuncs []func(claims jwt.MapClaims) error
Comment on lines +39 to +43
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add comprehensive documentation for token validation configuration.

These security-sensitive fields need detailed documentation explaining their purpose, security implications, and proper usage. The section comment "Token validation configuration" is insufficient for fields that can weaken security when misconfigured.

 	// Token validation configuration
+	// 
+	// SkipIssuerCheck bypasses issuer validation in OIDC tokens.
+	// WARNING: Only use in development or when implementing custom issuer validation.
+	// Skipping issuer checks allows tokens from any issuer to be accepted.
 	SkipIssuerCheck      bool
+	
+	// SkipAudienceCheck bypasses audience validation in OIDC tokens.
+	// WARNING: Only use in specific multi-tenant scenarios with custom audience validation.
+	// Skipping audience checks allows tokens intended for other services to be accepted.
 	SkipAudienceCheck    bool
+	
+	// SkipExpiryCheck bypasses expiration validation in OIDC tokens.
+	// WARNING: Only use for testing. Production systems should never accept expired tokens.
 	SkipExpiryCheck      bool
+	
+	// TokenValidationFuncs provides custom validation logic that runs after standard validation.
+	// Each function receives raw token claims and should return an error if validation fails.
+	// This field must be set programmatically (cannot be configured via environment variables).
+	// Use this to implement application-specific validation (e.g., custom claims, role checks).
 	TokenValidationFuncs []func(claims jwt.MapClaims) error
🤖 Prompt for AI Agents
In config.go around lines 39 to 43, the token validation configuration fields
currently have only the brief comment "Token validation configuration"; expand
this into comprehensive field-level documentation: for SkipIssuerCheck,
SkipAudienceCheck, and SkipExpiryCheck describe what each toggle does, the
security risks of enabling it, when (if ever) it may be acceptable to enable,
and recommend alternatives (e.g., validating via configuration-driven expected
issuer/audience values); for TokenValidationFuncs describe the function
signature, when these custom validators are invoked, expected behavior (return
nil on success, error on failure), threading/ordering considerations, and
guidance to avoid bypassing core checks; keep the comments concise but explicit,
and ensure they mention default/secure settings and that disabling checks should
require explicit justification.

}

// Validate validates the configuration
Expand Down Expand Up @@ -119,11 +129,15 @@ func SetupOAuth(cfg *Config) (provider.TokenValidator, error) {
func createValidator(cfg *Config, logger Logger) (provider.TokenValidator, error) {
// Convert root Config to provider.Config
providerCfg := &provider.Config{
Provider: cfg.Provider,
Issuer: cfg.Issuer,
Audience: cfg.Audience,
JWTSecret: cfg.JWTSecret,
Logger: logger,
Provider: cfg.Provider,
Issuer: cfg.Issuer,
Audience: cfg.Audience,
JWTSecret: cfg.JWTSecret,
Logger: logger,
SkipIssuerCheck: cfg.SkipIssuerCheck,
SkipAudienceCheck: cfg.SkipAudienceCheck,
SkipExpiryCheck: cfg.SkipExpiryCheck,
TokenValidatorFuncs: cfg.TokenValidationFuncs,
}

var validator provider.TokenValidator
Expand Down Expand Up @@ -217,12 +231,36 @@ func (b *ConfigBuilder) WithJWTSecret(secret []byte) *ConfigBuilder {
return b
}

// WithScopes sets the OIDC scopes
func (b *ConfigBuilder) WithScopes(scopes []string) *ConfigBuilder {
b.config.Scopes = scopes
return b
}

// WithLogger sets the logger
func (b *ConfigBuilder) WithLogger(logger Logger) *ConfigBuilder {
b.config.Logger = logger
return b
}

// WithSkipIssuerCheck sets issuer check toggle
func (b *ConfigBuilder) WithSkipIssuerCheck(skipIssuerCheck bool) *ConfigBuilder {
b.config.SkipIssuerCheck = skipIssuerCheck
return b
}

// WithSkipAudienceCheck sets audience check toggle
func (b *ConfigBuilder) WithSkipAudienceCheck(skipAudienceCheck bool) *ConfigBuilder {
b.config.SkipAudienceCheck = skipAudienceCheck
return b
}

// WithSkipExpiryCheck sets expiry check toggle
func (b *ConfigBuilder) WithSkipExpiryCheck(skipExpiryCheck bool) *ConfigBuilder {
b.config.SkipExpiryCheck = skipExpiryCheck
return b
}

// WithServerURL sets the full server URL directly
func (b *ConfigBuilder) WithServerURL(url string) *ConfigBuilder {
b.config.ServerURL = url
Expand Down Expand Up @@ -281,6 +319,12 @@ func FromEnv() (*Config, error) {

jwtSecret := getEnv("JWT_SECRET", "")

scopes := []string{}
scopesEnv := getEnv("OIDC_SCOPES", "")
if scopesEnv != "" {
scopes = strings.Split(scopesEnv, " ")
}

return NewConfigBuilder().
WithMode(getEnv("OAUTH_MODE", "")).
WithProvider(getEnv("OAUTH_PROVIDER", "")).
Expand All @@ -289,7 +333,24 @@ func FromEnv() (*Config, error) {
WithAudience(getEnv("OIDC_AUDIENCE", "")).
WithClientID(getEnv("OIDC_CLIENT_ID", "")).
WithClientSecret(getEnv("OIDC_CLIENT_SECRET", "")).
WithScopes(scopes).
WithSkipAudienceCheck(parseBoolEnv("OIDC_SKIP_AUDIENCE_CHECK", false)).
WithSkipIssuerCheck(parseBoolEnv("OIDC_SKIP_ISSUER_CHECK", false)).
WithSkipExpiryCheck(parseBoolEnv("OIDC_SKIP_EXPIRY_CHECK", false)).
WithServerURL(serverURL).
WithJWTSecret([]byte(jwtSecret)).
Build()
}

// parseBoolEnv parses a boolean environment variable
func parseBoolEnv(key string, defaultVal bool) bool {
val := getEnv(key, "")
if val == "" {
return defaultVal
}
parsed, err := strconv.ParseBool(val)
if err != nil {
return defaultVal
}
return parsed
}
9 changes: 8 additions & 1 deletion handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ type OAuth2Config struct {
Audience string
ClientID string
ClientSecret string
Scopes []string

// Server configuration
MCPHost string
Expand Down Expand Up @@ -96,7 +97,7 @@ func NewOAuth2Handler(cfg *OAuth2Config, logger Logger) *OAuth2Handler {
ClientID: cfg.ClientID,
ClientSecret: cfg.ClientSecret,
Endpoint: endpoint,
Scopes: []string{"openid", "profile", "email"},
Scopes: cfg.Scopes,
}

// Log client configuration type for debugging
Expand Down Expand Up @@ -177,6 +178,11 @@ func NewOAuth2ConfigFromConfig(cfg *Config, version string) *OAuth2Config {
mcpURL = getEnv("MCP_URL", fmt.Sprintf("%s://%s:%s", scheme, mcpHost, mcpPort))
}

scopes := cfg.Scopes
if len(scopes) == 0 {
scopes = []string{"openid", "profile", "email"}
}

return &OAuth2Config{
Enabled: true,
Mode: cfg.Mode,
Expand All @@ -186,6 +192,7 @@ func NewOAuth2ConfigFromConfig(cfg *Config, version string) *OAuth2Config {
Audience: cfg.Audience,
ClientID: cfg.ClientID,
ClientSecret: cfg.ClientSecret,
Scopes: scopes,
MCPHost: mcpHost,
MCPPort: mcpPort,
MCPURL: mcpURL,
Expand Down
4 changes: 2 additions & 2 deletions metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ func (h *OAuth2Handler) HandleOIDCDiscovery(w http.ResponseWriter, r *http.Reque
"token_endpoint_auth_methods_supported": []string{"none"},
"code_challenge_methods_supported": []string{"plain", "S256"},
"subject_types_supported": []string{"public"},
"scopes_supported": []string{"openid", "profile", "email"},
"scopes_supported": h.config.Scopes,
}

// Add provider-specific fields
Expand Down Expand Up @@ -282,7 +282,7 @@ func (h *OAuth2Handler) GetAuthorizationServerMetadata() map[string]interface{}
"grant_types_supported": []string{"authorization_code"},
"token_endpoint_auth_methods_supported": []string{"none"},
"code_challenge_methods_supported": []string{"plain", "S256"},
"scopes_supported": []string{"openid", "profile", "email"},
"scopes_supported": h.config.Scopes,
}

// Add provider-specific endpoints
Expand Down
39 changes: 26 additions & 13 deletions provider/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,15 @@ type Logger interface {

// Config holds OAuth configuration (subset needed by provider)
type Config struct {
Provider string
Issuer string
Audience string
JWTSecret []byte
Logger Logger
Provider string
Issuer string
Audience string
JWTSecret []byte
Logger Logger
SkipIssuerCheck bool
SkipAudienceCheck bool
SkipExpiryCheck bool
TokenValidatorFuncs []func(claims jwt.MapClaims) error
Comment on lines +33 to +41
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add documentation for security-sensitive configuration fields.

The newly added fields (SkipIssuerCheck, SkipAudienceCheck, SkipExpiryCheck, TokenValidatorFuncs) lack documentation explaining their purpose, usage, and security implications. These are security-critical settings that bypass standard OAuth2/OIDC validation checks.

Add documentation comments explaining:

  • What each Skip* flag does
  • When these flags should be used (and when they shouldn't)
  • Security risks of bypassing these checks
  • Example use cases for TokenValidatorFuncs

Example:

// Config holds OAuth configuration (subset needed by provider)
type Config struct {
	Provider            string
	Issuer              string
	Audience            string
	JWTSecret           []byte
	Logger              Logger
	
	// SkipIssuerCheck bypasses issuer validation in OIDC tokens.
	// WARNING: Only use this in development or when implementing custom issuer validation.
	// Skipping issuer checks allows tokens from any issuer to be accepted.
	SkipIssuerCheck     bool
	
	// SkipAudienceCheck bypasses audience validation in OIDC tokens.
	// WARNING: Only use this in specific multi-tenant scenarios with custom audience validation.
	// Skipping audience checks allows tokens intended for other services to be accepted.
	SkipAudienceCheck   bool
	
	// SkipExpiryCheck bypasses expiration validation in OIDC tokens.
	// WARNING: Only use this for testing. Production systems should never accept expired tokens.
	SkipExpiryCheck     bool
	
	// TokenValidatorFuncs provides custom validation logic that runs after standard OIDC validation.
	// Each function receives the raw token claims and should return an error if validation fails.
	// Use this to implement application-specific claim validation (e.g., checking custom claims, roles).
	TokenValidatorFuncs []func(claims jwt.MapClaims) error
}
🤖 Prompt for AI Agents
In provider/provider.go around lines 33 to 41, the new fields SkipIssuerCheck,
SkipAudienceCheck, SkipExpiryCheck and TokenValidatorFuncs lack documentation;
add Go doc comments above each field describing what the flag does, when it may
be used and when it must not (development-only, testing, or very specific
trusted multi-tenant scenarios), and the security risks of bypassing standard
OIDC validations (e.g., accepting tokens from any issuer, tokens intended for
other audiences, or expired tokens). For TokenValidatorFuncs document that these
are post-validation hooks that receive jwt.MapClaims and must return an error on
failure, give a short example use case (custom claim or role checks), and
recommend default behavior (do not set skip flags in production; prefer
TokenValidatorFuncs for application-specific checks). Ensure the comments use
clear WARNING language and appear immediately above each field.

}

// TokenValidator interface for OAuth token validation
Expand All @@ -52,10 +56,11 @@ type HMACValidator struct {

// OIDCValidator validates JWT tokens using OIDC/JWKS (Okta, Google, Azure)
type OIDCValidator struct {
verifier *oidc.IDTokenVerifier
provider *oidc.Provider
audience string
logger Logger
verifier *oidc.IDTokenVerifier
provider *oidc.Provider
audience string
TokenValidatorFuncs []func(claims jwt.MapClaims) error
logger Logger
}

// Initialize sets up the HMAC validator with JWT secret and audience
Expand Down Expand Up @@ -90,7 +95,6 @@ func (v *HMACValidator) ValidateToken(ctx context.Context, tokenString string) (
}
return []byte(v.secret), nil
})

if err != nil {
return nil, fmt.Errorf("failed to parse and validate token: %w", err)
}
Expand Down Expand Up @@ -204,15 +208,16 @@ func (v *OIDCValidator) Initialize(cfg *Config) error {
verifier := provider.Verifier(&oidc.Config{
ClientID: cfg.Audience, // Note: go-oidc uses ClientID field for audience validation - see https://github.com/coreos/go-oidc/blob/v3/oidc/verify.go#L85
SupportedSigningAlgs: []string{oidc.RS256, oidc.ES256},
SkipClientIDCheck: false, // Always validate if ClientID is provided
SkipExpiryCheck: false, // Verify expiration
SkipIssuerCheck: false, // Verify issuer
SkipClientIDCheck: cfg.SkipAudienceCheck,
SkipExpiryCheck: cfg.SkipExpiryCheck,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Logic: SkipAudienceCheck never disables audience validation because ValidateToken still calls validateAudience unconditionally. Please gate that call (or skip it) when the flag is true so the new config option has an effect.

SkipIssuerCheck: cfg.SkipIssuerCheck,
})

v.logger.Info("OAuth: OIDC validator initialized with audience validation: %s", cfg.Audience)

v.provider = provider
v.verifier = verifier
v.TokenValidatorFuncs = cfg.TokenValidatorFuncs
return nil
}

Expand Down Expand Up @@ -261,6 +266,14 @@ func (v *OIDCValidator) ValidateToken(ctx context.Context, tokenString string) (
return nil, fmt.Errorf("audience validation failed: %w", err)
}

// Run extra validation functions
for _, fn := range v.TokenValidatorFuncs {
err := fn(rawClaims)
if err != nil {
return nil, fmt.Errorf("validation function failed with error: %w", err)
}
}

return &User{
Subject: claims.Subject,
Username: claims.PreferredUsername,
Expand Down
Loading