diff --git a/internal/launcher/launcher.go b/internal/launcher/launcher.go index 729a44b7..2efae91c 100644 --- a/internal/launcher/launcher.go +++ b/internal/launcher/launcher.go @@ -113,10 +113,10 @@ func GetOrLaunch(l *Launcher, serverID string) (*mcp.Connection, error) { } // Log the command being executed - logger.LogInfo("backend", "Launching MCP backend server: %s, command=%s, args=%v", serverID, serverCfg.Command, serverCfg.Args) + logger.LogInfo("backend", "Launching MCP backend server: %s, command=%s, args=%v", serverID, serverCfg.Command, sanitize.SanitizeArgs(serverCfg.Args)) log.Printf("[LAUNCHER] Starting MCP server: %s", serverID) log.Printf("[LAUNCHER] Command: %s", serverCfg.Command) - log.Printf("[LAUNCHER] Args: %v", serverCfg.Args) + log.Printf("[LAUNCHER] Args: %v", sanitize.SanitizeArgs(serverCfg.Args)) logLauncher.Printf("Launching new server: serverID=%s, command=%s, inContainer=%v, isDirectCommand=%v", serverID, serverCfg.Command, l.runningInContainer, isDirectCommand) @@ -237,10 +237,10 @@ func GetOrLaunchForSession(l *Launcher, serverID, sessionID string) (*mcp.Connec } // Log the command being executed - logger.LogInfo("backend", "Launching MCP backend server for session: server=%s, session=%s, command=%s, args=%v", serverID, sessionID, serverCfg.Command, serverCfg.Args) + logger.LogInfo("backend", "Launching MCP backend server for session: server=%s, session=%s, command=%s, args=%v", serverID, sessionID, serverCfg.Command, sanitize.SanitizeArgs(serverCfg.Args)) log.Printf("[LAUNCHER] Starting MCP server for session: %s (session: %s)", serverID, sessionID) log.Printf("[LAUNCHER] Command: %s", serverCfg.Command) - log.Printf("[LAUNCHER] Args: %v", serverCfg.Args) + log.Printf("[LAUNCHER] Args: %v", sanitize.SanitizeArgs(serverCfg.Args)) logLauncher.Printf("Launching new session server: serverID=%s, sessionID=%s, command=%s", serverID, sessionID, serverCfg.Command) // Check for environment variable passthrough diff --git a/internal/logger/sanitize/sanitize.go b/internal/logger/sanitize/sanitize.go index 0e6ee48d..89a42b73 100644 --- a/internal/logger/sanitize/sanitize.go +++ b/internal/logger/sanitize/sanitize.go @@ -127,3 +127,35 @@ func SanitizeJSON(payloadBytes []byte) json.RawMessage { compactBytes, _ := json.Marshal(tmp) return json.RawMessage(compactBytes) } + +// SanitizeArgs returns a sanitized version of command arguments for safe logging. +// It specifically handles Docker-style environment variable arguments (-e VAR=VALUE) +// by truncating the values to prevent exposing sensitive data like API tokens. +// Other arguments are passed through unchanged. +func SanitizeArgs(args []string) []string { + if len(args) == 0 { + return args + } + + sanitized := make([]string, len(args)) + for i := 0; i < len(args); i++ { + arg := args[i] + + // Check if this is an environment variable value after a -e flag + // Format: -e VAR=VALUE + if i > 0 && args[i-1] == "-e" && strings.Contains(arg, "=") { + // Split on first = to get VAR and VALUE + parts := strings.SplitN(arg, "=", 2) + if len(parts) == 2 { + // Truncate the value part + sanitized[i] = parts[0] + "=" + TruncateSecret(parts[1]) + } else { + sanitized[i] = arg + } + } else { + // Pass through unchanged + sanitized[i] = arg + } + } + return sanitized +} diff --git a/internal/logger/sanitize/sanitize_test.go b/internal/logger/sanitize/sanitize_test.go index 16e54b12..574e0901 100644 --- a/internal/logger/sanitize/sanitize_test.go +++ b/internal/logger/sanitize/sanitize_test.go @@ -495,3 +495,156 @@ func TestTruncateSecretMap(t *testing.T) { }) } } + +func TestSanitizeArgs(t *testing.T) { + tests := []struct { + name string + input []string + expected []string + }{ + { + name: "nil args", + input: nil, + expected: nil, + }, + { + name: "empty args", + input: []string{}, + expected: []string{}, + }, + { + name: "args without env vars", + input: []string{"run", "--rm", "-i", "image:latest"}, + expected: []string{"run", "--rm", "-i", "image:latest"}, + }, + { + name: "single env var with long token", + input: []string{"run", "-e", "GITHUB_PERSONAL_ACCESS_TOKEN=ghp_1234567890123456789012345678901234567890"}, + expected: []string{"run", "-e", "GITHUB_PERSONAL_ACCESS_TOKEN=ghp_..."}, + }, + { + name: "multiple env vars with different lengths", + input: []string{ + "run", + "-e", "GITHUB_PERSONAL_ACCESS_TOKEN=ghp_1234567890123456789012345678901234567890", + "-e", "API_KEY=sk_test_1234567890", + "-e", "SHORT=abc", + "--rm", + "-i", + "image:latest", + }, + expected: []string{ + "run", + "-e", "GITHUB_PERSONAL_ACCESS_TOKEN=ghp_...", + "-e", "API_KEY=sk_t...", + "-e", "SHORT=...", + "--rm", + "-i", + "image:latest", + }, + }, + { + name: "realistic docker command with secrets", + input: []string{ + "run", + "--rm", + "-i", + "-e", "NO_COLOR=1", + "-e", "TERM=dumb", + "-e", "PYTHONUNBUFFERED=1", + "-e", "GITHUB_PERSONAL_ACCESS_TOKEN=ghp_abcdefghijklmnopqrstuvwxyz1234567890", + "-e", "GITHUB_READ_ONLY=1", + "ghcr.io/github/github-mcp-server:v0.29.0", + }, + expected: []string{ + "run", + "--rm", + "-i", + "-e", "NO_COLOR=...", + "-e", "TERM=...", + "-e", "PYTHONUNBUFFERED=...", + "-e", "GITHUB_PERSONAL_ACCESS_TOKEN=ghp_...", + "-e", "GITHUB_READ_ONLY=...", + "ghcr.io/github/github-mcp-server:v0.29.0", + }, + }, + { + name: "env var without equals sign", + input: []string{"run", "-e", "MYVAR", "image:latest"}, + expected: []string{"run", "-e", "MYVAR", "image:latest"}, + }, + { + name: "env var with empty value", + input: []string{"run", "-e", "EMPTY=", "image:latest"}, + expected: []string{"run", "-e", "EMPTY=", "image:latest"}, + }, + { + name: "env var with equals in value", + input: []string{ + "run", + "-e", "CONFIG=key=value=extra", + }, + expected: []string{ + "run", + "-e", "CONFIG=key=...", + }, + }, + { + name: "-e flag at end without value", + input: []string{"run", "--rm", "-e"}, + expected: []string{"run", "--rm", "-e"}, + }, + { + name: "mixed flags and env vars", + input: []string{ + "run", + "--name", "test-container", + "-e", "API_KEY=secret123456", + "--network", "host", + "-e", "TOKEN=mytoken", + "image:latest", + }, + expected: []string{ + "run", + "--name", "test-container", + "-e", "API_KEY=secr...", + "--network", "host", + "-e", "TOKEN=myto...", + "image:latest", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := SanitizeArgs(tt.input) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestSanitizeArgsDoesNotLeakSecrets(t *testing.T) { + // Test that actual secrets are not present in sanitized output + secretToken := "ghp_verysecrettokenthatshouldbehidden1234567890" + secretApiKey := "test_secret_key_1234567890abcdefghijklmnop" + + input := []string{ + "run", + "-e", fmt.Sprintf("GITHUB_TOKEN=%s", secretToken), + "-e", fmt.Sprintf("API_KEY=%s", secretApiKey), + "image:latest", + } + + result := SanitizeArgs(input) + + // Convert result to string for easy checking + resultStr := fmt.Sprintf("%v", result) + + // Verify secrets are NOT in the output + assert.NotContains(t, resultStr, secretToken, "Secret token should not be present in sanitized output") + assert.NotContains(t, resultStr, secretApiKey, "Secret API key should not be present in sanitized output") + + // Verify truncated versions ARE present + assert.Contains(t, resultStr, "GITHUB_TOKEN=ghp_...", "Truncated token should be present") + assert.Contains(t, resultStr, "API_KEY=test...", "Truncated API key should be present") +} diff --git a/internal/mcp/connection.go b/internal/mcp/connection.go index 69b51d42..ff07f63e 100644 --- a/internal/mcp/connection.go +++ b/internal/mcp/connection.go @@ -15,6 +15,7 @@ import ( "time" "github.com/githubnext/gh-aw-mcpg/internal/logger" + "github.com/githubnext/gh-aw-mcpg/internal/logger/sanitize" sdk "github.com/modelcontextprotocol/go-sdk/mcp" ) @@ -153,8 +154,8 @@ func setupHTTPRequest(ctx context.Context, url string, requestBody []byte, heade // NewConnection creates a new MCP connection using the official SDK func NewConnection(ctx context.Context, command string, args []string, env map[string]string) (*Connection, error) { - logger.LogInfo("backend", "Creating new MCP backend connection, command=%s, args=%v", command, args) - logConn.Printf("Creating new MCP connection: command=%s, args=%v", command, args) + logger.LogInfo("backend", "Creating new MCP backend connection, command=%s, args=%v", command, sanitize.SanitizeArgs(args)) + logConn.Printf("Creating new MCP connection: command=%s, args=%v", command, sanitize.SanitizeArgs(args)) ctx, cancel := context.WithCancel(ctx) // Create MCP client @@ -178,8 +179,8 @@ func NewConnection(ctx context.Context, command string, args []string, env map[s } } - logger.LogInfo("backend", "Starting MCP backend server, command=%s, args=%v", command, expandedArgs) - log.Printf("Starting MCP server command: %s %v", command, expandedArgs) + logger.LogInfo("backend", "Starting MCP backend server, command=%s, args=%v", command, sanitize.SanitizeArgs(expandedArgs)) + log.Printf("Starting MCP server command: %s %v", command, sanitize.SanitizeArgs(expandedArgs)) transport := &sdk.CommandTransport{Command: cmd} // Connect to the server (this handles the initialization handshake automatically) @@ -190,10 +191,10 @@ func NewConnection(ctx context.Context, command string, args []string, env map[s cancel() // Enhanced error context for debugging - logger.LogErrorMd("backend", "MCP backend connection failed, command=%s, args=%v, error=%v", command, expandedArgs, err) + logger.LogErrorMd("backend", "MCP backend connection failed, command=%s, args=%v, error=%v", command, sanitize.SanitizeArgs(expandedArgs), err) log.Printf("❌ MCP Connection Failed:") log.Printf(" Command: %s", command) - log.Printf(" Args: %v", expandedArgs) + log.Printf(" Args: %v", sanitize.SanitizeArgs(expandedArgs)) log.Printf(" Error: %v", err) // Check if it's a command not found error