From 1800e41d386aa9d2a46d9b9d806891db575e54d9 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 19:11:51 +0300 Subject: [PATCH 1/3] fix(security): stop writing the admin API key to the log files SEC-01. The root API key was landing in plaintext on disk at two places. internal/httpapi's access logger wrote `query`, `referer` and `path` verbatim for every request the router sees. `?apikey=` is an accepted credential source, the Web UI's SSE stream and the tray client both send the admin key that way, and the Web UI is opened as `/ui/?apikey=` so same-origin requests carry it in the Referer too - so http.log held the credential. cmd/mcpproxy logged a freshly auto-generated key three times at Warn: as `api_key`, inside `web_ui_url`, and again on the config-save-failure path - so main.log held it as well. The log fields now go through internal/oauth, which already owns the repo's redaction rules: the query-parameter name rule was extracted out of RedactURLQueryParamsWith into redactRawQueryParams so there is still exactly one implementation, and two renderers are built on it for input that arrives from an untrusted client and must not be assumed to parse. The generated key is printed once to an interactive terminal and written to the config file; the log sink gets only the masked prefix. The E2E scripts read the key from the config file instead of grepping it out of the server log. Co-Authored-By: Claude Opus 5 --- cmd/mcpproxy/api_key_banner.go | 126 +++++++ cmd/mcpproxy/api_key_banner_test.go | 159 +++++++++ cmd/mcpproxy/main.go | 35 +- docs/configuration.md | 4 +- docs/operations/reverse-proxy.md | 6 +- .../httpapi/http_logging_redaction_test.go | 138 ++++++++ internal/httpapi/server.go | 36 +- internal/oauth/logging.go | 217 ++++++++++-- internal/oauth/logging_query_test.go | 310 ++++++++++++++++++ .../oauth/round8_renderer_discovery_test.go | 3 + scripts/test-api-e2e.sh | 26 +- scripts/test-extract-api-key.sh | 99 ++++-- 12 files changed, 1047 insertions(+), 112 deletions(-) create mode 100644 cmd/mcpproxy/api_key_banner.go create mode 100644 cmd/mcpproxy/api_key_banner_test.go create mode 100644 internal/httpapi/http_logging_redaction_test.go create mode 100644 internal/oauth/logging_query_test.go diff --git a/cmd/mcpproxy/api_key_banner.go b/cmd/mcpproxy/api_key_banner.go new file mode 100644 index 000000000..627e7346f --- /dev/null +++ b/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()) +} diff --git a/cmd/mcpproxy/api_key_banner_test.go b/cmd/mcpproxy/api_key_banner_test.go new file mode 100644 index 000000000..2b237bbe5 --- /dev/null +++ b/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) + } + } +} diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index 7e6937295..4a2a8d1e8 100644 --- a/cmd/mcpproxy/main.go +++ b/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) diff --git a/docs/configuration.md b/docs/configuration.md index 92e530e97..1ebd1eed2 100644 --- a/docs/configuration.md +++ b/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 diff --git a/docs/operations/reverse-proxy.md b/docs/operations/reverse-proxy.md index 4e254d0e5..70e1a7f7e 100644 --- a/docs/operations/reverse-proxy.md +++ b/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: diff --git a/internal/httpapi/http_logging_redaction_test.go b/internal/httpapi/http_logging_redaction_test.go new file mode 100644 index 000000000..9092c1939 --- /dev/null +++ b/internal/httpapi/http_logging_redaction_test.go @@ -0,0 +1,138 @@ +package httpapi + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" +) + +// SEC-01: the access logger writes every request's raw query string and Referer +// to ~/Library/Logs/mcpproxy/http.log. `?apikey=` is an accepted credential +// source (Server.resolveAuth), and the Web UI SSE stream plus the tray client +// both send the ROOT admin key that way, so the credential was landing at rest +// in a plaintext log file. +// +// The middleware only touches s.httpLogger, so no router or live server is +// needed here. +const sec01Secret = "SUPERSECRETKEY0123456789abcdef01" + +func newLoggingMiddlewareHarness(t *testing.T) (http.Handler, *observer.ObservedLogs) { + t.Helper() + core, logs := observer.New(zapcore.InfoLevel) + srv := &Server{httpLogger: zap.New(core)} + h := srv.httpLoggingMiddleware()(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + return h, logs +} + +// renderedFields flattens one observed entry into a single string holding the +// message and every field value, which is what actually reaches the log file. +func renderedFields(t *testing.T, logs *observer.ObservedLogs) (string, map[string]string) { + t.Helper() + all := logs.All() + if len(all) != 1 { + t.Fatalf("expected exactly 1 log entry, got %d", len(all)) + } + entry := all[0] + var sb strings.Builder + sb.WriteString(entry.Message) + fields := map[string]string{} + for k, v := range entry.ContextMap() { + sb.WriteString(" ") + s := fmt.Sprintf("%v", v) + sb.WriteString(k) + sb.WriteString("=") + sb.WriteString(s) + fields[k] = s + } + return sb.String(), fields +} + +func TestHTTPLoggingMiddleware_RedactsCredentialQuery(t *testing.T) { + h, logs := newLoggingMiddlewareHarness(t) + + req := httptest.NewRequest(http.MethodGet, "/events?apikey="+sec01Secret+"&foo=bar", nil) + req.Header.Set("Referer", "http://127.0.0.1:8080/ui/?apikey="+sec01Secret) + h.ServeHTTP(httptest.NewRecorder(), req) + + rendered, fields := renderedFields(t, logs) + if strings.Contains(rendered, sec01Secret) { + t.Fatalf("SEC-01: raw API key reached the http log line: %s", rendered) + } + if !strings.Contains(fields["query"], "foo=bar") { + t.Fatalf("non-sensitive query parameter must survive verbatim, got %q", fields["query"]) + } + if !strings.Contains(fields["query"], "apikey") { + t.Fatalf("the parameter NAME is the diagnostic value of the line, got %q", fields["query"]) + } +} + +// codex round 1 finding 1: a Referer is entirely client-controlled and does not +// have to parse. Before the fix, one bad escape in its path made the redactor +// fall back to a regex that has no rule for `apikey`, so the 64-hex admin key +// reached http.log verbatim. +func TestHTTPLoggingMiddleware_RedactsMalformedReferer(t *testing.T) { + const adminKey = "6930184070f362383e7cddbd1184b3b8ed66f84717eedd8906ab8743dfa746cd" + + for _, referer := range []string{ + "http://127.0.0.1:8080/%zz?apikey=" + adminKey, + "http://127.0.0.1:8080/ui%/?apikey=" + adminKey, + "http://127.0.0.1:8080/ui/?apikey=" + adminKey, + } { + h, logs := newLoggingMiddlewareHarness(t) + req := httptest.NewRequest(http.MethodGet, "/api/v1/servers", nil) + req.Header.Set("Referer", referer) + h.ServeHTTP(httptest.NewRecorder(), req) + + rendered, _ := renderedFields(t, logs) + if strings.Contains(rendered, adminKey) { + t.Fatalf("admin key reached the log line via Referer %q: %s", referer, rendered) + } + } +} + +func TestHTTPLoggingMiddleware_RedactionTable(t *testing.T) { + cases := []struct { + name string + rawQuery string + mustSurive []string + }{ + {name: "empty query", rawQuery: "", mustSurive: nil}, + {name: "unparseable query", rawQuery: "%zz&apikey=" + sec01Secret}, + {name: "token param", rawQuery: "token=" + sec01Secret}, + {name: "key param", rawQuery: "key=" + sec01Secret + "&page=2", mustSurive: []string{"page=2"}}, + {name: "api_key param", rawQuery: "api_key=" + sec01Secret}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + h, logs := newLoggingMiddlewareHarness(t) + target := "/api/v1/servers" + if tc.rawQuery != "" { + target += "?" + tc.rawQuery + } + req := httptest.NewRequest(http.MethodGet, target, nil) + h.ServeHTTP(httptest.NewRecorder(), req) + + rendered, fields := renderedFields(t, logs) + if strings.Contains(rendered, sec01Secret) { + t.Fatalf("raw credential reached the log line: %s", rendered) + } + if tc.rawQuery == "" && fields["query"] != "" { + t.Fatalf("empty query must stay empty, got %q", fields["query"]) + } + for _, want := range tc.mustSurive { + if !strings.Contains(fields["query"], want) { + t.Fatalf("expected %q to survive in %q", want, fields["query"]) + } + } + }) + } +} diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 894093ae9..b7f078f99 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -475,7 +475,7 @@ func (s *Server) apiKeyAuthMiddleware() func(http.Handler) http.Handler { source := transport.GetConnectionSource(r.Context()) if source == transport.ConnectionSourceTray { s.logger.Debugw("Tray connection - skipping API key validation", - zap.String("path", r.URL.Path), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), zap.String("remote_addr", r.RemoteAddr), zap.String("source", string(source))) ctx := auth.WithAuthContext(r.Context(), auth.AdminContext()) @@ -503,7 +503,7 @@ func (s *Server) apiKeyAuthMiddleware() func(http.Handler) http.Handler { // Empty API key is not allowed - this prevents accidental exposure if cfg.APIKey == "" { s.logger.Warnw("TCP connection rejected - API key not configured", - zap.String("path", r.URL.Path), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), zap.String("remote_addr", r.RemoteAddr)) s.writeError(w, r, http.StatusUnauthorized, "API key authentication required but not configured. Please set MCPPROXY_API_KEY or configure api_key in config file.") return @@ -549,7 +549,7 @@ func (s *Server) authenticateWithPrecedence(w http.ResponseWriter, r *http.Reque } s.logger.Warnw("TCP connection with missing API key", - zap.String("path", r.URL.Path), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), zap.String("remote_addr", r.RemoteAddr)) s.writeError(w, r, http.StatusUnauthorized, "Invalid or missing API key") } @@ -565,14 +565,14 @@ func (s *Server) authenticateExplicitToken(w http.ResponseWriter, r *http.Reques } if token != "" && token == cfg.APIKey { s.logger.Debugw("TCP connection with valid API key", - zap.String("path", r.URL.Path), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), zap.String("remote_addr", r.RemoteAddr)) ctx := auth.WithAuthContext(r.Context(), auth.AdminContext()) next.ServeHTTP(w, r.WithContext(ctx)) return } s.logger.Warnw("TCP connection with invalid API key", - zap.String("path", r.URL.Path), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), zap.String("remote_addr", r.RemoteAddr)) s.writeError(w, r, http.StatusUnauthorized, "Invalid or missing API key") } @@ -592,7 +592,7 @@ func (s *Server) authenticateBearer(w http.ResponseWriter, r *http.Request, next } if token != "" && token == cfg.APIKey { s.logger.Debugw("TCP connection with valid API key", - zap.String("path", r.URL.Path), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), zap.String("remote_addr", r.RemoteAddr)) ctx := auth.WithAuthContext(r.Context(), auth.AdminContext()) next.ServeHTTP(w, r.WithContext(ctx)) @@ -604,7 +604,7 @@ func (s *Server) authenticateBearer(w http.ResponseWriter, r *http.Request, next } s.logger.Warnw("TCP connection with invalid API key", - zap.String("path", r.URL.Path), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), zap.String("remote_addr", r.RemoteAddr)) s.writeError(w, r, http.StatusUnauthorized, "Invalid or missing API key") } @@ -613,7 +613,7 @@ func (s *Server) authenticateBearer(w http.ResponseWriter, r *http.Request, next func (s *Server) handleAgentTokenAuth(w http.ResponseWriter, r *http.Request, next http.Handler, token string) { if s.tokenStore == nil || s.dataDir == "" { s.logger.Warnw("Agent token presented but token store not configured", - zap.String("path", r.URL.Path), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), zap.String("remote_addr", r.RemoteAddr)) s.writeError(w, r, http.StatusUnauthorized, "Agent tokens are not configured on this server") return @@ -629,7 +629,7 @@ func (s *Server) handleAgentTokenAuth(w http.ResponseWriter, r *http.Request, ne agentToken, err := s.tokenStore.ValidateAgentToken(token, hmacKey) if err != nil { s.logger.Warnw("Agent token validation failed", - zap.String("path", r.URL.Path), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), zap.String("remote_addr", r.RemoteAddr), zap.String("error", err.Error())) s.writeError(w, r, http.StatusUnauthorized, fmt.Sprintf("Agent token invalid: %s", err.Error())) @@ -665,7 +665,7 @@ func (s *Server) handleAgentTokenAuth(w http.ResponseWriter, r *http.Request, ne s.logger.Debugw("Agent token authenticated", zap.String("agent_name", agentToken.Name), zap.String("token_prefix", agentToken.TokenPrefix), - zap.String("path", r.URL.Path), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), zap.String("remote_addr", r.RemoteAddr)) next.ServeHTTP(w, r.WithContext(ctx)) @@ -1139,16 +1139,24 @@ func (s *Server) httpLoggingMiddleware() func(http.Handler) http.Handler { duration := time.Since(start) - // Log request details to http.log + // Log request details to http.log. + // + // SEC-01: `query` and `referer` both carry `?apikey=` — it is an + // accepted credential source (see resolveAuth), the Web UI's SSE + // stream and the tray client send the ROOT admin key that way, and + // the Web UI is opened as /ui/?apikey= so same-origin + // subresource requests put it in the Referer too. Unredacted, this + // line wrote the admin credential to disk on every request. The + // renderers are internal/oauth's — one rule for every log sink. s.httpLogger.Info("HTTP API Request", zap.String("method", r.Method), - zap.String("path", r.URL.Path), - zap.String("query", r.URL.RawQuery), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), + zap.String("query", oauth.LogSafeQueryString(r.URL.RawQuery)), zap.String("remote_addr", r.RemoteAddr), zap.String("user_agent", r.UserAgent()), zap.Int("status", ww.statusCode), zap.Duration("duration", duration), - zap.String("referer", r.Referer()), + zap.String("referer", oauth.LogSafeRequestURL(r.Referer())), zap.Int64("content_length", r.ContentLength), ) }) diff --git a/internal/oauth/logging.go b/internal/oauth/logging.go index 552ea9063..12eb3bc32 100644 --- a/internal/oauth/logging.go +++ b/internal/oauth/logging.go @@ -452,39 +452,9 @@ func RedactURLQueryParamsWith(rawURL string, mask func(string) string) string { } } - // Edit RawQuery by hand rather than via url.Values.Encode(): Encode - // re-percent-encodes and reorders every parameter, which would mangle - // reference values like ${env:NAME} into an unrecognizable form and - // defeat the UI's keyring-chip detection. Here only the masked value - // changes; untouched parameters keep their exact original bytes. if u.RawQuery != "" { - parts := strings.Split(u.RawQuery, "&") - queryChanged := false - for i, part := range parts { - eq := strings.IndexByte(part, '=') - if eq < 0 { - continue - } - key := part[:eq] - decKey, keyErr := url.QueryUnescape(key) - if keyErr != nil { - decKey = key - } - if !isSensitiveQueryParam(decKey) { - continue - } - decVal, valErr := url.QueryUnescape(part[eq+1:]) - if valErr != nil { - decVal = part[eq+1:] - } - if isConfigReference(decVal) { - continue - } - parts[i] = key + "=" + url.QueryEscape(mask(decVal)) - queryChanged = true - } - if queryChanged { - u.RawQuery = strings.Join(parts, "&") + if masked := redactRawQueryParams(u.RawQuery, mask); masked != u.RawQuery { + u.RawQuery = masked changed = true } } @@ -495,6 +465,60 @@ func RedactURLQueryParamsWith(rawURL string, mask func(string) string) string { return u.String() } +// redactRawQueryParams masks the values of sensitive parameters in a BARE query +// string — no scheme, host or path, and no url.Parse anywhere in it. +// +// It is the one place the name rule is applied to a query, called both by +// RedactURLQueryParamsWith (which hands it a parsed u.RawQuery) and by +// LogSafeQueryString (which has only the raw bytes of an http.Request's +// RawQuery). Keeping it parse-free is what makes the second caller FAIL-CLOSED: +// a query is not required to be parseable as part of a URL — an HTAB is legal +// in an HTTP header value and a literal '#' is legal in a request target — and +// anything that fell back to a regex on those inputs published the credential. +// +// The query is edited by hand rather than through url.Values.Encode(): Encode +// re-percent-encodes and reorders every parameter, which would mangle reference +// values like ${env:NAME} into an unrecognizable form and defeat the UI's +// keyring-chip detection. Here only a masked value's bytes change; every +// untouched parameter keeps its exact original bytes and position. +// +// A component with no '=' has no name to judge and is left alone; the +// value-shaped detector the callers run afterwards is what covers it. +func redactRawQueryParams(rawQuery string, mask func(string) string) string { + if rawQuery == "" { + return rawQuery + } + parts := strings.Split(rawQuery, "&") + changed := false + for i, part := range parts { + eq := strings.IndexByte(part, '=') + if eq < 0 { + continue + } + key := part[:eq] + decKey, keyErr := url.QueryUnescape(key) + if keyErr != nil { + decKey = key + } + if !isSensitiveQueryParam(decKey) { + continue + } + decVal, valErr := url.QueryUnescape(part[eq+1:]) + if valErr != nil { + decVal = part[eq+1:] + } + if isConfigReference(decVal) { + continue + } + parts[i] = key + "=" + url.QueryEscape(mask(decVal)) + changed = true + } + if !changed { + return rawQuery + } + return strings.Join(parts, "&") +} + // configRefPattern matches a value that is ENTIRELY a single ${keyring:NAME} or // ${env:VAR} reference. Anchored on both ends (Issue #872, Codex round) so a // composite like `${env:NAME}garbage` — which a prefix check would wave through @@ -987,6 +1011,135 @@ func LogSafeURL(rawURL string) string { return AuditRedaction.URLValueDeep(rawURL) } +// LogSafeQueryString is LogSafeURL for a BARE query string — `r.URL.RawQuery`, +// with no scheme, host or path in front of it. +// +// SEC-01: internal/httpapi's access logger writes `zap.String("query", +// r.URL.RawQuery)` for every request the router sees, and `?apikey=` is an +// accepted credential source, so the root admin key was landing at rest in +// http.log. +// +// It runs the shared name rule (redactRawQueryParams) and then the same +// value-shaped detector AuditRedaction applies to a whole URL, so a credential +// under an unrecognised parameter name is still caught. What it deliberately +// does NOT do is go through url.Parse (codex round 2): a request's query is not +// required to be parseable as part of a URL — an HTAB is legal inside an HTTP +// header value, and url.ParseRequestURI leaves a literal '#' in RawQuery — and +// every parse-dependent path falls back to a regex with no `apikey` rule, which +// is how the credential got published in the first place. +func LogSafeQueryString(rawQuery string) string { + if rawQuery == "" { + return rawQuery + } + return AuditRedaction.finish(rawQuery, redactRawQueryParams(rawQuery, AuditRedaction.masker())) +} + +// LogSafeRequestURL renders a URL that arrived from an UNTRUSTED client for a +// log field - a Referer header, most of all. Unlike LogSafeURL it assumes +// nothing: not that the URL parses, and not that the credential is in the query. +// +// SEC-01, codex rounds 1-4. Every assumption LogSafeURL makes was a way for the +// admin key to reach http.log verbatim, because a client controls its Referer +// completely and a 64-character hex key has no vendor shape for the value +// detector to catch on its own: +// +// - url.Parse rejects the URL (`http://host/%zz?apikey=`, or an HTAB, +// which is legal inside a header value) and the redactor drops to the regex +// RedactURL, which has no `apikey` rule at all; +// - the credential sits in the FRAGMENT (`http://host/ui/#?apikey=`, or +// hash routing `#/servers?apikey=`), which no URL renderer applies the +// name rule to; +// - the credential sits in the PATH (`http://host/ui/apikey=`), likewise; +// - the parameter NAME is percent-encoded (`api%6bey=`), which the +// free-form scrubber cannot see because it never decodes. +// +// So the URL is taken apart by hand and every piece is redacted in its own +// right, with no parse anywhere on the path: userinfo first, against the WHOLE +// raw string, because in a malformed URL either delimiter can fall between the +// password and its '@' (`https://user:?x@`, `https://user:#x@`) and +// leave nothing for the userinfo rule to match; then the fragment, the query, +// and what is left. +// +// The one thing this does NOT do, which LogSafeURL does, is decode a query +// parameter whose VALUE is itself a URL (the RFC 8707 `resource` parameter). +// That is a shape mcpproxy's own authorize URLs have and a browser Referer does +// not, and buying it back would mean parsing - the one thing an untrusted URL +// cannot be relied on to survive. +func LogSafeRequestURL(rawURL string) string { + if rawURL == "" { + return rawURL + } + scrubbed := redactURLUserinfo(rawURL) + main, fragment, hasFragment := strings.Cut(scrubbed, "#") + base, rawQuery, hasQuery := strings.Cut(main, "?") + + out := logSafeURLComponent(base) + if hasQuery { + out += "?" + LogSafeQueryString(rawQuery) + } + if hasFragment { + out += "#" + logSafeFragment(fragment) + } + return out +} + +// LogSafeRequestPath renders a request PATH for a log field. +// +// SEC-01, codex round 6 finding 2. internal/httpapi logs `r.URL.Path` on the +// access line and on every authentication decision, and a path is as +// client-controlled as the query beside it, so it had the same shape of +// exposure LogSafeRequestURL now closes for the Referer. +func LogSafeRequestPath(path string) string { + return logSafeURLComponent(path) +} + +// logSafeURLComponent renders the part of a URL that is NOT a query string - +// scheme, host and path, or the part of a fragment before its own '?'. +// +// The name rule runs per PATH SEGMENT, which is what makes +// `…/ui/api%6bey=` reachable by it at all: it matches on the DECODED, +// normalised name, and only when that name is the whole of what precedes the +// '='. Then the value-shaped detector runs over the result, for a credential +// no name rule can see. +// +// ScrubUpstreamText is deliberately NOT used whole, though it is the standard +// rule for free-form text (codex round 6 finding 3). Its secretPattern is +// unanchored, so it rewrites `/monkey=banana` to `/monkey=***REDACTED***` - +// acceptable for an error string nobody parses, a daily annoyance in the `path` +// field of an access log, and redundant here because the per-segment name rule +// covers the same ground with decoding and a real word boundary. +// +// Its other two rules do earn their place and are applied on their own (codex +// round 7): a `user:pass@` in an embedded URL and a `Bearer ` - which a +// decoded path really can carry, since r.URL.Path arrives percent-decoded - are +// exactly the shapes no name rule and no value detector recognise. +func logSafeURLComponent(s string) string { + if s == "" { + return s + } + s = tokenPattern.ReplaceAllString(redactURLUserinfo(s), "${1}"+redactedMarker) + segments := strings.Split(s, "/") + for i, segment := range segments { + segments[i] = LogSafeQueryString(segment) + } + return MaskDetectedSecrets(strings.Join(segments, "/")) +} + +// logSafeFragment redacts a URL fragment. No URL renderer applies the name rule +// there, but a fragment carries `k=v` pairs as readily as a query does — hash +// routing puts a whole path and query after the '#' — so the same rule runs over +// whatever follows its first '?', or over the whole fragment when it has none. +func logSafeFragment(fragment string) string { + if fragment == "" { + return fragment + } + base, rawQuery, hasQuery := strings.Cut(fragment, "?") + if !hasQuery { + return logSafeURLComponent(fragment) + } + return logSafeURLComponent(base) + "?" + LogSafeQueryString(rawQuery) +} + // logSafeURLs is logSafeURL over a slice — the RFC 8414 candidate list is // logged whole (`urls_tried`) and every entry is derived from the configured // server URL. diff --git a/internal/oauth/logging_query_test.go b/internal/oauth/logging_query_test.go new file mode 100644 index 000000000..7c2cafd3a --- /dev/null +++ b/internal/oauth/logging_query_test.go @@ -0,0 +1,310 @@ +package oauth + +import ( + "net/url" + "strings" + "testing" +) + +// SEC-01. LogSafeQueryString renders a BARE query string (an http.Request's +// RawQuery) for a log sink. It must apply the SAME rule LogSafeURL applies to a +// whole URL - feeding the bare string to url.Parse reads it as a path, so the +// name rule would never fire. +func TestLogSafeQueryString(t *testing.T) { + const secret = "SUPERSECRETKEY0123456789abcdef01" + + cases := []struct { + name string + rawQuery string + mustSurvive []string + wantExact string + }{ + {name: "empty", rawQuery: "", wantExact: ""}, + { + name: "apikey masked, siblings verbatim", + rawQuery: "apikey=" + secret + "&foo=bar&page=2", + mustSurvive: []string{"apikey=", "foo=bar", "page=2"}, + }, + {name: "api_key", rawQuery: "api_key=" + secret, mustSurvive: []string{"api_key="}}, + {name: "token", rawQuery: "token=" + secret, mustSurvive: []string{"token="}}, + {name: "key", rawQuery: "key=" + secret, mustSurvive: []string{"key="}}, + {name: "secret", rawQuery: "secret=" + secret, mustSurvive: []string{"secret="}}, + { + name: "unparseable escape does not defeat the rule", + rawQuery: "%zz&apikey=" + secret, + mustSurvive: []string{"apikey="}, + }, + { + name: "no sensitive parameter is returned byte-for-byte", + rawQuery: "foo=bar&page=2&filter=a%20b", + wantExact: "foo=bar&page=2&filter=a%20b", + }, + { + name: "reference values are labels, not secrets", + rawQuery: "apikey=${env:MCPPROXY_API_KEY}&foo=bar", + wantExact: "apikey=${env:MCPPROXY_API_KEY}&foo=bar", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := LogSafeQueryString(tc.rawQuery) + if strings.Contains(got, secret) { + t.Fatalf("credential survived redaction: %q", got) + } + if tc.wantExact != "" || tc.rawQuery == "" { + if got != tc.wantExact { + t.Fatalf("want %q, got %q", tc.wantExact, got) + } + } + for _, want := range tc.mustSurvive { + if !strings.Contains(got, want) { + t.Fatalf("expected %q to survive in %q", want, got) + } + } + if strings.HasPrefix(got, "?") { + t.Fatalf("the wrapping %q must not leak into the field: %q", "?", got) + } + }) + } +} + +// SEC-01, codex round 1 finding 1. A Referer is entirely client-controlled, so +// it does not have to be a parseable URL. LogSafeURL masks nothing in a URL +// url.Parse rejects - RedactURLQueryParamsWith falls back to the regex +// RedactURL, which has no rule for `apikey`, and the generated key is 64 hex +// characters, which the value-shaped detector does not recognise on its own. A +// single bad escape anywhere in the path was enough to put the root credential +// back into http.log verbatim. +func TestLogSafeRequestURL_MalformedURLStillMasksTheCredential(t *testing.T) { + // The real shape: what config.generateAPIKey() produces. + const adminKey = "6930184070f362383e7cddbd1184b3b8ed66f84717eedd8906ab8743dfa746cd" + + cases := []struct { + name string + rawURL string + }{ + {name: "bad escape in path", rawURL: "http://127.0.0.1:8080/%zz?apikey=" + adminKey}, + {name: "truncated escape", rawURL: "http://127.0.0.1:8080/ui%/?apikey=" + adminKey}, + {name: "missing scheme", rawURL: ":::/bad?apikey=" + adminKey}, + {name: "control byte in path", rawURL: "http://127.0.0.1:8080/\x7f?apikey=" + adminKey}, + {name: "well-formed still works", rawURL: "http://127.0.0.1:8080/ui/?apikey=" + adminKey}, + {name: "query only", rawURL: "?apikey=" + adminKey}, + // codex round 2 finding 2: an HTAB is legal inside an HTTP header + // value and makes url.Parse fail, so every parse-dependent path fell + // back to a regex with no `apikey` rule. + {name: "HTAB in a sibling parameter", rawURL: "http://127.0.0.1:8080/ui/?note=\t&apikey=" + adminKey}, + {name: "HTAB and a bad escape", rawURL: "http://127.0.0.1:8080/%zz?note=\t&apikey=" + adminKey}, + // codex round 2 finding 1: the fragment never saw the name rule. + {name: "credential in the fragment", rawURL: "http://127.0.0.1:8080/ui/#?apikey=" + adminKey}, + {name: "hash routing", rawURL: "http://127.0.0.1:8080/ui/#/servers?apikey=" + adminKey}, + {name: "fragment with no question mark", rawURL: "http://127.0.0.1:8080/ui/#apikey=" + adminKey}, + {name: "query and fragment both", rawURL: "http://127.0.0.1:8080/ui/?apikey=" + adminKey + "#token=" + adminKey}, + // codex round 4: the part of a fragment BEFORE its own '?' bypassed + // the query name rule, and the free-form scrubber never + // percent-decodes a parameter name. + {name: "credential before the fragment's own query", rawURL: "http://127.0.0.1:8080/ui/#apikey=" + adminKey + "?route=x"}, + {name: "percent-encoded parameter name in the fragment", rawURL: "http://127.0.0.1:8080/ui/#api%6bey=" + adminKey + "?route=x"}, + {name: "percent-encoded parameter name in the query", rawURL: "http://127.0.0.1:8080/ui/?api%6bey=" + adminKey}, + // Same class, in a PATH segment of a URL that parses perfectly well - + // which is why this renderer decomposes by hand instead of trusting + // LogSafeURL when url.Parse happens to succeed. + {name: "credential in the path", rawURL: "http://127.0.0.1:8080/ui/apikey=" + adminKey}, + {name: "percent-encoded parameter name in the path", rawURL: "http://127.0.0.1:8080/ui/api%6bey=" + adminKey}, + {name: "userinfo of a well-formed URL", rawURL: "http://user:" + adminKey + "@127.0.0.1:8080/ui/"}, + // codex round 5: the same, behind a path prefix - the name has to be + // judged PER SEGMENT or `/servers/api%6bey` normalises to nothing. + {name: "encoded name behind a path prefix", rawURL: "http://127.0.0.1:8080/ui/#/servers/api%6bey=" + adminKey + "?route=x"}, + {name: "encoded name behind a mangled path prefix", rawURL: "http://127.0.0.1:8080/ui/%zz/api%6bey=" + adminKey}, + {name: "encoded separator in the name", rawURL: "http://127.0.0.1:8080/a/b/c/API%2DKEY=" + adminKey}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := LogSafeRequestURL(tc.rawURL) + if strings.Contains(got, adminKey) { + t.Fatalf("admin API key survived into the log field: %q", got) + } + }) + } +} + +// The base half of an unparseable URL keeps the regex fallback's +// `user:pass@host` rule: a basic-auth password has no value shape a detector +// can recognise, so dropping RedactURL there would publish it. +func TestLogSafeRequestURL_MalformedURLStillMasksUserinfo(t *testing.T) { + const pw = "hunter2hunter2hunter2" + + for _, rawURL := range []string{ + "https://admin:" + pw + "@127.0.0.1:8080/%zz?apikey=deadbeefdeadbeef", + // codex round 2 finding 2: the first '?' falls BETWEEN the password + // and its '@', so splitting before scrubbing left the password in a + // substring the userinfo rule could no longer match. + "https://admin:" + pw + "?x@127.0.0.1:8080/%zz", + "https://admin:" + pw + "?x@127.0.0.1:8080/%zz?apikey=deadbeefdeadbeef", + // codex round 3 finding 3: the same cut, made by the OTHER delimiter. + "https://admin:" + pw + "#x@127.0.0.1:8080/%zz", + "https://admin:" + pw + "#x@127.0.0.1:8080/%zz?apikey=deadbeefdeadbeef", + } { + if got := LogSafeRequestURL(rawURL); strings.Contains(got, pw) { + t.Fatalf("basic-auth password survived into the log field: %q", got) + } + } +} + +// The non-credential parts of a referer are the reason the field exists; a URL +// with nothing sensitive in it must come back byte-for-byte. +// +// codex round 6 finding 3: this table used to contain no path with an '=', no +// percent-encoded segment and no config reference, so it did not notice that +// ScrubUpstreamText's unanchored `=` regex was rewriting +// `/monkey=banana` to `/monkey=***REDACTED***`. +func TestLogSafeRequestURL_PreservesBenignURLs(t *testing.T) { + for _, rawURL := range []string{ + "http://127.0.0.1:8080/ui/", + "http://127.0.0.1:8080/ui/?page=2&filter=a%20b", + "http://127.0.0.1:8080/ui/#/servers/everything", + "http://127.0.0.1:8080/ui/?page=2#/servers/everything", + // A path segment that merely CONTAINS a marker word. + "http://127.0.0.1:8080/monkey=banana", + "http://127.0.0.1:8080/api/v1/tokens/list", + "http://127.0.0.1:8080/donkey=hotay/passwords=plural", + // Percent-encoded but benign. + "http://127.0.0.1:8080/a%20b/c%2Fd?page=2", + // References are labels, not secrets, in every position. + "http://127.0.0.1:8080/ui/?url=${env:MCPPROXY_URL}&page=2", + "http://127.0.0.1:8080/ui/?apikey=${keyring:mcpproxy}", + } { + if got := LogSafeRequestURL(rawURL); got != rawURL { + t.Fatalf("benign referer was rewritten:\n want %q\n got %q", rawURL, got) + } + } +} + +// The same guarantee for the `path` field, which is the most-read field in +// http.log and must stay readable. +func TestLogSafeRequestPath(t *testing.T) { + const adminKey = "6930184070f362383e7cddbd1184b3b8ed66f84717eedd8906ab8743dfa746cd" + + for _, path := range []string{ + "/", + "/api/v1/servers", + "/api/v1/servers/everything/tools", + "/ui/", + "/monkey=banana", + "/api/v1/tokens", + "/events", + } { + if got := LogSafeRequestPath(path); got != path { + t.Fatalf("benign path was rewritten:\n want %q\n got %q", path, got) + } + } + + for _, path := range []string{ + "/ui/apikey=" + adminKey, + "/ui/api%6bey=" + adminKey, + "/a/b/token=" + adminKey, + } { + if got := LogSafeRequestPath(path); strings.Contains(got, adminKey) { + t.Fatalf("credential survived in the path field: %q", got) + } + } +} + +// codex round 7: dropping ScrubUpstreamText from the component rule must not +// drop the two of its rules that no name rule and no value detector can +// replace. r.URL.Path arrives percent-DECODED, so a path really can carry a +// `Bearer ` with a space in it, and an embedded URL really can carry +// `user:pass@`. +func TestLogSafeRequestPath_KeepsTheBearerAndUserinfoRules(t *testing.T) { + const pw = "hunter2" + + for _, path := range []string{ + "/api/v1/servers/Bearer " + pw, + "/proxy/https://alice:" + pw + "@host/mcp", + } { + got := LogSafeRequestPath(path) + if strings.Contains(got, pw) { + t.Fatalf("credential survived in the path field: %q", got) + } + } +} + +func TestLogSafeRequestURL_EmptyAndQueryless(t *testing.T) { + if got := LogSafeRequestURL(""); got != "" { + t.Fatalf("empty referer must stay empty, got %q", got) + } + // Malformed, query-less and credential-free: the path survives (that is + // the diagnostic) and nothing panics. + if got := LogSafeRequestURL("http://127.0.0.1:8080/%zz"); !strings.Contains(got, "127.0.0.1:8080") { + t.Fatalf("the host must survive a malformed referer, got %q", got) + } +} + +// The boundary of the NAME rule, pinned so it is a decision and not a surprise. +// +// The rule needs a name it can decode and normalise, sitting alone in front of +// an '='. Three shapes have no such name, so only the value-shaped detector +// sees them - and a 64-character hex admin key has no vendor shape for it: +// +// - a component with no '=' at all (`?`); +// - a component whose '=' is itself percent-encoded (`api%6bey%3d`); +// - a component whose path delimiter is percent-encoded, which makes the +// name something other than a parameter name (`/ui%2Fapi%6bey=`). +// +// Left that way deliberately (codex round 6 finding 1, declined). mcpproxy only +// ever reads the credential from a NAMED `apikey` query parameter - see +// httpapi.Server's auth resolution - and nothing in this codebase emits any of +// these shapes, so none of them is a channel the proxy's OWN key travels on; +// reaching them means a client deliberately encoding its own credential into a +// path. The real gap underneath is that the value detector does not recognise a +// bare hex string, which is repo-wide and not something to change here on a +// guess. +func TestLogSafeQueryString_DocumentedNameRuleBoundary(t *testing.T) { + const adminKey = "6930184070f362383e7cddbd1184b3b8ed66f84717eedd8906ab8743dfa746cd" + + for _, in := range []string{ + adminKey, + "api%6bey%3d" + adminKey, + } { + if got := LogSafeQueryString(in); got != in { + t.Fatalf("documented boundary changed - revisit the comment above:\n in %q\nout %q", in, got) + } + } + const encodedDelimiter = "http://host/ui%2Fapi%6bey=" + adminKey + if got := LogSafeRequestURL(encodedDelimiter); got != encodedDelimiter { + t.Fatalf("documented boundary changed - revisit the comment above: %q", got) + } +} + +// A literal '#' in RawQuery is an ordinary value byte to url.ParseQuery (Go does +// not split a fragment off a request target), and the parameters after it are +// still parameters. Pin both: the credential after the '#' is masked, and the +// benign parameter's decoded value is unchanged. +func TestLogSafeQueryString_HashIsAValueByteNotAFragment(t *testing.T) { + const secret = "SUPERSECRETKEY0123456789abcdef01" + + got := LogSafeQueryString("note=a#b&apikey=" + secret) + if strings.Contains(got, secret) { + t.Fatalf("a credential after a literal '#' escaped the name rule: %q", got) + } + values, err := url.ParseQuery(got) + if err != nil { + t.Fatalf("output must still be a parseable query string: %v", err) + } + if values.Get("note") != "a#b" { + t.Fatalf("the benign parameter's decoded value must be unchanged, got %q", values.Get("note")) + } +} + +// The value-shaped detector has to run too: a credential under a parameter name +// no matcher enumerates is exactly what the name rule cannot see. This is the +// property LogSafeURL was introduced for (issue #1158) and the reason this +// helper delegates to it instead of growing a second rule. +func TestLogSafeQueryString_RunsValueShapedDetector(t *testing.T) { + const ghp = "ghp_1234567890abcdefghijABCDEFGHIJ123456" + got := LogSafeQueryString("opaque=" + ghp) + if strings.Contains(got, ghp) { + t.Fatalf("vendor-shaped credential under an opaque name survived: %q", got) + } +} diff --git a/internal/oauth/round8_renderer_discovery_test.go b/internal/oauth/round8_renderer_discovery_test.go index 55ec76722..2051b6750 100644 --- a/internal/oauth/round8_renderer_discovery_test.go +++ b/internal/oauth/round8_renderer_discovery_test.go @@ -47,6 +47,9 @@ var exportedStringFuncs = map[string]func(string) string{ // Issue #1158, review round 2. Both emit masks, so the fail-closed net has // to know their markers even though no write door echoes them back. "LogSafeURL": LogSafeURL, + "LogSafeQueryString": LogSafeQueryString, + "LogSafeRequestURL": LogSafeRequestURL, + "LogSafeRequestPath": LogSafeRequestPath, "LogSafeCallbackQuery": LogSafeCallbackQuery, } diff --git a/scripts/test-api-e2e.sh b/scripts/test-api-e2e.sh index bdf7f9264..1f0b7e168 100755 --- a/scripts/test-api-e2e.sh +++ b/scripts/test-api-e2e.sh @@ -129,15 +129,20 @@ log_skip() { TESTS_SKIPPED=$((TESTS_SKIPPED + 1)) } -# Extract API key from server logs -# Optional $1 overrides the log path (used by scripts/test-extract-api-key.sh). -# -a forces grep to treat the log as text: the server log can contain NUL bytes -# (and ANSI color codes), which otherwise make grep report "Binary file ... matches" -# instead of the match, corrupting API_KEY. See MCP-2404. +# Extract the API key from the CONFIG FILE the server was started with. +# Optional $1 overrides the config path (used by scripts/test-extract-api-key.sh). +# +# SEC-01: the auto-generated key is deliberately no longer written to the server +# log - it is a root credential and the log is a plaintext file at rest. The +# config file is where mcpproxy persists it (the same source +# scripts/dev-server-edition.sh reads), so that is what we read. +# +# The write-back happens during startup, so callers poll: an empty API_KEY here +# means "not written yet", not "failed". extract_api_key() { - local log_file="${1:-/tmp/mcpproxy_e2e.log}" - if [ -f "$log_file" ]; then - API_KEY=$(grep -ao '"api_key": "[^"]*"' "$log_file" | sed 's/.*"api_key": "\([^"]*\)".*/\1/' | head -1) + local config_file="${1:-${CONFIG_FILE:-}}" + if [ -n "$config_file" ] && [ -f "$config_file" ]; then + API_KEY=$(jq -r '.api_key // empty' "$config_file" 2>/dev/null) if [ ! -z "$API_KEY" ]; then echo "Extracted API key: ${API_KEY:0:8}..." fi @@ -1180,9 +1185,10 @@ AUDIT_API_KEY="" AUDIT_PID="" AUDIT_MCP_SESSION_ID="" +# See extract_api_key: the key is read from the config file, not the log. extract_audit_api_key() { - if [ -f "$AUDIT_SERVER_LOG" ]; then - AUDIT_API_KEY=$(grep -ao '"api_key": "[^"]*"' "$AUDIT_SERVER_LOG" | sed 's/.*"api_key": "\([^"]*\)".*/\1/' | head -1) + if [ -f "$AUDIT_CONFIG_FILE" ]; then + AUDIT_API_KEY=$(jq -r '.api_key // empty' "$AUDIT_CONFIG_FILE" 2>/dev/null) fi } diff --git a/scripts/test-extract-api-key.sh b/scripts/test-extract-api-key.sh index 18eea8ae8..6d257840f 100755 --- a/scripts/test-extract-api-key.sh +++ b/scripts/test-extract-api-key.sh @@ -1,14 +1,15 @@ #!/bin/bash # -# Unit test for extract_api_key() in scripts/test-api-e2e.sh (MCP-2404). +# Unit test for extract_api_key() in scripts/test-api-e2e.sh. # -# The server log can contain NUL bytes and ANSI color codes. Without `grep -a`, -# grep treats the log as binary and emits "Binary file ... matches" instead of -# the match, so API_KEY becomes garbage and every authed curl is rejected. +# SEC-01: the auto-generated API key is no longer written to the server log (it +# is the root credential and the log is a plaintext file at rest), so the E2E +# script reads it from the CONFIG FILE mcpproxy persists it to. This test pins +# that contract, including the two states the polling loop has to tolerate: +# a config file that does not exist yet, and one whose api_key is still empty. # -# This test extracts the *real* extract_api_key() function from test-api-e2e.sh -# (so it stays in lockstep with the code under test) and runs it against a -# fixture log that mixes ANSI escapes, a NUL byte, and the api_key line. +# It extracts the *real* extract_api_key() function from test-api-e2e.sh so it +# stays in lockstep with the code under test. set -u @@ -20,6 +21,11 @@ if [ ! -f "$TARGET" ]; then exit 1 fi +if ! command -v jq >/dev/null 2>&1; then + echo "FAIL: jq is required by extract_api_key()" + exit 1 +fi + # Pull just the extract_api_key() function out of the real script and load it, # without executing the rest of the (top-level) integration script. FUNC_SRC="$(awk '/^extract_api_key\(\) \{/,/^\}/' "$TARGET")" @@ -29,31 +35,66 @@ if [ -z "$FUNC_SRC" ]; then fi eval "$FUNC_SRC" +# Guard against a silent regression to log scraping: the function must not read +# a log file any more. +if echo "$FUNC_SRC" | grep -q 'log_file\|mcpproxy_e2e.log'; then + echo "FAIL: extract_api_key() still reads the server log; the key is no longer logged (SEC-01)" + exit 1 +fi + EXPECTED_KEY="4197c426deadbeef0123456789abcdef" -FIXTURE="$(mktemp -t mcpproxy_e2e_fixture.XXXXXX)" -trap 'rm -f "$FIXTURE"' EXIT +FIXTURE_DIR="$(mktemp -d "${TMPDIR:-/tmp}/mcpproxy_key_fixture.XXXXXX")" +trap 'rm -rf "$FIXTURE_DIR"' EXIT + +FAILED=0 -# Build a fixture log that reproduces the real-world failure: ANSI color codes -# plus an embedded NUL byte, then the api_key line. +check() { + local label="$1" expected="$2" actual="$3" + if [ "$expected" = "$actual" ]; then + echo "PASS: $label" + else + echo "FAIL: $label — expected '$expected' but got '$actual'" + FAILED=1 + fi +} + +# 1. A persisted config file carries the key. +POPULATED="$FIXTURE_DIR/populated.json" +cat > "$POPULATED" < "$FIXTURE" - -# Sanity check: the fixture really does contain a NUL byte (the trigger). -if ! grep -qa $'\x00' "$FIXTURE"; then - echo "FAIL: fixture is missing the NUL byte that triggers the bug" - exit 1 -fi + "listen": "127.0.0.1:8081", + "api_key": "$EXPECTED_KEY", + "mcpServers": [] +} +JSON +API_KEY="" +extract_api_key "$POPULATED" > /dev/null +check "reads api_key from the config file" "$EXPECTED_KEY" "$API_KEY" +# 2. The server has not written the key back yet — the polling loop must see an +# empty value rather than a parse error or the literal "null". +EMPTY="$FIXTURE_DIR/empty.json" +cat > "$EMPTY" <<'JSON' +{ + "listen": "127.0.0.1:8081", + "api_key": "", + "mcpServers": [] +} +JSON API_KEY="" -extract_api_key "$FIXTURE" > /dev/null +extract_api_key "$EMPTY" > /dev/null +check "empty api_key yields an empty result" "" "$API_KEY" -if [ "$API_KEY" = "$EXPECTED_KEY" ]; then - echo "PASS: extract_api_key() returned the key from a NUL/ANSI log ($EXPECTED_KEY)" - exit 0 -else - echo "FAIL: expected '$EXPECTED_KEY' but got '$API_KEY'" - exit 1 -fi +# 3. Config file missing entirely (first poll, before the server writes it). +API_KEY="" +extract_api_key "$FIXTURE_DIR/does-not-exist.json" > /dev/null +check "missing config file yields an empty result" "" "$API_KEY" + +# 4. Half-written config (atomic replace not finished) must not abort the loop. +PARTIAL="$FIXTURE_DIR/partial.json" +printf '{ "listen": "127.0.0.1:8081", "api_k' > "$PARTIAL" +API_KEY="" +extract_api_key "$PARTIAL" > /dev/null +check "truncated config file yields an empty result" "" "$API_KEY" + +exit "$FAILED" From 1c3dcc856f03037c310e16647a9a27fc6ce64de2 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 23 Sep 2026 06:58:48 +0300 Subject: [PATCH 2/3] fix(security): cap SEC-01 redaction input to stop a pre-auth DoS amplification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit httpLoggingMiddleware runs LogSafeRequestPath/LogSafeQueryString/ LogSafeRequestURL on every request's path, query and Referer, mounted at the chi router root before apiKeyAuthMiddleware. logSafeURLComponent splits its input on '/' and runs the full value-shaped detector (44 regex patterns plus an entropy pass) once per segment, so an unauthenticated request built from many minimal path segments (well inside the existing 1MB MaxHeaderBytes budget) turned one request into hundreds of thousands of detector passes — verified reproducible at ~800ms of CPU for a single request. Bound the cost at the input instead of the algorithm: cap path/query/URL to 4KB, on a rune boundary, before any redaction rule runs. A few KB is already generous for what an access log line needs. Co-Authored-By: Claude Opus 5 --- internal/oauth/logging.go | 44 +++++++++++- internal/oauth/logging_dos_test.go | 107 +++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+), 1 deletion(-) create mode 100644 internal/oauth/logging_dos_test.go diff --git a/internal/oauth/logging.go b/internal/oauth/logging.go index 12eb3bc32..31c7032ad 100644 --- a/internal/oauth/logging.go +++ b/internal/oauth/logging.go @@ -1011,6 +1011,41 @@ func LogSafeURL(rawURL string) string { return AuditRedaction.URLValueDeep(rawURL) } +// maxLogSafeRequestBytes bounds the work LogSafeRequestPath, LogSafeQueryString +// and LogSafeRequestURL will do on a single UNTRUSTED input. +// +// SEC-01 second-lens review (PR #1350): internal/httpapi's httpLoggingMiddleware +// runs these three renderers on r.URL.Path, r.URL.RawQuery and r.Referer() for +// EVERY request, mounted at the chi router root before apiKeyAuthMiddleware — +// so they run pre-auth, on every connection the listener accepts, logged AFTER +// the response is written and so unbounded by http.Server's ReadTimeout / +// WriteTimeout / ReadHeaderTimeout (none of which interrupt a goroutine already +// running handler code). logSafeURLComponent splits its input on '/' and calls +// the shared value-shaped detector (Detector.MaskText — every built-in regex +// pattern plus a Shannon-entropy pass, allocating a fresh map per pattern per +// call) once PER SEGMENT, so a request path built from many minimal segments +// turns one request into hundreds of thousands of full detector passes, well +// inside the pre-existing 1MB MaxHeaderBytes budget the request line shares +// (internal/server/server.go) — an unauthenticated CPU/allocation amplification +// this redaction fix itself introduced. +// +// A few KB is already generous for what an access log line needs to be useful, +// so the fix bounds the INPUT rather than the algorithm: every entry point cuts +// its argument to this length, on a rune boundary, before any redaction rule +// runs — so the cost of rendering one log field cannot scale with what the +// client sent. +const maxLogSafeRequestBytes = 4096 + +// capLogSafeRequestInput cuts s to maxLogSafeRequestBytes, marking the cut, so +// every redaction rule downstream runs over a bounded string regardless of how +// long the caller-supplied s is. +func capLogSafeRequestInput(s string) string { + if len(s) <= maxLogSafeRequestBytes { + return s + } + return s[:safeTruncateBytes(s, maxLogSafeRequestBytes)] + capEllipsis +} + // LogSafeQueryString is LogSafeURL for a BARE query string — `r.URL.RawQuery`, // with no scheme, host or path in front of it. // @@ -1031,6 +1066,7 @@ func LogSafeQueryString(rawQuery string) string { if rawQuery == "" { return rawQuery } + rawQuery = capLogSafeRequestInput(rawQuery) return AuditRedaction.finish(rawQuery, redactRawQueryParams(rawQuery, AuditRedaction.masker())) } @@ -1069,6 +1105,7 @@ func LogSafeRequestURL(rawURL string) string { if rawURL == "" { return rawURL } + rawURL = capLogSafeRequestInput(rawURL) scrubbed := redactURLUserinfo(rawURL) main, fragment, hasFragment := strings.Cut(scrubbed, "#") base, rawQuery, hasQuery := strings.Cut(main, "?") @@ -1089,8 +1126,13 @@ func LogSafeRequestURL(rawURL string) string { // access line and on every authentication decision, and a path is as // client-controlled as the query beside it, so it had the same shape of // exposure LogSafeRequestURL now closes for the Referer. +// +// Capped at entry (SEC-01 second-lens review, PR #1350): logSafeURLComponent +// splits on '/' and runs the full value-shaped detector per segment, so an +// uncapped path let a client turn one request into an unbounded number of +// detector passes. See maxLogSafeRequestBytes. func LogSafeRequestPath(path string) string { - return logSafeURLComponent(path) + return logSafeURLComponent(capLogSafeRequestInput(path)) } // logSafeURLComponent renders the part of a URL that is NOT a query string - diff --git a/internal/oauth/logging_dos_test.go b/internal/oauth/logging_dos_test.go new file mode 100644 index 000000000..2ad122768 --- /dev/null +++ b/internal/oauth/logging_dos_test.go @@ -0,0 +1,107 @@ +package oauth + +import ( + "strings" + "testing" + "time" +) + +// SEC-01 second-lens review (PR #1350): httpLoggingMiddleware runs +// LogSafeRequestPath / LogSafeQueryString / LogSafeRequestURL on every +// request's r.URL.Path, r.URL.RawQuery and r.Referer(), mounted at the chi +// router root BEFORE apiKeyAuthMiddleware — so these renderers run on every +// unauthenticated connection the listener accepts, logged AFTER the response +// is already written and so not bounded by http.Server's ReadTimeout / +// WriteTimeout / ReadHeaderTimeout (those stop future reads/writes on the +// connection, not a goroutine already executing handler code). +// +// logSafeURLComponent splits its input on '/' and runs the shared value-shaped +// detector (Detector.MaskText: every built-in regex pattern plus a +// Shannon-entropy pass, with a fresh map allocated per pattern per call) once +// PER SEGMENT. A request path built from many minimal two-byte segments +// ("/a" repeated) fits comfortably inside the pre-existing 1MB MaxHeaderBytes +// budget and turns one request into hundreds of thousands of full detector +// passes — a large, unauthenticated CPU/allocation amplification introduced by +// the SEC-01 fix itself. +// +// These renderers must bound the cost of ANY single call independent of how +// long the caller-supplied string is: cut the input before running any +// redaction rule, so a request built from many small segments (or one huge +// query string, or a huge Referer) costs no more than a normal one. +func TestLogSafeRequestPath_BoundsCostOfAdversarialManySegmentPath(t *testing.T) { + if testing.Short() { + t.Skip("timing-sensitive; skipped under -short") + } + + // ~500,000 two-byte segments, matching the amplification the review + // described (well inside the 1MB header budget the request line shares). + adversarial := strings.Repeat("/a", 500_000) + + start := time.Now() + _ = LogSafeRequestPath(adversarial) + elapsed := time.Since(start) + + // Bounded by the input cap, not by the number of segments: this must stay + // fast (low tens of milliseconds) regardless of how many segments the + // client sent. A generous ceiling avoids flaking under CI load while still + // catching the unbounded-per-segment behavior, which took long enough to + // be a usable denial-of-service on its own. + if elapsed > 500*time.Millisecond { + t.Fatalf("LogSafeRequestPath took %s on a %d-byte adversarial path — cost must be bounded, not O(segments)", elapsed, len(adversarial)) + } +} + +func TestLogSafeQueryString_BoundsCostOfHugeQuery(t *testing.T) { + if testing.Short() { + t.Skip("timing-sensitive; skipped under -short") + } + + adversarial := strings.Repeat("a", 2_000_000) + + start := time.Now() + _ = LogSafeQueryString(adversarial) + elapsed := time.Since(start) + + if elapsed > 500*time.Millisecond { + t.Fatalf("LogSafeQueryString took %s on a %d-byte adversarial query — cost must be bounded", elapsed, len(adversarial)) + } +} + +func TestLogSafeRequestURL_BoundsCostOfHugeReferer(t *testing.T) { + if testing.Short() { + t.Skip("timing-sensitive; skipped under -short") + } + + adversarial := "http://host" + strings.Repeat("/a", 500_000) + + start := time.Now() + _ = LogSafeRequestURL(adversarial) + elapsed := time.Since(start) + + if elapsed > 500*time.Millisecond { + t.Fatalf("LogSafeRequestURL took %s on a %d-byte adversarial referer — cost must be bounded", elapsed, len(adversarial)) + } +} + +// A credential near the START of an oversized input — well within what any +// real access log line needs — must still be masked. The cap trades +// completeness on pathological input for bounded cost; it must not silently +// stop working on realistic ones. +func TestLogSafeRequestPath_StillMasksCredentialsWithinTheCap(t *testing.T) { + const adminKey = "6930184070f362383e7cddbd1184b3b8ed66f84717eedd8906ab8743dfa746cd" + + path := "/ui/apikey=" + adminKey + strings.Repeat("/a", 10) + got := LogSafeRequestPath(path) + if strings.Contains(got, adminKey) { + t.Fatalf("credential within the cap survived: %q", got) + } +} + +func TestLogSafeQueryString_StillMasksCredentialsWithinTheCap(t *testing.T) { + const secret = "SUPERSECRETKEY0123456789abcdef01" + + got := LogSafeQueryString("apikey=" + secret + "&" + strings.Repeat("a", 10)) + if strings.Contains(got, secret) { + t.Fatalf("credential within the cap survived: %q", got) + } +} From ac871ef895356284157de2cb60ca6a37d390e233 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 23 Sep 2026 07:43:11 +0300 Subject: [PATCH 3/3] fix(security): redact admin API key from bare path/query/referer segments SEC-01 follow-up (PR #1350): LogSafeRequestPath, LogSafeQueryString and LogSafeRequestURL mask a credential by NAME (apikey=) or by VENDOR SHAPE (ghp_..., sk-...), but mcpproxy's auto-generated admin API key is bare 64-character hex with no enclosing name and no vendor prefix. Hex's 4-bit-per-symbol ceiling also means a perfectly random hex string's Shannon entropy (max 4.0) never clears the value-shaped detector's 4.5 threshold, so neither existing rule can ever catch it, in any position, regardless of tuning. A live instance reproduced the key landing verbatim in main.log as a bare path segment (GET /api/v1/status/). Fix: internal/oauth/logging.go gains redactKnownSecrets(s, secrets), an exact-value redaction pass run before the name/shape rules. The three renderers each take a trailing ...knownSecrets variadic. A shape rule for "a 64-hex-char path/query segment" was considered and rejected: this same API legitimately logs SHA-256 tool hashes and other 64-hex-char identifiers (activity/request ids) as diagnostic path segments, so a rule tight enough to name the key's format is equally tight around theirs. Exact-value match has zero false positives by construction and covers every shape (path, query, referer, fragment) at once, including a user-overridden admin key that need not be hex at all. internal/httpapi/server.go's currentAdminAPIKey() reads the live cfg.APIKey fresh from the controller (nil-safe when s.controller is nil) and threads it into all 13 oauth.LogSafe* call sites. Agent tokens (mcp_agt_ prefix) are handled differently: unlike the admin key they carry a distinctive vendor prefix, so internal/security/patterns gained an agentTokenPattern (mcp_agt_[0-9a-fA-F]{64}) instead of needing per-request secret plumbing. internal/oauth/round8_renderer_discovery_test.go's AST-based "every mask renderer is bound or exempted" discovery net is extended to recognize the new func(string, ...string) string shape. zcode (GLM-5.3) review round 1 applied: thread cfg.APIKey through handleAgentTokenAuth instead of a redundant currentAdminAPIKey() call; widen the agent-token pattern to accept uppercase hex (ValidateTokenFormat already does via hex.DecodeString); tighten two doc comments that overstated what the fixed-length regex and "bare hex" framing guarantee. Co-Authored-By: Claude Opus 5 --- internal/httpapi/server.go | 72 +++++++--- internal/oauth/logging.go | 92 ++++++++++++- internal/oauth/logging_exact_secret_test.go | 128 ++++++++++++++++++ .../oauth/round8_renderer_discovery_test.go | 43 +++++- internal/security/patterns/tokens.go | 30 ++++ internal/security/patterns/tokens_test.go | 69 ++++++++++ 6 files changed, 411 insertions(+), 23 deletions(-) create mode 100644 internal/oauth/logging_exact_secret_test.go diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index b7f078f99..e6d8aca17 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -464,6 +464,31 @@ func (s *Server) Router() *chi.Mux { return s.router } +// currentAdminAPIKey returns the currently-configured admin API key, read +// fresh from the controller on every call, or "" when config is unavailable +// (the same testing scenario apiKeyAuthMiddleware already tolerates). +// +// SEC-01 gap fix (PR #1350, live-verification follow-up): every +// oauth.LogSafeRequestPath / LogSafeQueryString / LogSafeRequestURL call site +// below passes this so the admin key is redacted by EXACT VALUE wherever it +// appears in a path, query or referer — the one rule that catches it with no +// vendor shape to key on. Reading it fresh (rather than caching it once) means +// a key rotated between requests is covered immediately, with nothing to keep +// in sync; the cost is one more RLock through the controller per log line, +// the same one apiKeyAuthMiddleware already pays for every authenticated +// request. See oauth.redactKnownSecrets for the full false-positive analysis. +func (s *Server) currentAdminAPIKey() string { + if s.controller == nil { + return "" + } + cfgIface := s.controller.GetCurrentConfig() + cfg, ok := cfgIface.(*config.Config) + if !ok || cfg == nil { + return "" + } + return cfg.APIKey +} + // apiKeyAuthMiddleware creates middleware for API key authentication. // Connections from Unix socket/named pipe (tray) are trusted and skip API key validation. // Supports both global API key (admin) and agent tokens (mcp_agt_ prefix) with scope enforcement. @@ -475,7 +500,7 @@ func (s *Server) apiKeyAuthMiddleware() func(http.Handler) http.Handler { source := transport.GetConnectionSource(r.Context()) if source == transport.ConnectionSourceTray { s.logger.Debugw("Tray connection - skipping API key validation", - zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path, s.currentAdminAPIKey())), zap.String("remote_addr", r.RemoteAddr), zap.String("source", string(source))) ctx := auth.WithAuthContext(r.Context(), auth.AdminContext()) @@ -503,7 +528,7 @@ func (s *Server) apiKeyAuthMiddleware() func(http.Handler) http.Handler { // Empty API key is not allowed - this prevents accidental exposure if cfg.APIKey == "" { s.logger.Warnw("TCP connection rejected - API key not configured", - zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path, cfg.APIKey)), zap.String("remote_addr", r.RemoteAddr)) s.writeError(w, r, http.StatusUnauthorized, "API key authentication required but not configured. Please set MCPPROXY_API_KEY or configure api_key in config file.") return @@ -549,7 +574,7 @@ func (s *Server) authenticateWithPrecedence(w http.ResponseWriter, r *http.Reque } s.logger.Warnw("TCP connection with missing API key", - zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path, cfg.APIKey)), zap.String("remote_addr", r.RemoteAddr)) s.writeError(w, r, http.StatusUnauthorized, "Invalid or missing API key") } @@ -560,19 +585,19 @@ func (s *Server) authenticateWithPrecedence(w http.ResponseWriter, r *http.Reque // the cookie do). func (s *Server) authenticateExplicitToken(w http.ResponseWriter, r *http.Request, next http.Handler, cfg *config.Config, token string) { if token != "" && strings.HasPrefix(token, auth.TokenPrefixStr) { - s.handleAgentTokenAuth(w, r, next, token) + s.handleAgentTokenAuth(w, r, next, cfg.APIKey, token) return } if token != "" && token == cfg.APIKey { s.logger.Debugw("TCP connection with valid API key", - zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path, cfg.APIKey)), zap.String("remote_addr", r.RemoteAddr)) ctx := auth.WithAuthContext(r.Context(), auth.AdminContext()) next.ServeHTTP(w, r.WithContext(ctx)) return } s.logger.Warnw("TCP connection with invalid API key", - zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path, cfg.APIKey)), zap.String("remote_addr", r.RemoteAddr)) s.writeError(w, r, http.StatusUnauthorized, "Invalid or missing API key") } @@ -587,12 +612,12 @@ func (s *Server) authenticateBearer(w http.ResponseWriter, r *http.Request, next } if token != "" && strings.HasPrefix(token, auth.TokenPrefixStr) { - s.handleAgentTokenAuth(w, r, next, token) + s.handleAgentTokenAuth(w, r, next, cfg.APIKey, token) return } if token != "" && token == cfg.APIKey { s.logger.Debugw("TCP connection with valid API key", - zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path, cfg.APIKey)), zap.String("remote_addr", r.RemoteAddr)) ctx := auth.WithAuthContext(r.Context(), auth.AdminContext()) next.ServeHTTP(w, r.WithContext(ctx)) @@ -604,16 +629,22 @@ func (s *Server) authenticateBearer(w http.ResponseWriter, r *http.Request, next } s.logger.Warnw("TCP connection with invalid API key", - zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path, cfg.APIKey)), zap.String("remote_addr", r.RemoteAddr)) s.writeError(w, r, http.StatusUnauthorized, "Invalid or missing API key") } // handleAgentTokenAuth validates an agent token and sets the appropriate AuthContext. -func (s *Server) handleAgentTokenAuth(w http.ResponseWriter, r *http.Request, next http.Handler, token string) { +// +// adminAPIKey is the caller's already-resolved cfg.APIKey (zcode review round +// 1, PR #1350 follow-up): both call sites above already hold cfg, so passing +// it through avoids a second, redundant s.controller.GetCurrentConfig() read +// on every agent-token-authenticated request that currentAdminAPIKey() would +// otherwise perform. +func (s *Server) handleAgentTokenAuth(w http.ResponseWriter, r *http.Request, next http.Handler, adminAPIKey, token string) { if s.tokenStore == nil || s.dataDir == "" { s.logger.Warnw("Agent token presented but token store not configured", - zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path, adminAPIKey)), zap.String("remote_addr", r.RemoteAddr)) s.writeError(w, r, http.StatusUnauthorized, "Agent tokens are not configured on this server") return @@ -629,7 +660,7 @@ func (s *Server) handleAgentTokenAuth(w http.ResponseWriter, r *http.Request, ne agentToken, err := s.tokenStore.ValidateAgentToken(token, hmacKey) if err != nil { s.logger.Warnw("Agent token validation failed", - zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path, adminAPIKey)), zap.String("remote_addr", r.RemoteAddr), zap.String("error", err.Error())) s.writeError(w, r, http.StatusUnauthorized, fmt.Sprintf("Agent token invalid: %s", err.Error())) @@ -665,7 +696,7 @@ func (s *Server) handleAgentTokenAuth(w http.ResponseWriter, r *http.Request, ne s.logger.Debugw("Agent token authenticated", zap.String("agent_name", agentToken.Name), zap.String("token_prefix", agentToken.TokenPrefix), - zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path, adminAPIKey)), zap.String("remote_addr", r.RemoteAddr)) next.ServeHTTP(w, r.WithContext(ctx)) @@ -1148,15 +1179,24 @@ func (s *Server) httpLoggingMiddleware() func(http.Handler) http.Handler { // subresource requests put it in the Referer too. Unredacted, this // line wrote the admin credential to disk on every request. The // renderers are internal/oauth's — one rule for every log sink. + // + // SEC-01 follow-up (PR #1350): the admin key has no vendor shape and + // is bare hex, so neither the name rule nor the value-shaped detector + // those renderers otherwise run can catch it as a raw PATH segment + // (`/api/v1/status/`) or a Referer fragment with no `apikey=` + // wrapper. currentAdminAPIKey() gives each renderer the live secret so + // it is caught by EXACT VALUE wherever it lands, on top of the + // existing name/shape rules. One controller read per request line. + adminKey := s.currentAdminAPIKey() s.httpLogger.Info("HTTP API Request", zap.String("method", r.Method), - zap.String("path", oauth.LogSafeRequestPath(r.URL.Path)), - zap.String("query", oauth.LogSafeQueryString(r.URL.RawQuery)), + zap.String("path", oauth.LogSafeRequestPath(r.URL.Path, adminKey)), + zap.String("query", oauth.LogSafeQueryString(r.URL.RawQuery, adminKey)), zap.String("remote_addr", r.RemoteAddr), zap.String("user_agent", r.UserAgent()), zap.Int("status", ww.statusCode), zap.Duration("duration", duration), - zap.String("referer", oauth.LogSafeRequestURL(r.Referer())), + zap.String("referer", oauth.LogSafeRequestURL(r.Referer(), adminKey)), zap.Int64("content_length", r.ContentLength), ) }) diff --git a/internal/oauth/logging.go b/internal/oauth/logging.go index 31c7032ad..3e036a24a 100644 --- a/internal/oauth/logging.go +++ b/internal/oauth/logging.go @@ -1046,6 +1046,70 @@ func capLogSafeRequestInput(s string) string { return s[:safeTruncateBytes(s, maxLogSafeRequestBytes)] + capEllipsis } +// minKnownSecretLen guards redactKnownSecrets against a pathologically short +// needle. Every real caller passes a generated credential (the 64-hex admin +// key, an mcp_agt_ token) that clears this by a wide margin; the floor exists +// so a future caller that accidentally hands this a one- or two-character +// value (an empty-string guard that slipped, a config default) cannot turn +// exact-match redaction into "black out every path that happens to contain +// this common substring". +const minKnownSecretLen = 8 + +// redactKnownSecrets replaces every exact, case-sensitive occurrence of a +// caller-supplied secret with the audit marker, before any name- or +// shape-based rule below runs. +// +// SEC-01 gap fix (PR #1350, live-verification follow-up): LogSafeRequestPath +// masks a credential by NAME (`apikey=`) or by VENDOR SHAPE +// (`ghp_…`), but mcpproxy's own auto-generated admin API key +// (config.generateAPIKey) is, in the ordinary case, bare 64-character hex +// with no enclosing name and no vendor prefix, and hex's 4-bit-per-symbol +// ceiling means even a perfectly random hex string's Shannon entropy (max +// 4.0) never clears the value-shaped detector's 4.5 threshold — so NEITHER +// existing rule can ever catch it, in any position, no matter how they are +// tuned. A live instance reproduced the key landing verbatim in main.log as +// a bare PATH SEGMENT. +// +// "In the ordinary case" because generateAPIKey's crypto/rand failure +// fallback emits `mcpproxy_` instead (a different, non-hex shape), +// and an operator can override the key entirely via MCPPROXY_API_KEY or +// config file with any string. Exact-match redaction does not care which +// shape it is — that is precisely why it is the fix here rather than a rule +// tied to "64 hex chars". +// +// The alternative — a shape rule for "a full path/query segment that is +// exactly 64 hex chars" — was rejected: this same API logs SHA-256 tool +// hashes and other 64-hex-char identifiers (activity/request ids) as +// legitimate, diagnostically useful path segments, so a shape rule tight +// enough to name the key's format is also tight enough to name theirs, and a +// looser one is back to false positives on ordinary log lines. Exact-value +// match has none of that ambiguity: it only ever matches the bytes of a +// secret this process actually holds right now, so it has zero false +// positives by construction, and it covers every SHAPE the secret might sit +// in at once — path, query, referer, fragment — including ones nobody has +// thought to write a rule for yet. Its cost, paid once at each of the three +// entry points below rather than per rule, is that the caller must know and +// pass the live secret; internal/httpapi's Server.currentAdminAPIKey reads it +// fresh from the controller on every call, so a rotated key is covered +// immediately and there is nothing to keep in sync. +// +// The admin key is the only secret plumbed this way. An mcp_agt_ agent +// token IS vendor-shaped (see agentTokenPattern in +// internal/security/patterns/tokens.go) and so is already covered by +// MaskDetectedSecrets below without needing its live value threaded through +// — see TestLogSafeRequestPath_AgentTokenCoveredByShapeRule. +func redactKnownSecrets(s string, secrets []string) string { + for _, secret := range secrets { + if len(secret) < minKnownSecretLen { + continue + } + if strings.Contains(s, secret) { + s = strings.ReplaceAll(s, secret, redactedMarker) + } + } + return s +} + // LogSafeQueryString is LogSafeURL for a BARE query string — `r.URL.RawQuery`, // with no scheme, host or path in front of it. // @@ -1062,10 +1126,16 @@ func capLogSafeRequestInput(s string) string { // header value, and url.ParseRequestURI leaves a literal '#' in RawQuery — and // every parse-dependent path falls back to a regex with no `apikey` rule, which // is how the credential got published in the first place. -func LogSafeQueryString(rawQuery string) string { +// +// knownSecrets, when given, are redacted by EXACT VALUE before the name and +// value-shaped rules run — see redactKnownSecrets. Omit it (or pass "") for +// the internal recursive uses in this file, which run on a string a caller +// above has already put through this pass once. +func LogSafeQueryString(rawQuery string, knownSecrets ...string) string { if rawQuery == "" { return rawQuery } + rawQuery = redactKnownSecrets(rawQuery, knownSecrets) rawQuery = capLogSafeRequestInput(rawQuery) return AuditRedaction.finish(rawQuery, redactRawQueryParams(rawQuery, AuditRedaction.masker())) } @@ -1101,10 +1171,16 @@ func LogSafeQueryString(rawQuery string) string { // That is a shape mcpproxy's own authorize URLs have and a browser Referer does // not, and buying it back would mean parsing - the one thing an untrusted URL // cannot be relied on to survive. -func LogSafeRequestURL(rawURL string) string { +// +// knownSecrets, when given, are redacted by EXACT VALUE against the WHOLE raw +// string before it is taken apart — see redactKnownSecrets. This is what +// catches the admin key sitting in the path or fragment of a Referer with no +// `apikey=` wrapper at all, which none of the piecewise rules above can. +func LogSafeRequestURL(rawURL string, knownSecrets ...string) string { if rawURL == "" { return rawURL } + rawURL = redactKnownSecrets(rawURL, knownSecrets) rawURL = capLogSafeRequestInput(rawURL) scrubbed := redactURLUserinfo(rawURL) main, fragment, hasFragment := strings.Cut(scrubbed, "#") @@ -1131,7 +1207,17 @@ func LogSafeRequestURL(rawURL string) string { // splits on '/' and runs the full value-shaped detector per segment, so an // uncapped path let a client turn one request into an unbounded number of // detector passes. See maxLogSafeRequestBytes. -func LogSafeRequestPath(path string) string { +// +// knownSecrets, when given, are redacted by EXACT VALUE before the path is +// split into segments — see redactKnownSecrets. This is the fix for the +// admin key surviving as a bare PATH SEGMENT: it has no `name=` wrapper for +// the per-segment name rule to key on and, being plain hex, tops out at 4.0 +// bits/char of Shannon entropy — under the value-shaped detector's 4.5 +// threshold — so no shape-based rule below this call can ever catch it. +// internal/httpapi.Server.currentAdminAPIKey is the one caller that passes +// it, reading the live key fresh on every request. +func LogSafeRequestPath(path string, knownSecrets ...string) string { + path = redactKnownSecrets(path, knownSecrets) return logSafeURLComponent(capLogSafeRequestInput(path)) } diff --git a/internal/oauth/logging_exact_secret_test.go b/internal/oauth/logging_exact_secret_test.go new file mode 100644 index 000000000..12993153e --- /dev/null +++ b/internal/oauth/logging_exact_secret_test.go @@ -0,0 +1,128 @@ +package oauth + +import ( + "strings" + "testing" +) + +// SEC-01 gap fix (PR #1350, live-verification follow-up). +// +// LogSafeRequestPath, LogSafeQueryString and LogSafeRequestURL mask a +// credential by NAME (`apikey=`) or by VENDOR SHAPE (`ghp_…`, +// `sk-…`). The admin API key mcpproxy auto-generates +// (config.generateAPIKey) is neither: it is a bare 64-character lowercase +// hex string with no enclosing name and no vendor prefix. Worse, hex has a +// 4-bit-per-symbol ceiling (16 possible runes), so even a perfectly random +// hex string's Shannon entropy tops out at 4.0 — below the detector's 4.5 +// "possible secret" threshold (internal/security/entropy.go) — so the +// value-shaped detector can never catch it either, no matter how it is +// tuned. A live instance reproduced the key landing verbatim in +// ~/.mcpproxy/logs/main.log as a bare PATH SEGMENT +// (`GET /api/v1/status/`). +// +// These renderers now accept the caller's currently-configured secret(s) and +// redact every EXACT occurrence before any name- or shape-based rule runs. +// Exact match is the only rule that cannot be defeated by inventing a new +// shape, and it is also the only one that will not fire on a same-shaped +// value that happens NOT to be the live secret — see the +// "*_UnrelatedHexPathSegmentsSurvive" tests below, which pin that a SHA-256 +// tool hash or a ULID-shaped activity id sitting right next to the real key +// in the same path is left untouched. +const sec01AdminKey = "ccc01b2ee10ad391e6a083b675023bba7124a8c8c94203b9cc4f2683831df1ee" + +// A tool hash / activity id: same shape (hex) and comparable length to the +// admin key, but NOT the configured secret. Chosen to be exactly 64 hex +// chars, the shape a real SHA-256 digest takes, so it also exercises the +// "same length as the key" edge the naive fix (mask every 64-hex segment) +// would get wrong. +const sec01UnrelatedHex = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b85" + +func TestLogSafeRequestPath_ExactSecretRedactsBarePathSegment(t *testing.T) { + path := "/api/v1/status/" + sec01AdminKey + + got := LogSafeRequestPath(path, sec01AdminKey) + if strings.Contains(got, sec01AdminKey) { + t.Fatalf("admin key survived in the path field: %q", got) + } +} + +func TestLogSafeRequestPath_UnrelatedHexPathSegmentsSurvive(t *testing.T) { + // The false-positive story: a tool-hash / activity-id path segment right + // next to the redacted key must not be swept up too. + path := "/api/v1/activity/" + sec01UnrelatedHex + "/tool/" + sec01AdminKey + + got := LogSafeRequestPath(path, sec01AdminKey) + if strings.Contains(got, sec01AdminKey) { + t.Fatalf("admin key survived in the path field: %q", got) + } + if !strings.Contains(got, sec01UnrelatedHex) { + t.Fatalf("unrelated 64-hex path segment (activity id / tool hash) was over-redacted: %q", got) + } +} + +func TestLogSafeQueryString_ExactSecretRedactsBareValue(t *testing.T) { + got := LogSafeQueryString(sec01AdminKey, sec01AdminKey) + if strings.Contains(got, sec01AdminKey) { + t.Fatalf("admin key survived in the query field: %q", got) + } +} + +func TestLogSafeQueryString_UnrelatedHexValueSurvives(t *testing.T) { + got := LogSafeQueryString("request_id="+sec01UnrelatedHex, sec01AdminKey) + if !strings.Contains(got, sec01UnrelatedHex) { + t.Fatalf("unrelated 64-hex query value (request id) was over-redacted: %q", got) + } +} + +func TestLogSafeRequestURL_ExactSecretRedactsPathAndFragment(t *testing.T) { + for _, referer := range []string{ + "http://127.0.0.1:8080/ui/" + sec01AdminKey, + "http://127.0.0.1:8080/ui/#/servers/" + sec01AdminKey, + } { + got := LogSafeRequestURL(referer, sec01AdminKey) + if strings.Contains(got, sec01AdminKey) { + t.Fatalf("admin key survived in the referer field: %q -> %q", referer, got) + } + } +} + +// No known secret configured (the empty-string case an unconfigured or +// testing scenario produces, see httpapi.Server.currentAdminAPIKey) must be a +// no-op: the exact-match pass never runs on an empty needle. +func TestLogSafeRequestPath_EmptyKnownSecretIsNoop(t *testing.T) { + path := "/api/v1/status/" + sec01UnrelatedHex + + got := LogSafeRequestPath(path, "") + if got != LogSafeRequestPath(path) { + t.Fatalf("an empty known secret changed the rendering: %q vs %q", got, LogSafeRequestPath(path)) + } +} + +// Backward compatibility: every existing single-argument call site (internal +// recursive use inside this file, and every caller before SEC-01's follow-up) +// must keep compiling and behaving exactly as before. +func TestLogSafeRequestPath_NoKnownSecretsArgStillCompiles(t *testing.T) { + const path = "/api/v1/servers" + if got := LogSafeRequestPath(path); got != path { + t.Fatalf("benign path was rewritten: got %q", got) + } +} + +// Unlike the admin key, an mcp_agt_ agent token IS vendor-shaped (see +// agentTokenPattern in internal/security/patterns/tokens.go), so it is +// already caught by the value-shaped detector (MaskDetectedSecrets) with NO +// known-secret value threaded through — exercised here at the same three +// entry points the admin key needed exact-match plumbing for. +func TestLogSafeRequestPath_AgentTokenCoveredByShapeRule(t *testing.T) { + const agentToken = "mcp_agt_" + "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4" + + if got := LogSafeRequestPath("/api/v1/status/" + agentToken); strings.Contains(got, agentToken) { + t.Fatalf("agent token survived in the path field: %q", got) + } + if got := LogSafeQueryString("opaque=" + agentToken); strings.Contains(got, agentToken) { + t.Fatalf("agent token survived in the query field: %q", got) + } + if got := LogSafeRequestURL("http://127.0.0.1:8080/ui/" + agentToken); strings.Contains(got, agentToken) { + t.Fatalf("agent token survived in the referer field: %q", got) + } +} diff --git a/internal/oauth/round8_renderer_discovery_test.go b/internal/oauth/round8_renderer_discovery_test.go index 2051b6750..5faf3020b 100644 --- a/internal/oauth/round8_renderer_discovery_test.go +++ b/internal/oauth/round8_renderer_discovery_test.go @@ -35,6 +35,15 @@ import ( // exportedStringFuncs binds every exported `func(string) string` in this // package that renders or rewrites a value. TestExportedStringFuncs_AreAllBound // fails when the package grows one that is missing here. +// +// LogSafeQueryString, LogSafeRequestURL and LogSafeRequestPath are wrapped in +// a closure that calls them with no knownSecrets: SEC-01's follow-up (PR +// #1350) grew them a trailing `...string` for exact-value redaction (see +// isStringVariadicToString below), which no longer matches the map's +// `func(string) string` value type, but the property this map exists to +// check — that the rendering carries a marker the fail-closed net recognises +// — must still hold for their name-rule/shape-rule output with no secret +// configured, exactly as it did before the signature grew. var exportedStringFuncs = map[string]func(string) string{ "MaskValue": MaskValue, "AuditMaskValue": AuditMaskValue, @@ -47,9 +56,9 @@ var exportedStringFuncs = map[string]func(string) string{ // Issue #1158, review round 2. Both emit masks, so the fail-closed net has // to know their markers even though no write door echoes them back. "LogSafeURL": LogSafeURL, - "LogSafeQueryString": LogSafeQueryString, - "LogSafeRequestURL": LogSafeRequestURL, - "LogSafeRequestPath": LogSafeRequestPath, + "LogSafeQueryString": func(s string) string { return LogSafeQueryString(s) }, + "LogSafeRequestURL": func(s string) string { return LogSafeRequestURL(s) }, + "LogSafeRequestPath": func(s string) string { return LogSafeRequestPath(s) }, "LogSafeCallbackQuery": LogSafeCallbackQuery, } @@ -240,7 +249,7 @@ func discoverExportedStringFuncs(t *testing.T) []string { continue } if fn.Recv == nil { - if isStringToString(fn.Type) { + if isStringToString(fn.Type) || isStringVariadicToString(fn.Type) { names = append(names, fn.Name.Name) } continue @@ -312,6 +321,32 @@ func isStringIdent(e ast.Expr) bool { return ok && id.Name == "string" } +// isStringVariadicToString reports whether ft is `func(string, ...string) string` +// — one required string argument, a trailing `...string`, one string result. +// This is the shape LogSafeRequestPath, LogSafeQueryString and +// LogSafeRequestURL grew for the SEC-01 known-secret exact-match pass (PR +// #1350 follow-up): the trailing knownSecrets is redacted by exact value +// before any other rule runs, so a function of this shape is exactly as much +// a mask rendering as isStringToString's narrower `func(string) string` and +// belongs in the same discovery net. +func isStringVariadicToString(ft *ast.FuncType) bool { + if ft.Params == nil || ft.Results == nil { + return false + } + if len(ft.Results.List) != 1 || !isStringIdent(ft.Results.List[0].Type) { + return false + } + fields := ft.Params.List + if len(fields) != 2 { + return false + } + if !isStringIdent(fields[0].Type) || len(fields[0].Names) > 1 { + return false + } + ell, ok := fields[1].Type.(*ast.Ellipsis) + return ok && isStringIdent(ell.Elt) +} + // packageDir returns the directory this test file lives in. func packageDir(t *testing.T) string { t.Helper() diff --git a/internal/security/patterns/tokens.go b/internal/security/patterns/tokens.go index 75827961d..4cabb6f6e 100644 --- a/internal/security/patterns/tokens.go +++ b/internal/security/patterns/tokens.go @@ -32,12 +32,42 @@ func GetTokenPatterns() []*Pattern { cohereKeyPattern(), deepseekKeyPattern(), togetherAIKeyPattern(), + // mcpproxy's own agent tokens + agentTokenPattern(), // Generic tokens jwtTokenPattern(), bearerTokenPattern(), } } +// mcpproxy agent token (internal/auth.GenerateToken / ValidateTokenFormat): +// mcp_agt_ followed by exactly 64 hex characters (32 random bytes), 72 chars +// total. SEC-01 follow-up (PR #1350): this is the ONLY bare-hex secret +// mcpproxy issues that has its own vendor prefix — the admin API key does +// not, which is why that one is fixed by exact-value match +// (internal/oauth/logging.go's redactKnownSecrets) instead of a shape rule +// here. The character class is case-insensitive (`[0-9a-fA-F]`) to match +// ValidateTokenFormat exactly: it decodes the suffix with hex.DecodeString, +// which accepts either case, even though GenerateToken only ever emits +// lowercase — a token this codebase would validate as genuine must also be +// one this pattern can mask. +// +// The fixed length ({64}, not an open-ended `[0-9a-fA-F]+`) is deliberate: +// it stops the match at exactly one token's worth of hex, so a SECOND +// concatenated hex value right after a genuine token (a longer, unrelated +// blob glued to the suffix) is not swallowed into the same match and left +// unmasked. It does NOT, and cannot, distinguish the prefix from an +// unrelated ≥64-hex-char blob that happens to start there — that case still +// matches, which only means MORE is masked, the fail-closed direction. +func agentTokenPattern() *Pattern { + return NewPattern("mcp_agent_token"). + WithRegex(`mcp_agt_[0-9a-fA-F]{64}`). + WithCategory(CategoryAPIToken). + WithSeverity(SeverityCritical). + WithDescription("mcpproxy agent token"). + Build() +} + // GitHub Personal Access Token (classic and fine-grained) func githubPATPattern() *Pattern { // ghp_ = classic PAT, github_pat_ = fine-grained PAT diff --git a/internal/security/patterns/tokens_test.go b/internal/security/patterns/tokens_test.go index 2f4f6206a..407b935ad 100644 --- a/internal/security/patterns/tokens_test.go +++ b/internal/security/patterns/tokens_test.go @@ -1694,3 +1694,72 @@ func TestAllLLMPatternsExist(t *testing.T) { }) } } + +// SEC-01 follow-up (PR #1350): agent tokens (internal/auth.GenerateToken) +// carry the mcp_agt_ prefix followed by 64 lowercase hex chars — a vendor +// shape exactly as distinctive as ghp_/gho_/glpat- above — but, unlike those, +// had no pattern here at all, so MaskDetectedSecrets (internal/oauth) never +// caught one sitting bare in a URL path/query/fragment the way it already +// catches an opaque ghp_ token. The admin API key is NOT fixed this way +// (see internal/oauth/logging.go's redactKnownSecrets doc): it has no prefix +// to key on. An agent token does, so a shape rule is the right tool here and +// carries none of the false-positive risk a bare-hex rule would. +func TestAgentTokenPattern(t *testing.T) { + tests := []struct { + name string + input string + wantMatch bool + }{ + { + name: "well-formed agent token", + input: "mcp_agt_" + strings.Repeat("a1", 32), + wantMatch: true, + }, + { + name: "agent token embedded in a bare path segment", + input: "/api/v1/status/mcp_agt_" + strings.Repeat("b2", 32), + wantMatch: true, + }, + { + name: "agent token under an opaque query value", + input: "opaque=mcp_agt_" + strings.Repeat("c3", 32), + wantMatch: true, + }, + { + // zcode review round 1 (PR #1350 follow-up): ValidateTokenFormat + // decodes the suffix with hex.DecodeString, which is + // case-insensitive, so a token this codebase validates as genuine + // must also be one this pattern masks. + name: "well-formed agent token, uppercase hex", + input: "mcp_agt_" + strings.Repeat("A1", 32), + wantMatch: true, + }, + { + name: "wrong prefix", + input: "mcp_tok_" + strings.Repeat("a1", 32), + wantMatch: false, + }, + { + name: "prefix with too few hex chars", + input: "mcp_agt_" + strings.Repeat("a1", 10), + wantMatch: false, + }, + } + + patterns := GetTokenPatterns() + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pattern := findPatternByName(patterns, "mcp_agent_token") + if pattern == nil { + t.Fatalf("mcp_agent_token pattern not found") + } + matches := pattern.Match(tt.input) + if tt.wantMatch { + assert.NotEmpty(t, matches, "expected match for: %s", tt.input) + } else { + assert.Empty(t, matches, "expected no match for: %s", tt.input) + } + }) + } +}