bugfix: OAuth Discovery Robustness, Logging, and Test Coverage

.github/PULL_REQUEST_TEMPLATE.md

@@ -0,0 +1,6 @@
+**What I did**
+
+**Related issue**
+<!-- If this is a bug fix, make sure your description includes "fixes #xxxx", or "closes #xxxx" -->
+
+**(not mandatory) A picture of a cute animal, if possible in relation to what you did**

.github/workflows/ci.yml

@@ -0,0 +1,35 @@
+name: CI
+
+permissions:
+  contents: read
+
+on:
+  pull_request:
+  push:
+    branches:
+      - main
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4
+
+      - name: Set up Docker Buildx
+        uses: docker/setup-buildx-action@v3
+
+      - name: Run tests
+        run: make test
+
+  lint:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4
+
+      - name: Set up Docker Buildx
+        uses: docker/setup-buildx-action@v3
+
+      - name: Run linter for Linux
+        run: make lint-linux

dcr.go

@@ -9,6 +9,8 @@ import (
 	"net/http"
 )
 
+const DefaultRedirectURI = "https://mcp.docker.com/oauth/callback"
+
 // PerformDCR performs Dynamic Client Registration with the authorization server
 // Returns client credentials for the registered public client
 //
@@ -23,10 +25,8 @@ func PerformDCR(ctx context.Context, discovery *Discovery, serverName string) (*
 
 	// Build DCR request for PUBLIC client
 	registration := DCRRequest{
-		ClientName: fmt.Sprintf("MCP Gateway - %s", serverName),
-		RedirectURIs: []string{
-			"https://mcp.docker.com/oauth/callback", // mcp-oauth proxy callback only
-		},
+		ClientName:              fmt.Sprintf("MCP Gateway - %s", serverName),
+		RedirectURIs:            []string{DefaultRedirectURI},
 		TokenEndpointAuthMethod: "none", // PUBLIC client (no client secret)
 		GrantTypes:              []string{"authorization_code", "refresh_token"},
 		ResponseTypes:           []string{"code"},
@@ -41,7 +41,6 @@ func PerformDCR(ctx context.Context, discovery *Discovery, serverName string) (*
 	// Add requested scopes if provided
 	if len(discovery.Scopes) > 0 {
 		registration.Scope = joinScopes(discovery.Scopes)
-	} else {
 	}
 
 	// Marshal the registration request

dcr_test.go

@@ -0,0 +1,95 @@
+package oauth
+
+import (
+	"context"
+	"encoding/json"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+// TestPerformDCR_PublicClient verifies Dynamic Client Registration
+// for public clients (no client secret)
+func TestPerformDCR_PublicClient(t *testing.T) {
+	var capturedRequest *DCRRequest
+
+	// Mock registration endpoint
+	regServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// Capture and verify the request
+		body, _ := io.ReadAll(r.Body)
+		_ = json.Unmarshal(body, &capturedRequest)
+
+		// Return successful registration response
+		_ = json.NewEncoder(w).Encode(DCRResponse{
+			ClientID:                "test-client-id-123",
+			TokenEndpointAuthMethod: "none",
+			GrantTypes:              []string{"authorization_code", "refresh_token"},
+			RedirectURIs:            []string{"https://mcp.docker.com/oauth/callback"},
+		})
+	}))
+	defer regServer.Close()
+
+	// Create discovery with registration endpoint
+	discovery := &Discovery{
+		RegistrationEndpoint:  regServer.URL,
+		AuthorizationEndpoint: "https://auth.example.com/authorize",
+		TokenEndpoint:         "https://auth.example.com/token",
+		ResourceURL:           "https://api.example.com",
+		Scopes:                []string{"read", "write"},
+	}
+
+	// Perform DCR
+	creds, err := PerformDCR(context.Background(), discovery, "test-server")
+	// Verify no error
+	if err != nil {
+		t.Fatalf("DCR failed: %v", err)
+	}
+
+	// Verify credentials
+	if creds.ClientID != "test-client-id-123" {
+		t.Errorf("Expected ClientID=test-client-id-123, got %s", creds.ClientID)
+	}
+	if !creds.IsPublic {
+		t.Error("Expected IsPublic=true for public client")
+	}
+	if creds.ServerURL != "https://api.example.com" {
+		t.Errorf("Expected ServerURL=https://api.example.com, got %s", creds.ServerURL)
+	}
+
+	// Verify DCR request was correct
+	if capturedRequest == nil {
+		t.Fatal("DCR request not captured")
+	}
+	if capturedRequest.TokenEndpointAuthMethod != "none" {
+		t.Errorf("Expected token_endpoint_auth_method=none for public client, got %s", capturedRequest.TokenEndpointAuthMethod)
+	}
+	if len(capturedRequest.RedirectURIs) == 0 {
+		t.Error("Expected redirect_uris to be set")
+	}
+	if len(capturedRequest.GrantTypes) == 0 {
+		t.Error("Expected grant_types to be set")
+	}
+}
+
+// TestPerformDCR_NoRegistrationEndpoint verifies error handling
+// when registration endpoint is not available
+func TestPerformDCR_NoRegistrationEndpoint(t *testing.T) {
+	// Create discovery WITHOUT registration endpoint
+	discovery := &Discovery{
+		AuthorizationEndpoint: "https://auth.example.com/authorize",
+		TokenEndpoint:         "https://auth.example.com/token",
+		RegistrationEndpoint:  "", // Empty - DCR not supported
+	}
+
+	// Attempt DCR
+	creds, err := PerformDCR(context.Background(), discovery, "test-server")
+
+	// Verify error occurred
+	if err == nil {
+		t.Fatal("Expected error when registration endpoint missing")
+	}
+	if creds != nil {
+		t.Error("Expected nil credentials on error")
+	}
+}

discovery.go

@@ -21,13 +21,21 @@ import (
 // - Gracefully handles servers with partial MCP compliance
 //
 // ROBUST DISCOVERY FLOW (Inspector-inspired):
-// 1. Make request to MCP server to trigger 401 response
-// 2. Default authorization server to MCP server domain
-// 3. Try to parse WWW-Authenticate header for resource_metadata URL
-// 4. If resource metadata available, try to fetch it (optional)
-// 5. Always fetch Authorization Server Metadata (required)
-// 6. Build discovery result with whatever information is available
+// 1. Make initial MCP request (expect 401 if OAuth required)
+// 2. Parse WWW-Authenticate header (if present)
+// 3. Initialize with intelligent defaults (fallback auth server = MCP domain)
+// 4. Fetch resource metadata (from header URL or well-known endpoint fallback)
+// 5. Fetch Authorization Server Metadata (REQUIRED)
+// 6. Build discovery result with all gathered information
+//
+// FALLBACK BEHAVIOR: If WWW-Authenticate missing/unparseable, falls back to
+// RFC 9728-required /.well-known/oauth-protected-resource endpoint
 func DiscoverOAuthRequirements(ctx context.Context, serverURL string) (*Discovery, error) {
+	// Extract logger from context (or use noop if not provided)
+	logger := loggerFromContext(ctx)
+
+	logger.Infof("starting OAuth discovery for server: %s", serverURL)
+
 	// Create HTTP client with reasonable timeout
 	client := &http.Client{
 		Timeout: 30 * time.Second,
@@ -59,28 +67,38 @@ func DiscoverOAuthRequirements(ctx context.Context, serverURL string) (*Discover
 	}
 	defer resp.Body.Close()
 
-	// If not 401, OAuth is not required (Authorization is OPTIONAL per MCP spec Section 2.1)
+	logger.Infof("MCP server response: status=%d", resp.StatusCode)
+
+	// If not 401, OAuth might not be required (Authorization is OPTIONAL per MCP spec Section 2.1)
+	// We log a warning but continue discovery attempt in case server is misconfigured
 	if resp.StatusCode != http.StatusUnauthorized {
-		return &Discovery{
-			RequiresOAuth: false,
-		}, nil
+		logger.Warnf("expected 401 Unauthorized, got %d - OAuth may not be required", resp.StatusCode)
 	}
 
 	// STEP 2: Parse WWW-Authenticate header (if present)
 	// MCP Spec Section 4.1: "MCP servers MUST use the HTTP header WWW-Authenticate when returning a 401 Unauthorized"
 	wwwAuth := resp.Header.Get("WWW-Authenticate")
-	if wwwAuth == "" {
-		return nil, fmt.Errorf("server returned 401 but no WWW-Authenticate header")
-	}
 
-	challenges, err := ParseWWWAuthenticate(wwwAuth)
-	if err != nil {
-		return nil, fmt.Errorf("parsing WWW-Authenticate header: %w", err)
+	var challenges []WWWAuthenticateChallenge
+	if wwwAuth != "" {
+		logger.Infof("WWW-Authenticate header present: %s", wwwAuth)
+		var err error
+		challenges, err = ParseWWWAuthenticate(wwwAuth)
+		if err != nil {
+			// WWW-Authenticate header exists but isn't parseable - log but continue
+			logger.Warnf("could not parse WWW-Authenticate header: %v", err)
+			challenges = nil
+		} else {
+			logger.Infof("parsed %d WWW-Authenticate challenge(s)", len(challenges))
+		}
+	} else {
+		logger.Infof("no WWW-Authenticate header present - will try well-known endpoint")
 	}
 
 	// STEP 3: Initialize with intelligent defaults (Inspector pattern)
 	// Default authorization server to MCP server's domain
 	defaultAuthServerURL := fmt.Sprintf("%s://%s", parsedURL.Scheme, parsedURL.Host)
+	logger.Debugf("default authorization server: %s", defaultAuthServerURL)
 
 	// Initialize discovery with defaults
 	var resourceMetadata *ProtectedResourceMetadata
@@ -89,29 +107,43 @@ func DiscoverOAuthRequirements(ctx context.Context, serverURL string) (*Discover
 
 	// STEP 4: Try to get resource metadata (OPTIONAL - don't fail if missing)
 	// RFC 9728 Section 5.1: resource_metadata parameter in WWW-Authenticate
-	resourceMetadataURL := FindResourceMetadataURL(challenges)
+	resourceMetadataURL := ""
+	if challenges != nil {
+		resourceMetadataURL = FindResourceMetadataURL(challenges)
+	}
+
 	if resourceMetadataURL != "" {
 		// Resource metadata URL found - try to fetch it
+		logger.Infof("fetching protected resource metadata from: %s", resourceMetadataURL)
 		resourceMetadata, resourceMetadataError = fetchOAuthProtectedResourceMetadata(ctx, client, resourceMetadataURL)
 		if resourceMetadataError == nil && resourceMetadata != nil && resourceMetadata.AuthorizationServer != "" {
 			// Use authorization server from resource metadata if available
 			authServerURL = resourceMetadata.AuthorizationServer
+			logger.Infof("resource metadata retrieved, auth server: %s", authServerURL)
+		} else if resourceMetadataError != nil {
+			logger.Warnf("failed to fetch resource metadata: %v", resourceMetadataError)
 		}
 	} else {
 		// No resource_metadata in WWW-Authenticate - try well-known endpoint
 		wellKnownURL := fmt.Sprintf("%s/.well-known/oauth-protected-resource", defaultAuthServerURL)
+		logger.Infof("fallback: trying well-known resource metadata endpoint: %s", wellKnownURL)
 		resourceMetadata, resourceMetadataError = fetchOAuthProtectedResourceMetadata(ctx, client, wellKnownURL)
 		if resourceMetadataError == nil && resourceMetadata != nil && resourceMetadata.AuthorizationServer != "" {
 			authServerURL = resourceMetadata.AuthorizationServer
+			logger.Infof("resource metadata from well-known endpoint, auth server: %s", authServerURL)
 		}
 	}
 
 	// STEP 5: Fetch Authorization Server Metadata (REQUIRED)
 	// MCP Spec Section 3.1: "Authorization servers MUST provide OAuth 2.0 Authorization Server Metadata (RFC8414)"
+	logger.Infof("fetching authorization server metadata from: %s", authServerURL)
 	authServerMetadata, err := fetchAuthorizationServerMetadata(ctx, client, authServerURL)
 	if err != nil {
+		logger.Warnf("failed to fetch authorization server metadata: %v", err)
 		return nil, fmt.Errorf("fetching authorization server metadata from %s: %w", authServerURL, err)
 	}
+	logger.Infof("auth server metadata retrieved: token_endpoint=%s, registration_endpoint=%s",
+		authServerMetadata.TokenEndpoint, authServerMetadata.RegistrationEndpoint)
 
 	// STEP 6: Build discovery result with all available information
 	discovery := &Discovery{
@@ -155,6 +187,9 @@ func DiscoverOAuthRequirements(ctx context.Context, serverURL string) (*Discover
 		discovery.Scopes = FindRequiredScopes(challenges)
 	}
 
+	logger.Infof("discovery complete: auth_server=%s, scopes=%v, pkce=%v",
+		discovery.AuthorizationServer, discovery.Scopes, discovery.SupportsPKCE)
+
 	return discovery, nil
 }