Merge pull request #333 from githubnext/copilot/refactor-semantic-function-clustering

Consolidate duplicate sanitization and auth parsing logic

Author: lpcox

internal/auth/header.go

@@ -5,19 +5,41 @@
 // which requires Authorization headers to contain the API key directly
 // without any scheme prefix (e.g., NOT "Bearer <key>").
 //
-// Example usage:
+// The package provides both full parsing with error handling (ParseAuthHeader)
+// and convenience methods for specific use cases (ExtractAgentID, ValidateAPIKey).
 //
+// Usage Guidelines:
+//
+//   - Use ParseAuthHeader() for complete authentication with error handling:
+//     Returns both API key and agent ID, with errors for missing/invalid headers.
+//
+//   - Use ExtractAgentID() when you only need the agent ID and want automatic
+//     fallback to "default" instead of error handling.
+//
+//   - Use ValidateAPIKey() to check if a provided key matches the expected value.
+//     Automatically handles the case where authentication is disabled (no expected key).
+//
+// Example:
+//
+//	// Full authentication
 //	apiKey, agentID, err := auth.ParseAuthHeader(r.Header.Get("Authorization"))
 //	if err != nil {
-//		// Handle error
+//		return err
 //	}
+//	if !auth.ValidateAPIKey(apiKey, expectedKey) {
+//		return errors.New("invalid API key")
+//	}
+//
+//	// Extract agent ID only (for context, not authentication)
+//	agentID := auth.ExtractAgentID(r.Header.Get("Authorization"))
 package auth
 
 import (
 	"errors"
 	"strings"
 
 	"github.com/githubnext/gh-aw-mcpg/internal/logger"
+	"github.com/githubnext/gh-aw-mcpg/internal/logger/sanitize"
 )
 
 var log = logger.New("auth:header")
@@ -29,18 +51,6 @@ var (
 	ErrInvalidAuthHeader = errors.New("invalid Authorization header format")
 )
 
-// sanitizeForLogging returns a sanitized version of the input string for safe logging.
-// It shows only the first 4 characters followed by "..." to prevent exposing sensitive data.
-// For strings with 4 or fewer characters, it returns only "...".
-func sanitizeForLogging(input string) string {
-	if len(input) > 4 {
-		return input[:4] + "..."
-	} else if len(input) > 0 {
-		return "..."
-	}
-	return ""
-}
-
 // ParseAuthHeader parses the Authorization header and extracts the API key and agent ID.
 // Per MCP spec 7.1, the Authorization header should contain the API key directly
 // without any Bearer prefix or other scheme.
@@ -54,7 +64,7 @@ func sanitizeForLogging(input string) string {
 //   - agentID: The extracted agent/session identifier
 //   - error: ErrMissingAuthHeader if header is empty, nil otherwise
 func ParseAuthHeader(authHeader string) (apiKey string, agentID string, error error) {
-	log.Printf("Parsing auth header: sanitized=%s, length=%d", sanitizeForLogging(authHeader), len(authHeader))
+	log.Printf("Parsing auth header: sanitized=%s, length=%d", sanitize.TruncateSecret(authHeader), len(authHeader))
 
 	if authHeader == "" {
 		log.Print("Auth header missing, returning error")
@@ -96,3 +106,23 @@ func ValidateAPIKey(provided, expected string) bool {
 	log.Printf("API key validation result: matches=%t", matches)
 	return matches
 }
+
+// ExtractAgentID extracts the agent ID from an Authorization header.
+// This is a convenience wrapper around ParseAuthHeader that only returns the agent ID.
+// Returns "default" if the header is empty or cannot be parsed.
+//
+// This function is intended for use cases where you only need the agent ID
+// and don't need full error handling. For complete authentication handling,
+// use ParseAuthHeader instead.
+func ExtractAgentID(authHeader string) string {
+	if authHeader == "" {
+		return "default"
+	}
+
+	_, agentID, err := ParseAuthHeader(authHeader)
+	if err != nil {
+		return "default"
+	}
+
+	return agentID
+}

internal/auth/header_test.go

@@ -5,9 +5,11 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/githubnext/gh-aw-mcpg/internal/logger/sanitize"
 )
 
-func TestSanitizeForLogging(t *testing.T) {
+func TestTruncateSecret(t *testing.T) {
 	tests := []struct {
 		name  string
 		input string
@@ -62,7 +64,7 @@ func TestSanitizeForLogging(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := sanitizeForLogging(tt.input)
+			got := sanitize.TruncateSecret(tt.input)
 			assert.Equal(t, tt.want, got)
 		})
 	}
@@ -235,3 +237,49 @@ func TestValidateAPIKey(t *testing.T) {
 		})
 	}
 }
+
+func TestExtractAgentID(t *testing.T) {
+	tests := []struct {
+		name       string
+		authHeader string
+		want       string
+	}{
+		{
+			name:       "Empty header returns default",
+			authHeader: "",
+			want:       "default",
+		},
+		{
+			name:       "Plain API key",
+			authHeader: "my-api-key",
+			want:       "my-api-key",
+		},
+		{
+			name:       "Bearer token",
+			authHeader: "Bearer my-token-123",
+			want:       "my-token-123",
+		},
+		{
+			name:       "Agent format",
+			authHeader: "Agent agent-abc",
+			want:       "agent-abc",
+		},
+		{
+			name:       "Long API key",
+			authHeader: "my-super-long-api-key-with-many-characters",
+			want:       "my-super-long-api-key-with-many-characters",
+		},
+		{
+			name:       "API key with special characters",
+			authHeader: "key!@#$%^&*()",
+			want:       "key!@#$%^&*()",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := ExtractAgentID(tt.authHeader)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}

internal/guard/context.go

@@ -1,8 +1,31 @@
+// Package guard provides security context management and guard registry for the MCP Gateway.
+//
+// This package is responsible for managing security labels (DIFC - Decentralized Information
+// Flow Control) and extracting agent identifiers from request contexts.
+//
+// Relationship with internal/auth:
+//
+// - internal/auth: Primary authentication logic (header parsing, validation)
+// - internal/guard: Security context management (agent ID tracking, guard registry)
+//
+// For new code, prefer using internal/auth for authentication-related operations.
+// The guard package's ExtractAgentIDFromAuthHeader() is deprecated and delegates
+// to auth.ExtractAgentID() for centralized authentication logic.
+//
+// Example:
+//
+//	// Store agent ID in context (use auth.ExtractAgentID for parsing)
+//	agentID := auth.ExtractAgentID(authHeader)
+//	ctx = guard.SetAgentIDInContext(ctx, agentID)
+//
+//	// Retrieve agent ID from context
+//	agentID := guard.GetAgentIDFromContext(ctx) // Returns "default" if not found
 package guard
 
 import (
 	"context"
-	"strings"
+
+	"github.com/githubnext/gh-aw-mcpg/internal/auth"
 )
 
 // ContextKey is used for storing values in context
@@ -30,35 +53,14 @@ func SetAgentIDInContext(ctx context.Context, agentID string) context.Context {
 	return context.WithValue(ctx, AgentIDContextKey, agentID)
 }
 
-// ExtractAgentIDFromAuthHeader extracts agent ID from Authorization header
+// ExtractAgentIDFromAuthHeader extracts agent ID from Authorization header.
 //
-// Note: For MCP spec 7.1 compliant parsing, see internal/auth.ParseAuthHeader()
-// which provides centralized authentication header parsing.
+// Deprecated: Use auth.ExtractAgentID() instead for centralized authentication parsing.
+// This function is maintained for backward compatibility but delegates to the auth package.
 //
-// This function supports formats:
-//   - "Bearer <token>" - uses token as agent ID
-//   - "Agent <agent-id>" - uses agent-id directly
-//   - Any other format - uses the entire value as agent ID
+// For MCP spec 7.1 compliant parsing with full error handling, use auth.ParseAuthHeader().
 func ExtractAgentIDFromAuthHeader(authHeader string) string {
-	if authHeader == "" {
-		return "default"
-	}
-
-	// Handle "Bearer <token>" format
-	if strings.HasPrefix(authHeader, "Bearer ") {
-		token := strings.TrimPrefix(authHeader, "Bearer ")
-		// Use the token as the agent ID
-		// In production, you might want to validate/decode the token
-		return token
-	}
-
-	// Handle "Agent <agent-id>" format
-	if strings.HasPrefix(authHeader, "Agent ") {
-		return strings.TrimPrefix(authHeader, "Agent ")
-	}
-
-	// Use the entire header value as agent ID
-	return authHeader
+	return auth.ExtractAgentID(authHeader)
 }
 
 // GetRequestStateFromContext retrieves guard request state from context

internal/launcher/launcher.go

@@ -10,32 +10,13 @@ import (
 
 	"github.com/githubnext/gh-aw-mcpg/internal/config"
 	"github.com/githubnext/gh-aw-mcpg/internal/logger"
+	"github.com/githubnext/gh-aw-mcpg/internal/logger/sanitize"
 	"github.com/githubnext/gh-aw-mcpg/internal/mcp"
 	"github.com/githubnext/gh-aw-mcpg/internal/tty"
 )
 
 var logLauncher = logger.New("launcher:launcher")
 
-// sanitizeEnvForLogging returns a sanitized version of environment variables
-// where each value is truncated to first 4 characters followed by "..."
-// This prevents sensitive information like API keys from being logged in full.
-func sanitizeEnvForLogging(env map[string]string) map[string]string {
-	if env == nil {
-		return nil
-	}
-	sanitized := make(map[string]string, len(env))
-	for key, value := range env {
-		if len(value) <= 4 {
-			// If value is 4 chars or less, just use "..."
-			sanitized[key] = "..."
-		} else {
-			// Show first 4 characters and append "..."
-			sanitized[key] = value[:4] + "..."
-		}
-	}
-	return sanitized
-}
-
 // Launcher manages backend MCP server connections
 type Launcher struct {
 	ctx                context.Context
@@ -161,7 +142,7 @@ func GetOrLaunch(l *Launcher, serverID string) (*mcp.Connection, error) {
 	}
 
 	if len(serverCfg.Env) > 0 {
-		log.Printf("[LAUNCHER] Additional env vars: %v", sanitizeEnvForLogging(serverCfg.Env))
+		log.Printf("[LAUNCHER] Additional env vars: %v", sanitize.TruncateSecretMap(serverCfg.Env))
 	}
 
 	// Create connection
@@ -174,7 +155,7 @@ func GetOrLaunch(l *Launcher, serverID string) (*mcp.Connection, error) {
 		log.Printf("[LAUNCHER] Debug Information:")
 		log.Printf("[LAUNCHER]   - Command: %s", serverCfg.Command)
 		log.Printf("[LAUNCHER]   - Args: %v", serverCfg.Args)
-		log.Printf("[LAUNCHER]   - Env vars: %v", sanitizeEnvForLogging(serverCfg.Env))
+		log.Printf("[LAUNCHER]   - Env vars: %v", sanitize.TruncateSecretMap(serverCfg.Env))
 		log.Printf("[LAUNCHER]   - Running in container: %v", l.runningInContainer)
 		log.Printf("[LAUNCHER]   - Is direct command: %v", isDirectCommand)
 

internal/launcher/launcher_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/githubnext/gh-aw-mcpg/internal/config"
+	"github.com/githubnext/gh-aw-mcpg/internal/logger/sanitize"
 )
 
 // loadConfigFromJSON is a test helper that creates a config from JSON via stdin
@@ -180,7 +181,7 @@ func TestMixedHTTPAndStdioServers(t *testing.T) {
 	assert.Equal(t, "stdio", stdioServer.Type)
 }
 
-func TestSanitizeEnvForLogging(t *testing.T) {
+func TestTruncateSecretMap(t *testing.T) {
 	tests := []struct {
 		name     string
 		input    map[string]string
@@ -242,14 +243,14 @@ func TestSanitizeEnvForLogging(t *testing.T) {
 				"EMPTY": "",
 			},
 			expected: map[string]string{
-				"EMPTY": "...",
+				"EMPTY": "",
 			},
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result := sanitizeEnvForLogging(tt.input)
+			result := sanitize.TruncateSecretMap(tt.input)
 
 			if tt.expected == nil {
 				assert.Nil(t, result)

internal/logger/global_patterns.go internal/logger/global_helpers.gointernal/logger/global_patterns.go renamed to internal/logger/global_helpers.go

@@ -1,3 +1,17 @@
+// Package logger provides structured logging for the MCP Gateway.
+//
+// This file contains helper functions for managing global logger state with proper
+// mutex handling. These helpers encapsulate common patterns for initializing and
+// closing global loggers (FileLogger, JSONLLogger, MarkdownLogger) to reduce code
+// duplication while maintaining thread safety.
+//
+// Functions in this file follow a consistent pattern:
+//
+// - init*: Initialize a global logger with proper locking and cleanup of any existing logger
+// - close*: Close and clear a global logger with proper locking
+//
+// These helpers are used internally by the logger package and should not be called
+// directly by external code. Use the public Init* and Close* functions instead.
 package logger
 
 // This file contains helper functions that encapsulate the common patterns