fix(oauth): address code review feedback - add timeout, clarify comments, clean tests

Author: technicalpickles

docs/code-reviews/2025-12-10-pr188-rfc8707-resource-auto-detection.md

@@ -0,0 +1,133 @@
+# Code Review: PR #188 - Auto-detect RFC 8707 Resource Parameter for OAuth
+
+**Date**: 2025-12-10
+**PR**: https://github.com/smart-mcp-proxy/mcpproxy-go/pull/188
+**Reviewer**: Claude Code
+
+## Overview
+
+This PR implements automatic detection of the RFC 8707 `resource` parameter from RFC 9728 Protected Resource Metadata, enabling zero-configuration OAuth for providers like Runlayer/AnySource. This is a well-structured feature addition that improves user experience by eliminating manual `extra_params` configuration.
+
+## Summary of Changes
+
+| File | Type | Description |
+|------|------|-------------|
+| `internal/oauth/config.go` | New API | Added `CreateOAuthConfigWithExtraParams()` and `autoDetectResource()` |
+| `internal/oauth/discovery.go` | Enhancement | Added `DiscoverProtectedResourceMetadata()` |
+| `internal/upstream/core/connection.go` | Integration | Updated 4 call sites to use new API |
+| `cmd/mcpproxy/auth_cmd.go` | UX | Added resource status display |
+| `internal/management/diagnostics.go` | UX | Updated doctor resolution message |
+| `CLAUDE.md` | Docs | Comprehensive documentation |
+| Tests | Verification | Extensive unit and E2E tests |
+
+---
+
+## Positives
+
+### 1. Clean API Design
+The new `CreateOAuthConfigWithExtraParams()` function returns both the config and extraParams, enabling the connection layer to inject params into the authorization URL. The backward-compatible `CreateOAuthConfig()` is preserved for existing callers.
+
+### 2. Proper Priority Order
+```go
+// Resource auto-detection logic (in priority order):
+// 1. Manual extra_params.resource from config (highest priority)
+// 2. Auto-detected resource from RFC 9728 Protected Resource Metadata
+// 3. Fallback to server URL if metadata unavailable
+```
+
+This correctly preserves backward compatibility - manual overrides always win.
+
+### 3. Excellent Test Coverage
+- Unit tests for `DiscoverProtectedResourceMetadata()` with error cases
+- Unit tests for resource auto-detection and manual override
+- E2E test validating full metadata discovery flow
+- Tests for parameter merging behavior
+
+### 4. Good Documentation
+- CLAUDE.md updated with clear explanation and examples
+- Inline code comments explain RFC references
+- Diagnostic commands documented for troubleshooting
+
+---
+
+## Issues and Suggestions
+
+### 1. **Network Request in Config Creation** (Medium Risk) - FIXED
+
+```go:internal/oauth/config.go
+func autoDetectResource(serverConfig *config.ServerConfig, logger *zap.Logger) string {
+    resp, err := http.Post(serverConfig.URL, "application/json", strings.NewReader("{}"))
+```
+
+**Concern**: The `CreateOAuthConfigWithExtraParams()` function makes a synchronous HTTP POST request during config creation. This can cause:
+- Slowdown during startup if the server is slow/unreachable
+- Potential timeout issues affecting connection initialization
+- No timeout specified on the `http.Post` call
+
+**Fix**: Added 5-second timeout to preflight request client.
+
+### 2. **Fallback Logic Inconsistency** (Low Risk) - FIXED
+
+In `autoDetectResource()`:
+- On network error: returns `serverConfig.URL` (fallback)
+- On non-401 response: returns `""` (no resource)
+- On 401 without metadata URL: returns `serverConfig.URL` (fallback)
+
+The non-401 case returning empty string seems correct (server doesn't require OAuth), but the behavior isn't immediately obvious from the code.
+
+**Fix**: Added clarifying comment explaining why non-401 returns empty.
+
+### 3. **Unused Variable in E2E Test** - FIXED
+
+```go:internal/server/e2e_oauth_zero_config_test.go
+ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+defer cancel()
+```
+
+The `ctx` variable is created but never passed to the discovery function.
+
+**Fix**: Removed unused context variable and related check.
+
+### 4. **Minor: Unused import check** - FIXED
+
+```go:internal/server/e2e_oauth_zero_config_test.go
+// compile-time check to silence unused import warning
+var _ = zap.NewNop
+```
+
+**Fix**: Removed unused zap import and dummy reference.
+
+---
+
+## Security Considerations
+
+**Positive**: The implementation correctly:
+- Respects manual configuration over auto-detected values
+- Masks sensitive params in logs (existing `maskExtraParams` function)
+- Uses standard HTTP methods for discovery
+
+**Note**: The POST request with empty body `{}` to detect 401 is per MCP spec. Consider documenting this as it might appear suspicious in network logs.
+
+---
+
+## Performance Considerations
+
+The preflight POST request adds latency to connection establishment. For servers that don't advertise RFC 9728 metadata, this is overhead with no benefit. Consider:
+- Caching metadata discovery results
+- Making discovery async (non-blocking)
+
+However, this is likely acceptable for v1 since it only happens during initial connection.
+
+---
+
+## Verdict
+
+**Approved with minor fixes applied.**
+
+This is a well-implemented feature with good test coverage and documentation. The code follows project conventions and the API design is clean.
+
+### Fixes Applied:
+1. Added timeout to `http.Post` call in `autoDetectResource()`
+2. Added clarifying comment for non-401 return behavior
+3. Removed unused `ctx` variable in E2E test
+4. Removed unused zap import workaround in E2E test

internal/oauth/config.go

@@ -357,8 +357,11 @@ func CreateOAuthConfigWithExtraParams(serverConfig *config.ServerConfig, storage
 // autoDetectResource attempts to discover the RFC 8707 resource parameter.
 // Returns the detected resource URL, or server URL as fallback, or empty string on failure.
 func autoDetectResource(serverConfig *config.ServerConfig, logger *zap.Logger) string {
+	// Use a client with timeout to avoid blocking on slow/unreachable servers
+	client := &http.Client{Timeout: 5 * time.Second}
+
 	// POST is the only method guaranteed by MCP spec for the main endpoint
-	resp, err := http.Post(serverConfig.URL, "application/json", strings.NewReader("{}"))
+	resp, err := client.Post(serverConfig.URL, "application/json", strings.NewReader("{}"))
 	if err != nil {
 		logger.Debug("Failed to make preflight request for resource detection",
 			zap.String("server", serverConfig.Name),
@@ -407,8 +410,9 @@ func autoDetectResource(serverConfig *config.ServerConfig, logger *zap.Logger) s
 		return serverConfig.URL
 	}
 
-	// Non-401 response - server might not require OAuth or is accessible
-	// Don't set resource parameter in this case
+	// Non-401 response means server doesn't require authentication at this endpoint.
+	// Return empty string to avoid adding unnecessary resource parameter to OAuth flows.
+	// This is correct behavior: resource parameter is only needed for OAuth-protected servers.
 	logger.Debug("Server did not return 401, skipping resource auto-detection",
 		zap.String("server", serverConfig.Name),
 		zap.Int("status_code", resp.StatusCode))

internal/server/e2e_oauth_zero_config_test.go

@@ -1,7 +1,6 @@
 package server
 
 import (
-	"context"
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
@@ -17,9 +16,6 @@ import (
 	"go.uber.org/zap"
 )
 
-// compile-time check to silence unused import warning
-var _ = zap.NewNop
-
 // TestE2E_ZeroConfigOAuth_ResourceParameterExtraction validates that the system
 // correctly extracts resource parameters from Protected Resource Metadata (RFC 9728)
 // when no explicit OAuth configuration is provided.
@@ -213,11 +209,7 @@ func TestE2E_ProtectedResourceMetadataDiscovery(t *testing.T) {
 	}))
 	defer metadataServer.Close()
 
-	// Test direct metadata discovery
-	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
-	defer cancel()
-
-	// Use the oauth package's discovery function
+	// Test direct metadata discovery using the oauth package's discovery function
 	metadata, err := oauth.DiscoverProtectedResourceMetadata(metadataServer.URL, 5*time.Second)
 
 	require.NoError(t, err, "Metadata discovery should succeed")
@@ -235,12 +227,4 @@ func TestE2E_ProtectedResourceMetadataDiscovery(t *testing.T) {
 	t.Logf("   Resource: %s", metadata.Resource)
 	t.Logf("   Scopes: %v", metadata.ScopesSupported)
 	t.Logf("   Auth Servers: %v", metadata.AuthorizationServers)
-
-	// Ensure context wasn't cancelled
-	select {
-	case <-ctx.Done():
-		t.Fatal("Context should not be cancelled")
-	default:
-		// Context is still active, good
-	}
 }