fix(security): stop writing the admin API key to the log files

cmd/mcpproxy/api_key_banner.go

@@ -0,0 +1,126 @@
+package main
+
+import (
+	"fmt"
+	"io"
+	"os"
+	"strings"
+
+	"golang.org/x/term"
+
+	"go.uber.org/zap"
+)
+
+// generatedAPIKeyInfo describes a freshly auto-generated admin API key and what
+// happened when we tried to persist it to the config file.
+type generatedAPIKeyInfo struct {
+	APIKey     string
+	Listen     string
+	Source     string
+	ConfigPath string
+	SaveErr    error
+}
+
+// webUIURL is the ready-to-use Web UI URL, credential included. It is only ever
+// written to an interactive terminal.
+func (i generatedAPIKeyInfo) webUIURL() string {
+	return i.webUIURLWith(i.APIKey)
+}
+
+// webUIURLLogSafe is the same URL built with the key ALREADY masked.
+//
+// codex round 3 finding 1: building it with the raw key and handing the result
+// to a redactor is one parse failure away from publishing the key - `listen` is
+// operator-supplied, and a value url.Parse rejects sends the redactor down its
+// regex fallback, which has no `apikey` rule. The raw key is never put into a
+// string bound for the log sink in the first place.
+func (i generatedAPIKeyInfo) webUIURLLogSafe() string {
+	return i.webUIURLWith(maskAPIKey(i.APIKey))
+}
+
+func (i generatedAPIKeyInfo) webUIURLWith(key string) string {
+	return fmt.Sprintf("http://%s/ui/?apikey=%s", i.Listen, key)
+}
+
+// stderrIsTerminal reports whether the human sink is an interactive terminal.
+var stderrIsTerminal = func() bool {
+	return term.IsTerminal(int(os.Stderr.Fd()))
+}
+
+// announceGeneratedAPIKey reports a newly auto-generated admin API key.
+//
+// SEC-01: the raw key used to be written to the PERSISTENT log sink three
+// times - as `api_key`, embedded in the `web_ui_url` field, and once more on
+// the config-save-failure path - so ~/Library/Logs/mcpproxy/main.log held the
+// root credential in plaintext, with the log file's own permissions and its own
+// retention.
+//
+// The split: the raw key goes only to `out` (stderr), and only when stderr is
+// an interactive terminal. The operator has to be able to see it once, but a
+// service manager or a CI redirect persists that stream just like a log file,
+// so withholding it there is the whole point rather than an inconvenience
+// (codex round 3 finding 2). The log sink gets the masked prefix, a Web-UI URL
+// built with the key already masked, and the config path - which is where the
+// key is authoritatively stored.
+//
+// Never stdout: an empty or ":0" listen address selects the stdio MCP
+// transport, where stdout carries JSON-RPC frames.
+func announceGeneratedAPIKey(logger *zap.Logger, out io.Writer, isTTY bool, info generatedAPIKeyInfo) {
+	logger.Warn("API key was auto-generated for security",
+		zap.String("api_key_prefix", maskAPIKey(info.APIKey)),
+		zap.String("web_ui_url", info.webUIURLLogSafe()),
+		zap.String("config_path", info.ConfigPath),
+		zap.String("source", info.Source))
+
+	if info.SaveErr != nil {
+		logger.Warn("Failed to save the auto-generated API key to the config file; "+
+			"it cannot be recovered and a different key will be generated on the next restart. "+
+			"Set MCPPROXY_API_KEY, or make the config path writable, and restart",
+			zap.Error(info.SaveErr),
+			zap.String("config_path", info.ConfigPath))
+	} else {
+		logger.Info("Auto-generated API key saved to config file",
+			zap.String("config_path", info.ConfigPath))
+	}
+
+	writeGeneratedAPIKeyBanner(out, isTTY, info)
+}
+
+// writeGeneratedAPIKeyBanner writes the human-facing banner. On a terminal it
+// carries the key itself; otherwise it only says where the key lives, or - when
+// it could not be stored anywhere - how to supply one that can.
+func writeGeneratedAPIKeyBanner(out io.Writer, isTTY bool, info generatedAPIKeyInfo) {
+	if out == nil {
+		return
+	}
+	frame := strings.Repeat("*", 80)
+	var b strings.Builder
+
+	b.WriteString(frame + "\n")
+	b.WriteString("An API key was auto-generated for this instance.\n")
+
+	if isTTY {
+		b.WriteString("API key:    " + info.APIKey + "\n")
+		b.WriteString("Web UI:     " + info.webUIURL() + "\n")
+	} else {
+		b.WriteString("The key is not printed here because this output is not a terminal,\n")
+		b.WriteString("and a redirected stream is as persistent as a log file.\n")
+	}
+
+	if info.SaveErr != nil {
+		b.WriteString("WARNING: it could NOT be saved to " + info.ConfigPath + "\n")
+		b.WriteString("         (" + info.SaveErr.Error() + ")\n")
+		if !isTTY {
+			b.WriteString("There is therefore no way to read this key back. Set MCPPROXY_API_KEY to a\n")
+			b.WriteString("key of your own, or make that path writable, and restart.\n")
+		} else {
+			b.WriteString("Copy it now: a different key is generated on the next restart.\n")
+		}
+	} else {
+		b.WriteString("Stored in:  " + info.ConfigPath + " (field \"api_key\")\n")
+		b.WriteString("For your security it is no longer written to the log files.\n")
+	}
+	b.WriteString(frame + "\n")
+
+	_, _ = io.WriteString(out, b.String())
+}

cmd/mcpproxy/api_key_banner_test.go

@@ -0,0 +1,159 @@
+package main
+
+import (
+	"bytes"
+	"errors"
+	"fmt"
+	"io"
+	"strings"
+	"testing"
+
+	"go.uber.org/zap"
+	"go.uber.org/zap/zapcore"
+	"go.uber.org/zap/zaptest/observer"
+)
+
+// SEC-01: on auto-generation the raw admin API key was written into the
+// persistent log sink (~/Library/Logs/mcpproxy/main.log) three times - as the
+// `api_key` field, embedded in the `web_ui_url` field, and again on the
+// config-save-failure path. The operator still has to SEE the key once, so it
+// goes to the human sink (stderr) only; the log file gets the masked prefix and
+// a redacted URL.
+const bannerSecret = "9f2c4d6e8a0b1c3d5e7f9a1b3c5d7e9f"
+
+func renderObserved(t *testing.T, logs *observer.ObservedLogs) string {
+	t.Helper()
+	var sb strings.Builder
+	for _, e := range logs.All() {
+		sb.WriteString(e.Message)
+		for k, v := range e.ContextMap() {
+			fmt.Fprintf(&sb, " %s=%v", k, v)
+		}
+		sb.WriteString("\n")
+	}
+	return sb.String()
+}
+
+func TestGeneratedAPIKeyBanner_KeyNeverReachesLogSink(t *testing.T) {
+	for _, tc := range []struct {
+		name    string
+		saveErr error
+	}{
+		{name: "persisted"},
+		{name: "save failed", saveErr: errors.New("permission denied")},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			core, logs := observer.New(zapcore.DebugLevel)
+			var out bytes.Buffer
+
+			announceGeneratedAPIKey(zap.New(core), &out, true, generatedAPIKeyInfo{
+				APIKey:     bannerSecret,
+				Listen:     "127.0.0.1:8080",
+				Source:     "generated",
+				ConfigPath: "/tmp/mcp_config.json",
+				SaveErr:    tc.saveErr,
+			})
+
+			rendered := renderObserved(t, logs)
+			if strings.Contains(rendered, bannerSecret) {
+				t.Fatalf("SEC-01: raw API key reached the persistent log sink:\n%s", rendered)
+			}
+			if !strings.Contains(rendered, maskAPIKey(bannerSecret)) {
+				t.Fatalf("log sink should still carry the masked key prefix, got:\n%s", rendered)
+			}
+
+			banner := out.String()
+			if !strings.Contains(banner, bannerSecret) {
+				t.Fatalf("the operator must be able to see the key once on the human sink, got:\n%s", banner)
+			}
+			if !strings.Contains(banner, "http://127.0.0.1:8080/ui/?apikey="+bannerSecret) {
+				t.Fatalf("banner should carry the ready-to-use Web UI URL, got:\n%s", banner)
+			}
+			if !strings.Contains(banner, "/tmp/mcp_config.json") {
+				t.Fatalf("banner should name the config path, got:\n%s", banner)
+			}
+		})
+	}
+}
+
+// When stderr is not a terminal (systemd, launchd, a CI redirect) the raw key
+// must not be blurted into whatever file the stream was redirected to; the
+// operator is pointed at the config file instead.
+func TestGeneratedAPIKeyBanner_NonTTYDoesNotPrintTheKey(t *testing.T) {
+	core, logs := observer.New(zapcore.DebugLevel)
+	var out bytes.Buffer
+
+	announceGeneratedAPIKey(zap.New(core), &out, false, generatedAPIKeyInfo{
+		APIKey:     bannerSecret,
+		Listen:     "127.0.0.1:8080",
+		Source:     "generated",
+		ConfigPath: "/tmp/mcp_config.json",
+	})
+
+	if strings.Contains(out.String(), bannerSecret) {
+		t.Fatalf("non-TTY stderr must not carry the raw key, got:\n%s", out.String())
+	}
+	if !strings.Contains(out.String(), "/tmp/mcp_config.json") {
+		t.Fatalf("non-TTY banner must point at the config file, got:\n%s", out.String())
+	}
+	if strings.Contains(renderObserved(t, logs), bannerSecret) {
+		t.Fatal("SEC-01: raw API key reached the persistent log sink")
+	}
+}
+
+// codex rounds 1 and 3 finding 2. Round 1 objected that withholding the key on
+// a non-terminal stream leaves an unsaved key unrecoverable; round 3 objected
+// that printing it there is the very exposure this change removes, because
+// launchd, systemd and CI all persist that stream. Round 3 wins: the raw key is
+// never written to a non-terminal sink. What the operator gets instead is the
+// action that fixes it.
+func TestGeneratedAPIKeyBanner_NonTTYWithdrawsTheKeyEvenWhenUnsaved(t *testing.T) {
+	core, logs := observer.New(zapcore.DebugLevel)
+	var out bytes.Buffer
+
+	announceGeneratedAPIKey(zap.New(core), &out, false, generatedAPIKeyInfo{
+		APIKey:     bannerSecret,
+		Listen:     "127.0.0.1:8080",
+		Source:     "generated",
+		ConfigPath: "/read-only/mcp_config.json",
+		SaveErr:    errors.New("permission denied"),
+	})
+
+	banner := out.String()
+	if strings.Contains(banner, bannerSecret) {
+		t.Fatalf("SEC-01: raw key written to a non-terminal stream:\n%s", banner)
+	}
+	if !strings.Contains(banner, "MCPPROXY_API_KEY") {
+		t.Fatalf("banner must name the way out when the key cannot be stored, got:\n%s", banner)
+	}
+	if strings.Contains(renderObserved(t, logs), bannerSecret) {
+		t.Fatal("SEC-01: raw API key reached the persistent log sink")
+	}
+}
+
+// codex round 3 finding 1: the logged web_ui_url used to be BUILT with the raw
+// key and then handed to a redactor. `listen` is operator-supplied, so one
+// value url.Parse rejects sent the redactor down a regex fallback with no
+// `apikey` rule and republished the key. It is now built pre-masked, so no
+// string carrying the raw key is ever handed to the logger at all.
+func TestGeneratedAPIKeyBanner_MalformedListenCannotLeakTheKey(t *testing.T) {
+	for _, listen := range []string{
+		"127.0.0.1:8080",
+		"%zz",
+		"127.0.0.1:8080/\x7f",
+		"",
+		":0",
+		"user:pw@host:8080",
+	} {
+		core, logs := observer.New(zapcore.DebugLevel)
+		announceGeneratedAPIKey(zap.New(core), io.Discard, false, generatedAPIKeyInfo{
+			APIKey:     bannerSecret,
+			Listen:     listen,
+			Source:     "generated",
+			ConfigPath: "/tmp/mcp_config.json",
+		})
+		if rendered := renderObserved(t, logs); strings.Contains(rendered, bannerSecret) {
+			t.Fatalf("listen=%q put the raw key in the log sink:\n%s", listen, rendered)
+		}
+	}
+}

cmd/mcpproxy/main.go

@@ -514,31 +514,20 @@ func runServer(cmd *cobra.Command, _ []string) error {
 	}
 
 	if wasGenerated {
-		// Frame the auto-generated key message for visibility
-		frameMsg := strings.Repeat("*", 80)
-		logger.Warn(frameMsg)
-		logger.Warn("API key was auto-generated for security. To access the Web UI and REST API, use this key:")
-		logger.Warn("",
-			zap.String("api_key", apiKey),
-			zap.String("web_ui_url", fmt.Sprintf("http://%s/ui/?apikey=%s", cfg.Listen, apiKey)),
-			zap.String("source", source.String()))
-		logger.Warn("Note: This key will be saved to your config file for persistence")
-		logger.Warn(frameMsg)
-
-		// Save the auto-generated key to config file for persistence
+		// Save the auto-generated key to config file for persistence, then
+		// report it. SEC-01: the raw key goes to the human sink only - see
+		// announceGeneratedAPIKey.
 		saver.setGeneratedAPIKey(apiKey)
 		configPathToSave := saver.path
-
-		if err := saver.save(cfg, configPathToSave); err != nil {
-			logger.Warn("Failed to save auto-generated API key to config file",
-				zap.Error(err),
-				zap.String("config_path", configPathToSave))
-			logger.Warn("The API key will be regenerated on next restart. To persist it, manually add it to your config file:")
-			logger.Warn("", zap.String("api_key", apiKey))
-		} else {
-			logger.Info("Auto-generated API key saved to config file",
-				zap.String("config_path", configPathToSave))
-		}
+		saveErr := saver.save(cfg, configPathToSave)
+
+		announceGeneratedAPIKey(logger, os.Stderr, stderrIsTerminal(), generatedAPIKeyInfo{
+			APIKey:     apiKey,
+			Listen:     cfg.Listen,
+			Source:     source.String(),
+			ConfigPath: configPathToSave,
+			SaveErr:    saveErr,
+		})
 	} else {
 		// Mask API key when it comes from environment or config file
 		maskedKey := maskAPIKey(apiKey)

docs/configuration.md

@@ -670,7 +670,7 @@ See [OAuth Documentation](mcp-go-oauth.md) for complete details.
 
 | Field | Type | Default | Description |
 |-------|------|---------|-------------|
-| `api_key` | string | Auto-generated | API key for REST API authentication. Required; if empty, one is auto-generated and enforced (logged on startup) |
+| `api_key` | string | Auto-generated | API key for REST API authentication. Required; if empty, one is auto-generated, enforced, and written back to this config file (printed to the terminal once on first run; never written to the log files) |
 | `trusted_hosts` | string[] | `[]` | Non-loopback `Host` header values accepted on loopback listeners (reverse-proxy deployments). See below |
 | `trusted_proxies` | string[] | `[]` (trust nobody) | CIDRs or IP addresses whose `X-Forwarded-For` / `X-Real-IP` / `X-Forwarded-Proto` / `X-Forwarded-Host` headers are honoured; any other peer's forwarded headers are ignored and `RemoteAddr` is used. Env `MCPPROXY_TRUSTED_PROXIES`. Hot-reloadable. Invalid entry: `trusted_proxies[N] "value" is not a valid CIDR or IP address` (boot, PATCH and apply). See [Reverse Proxy Deployment](operations/reverse-proxy.md#trusted_proxies-forwarded-headers) |
 | `read_only_mode` | boolean | `false` | Prevent all configuration modifications |
@@ -681,7 +681,7 @@ See [OAuth Documentation](mcp-go-oauth.md) for complete details.
 **Security Notes:**
 - **API Key**: Set via `--api-key` flag, `MCPPROXY_API_KEY` environment variable, or config file
 - **Empty API Key**: Empty values are replaced with an auto-generated key; authentication is always enforced
-- **Auto-Generation**: If no API key is provided, one is generated and logged for easy access
+- **Auto-Generation**: If no API key is provided, one is generated, persisted to the config file, and printed once to the terminal (stderr). It is deliberately **not** written to the log files - to recover it later, read `api_key` from `~/.mcpproxy/mcp_config.json`
 - **Tray Integration**: Tray app automatically manages API keys for core communication
 
 ## Audit Log

docs/operations/reverse-proxy.md

@@ -173,8 +173,10 @@ Exposing MCPProxy beyond localhost changes the threat model. Two endpoint famili
 authenticate differently:
 
 - **REST API (`/api/v1/...`)** — an API key is **always** required. Pass it as the
-  `X-API-Key` header (recommended) or the `?apikey=` query parameter. The key is
-  auto-generated and logged on first start if you don't set one.
+  `X-API-Key` header (recommended) or the `?apikey=` query parameter. If you don't
+  set one, the key is auto-generated on first start, printed once to the terminal
+  and written to your config file; it is never written to the log files, so read
+  `api_key` from `~/.mcpproxy/mcp_config.json` to recover it.
 - **MCP endpoint (`/mcp`)** — **unauthenticated by default** for client
   compatibility. When you expose MCPProxy through a reverse proxy, enable
   `require_mcp_auth` so `/mcp` also rejects unauthenticated requests: