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

[Dumbris]

What

SEC-01: the root admin API key was written in plaintext to log files on disk, at two places.

internal/httpapi's access logger wrote query, referer and path verbatim for every request the router sees (it is mounted at the router root, so /events, /ui/ and the whole API). ?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=<KEY> so same-origin requests put it in the Referer as well — the credential was landing in http.log.

cmd/mcpproxy logged a freshly auto-generated key three times at Warn level: as api_key, embedded in web_ui_url, and once more on the config-save-failure path — so main.log held it too.

How

Log fields 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 of it, and two renderers are built on it for input that arrives from an untrusted client:

• LogSafeQueryString for a bare r.URL.RawQuery, and LogSafeRequestPath / LogSafeRequestURL for the path and the Referer. None of them calls url.Parse: a request's URL is not required to parse (an HTAB is legal inside a header value, a literal # is legal in a request target), and every parse-dependent path fell back to a regex that has no apikey rule. They decompose by hand and redact userinfo, fragment, query and each path segment in its own right.

The auto-generated key is printed once to stderr when stderr is an interactive terminal, and is written to the config file as before. The log sink gets maskAPIKey(key), a web_ui_url built with the key already masked, and the config path. Never stdout — an empty or :0 listen address selects the stdio MCP transport.

Least-surprising behaviour, decided here: on a non-terminal stderr (launchd, systemd, CI) the raw key is withheld even when the config save failed, because a redirected stream is as persistent as a log file; the banner and the Warn line name the remedy instead (MCPPROXY_API_KEY, or make the path writable). A failed save does not abort startup — that would be a larger breaking change than this fix should carry.

Scripts: scripts/test-api-e2e.sh used to obtain the key by grepping "api_key" out of the redirected server log. Both extraction functions now read it from the config file with jq, the way scripts/dev-server-edition.sh already did, and scripts/test-extract-api-key.sh tests the new contract.

?apikey= keeps working exactly as before; removing that credential channel is a separate breaking change.

Verified

• TDD: the httpapi and cmd/mcpproxy tests were written first and failed on the pre-fix code with the raw key visible in the captured log fields.
• ./scripts/test-api-e2e.sh — 0 failures, including the server-edition audit-log instance.
• go test -race on internal/oauth, internal/httpapi, cmd/mcpproxy, internal/transport, internal/upstream/..., internal/runtime/....
• Both golangci-lint passes (bare and --build-tags server).
• On a live instance: exercised ?apikey=, a malformed Referer, a #?apikey= fragment, hash routing and a key in the path; the raw key appears in no file under ~/Library/Logs/mcpproxy afterwards, and every request still authenticates.
• Cross-model review: codex gpt-5.6-sol, 8 rounds, clean verdict on the last one.

Known boundary

A credential whose parameter name cannot be decoded and normalised on its own — ?<KEY> with no =, api%6bey%3d<KEY>, /ui%2Fapi%6bey=<KEY> — is only seen by the value-shaped detector, which does not recognise a bare hex string. mcpproxy never reads its credential from any of those shapes, and the gap underneath is repo-wide; it is pinned in TestLogSafeQueryString_DocumentedNameRuleBoundary rather than guessed at here.

Follow-up, out of scope: internal/observability/tracing.go puts r.URL.String() into the http.url span attribute. Tracing is off by default and spans go to a collector rather than a log file, but it is the same credential.

🤖 Generated with Claude Code

[cloudflare-workers-and-pages]

Deploying mcpproxy-docs with    Cloudflare Pages

Latest commit:	ac871ef
Status:	✅  Deploy successful!
Preview URL:	https://1318a20f.mcpproxy-docs.pages.dev
Branch Preview URL:	https://fix-redact-apikey-from-logs.mcpproxy-docs.pages.dev

View logs

[github-actions]

📦 Build Artifacts

Workflow Run: View Run
Branch: fix/redact-apikey-from-logs

Available Artifacts

• archive-darwin-amd64 (30 MB)
• archive-darwin-arm64 (27 MB)
• archive-linux-amd64 (18 MB)
• archive-linux-arm64 (16 MB)
• archive-windows-amd64 (30 MB)
• archive-windows-arm64 (26 MB)
• frontend-dist-pr (0 MB)
• installer-dmg-darwin-amd64 (24 MB)
• installer-dmg-darwin-arm64 (22 MB)
• smart-mcp-proxymcpproxy-goUUCEY9.dockerbuild (0 MB)

How to Download

Option 1: GitHub Web UI (easiest)

1. Go to the workflow run page linked above
2. Scroll to the bottom "Artifacts" section
3. Click on the artifact you want to download

Option 2: GitHub CLI

gh run download 35819500390 --repo smart-mcp-proxy/mcpproxy-go

Note: Artifacts expire in 14 days.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 87.42515% with 21 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cmd/mcpproxy/main.go	0.00%	8 Missing ⚠️
internal/oauth/logging.go	89.47%	4 Missing and 4 partials ⚠️
cmd/mcpproxy/api_key_banner.go	92.00%	3 Missing and 1 partial ⚠️
internal/httpapi/server.go	96.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[Dumbris]

Second-lens review: fixed a pre-auth DoS amplification in the SEC-01 redaction path

Verified the reported finding against the code and it was genuine — reproduced it directly:

• httpLoggingMiddleware is mounted at the chi router root (s.router.Use(...), internal/httpapi/server.go), before apiKeyAuthMiddleware, so it runs on every request the listener accepts, authenticated or not.
• It calls oauth.LogSafeRequestPath / LogSafeQueryString / LogSafeRequestURL on r.URL.Path, r.URL.RawQuery and r.Referer() after next.ServeHTTP returns — i.e. after the response is already written, so ReadTimeout/WriteTimeout/ReadHeaderTimeout don't bound this work (they stop future reads/writes on the connection, not a goroutine already executing).
• logSafeURLComponent (the path/URL renderer) splits its input on / and runs the full value-shaped detector (security.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 2-byte segments (/a repeated) fits comfortably inside the existing 1MB MaxHeaderBytes budget (internal/server/server.go) and turns one unauthenticated request into hundreds of thousands of full detector passes.
• Reproduced with a direct unit benchmark before the fix: a single LogSafeRequestPath call on a ~1MB, 500k-segment adversarial path took ~800ms of CPU. Equivalent amplification exists via a huge bare query string (LogSafeQueryString) and a huge Referer (LogSafeRequestURL).

Fix

Bounded the cost at the input rather than the algorithm: LogSafeRequestPath, LogSafeQueryString and LogSafeRequestURL now cap their argument to 4KB (on a UTF-8 rune boundary) before any redaction rule runs. A few KB is already generous for what an access-log line needs to be useful, and capping at these three entry points bounds every downstream call (logSafeURLComponent's per-segment splitting, logSafeFragment, the nested LogSafeQueryString calls) transitively, including the other ~12 call sites of these functions in internal/httpapi/server.go (auth-decision logging), not just the access-log line that was reported.

Added regression tests (internal/oauth/logging_dos_test.go) that:

• fail before the fix (confirmed: ~650–800ms per call on adversarial input) and pass after (bounded, low-ms),
• confirm credentials within the cap are still masked, so this is a cost bound, not a detection regression.

No other findings from this round — the one MUST FIX item above was the only one raised, and there were no SHOULD FIX items.

gofmt, go build, both golangci-lint passes (bare and --build-tags server), and -race tests on internal/oauth, internal/httpapi, and internal/security all pass clean, with no new lint findings introduced (the two runs' remaining findings are pre-existing, unrelated to the touched files).

🤖 Generated with Claude Code

[Dumbris]

SEC-01 follow-up: admin key surviving as a bare path/query/referer segment

Live verification (and a direct repro against LogSafeRequestPath on this branch) found that a request like

GET /api/v1/status/<64-hex-admin-key>

still writes the key verbatim to main.log, even though every log site already routes through oauth.LogSafeRequestPath/LogSafeQueryString/LogSafeRequestURL.

Root cause

Not a missed call site — the renderers themselves can't catch this shape. mcpproxy's auto-generated admin key (config.generateAPIKey) is bare 64-character hex with no separator and no vendor prefix:

• The name rule (apikey=<value>, token=<value>, …) only fires when the value sits after a recognized parameter name. A bare path segment or an unwrapped query/fragment value has no name to key on.
• The value-shaped detector (MaskDetectedSecrets, Shannon-entropy + vendor regexes) can't catch it either, for a fundamental reason: hex has only 16 symbols, so the maximum possible Shannon entropy of a hex string is log2(16) = 4.0 — below the detector's 4.5 "possible secret" threshold. No amount of tuning that threshold fixes this; a perfectly random hex string can never clear it.

The fix: exact-value redaction, not a shape rule

I considered a shape rule (e.g. "redact a full path/query segment that is exactly 64 hex chars") and rejected it. This same API logs SHA-256 tool hashes and other 64-hex-char identifiers (activity/request ids) as legitimate, diagnostically useful path segments. A shape rule tight enough to name the admin key's format (64 lowercase hex, no delimiters) is exactly as tight as what a tool hash or request id looks like — there is no regex that distinguishes "the admin key" from "a SHA-256 digest that happens to sit in a path." Loosening the rule to be safe just reopens false positives on ordinary log lines.

Instead, internal/oauth/logging.go gained redactKnownSecrets(s, secrets): an exact-value pass that redacts every literal occurrence of a caller-supplied secret, run before the name/shape rules. LogSafeRequestPath, LogSafeQueryString and LogSafeRequestURL each grew a trailing ...knownSecrets parameter for it. internal/httpapi.Server.currentAdminAPIKey() reads the live cfg.APIKey fresh from the controller on every call (nil-safe when s.controller is nil, e.g. in tests) and is threaded into all 13 log call sites that render r.URL.Path / r.URL.RawQuery / r.Referer().

Why exact-value over shape:

• Zero false positives by construction — it only ever matches bytes this process actually holds as a live secret right now, so it can never catch a tool hash, activity id, or anything else that merely looks hex.
• Covers every shape at once (path, query, referer, fragment) in one pass, including shapes nobody has written a rule for yet.
• Also correct for a user-supplied admin key (MCPPROXY_API_KEY or api_key in config), which need not be hex at all — a shape rule tied to "64 hex chars" would miss it entirely; exact-value doesn't care what the key looks like.
• Cost is proportional to reading the live key at the logging site, which internal/httpapi already does per-request for auth (apiKeyAuthMiddleware); this reuses the same controller read.

False-positive guard: minKnownSecretLen = 8 — a defensive floor so a pathologically short/empty value can never turn exact-match into "redact every path containing this common substring." Pinned by test: TestLogSafeRequestPath_UnrelatedHexPathSegmentsSurvive puts a different 64-hex value (standing in for a SHA-256 tool hash / activity id) right next to the redacted key in the same path and asserts it survives untouched.

Agent tokens — checked, already covered differently

Agent tokens (mcp_agt_ + 64 hex, internal/auth.GenerateToken) are not fixed via exact-value match — and don't need to be. Unlike the admin key, they carry a distinctive vendor prefix (mcp_agt_), exactly like ghp_/gho_/glpat- etc. already in the pattern set. I added agentTokenPattern() (mcp_agt_[0-9a-f]{64}, fixed-length to avoid swallowing an unrelated hex blob sitting after the prefix) to internal/security/patterns/tokens.go, so MaskDetectedSecrets now catches an agent token wherever it appears — path, query, tool responses, activity log — with no per-request plumbing needed. TestLogSafeRequestPath_AgentTokenCoveredByShapeRule pins this end-to-end at all three renderer entry points with no known-secret argument passed.

Tests (TDD — written first, confirmed failing before the fix)

• internal/oauth/logging_exact_secret_test.go (new): the bare-path-segment leak, the same for query/referer, the unrelated-hex-survives false-positive check (twice — path and query), the empty-known-secret no-op case, backward-compat with zero knownSecrets args, and the agent-token shape-rule coverage check.
• internal/security/patterns/tokens_test.go: TestAgentTokenPattern — matches a well-formed token bare, under a path segment, and under an opaque query value; rejects the wrong prefix and a too-short hex suffix.
• internal/oauth/round8_renderer_discovery_test.go: extended the AST-based "every exported mask renderer is bound or exempted" discovery net to recognize the new func(string, ...string) string shape (isStringVariadicToString), so the three renderers stay covered by the existing fail-closed-net property test rather than falling out of it silently.

zcode (GLM-5.3) review round — findings applied

One read-only review round on internal/oauth/logging.go, internal/httpapi/server.go and the new/changed tests. Five genuine findings, all applied:

1. Redundant config read. handleAgentTokenAuth's two callers (authenticateExplicitToken, authenticateBearer) already hold cfg, but the function itself called s.currentAdminAPIKey() again — a second s.controller.GetCurrentConfig() RLock on every agent-token-authenticated request. Fixed: handleAgentTokenAuth now takes adminAPIKey string from its callers instead.
2. Case-sensitivity gap. internal/auth.ValidateTokenFormat decodes an agent token's hex suffix with hex.DecodeString, which is case-insensitive, but the new mcp_agt_ pattern was lowercase-only ([0-9a-f]{64}). A token the server would validate as genuine could theoretically go unmasked. Fixed: [0-9a-fA-F]{64}, with an uppercase-hex test case added.
3. Comment overclaim (pattern length). The doc comment on agentTokenPattern claimed the fixed {64} length stops it from matching "the prefix sitting in front of an unrelated hex blob it doesn't own" — true only for a shorter trailing blob; a longer one (≥64 hex chars right after the prefix) still matches. Reworded to say precisely what the fixed length does (stops the match at one token's worth of hex so a second concatenated secret isn't swallowed into the same match) and that over-matching a longer blob is harmless (fail-closed), not a false-positive risk.
4. Doc overclaim (key shape). redactKnownSecrets's doc said the admin key "is bare 64-character hex," which is only true when crypto/rand.Read succeeds — generateAPIKey's failure fallback emits mcpproxy_<nanos>, and an operator can override the key to any string via MCPPROXY_API_KEY/config. Reworded to "in the ordinary case," with the fallback and override noted explicitly (which reinforces the exact-match rationale rather than undermining it).
5. No blocking defects. Confirmed: the ordering of exact-match before capLogSafeRequestInput truncation is correct in all three entry points (no partial-secret leak); all 13 oauth.LogSafe* call sites in internal/httpapi/server.go pass a secret; no data race on cfg.APIKey reads (config is swapped by pointer, never mutated in place); performance cost is one RLock + type-assert per call, negligible next to the renderers' own regex/entropy work. One informational note left as-is (not fixed): a percent-encoded key under an opaque query/path/referer position evades both exact-match and the shape detector, matching this file's pre-existing, deliberately-declined NAME-rule boundary (TestLogSafeQueryString_DocumentedNameRuleBoundary) — reaching it requires a client that already knows the key, so it is log-evasion, not log-leak.

Verification

• go build ./... and go build -tags server -o /dev/null ./cmd/mcpproxy — clean.
• go test -race ./internal/oauth/... ./internal/httpapi/... ./internal/security/... — all pass, before and after applying the review findings.
• go test -race -tags server -timeout 20m -skip "E2E|Binary|MCPProtocol|TestInfoEndpoint|TestGracefulShutdownNoPanic|TestSocketInfoEndpoint" ./internal/serveredition/... ./internal/config/... ./internal/oauth/... ./internal/server/... ./internal/httpapi/... ./internal/storage/... — all pass.
• golangci-lint run --config .github/.golangci.yml ./... and again with --build-tags server — no new findings; the pre-existing findings on each invocation are identical before and after this diff (confirmed by lint-checking the base branch via a temporary stash).

Co-Authored-By: Claude Opus 5 noreply@anthropic.com