diff --git a/error_recovery.go b/error_recovery.go index fa53fd5..832b61b 100644 --- a/error_recovery.go +++ b/error_recovery.go @@ -521,12 +521,82 @@ func (re *RetryExecutor) ExecuteWithContext(ctx context.Context, fn func() error } // Execute runs the given function with retry logic (for backward compatibility) -// Execute executes a function with retry logic (backward compatibility). +// Execute runs a function with retry logic (backward compatibility). // This method provides the same functionality as ExecuteWithContext. func (re *RetryExecutor) Execute(ctx context.Context, fn func() error) error { return re.ExecuteWithContext(ctx, fn) } +// singleUseRetryableErrors are error fragments that prove the request was NEVER +// sent to the server (dial/connect failures). Retrying those is safe. +var singleUseRetryableErrors = []string{ + "connection refused", + "network unreachable", + "no route to host", + "temporary failure", +} + +// ExecuteSingleUseWithContext retries only on errors that prove the request was +// never sent (connect/dial failures). It intentionally does NOT retry on +// "timeout": for a single-use operation such as the authorization-code +// exchange, a timeout may mean the provider already consumed the one-time code, +// so re-sending it would surface invalid_grant permanently even though the +// first attempt succeeded. +func (re *RetryExecutor) ExecuteSingleUseWithContext(ctx context.Context, fn func() error) error { + re.RecordRequest() + var lastErr error + + for attempt := 1; attempt <= re.config.MaxAttempts; attempt++ { + err := fn() + if err == nil { + if attempt > 1 { + re.LogInfo("Operation succeeded after %d attempts", attempt) + } + re.RecordSuccess() + return nil + } + + lastErr = err + + if !isSubstringMatch(err.Error(), singleUseRetryableErrors) { + re.RecordFailure() + return err + } + + if attempt == re.config.MaxAttempts { + re.RecordFailure() + break + } + + delay := re.calculateDelay(attempt) + if attempt == 1 || attempt%3 == 0 { + re.LogDebug("Retrying single-use operation after %v (attempt %d/%d): %v", + delay, attempt, re.config.MaxAttempts, err) + } + + select { + case <-ctx.Done(): + re.RecordFailure() + return ctx.Err() + case <-time.After(delay): + } + } + + finalErr := fmt.Errorf("operation failed after %d attempts: %w", re.config.MaxAttempts, lastErr) + return finalErr +} + +// isSubstringMatch reports whether s contains any of the given fragments. +func isSubstringMatch(s string, fragments []string) bool { + lower := strings.ToLower(s) + for _, frag := range fragments { + if strings.Contains(lower, frag) { + return true + } + } + return false +} + // isRetryableError checks if an error should trigger a retry // isRetryableError determines if an error should trigger a retry attempt. // Checks error message against configured retryable error patterns. diff --git a/helpers.go b/helpers.go index c19b235..7446729 100644 --- a/helpers.go +++ b/helpers.go @@ -186,7 +186,9 @@ func (t *TraefikOidc) exchangeTokens(ctx context.Context, grantType string, code } var tokenResponse TokenResponse - if err := json.NewDecoder(resp.Body).Decode(&tokenResponse); err != nil { + // Cap the decoded body so a corrupted or malicious oversized 200-body cannot + // drive an unbounded read/allocation. Token responses are small (a few KB). + if err := json.NewDecoder(io.LimitReader(resp.Body, 1<<20)).Decode(&tokenResponse); err != nil { return nil, fmt.Errorf("failed to decode token response: %w", err) } diff --git a/metadata_cache.go b/metadata_cache.go index b384e28..c5792e3 100644 --- a/metadata_cache.go +++ b/metadata_cache.go @@ -146,6 +146,14 @@ func (mc *MetadataCache) GetProviderMetadata(ctx context.Context, providerURL st return nil, fmt.Errorf("failed to decode metadata: %w", err) } + // Require the core endpoints. An incomplete discovery document (missing + // authorization_endpoint / token_endpoint) would otherwise be accepted, + // cached, and adopted as the working auth/token URL, failing at runtime + // (wrong authorize redirect, token-exchange failure) instead of at startup. + if metadata.AuthURL == "" || metadata.TokenURL == "" { + return nil, fmt.Errorf("discovery document missing required endpoints (authorization_endpoint or token_endpoint)") + } + // Pin the advertised issuer to the configured provider host. The issuer is // the trust anchor for JWT issuer validation; rejecting a mismatch here // ensures a poisoned discovery document advertising an attacker-chosen diff --git a/review_r82_test.go b/review_r82_test.go new file mode 100644 index 0000000..92a010b --- /dev/null +++ b/review_r82_test.go @@ -0,0 +1,94 @@ +package traefikoidc + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +// TestMetadataDiscovery_RequiresEndpoints is a regression: an OIDC discovery +// document missing the core endpoints (authorization_endpoint or +// token_endpoint) was accepted, cached, and adopted as the working +// auth/token URL, failing only at runtime (wrong authorize redirect, token +// exchange failure). The fix validates the endpoints at startup. +func TestMetadataDiscovery_RequiresEndpoints(t *testing.T) { + mc := NewMetadataCacheWithLogger(nil, newNoOpLogger()) + + // Discovery document with a matching issuer but no authorization/token + // endpoint: the issuer check passes, so only the missing-endpoint + // validation can reject it. + var serverURL string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]interface{}{"issuer": serverURL}) + })) + defer server.Close() + serverURL = server.URL + + _, err := mc.GetProviderMetadata(context.Background(), server.URL, server.Client()) + if err == nil { + t.Fatalf("expected error on an incomplete discovery document") + } + if !strings.Contains(err.Error(), "missing required endpoints") { + t.Fatalf("unexpected error: %v", err) + } +} + +// TestRetryExecutor_SingleUse_DoesNotRetryTimeout guards the single-use +// (authorization-code exchange) retry policy: a timeout is NOT retried, +// because the provider may already have consumed the one-time code; re-sending +// would surface invalid_grant permanently even though the first attempt +// succeeded. +func TestRetryExecutor_SingleUse_DoesNotRetryTimeout(t *testing.T) { + cfg := DefaultRetryConfig() + cfg.MaxAttempts = 3 + cfg.EnableJitter = false + cfg.InitialDelay = time.Millisecond + cfg.MaxDelay = time.Millisecond + + re := NewRetryExecutor(cfg, newNoOpLogger()) + calls := 0 + err := re.ExecuteSingleUseWithContext(context.Background(), func() error { + calls++ + return errors.New("Post https://idp.example.com/token: context deadline exceeded (Client.Timeout)") + }) + if err == nil { + t.Fatal("expected error") + } + if calls != 1 { + t.Fatalf("expected single-use request to NOT retry a timeout, got %d calls", calls) + } +} + +// TestRetryExecutor_SingleUse_RetriesPreSendError guards that single-use still +// retries errors which prove the request was never sent (connection refused), +// so a transient dial failure of the token endpoint does not lose the +// (unconsumed) authorization code. +func TestRetryExecutor_SingleUse_RetriesPreSendError(t *testing.T) { + cfg := DefaultRetryConfig() + cfg.MaxAttempts = 3 + cfg.EnableJitter = false + cfg.InitialDelay = time.Millisecond + cfg.MaxDelay = time.Millisecond + + re := NewRetryExecutor(cfg, newNoOpLogger()) + calls := 0 + err := re.ExecuteSingleUseWithContext(context.Background(), func() error { + calls++ + if calls < 3 { + return errors.New("dial tcp 10.0.0.1:443: connect: connection refused") + } + return nil + }) + if err != nil { + t.Fatalf("expected success after retrying a connection-refused error, got %v", err) + } + if calls != 3 { + t.Fatalf("expected 3 attempts for a pre-send error, got %d", calls) + } +} diff --git a/token_resilience.go b/token_resilience.go index 38c6ae6..19e4a8c 100644 --- a/token_resilience.go +++ b/token_resilience.go @@ -104,8 +104,11 @@ func NewTokenResilienceManager(config TokenResilienceConfig, logger *Logger) *To return manager } -// ExecuteTokenOperation executes a token operation with full resilience support -func (trm *TokenResilienceManager) ExecuteTokenOperation(ctx context.Context, operation string, fn func() error) error { +// ExecuteTokenOperation executes a token operation with full resilience support. +// singleUse marks operations whose request must not be re-sent after a timeout +// (e.g. the authorization-code exchange, where the one-time code may already +// have been consumed server-side) — only pre-send (connect) failures are retried. +func (trm *TokenResilienceManager) ExecuteTokenOperation(ctx context.Context, operation string, singleUse bool, fn func() error) error { if trm.logger != nil { trm.logger.Debugf("Executing token operation %s with resilience", operation) } @@ -129,8 +132,14 @@ func (trm *TokenResilienceManager) ExecuteTokenOperation(ctx context.Context, op // Wrap with retry if enabled if trm.config.RetryEnabled && trm.retryExecutor != nil { originalOp := finalOperation - finalOperation = func() error { - return trm.retryExecutor.ExecuteWithContext(ctx, originalOp) + if singleUse { + finalOperation = func() error { + return trm.retryExecutor.ExecuteSingleUseWithContext(ctx, originalOp) + } + } else { + finalOperation = func() error { + return trm.retryExecutor.ExecuteWithContext(ctx, originalOp) + } } } @@ -152,7 +161,7 @@ func (trm *TokenResilienceManager) ExecuteTokenExchange(ctx context.Context, t * operation := fmt.Sprintf("token_exchange_%s", grantType) - err = trm.ExecuteTokenOperation(ctx, operation, func() error { + err = trm.ExecuteTokenOperation(ctx, operation, grantType == "authorization_code", func() error { result, err = t.exchangeTokens(ctx, grantType, codeOrToken, redirectURL, codeVerifier) return err }) @@ -165,7 +174,7 @@ func (trm *TokenResilienceManager) ExecuteTokenRefresh(ctx context.Context, t *T var result *TokenResponse var err error - err = trm.ExecuteTokenOperation(ctx, "token_refresh", func() error { + err = trm.ExecuteTokenOperation(ctx, "token_refresh", false, func() error { // Call exchangeTokens directly to avoid recursion back to getNewTokenWithRefreshToken // which would call ExecuteTokenRefresh again, causing infinite loop (issue #67) result, err = t.exchangeTokens(ctx, "refresh_token", refreshToken, "", "")