fix(security): redact upstream secrets in docker spawn argv debug logs (#712)

internal/shellwrap/redact.go

@@ -0,0 +1,98 @@
+package shellwrap
+
+import (
+	"regexp"
+	"strings"
+
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/secret"
+)
+
+// dockerEnvFlagRegex matches a docker env flag (`-e` / `--env`) followed by a
+// whitespace-separated value token within a command STRING. The value token is
+// either single/double-quoted (it may then contain spaces) or a run of
+// non-whitespace characters. Group 1 anchors the flag to a word boundary
+// (start-of-string or preceding whitespace) so it does not match inside other
+// flags or image names.
+var dockerEnvFlagRegex = regexp.MustCompile(`(^|\s)(--env|-e)(\s+)('[^']*'|"[^"]*"|\S+)`)
+
+// RedactDockerArgs returns a copy of a docker-style argv slice with the VALUE
+// of every `KEY=VALUE` env injection masked, leaving keys, flags, image names,
+// and other args untouched. It handles three shapes the spawn path produces:
+//
+//   - two-token form:   ["-e", "KEY=VALUE"]               (direct-exec docker)
+//   - glued single tok:  ["-eKEY=VALUE"] / ["--env=KEY=VALUE"]
+//   - command-string el: ["-l", "-c", "docker run -e KEY=VALUE img"] (shell wrap)
+//
+// The input slice is never mutated. A nil input returns nil.
+func RedactDockerArgs(args []string) []string {
+	if args == nil {
+		return nil
+	}
+	out := make([]string, 0, len(args))
+	for i := 0; i < len(args); i++ {
+		a := args[i]
+		// Two-token form: `-e`/`--env` followed by `KEY=VALUE`.
+		if (a == "-e" || a == "--env") && i+1 < len(args) {
+			out = append(out, a, maskEnvToken(args[i+1]))
+			i++
+			continue
+		}
+		out = append(out, redactArgElement(a))
+	}
+	return out
+}
+
+// redactArgElement masks env secrets inside a single argv element, covering the
+// glued single-token forms and the embedded command-string form.
+func redactArgElement(a string) string {
+	// Command-string element (e.g. the `-c` argument of a login-shell wrap):
+	// mask any embedded `-e KEY=VALUE` occurrences.
+	if strings.ContainsAny(a, " \t") {
+		return RedactDockerCommandString(a)
+	}
+	// Glued long form: `--env=KEY=VALUE`.
+	if rest, ok := strings.CutPrefix(a, "--env="); ok {
+		return "--env=" + maskEnvToken(rest)
+	}
+	// Glued short form: `-eKEY=VALUE`.
+	if strings.HasPrefix(a, "-e") && len(a) > 2 && strings.Contains(a[2:], "=") {
+		return "-e" + maskEnvToken(a[2:])
+	}
+	return a
+}
+
+// RedactDockerCommandString masks the VALUE of any `-e KEY=VALUE` /
+// `--env KEY=VALUE` flag found within a single command string, leaving the rest
+// of the string (subcommand, flags, image name) intact.
+func RedactDockerCommandString(s string) string {
+	return dockerEnvFlagRegex.ReplaceAllStringFunc(s, func(m string) string {
+		sub := dockerEnvFlagRegex.FindStringSubmatch(m)
+		// sub[1]=lead, sub[2]=flag, sub[3]=whitespace, sub[4]=value token
+		return sub[1] + sub[2] + sub[3] + maskEnvToken(sub[4])
+	})
+}
+
+// maskEnvToken masks the value portion of a `KEY=VALUE` token, preserving the
+// key and any surrounding single/double quotes. Tokens without a `=` or with an
+// empty value are returned unchanged (they are not secret-bearing env values).
+func maskEnvToken(tok string) string {
+	quote := ""
+	inner := tok
+	if len(inner) >= 2 {
+		if (inner[0] == '\'' && inner[len(inner)-1] == '\'') ||
+			(inner[0] == '"' && inner[len(inner)-1] == '"') {
+			quote = string(inner[0])
+			inner = inner[1 : len(inner)-1]
+		}
+	}
+	eq := strings.IndexByte(inner, '=')
+	if eq < 0 {
+		return tok
+	}
+	key := inner[:eq]
+	val := inner[eq+1:]
+	if val == "" {
+		return tok
+	}
+	return quote + key + "=" + secret.MaskSecretValue(val) + quote
+}

internal/shellwrap/redact_test.go

@@ -0,0 +1,120 @@
+package shellwrap
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+
+	"go.uber.org/zap"
+	"go.uber.org/zap/zapcore"
+	"go.uber.org/zap/zaptest/observer"
+)
+
+const (
+	secretValueSlack = "xoxc-abc123456789"
+	secretValueJira  = "xoxd-zzz987654321"
+)
+
+// assertNoSecret fails if s contains any of the cleartext secret values that
+// must never reach a log.
+func assertNoSecret(t *testing.T, s string) {
+	t.Helper()
+	for _, sv := range []string{secretValueSlack, secretValueJira} {
+		if strings.Contains(s, sv) {
+			t.Errorf("cleartext secret %q leaked in %q", sv, s)
+		}
+	}
+}
+
+func TestRedactDockerArgs_TwoTokenForm(t *testing.T) {
+	in := []string{"run", "--rm", "-i", "-e", "SLACK_TOKEN=" + secretValueSlack, "myimage:latest"}
+	out := RedactDockerArgs(in)
+
+	joined := strings.Join(out, " ")
+	assertNoSecret(t, joined)
+
+	// Non-secret args untouched.
+	for _, want := range []string{"run", "--rm", "-i", "myimage:latest"} {
+		if !contains(out, want) {
+			t.Errorf("expected non-secret arg %q to be preserved, got %v", want, out)
+		}
+	}
+	// Key and flag preserved.
+	if !strings.Contains(joined, "-e") || !strings.Contains(joined, "SLACK_TOKEN=") {
+		t.Errorf("expected -e and key SLACK_TOKEN= preserved, got %q", joined)
+	}
+	// Input slice must not be mutated.
+	if in[4] != "SLACK_TOKEN="+secretValueSlack {
+		t.Errorf("RedactDockerArgs mutated its input slice: %q", in[4])
+	}
+}
+
+func TestRedactDockerArgs_SingleGluedToken(t *testing.T) {
+	in := []string{"run", "-eSLACK_TOKEN=" + secretValueSlack, "myimage"}
+	out := RedactDockerArgs(in)
+	joined := strings.Join(out, " ")
+	assertNoSecret(t, joined)
+	if !strings.Contains(joined, "SLACK_TOKEN=") {
+		t.Errorf("expected key preserved, got %q", joined)
+	}
+}
+
+func TestRedactDockerArgs_CommandStringElement(t *testing.T) {
+	in := []string{"-l", "-c", "docker run --rm -e SLACK_TOKEN=" + secretValueSlack + " -e JIRA_TOKEN=" + secretValueJira + " myimage:latest"}
+	out := RedactDockerArgs(in)
+	joined := strings.Join(out, " ")
+	assertNoSecret(t, joined)
+	for _, want := range []string{"-l", "docker run", "--rm", "myimage:latest", "SLACK_TOKEN=", "JIRA_TOKEN="} {
+		if !strings.Contains(joined, want) {
+			t.Errorf("expected %q preserved in %q", want, joined)
+		}
+	}
+}
+
+func TestRedactDockerCommandString(t *testing.T) {
+	in := "docker run --rm -e SLACK_TOKEN=" + secretValueSlack + " --env JIRA_TOKEN=" + secretValueJira + " myimage"
+	out := RedactDockerCommandString(in)
+	assertNoSecret(t, out)
+	for _, want := range []string{"docker run", "--rm", "myimage", "SLACK_TOKEN=", "JIRA_TOKEN="} {
+		if !strings.Contains(out, want) {
+			t.Errorf("expected %q preserved in %q", want, out)
+		}
+	}
+}
+
+func TestRedactDockerArgs_NoEnvFlagsUnchanged(t *testing.T) {
+	in := []string{"run", "--rm", "-i", "ghcr.io/foo/bar:latest", "--config", "/etc/x.json"}
+	out := RedactDockerArgs(in)
+	if strings.Join(in, " ") != strings.Join(out, " ") {
+		t.Errorf("non-env args were altered: in=%v out=%v", in, out)
+	}
+}
+
+// Guard test: the WrapWithUserShell debug log must not emit cleartext secrets
+// in its wrapped_command / original_args fields.
+func TestWrapWithUserShell_LogsAreRedacted(t *testing.T) {
+	obsCore, recorded := observer.New(zapcore.DebugLevel)
+	logger := zap.New(obsCore)
+
+	WrapWithUserShell(logger, "docker", []string{"run", "--rm", "-e", "SLACK_TOKEN=" + secretValueSlack, "myimage"})
+
+	if recorded.Len() == 0 {
+		t.Fatal("expected a debug log entry from WrapWithUserShell")
+	}
+	for _, entry := range recorded.All() {
+		for k, v := range entry.ContextMap() {
+			assertNoSecret(t, k)
+			assertNoSecret(t, fmt.Sprintf("%v", v))
+		}
+		assertNoSecret(t, entry.Message)
+	}
+}
+
+func contains(ss []string, want string) bool {
+	for _, s := range ss {
+		if s == want {
+			return true
+		}
+	}
+	return false
+}

internal/shellwrap/shellwrap.go

@@ -106,11 +106,13 @@ func WrapWithUserShell(logger *zap.Logger, command string, args []string) (shell
 	commandString := strings.Join(parts, " ")
 
 	if logger != nil {
+		// Redact secret env values (`-e KEY=VALUE`) before logging — docker-command
+		// upstreams inject Slack/Jira tokens here and debug logs are written to disk.
 		logger.Debug("shellwrap: wrapping command with user login shell",
 			zap.String("original_command", command),
-			zap.Strings("original_args", args),
+			zap.Strings("original_args", RedactDockerArgs(args)),
 			zap.String("shell", shell),
-			zap.String("wrapped_command", commandString))
+			zap.String("wrapped_command", RedactDockerCommandString(commandString)))
 	}
 
 	isBash := isBashLikeShell(shell)

internal/upstream/core/connection_stdio.go

@@ -102,7 +102,7 @@ func (c *Client) connectStdio(ctx context.Context) error {
 		c.logger.Debug("Docker command detected, setting up container ID tracking",
 			zap.String("server", c.config.Name),
 			zap.String("command", c.config.Command),
-			zap.Strings("original_args", args))
+			zap.Strings("original_args", shellwrap.RedactDockerArgs(args)))
 
 		// CRITICAL: Clean up any existing containers first to prevent duplicates
 		// This makes container creation idempotent and safe to call multiple times
@@ -166,7 +166,7 @@ func (c *Client) connectStdio(ctx context.Context) error {
 			c.logger.Debug("Injected env vars into direct docker command",
 				zap.String("server", c.config.Name),
 				zap.Int("env_count", len(c.config.Env)),
-				zap.Strings("modified_args", argsToWrap))
+				zap.Strings("modified_args", shellwrap.RedactDockerArgs(argsToWrap)))
 		}
 
 		// Use shell wrapping for environment inheritance
@@ -205,9 +205,9 @@ func (c *Client) connectStdio(ctx context.Context) error {
 	c.logger.Debug("Initialized stdio transport",
 		zap.String("server", c.config.Name),
 		zap.String("final_command", finalCommand),
-		zap.Strings("final_args", finalArgs),
+		zap.Strings("final_args", shellwrap.RedactDockerArgs(finalArgs)),
 		zap.String("original_command", c.config.Command),
-		zap.Strings("original_args", args),
+		zap.Strings("original_args", shellwrap.RedactDockerArgs(args)),
 		zap.String("working_dir", c.config.WorkingDir),
 		zap.Bool("docker_isolation", c.isDockerCommand))
 
@@ -392,7 +392,7 @@ func (c *Client) wrapWithUserShell(command string, args []string) (shellCommand
 	c.logger.Debug("Wrapping command with user shell for full environment inheritance",
 		zap.String("server", c.config.Name),
 		zap.String("original_command", command),
-		zap.Strings("original_args", args),
+		zap.Strings("original_args", shellwrap.RedactDockerArgs(args)),
 		zap.String("shell", shellCommand))
 	return shellCommand, shellArgs
 }

internal/upstream/core/process_redact_test.go

@@ -0,0 +1,48 @@
+//go:build unix
+
+package core
+
+import (
+	"context"
+	"fmt"
+	"strings"
+	"testing"
+
+	"go.uber.org/zap"
+	"go.uber.org/zap/zapcore"
+	"go.uber.org/zap/zaptest/observer"
+)
+
+// TestProcessGroupCommandFunc_RedactsSecretArgs is a guard test for GH #712:
+// the "Process group configuration applied" debug log must not emit cleartext
+// upstream secrets passed as `-e KEY=VALUE` to docker-command upstreams.
+func TestProcessGroupCommandFunc_RedactsSecretArgs(t *testing.T) {
+	const secretValue = "xoxc-abc123456789"
+
+	obsCore, recorded := observer.New(zapcore.DebugLevel)
+	logger := zap.New(obsCore)
+
+	// client=nil keeps this off the spawn path: the CommandFunc builds the
+	// *exec.Cmd and logs but never starts a process.
+	fn := createProcessGroupCommandFunc(nil, "", logger)
+
+	cases := [][]string{
+		// Shell-wrapped form (secret embedded in the -c command string).
+		{"-l", "-c", "docker run --rm -e SLACK_TOKEN=" + secretValue + " myimage"},
+		// Direct-exec form (two-token -e KEY=VALUE).
+		{"run", "--rm", "-e", "SLACK_TOKEN=" + secretValue, "myimage"},
+	}
+	for _, args := range cases {
+		if _, err := fn(context.Background(), "/bin/sh", nil, args); err != nil {
+			t.Fatalf("CommandFunc returned error: %v", err)
+		}
+	}
+
+	for _, entry := range recorded.All() {
+		for _, v := range entry.ContextMap() {
+			if strings.Contains(fmt.Sprintf("%v", v), secretValue) {
+				t.Errorf("cleartext secret leaked in log field: %v", v)
+			}
+		}
+	}
+}

internal/upstream/core/process_unix.go

@@ -8,6 +8,7 @@ import (
 	"syscall"
 	"time"
 
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/shellwrap"
 	"go.uber.org/zap"
 )
 
@@ -39,7 +40,7 @@ func createProcessGroupCommandFunc(client *Client, workingDir string, logger *za
 
 		logger.Debug("Process group configuration applied",
 			zap.String("command", command),
-			zap.Strings("args", args),
+			zap.Strings("args", shellwrap.RedactDockerArgs(args)),
 			zap.String("working_dir", workingDir))
 
 		if client != nil {

internal/upstream/core/process_windows.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"os/exec"
 
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/shellwrap"
 	"go.uber.org/zap"
 )
 
@@ -33,7 +34,7 @@ func createProcessGroupCommandFunc(client *Client, workingDir string, logger *za
 
 		logger.Debug("Process group configuration applied (Windows)",
 			zap.String("command", command),
-			zap.Strings("args", args),
+			zap.Strings("args", shellwrap.RedactDockerArgs(args)),
 			zap.String("working_dir", workingDir))
 
 		if client != nil {