diff --git a/AGENTS.md b/AGENTS.md index c735b7c16..33dc31a48 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -32,7 +32,7 @@ Quick reference for AI agents working with MCP Gateway (Go-based MCP proxy serve - `validation.go`, `validation_env.go`, `validation_schema.go` - Fail-fast field, environment, and schema validation - `validation_test.go` - Comprehensive validation tests - `internal/difc/` - Decentralized Information Flow Control -- `internal/envutil/` - Environment variable utilities +- `internal/envutil/` - Environment variable and Docker env-arg utilities - `internal/guard/` - Security guards (NoopGuard, WasmGuard, WriteSinkGuard) - `internal/httputil/` - Shared HTTP helper utilities (server, proxy) - `internal/launcher/` - Backend process management diff --git a/internal/envutil/expand_env_args.go b/internal/envutil/expand_env_args.go index 0995966e5..0502922e9 100644 --- a/internal/envutil/expand_env_args.go +++ b/internal/envutil/expand_env_args.go @@ -10,32 +10,47 @@ import ( var logExpand = logger.New("envutil:expand") +// WalkDockerEnvArgs iterates Docker-style passthrough env args ("-e VAR_NAME") +// and calls fn with the index of the "-e" flag, the variable name, its current +// process value, and whether the variable exists in the environment. +func WalkDockerEnvArgs(args []string, fn func(index int, varName, value string, found bool)) { + for i := 0; i < len(args); i++ { + arg := args[i] + if arg == "-e" && i+1 < len(args) { + nextArg := args[i+1] + if nextArg != "" && !strings.Contains(nextArg, "=") { + value, found := os.LookupEnv(nextArg) + fn(i, nextArg, value, found) + } + i++ // Skip the next arg since we processed it + } + } +} + // ExpandEnvArgs expands Docker -e flags that reference environment variables. // Converts "-e VAR_NAME" to "-e VAR_NAME=value" by reading from the process environment. // If the variable is not set, the flag is passed through unchanged. func ExpandEnvArgs(args []string) []string { logExpand.Printf("Expanding env args: input_count=%d", len(args)) + expandedValues := make(map[int]string) + WalkDockerEnvArgs(args, func(index int, varName, value string, found bool) { + if found { + logExpand.Printf("Expanding env var: name=%s", varName) + expandedValues[index] = fmt.Sprintf("%s=%s", varName, value) + return + } + logExpand.Printf("Env var not found in process environment: name=%s", varName) + }) + result := make([]string, 0, len(args)) for i := 0; i < len(args); i++ { - arg := args[i] - - // Check if this is a -e flag - if arg == "-e" && i+1 < len(args) { - nextArg := args[i+1] - // If next arg doesn't contain '=', it's a variable reference - if len(nextArg) > 0 && !strings.Contains(nextArg, "=") { - // Look up the variable in the environment - if value, exists := os.LookupEnv(nextArg); exists { - logExpand.Printf("Expanding env var: name=%s", nextArg) - result = append(result, "-e") - result = append(result, fmt.Sprintf("%s=%s", nextArg, value)) - i++ // Skip the next arg since we processed it - continue - } - logExpand.Printf("Env var not found in process environment: name=%s", nextArg) - } + if expandedValue, ok := expandedValues[i]; ok { + result = append(result, "-e", expandedValue) + i++ // Skip the next arg (variable name) since we just processed it; loop increment will advance past the current -e. + continue } - result = append(result, arg) + + result = append(result, args[i]) } logExpand.Printf("Env args expansion complete: output_count=%d", len(result)) return result diff --git a/internal/envutil/expand_env_args_test.go b/internal/envutil/expand_env_args_test.go index a200d0a74..965cc2ec3 100644 --- a/internal/envutil/expand_env_args_test.go +++ b/internal/envutil/expand_env_args_test.go @@ -150,3 +150,52 @@ func TestExpandEnvArgs_OutputIsIndependentOfInput(t *testing.T) { result[0] = "MODIFIED" assert.Equal(t, "run", args[0], "Modifying result should not affect original slice") } + +func TestWalkDockerEnvArgs(t *testing.T) { + t.Setenv("SET_VAR", "value") + t.Setenv("EMPTY_VAR", "") + + type walkedArg struct { + index int + varName string + value string + found bool + } + + var walked []walkedArg + WalkDockerEnvArgs([]string{ + "run", + "-e", "SET_VAR", + "-e", "EXPLICIT=value", + "-e", "EMPTY_VAR", + "-e", "MISSING_VAR", + "-e", + }, func(index int, varName, value string, found bool) { + walked = append(walked, walkedArg{ + index: index, + varName: varName, + value: value, + found: found, + }) + }) + + assert.Equal(t, []walkedArg{ + {index: 1, varName: "SET_VAR", value: "value", found: true}, + {index: 5, varName: "EMPTY_VAR", value: "", found: true}, + {index: 7, varName: "MISSING_VAR", value: "", found: false}, + }, walked) + assert.Len(t, walked, 3) +} + +func TestWalkDockerEnvArgs_IgnoresExplicitAssignmentsAndTrailingFlag(t *testing.T) { + calls := 0 + + WalkDockerEnvArgs([]string{ + "-e", "EXPLICIT=value", + "-e", + }, func(index int, varName, value string, found bool) { + calls++ + }) + + assert.Zero(t, calls) +} diff --git a/internal/launcher/log_helpers.go b/internal/launcher/log_helpers.go index a2be41c7f..b83da9c91 100644 --- a/internal/launcher/log_helpers.go +++ b/internal/launcher/log_helpers.go @@ -2,10 +2,9 @@ package launcher import ( "log" - "os" - "strings" "github.com/github/gh-aw-mcpg/internal/config" + "github.com/github/gh-aw-mcpg/internal/envutil" "github.com/github/gh-aw-mcpg/internal/logger" "github.com/github/gh-aw-mcpg/internal/logger/sanitize" "github.com/github/gh-aw-mcpg/internal/mcp" @@ -38,23 +37,17 @@ func (l *Launcher) logLaunchStart(serverID, sessionID string, serverCfg *config. // logEnvPassthrough checks and logs environment variable passthrough status func (l *Launcher) logEnvPassthrough(args []string) { - for i := 0; i < len(args); i++ { - arg := args[i] - // If this arg is "-e", check the next argument - if arg == "-e" && i+1 < len(args) { - nextArg := args[i+1] - // Check if it's a passthrough (no = sign) vs explicit value (has = sign) - if !strings.Contains(nextArg, "=") { - // This is a passthrough variable, check if it exists in our environment - if val := os.Getenv(nextArg); val != "" { - log.Printf("[LAUNCHER] ✓ Env passthrough: %s=%s (from MCPG process)", nextArg, sanitize.TruncateSecret(val)) - } else { - log.Printf("[LAUNCHER] ✗ WARNING: Env passthrough for %s requested but NOT FOUND in MCPG process", nextArg) - } - } - i++ // Skip the next arg since we just processed it + envutil.WalkDockerEnvArgs(args, func(_ int, varName, value string, found bool) { + if !found { + log.Printf("[LAUNCHER] ✗ WARNING: Env passthrough for %s requested but NOT FOUND in MCPG process", varName) + return } - } + if value != "" { + log.Printf("[LAUNCHER] ✓ Env passthrough: %s=%s (from MCPG process)", varName, sanitize.TruncateSecret(value)) + return + } + log.Printf("[LAUNCHER] ⚠️ Env passthrough for %s is empty in MCPG process", varName) + }) } // logLaunchError logs detailed launch failure diagnostics diff --git a/internal/launcher/log_helpers_test.go b/internal/launcher/log_helpers_test.go index 9b46428b6..2cf9d16c1 100644 --- a/internal/launcher/log_helpers_test.go +++ b/internal/launcher/log_helpers_test.go @@ -187,6 +187,17 @@ func TestLauncher_LogEnvPassthrough(t *testing.T) { "NOT FOUND", }, }, + { + name: "passthrough empty variable value logs empty warning", + args: []string{"run", "-e", "EMPTY_VAR"}, + setupEnv: func(t *testing.T) { + t.Setenv("EMPTY_VAR", "") + }, + wantInLog: []string{ + "empty in MCPG process", + "EMPTY_VAR", + }, + }, { name: "explicit value not passthrough", args: []string{"run", "-e", "VAR=value"},