Repository navigation
fix(security): stop credentials reaching logs, stdout and structured errors - #1181
Merged
Merged
Conversation
…errors (#1158) A default `mcpproxy serve` at DEBUG published an argv secret 12 times and a URL-query secret 8 times in a 20-second startup, with no trace mode enabled. Verified on a live instance: 24 cleartext occurrences across main.log, server-<name>.log and the terminal, now 0. Four classes, four rules, all routed through seams the #1148/#1157 sweep already built rather than a new matcher. 1. Spawn argv. Every spawn log site used shellwrap.RedactDockerArgs alone - a STRUCTURAL rule that masks the value half of `-e KEY=VALUE` and is blind to `--api-key sk-live-...`. New oauth.Redaction.SpawnArgv / SpawnCommandString compose it with the SEMANTIC flag-name + value-shape rule (Redaction.Argv) and reach inside the single `-c "<whole command line>"` element the login-shell wrap produces - the form that actually leaked, since every non-docker stdio server on macOS spawns that way. Redaction.Argv itself is untouched: it backs the read doors and CheckArgvMaskEcho on the write doors, so the spawn rule has to be additive and log-only. There is deliberately no "already masked, skip" guard between the two stages - the bullet marker carries no character in secretPattern's value class, so stage 2 is provably a no-op on a stage-1 rendering, and a whole-token guard would silence stage 2 for an entire command string as soon as one benign `-e` put a marker in it. 2. The OAuth authorization URL - the largest leak, and one the site list missed. autoDetectResource returns the configured server URL verbatim on every fallback branch, that becomes extraParams["resource"], and all THREE authorize-URL emission paths splice it into the query and then log it at INFO and print it with fmt.Printf. Both sinks on all three paths now go through logSafeAuthURL, and the per-value extra-param Debug lines through Redaction.ExtraParamValue. The credential arrives percent-encoded one level down, where neither the sensitive-parameter name rule nor secretPattern can see it, so URLValueDeep decodes one nesting level. maskExtraParams no longer treats `resource` as a public endpoint. 3. URL log fields inside internal/oauth (~75 sites across discovery.go, config.go, transport_wrapper.go, persistent_token_store.go), plus the configured-URL field in runtime.go and the registry-source URLs in internal/server. They use the AUDIT rendering, not the name-rule-only RedactURLQueryParams the issue names, because a credential under an unrecognised parameter name still reached the log otherwise. url.URL.Redacted() is not used anywhere - it masks the userinfo password and leaves `?token=` intact. 4. The structured OAuth error is scrubbed where it is BUILT (createMetadataError and the seven contracts.OAuthErrorDetails literals), not where it is serialized: the encoder sees an opaque struct and cannot know which leaves are URLs, and every future consumer inherits the fix. Also: the trace transport's stdout Printf copies and zap fields (URL, headers, request/response bodies, SSE frames) - both sinks or neither; logSafeErrorField upgraded to ScrubUpstreamText; the launcher's startup banner takes an injected Spec.RedactArgs whose nil default OMITS argv, so a future caller that forgets it loses a diagnostic rather than a secret. Diagnostics are preserved throughout: flag names, env KEY names, and each URL's scheme/host/path survive; only values are replaced. Two AST guard tests derive the rule from the source across BOTH internal/oauth and internal/upstream/core, so the next URL log field or a re-raw'd authorize-URL Printf fails CI rather than shipping. Behaviour change to note: log lines, `mcpproxy upstream logs`, the `upstream_servers tail_log` MCP tool, `mcpproxy auth status/login` output and the 400 body of POST /api/v1/servers/{id}/login now render masked values where they previously showed cleartext. No config, storage, contract or frontend change.
…first pass left Round-2 cross-review of #1158. Seven blocking/major findings, three confirmed minors, three judged-then-fixed, and one design comment that was simply wrong. The sharpest was pre-existing on main rather than introduced here: the OAuth CALLBACK handler logged the authorization CODE at Info, plus the whole raw callback query, on every single login. The code is a single-use credential redeemable at the token endpoint, and it sat in ~/.mcpproxy/logs/main.log for the window before mcpproxy spent it — forever, for a flow that never completed. `state` (the CSRF nonce that pairs with it) was on five more lines and inside two operator-visible error messages; it is now a truncated SHA-256, which is all the log lines ever used it for. The rest is one theme: masking a FIELD does not mask the STATEMENT. - A *url.Error renders the request URL inside its own text, so fourteen `zap.Error(err)` calls in discovery.go published, on the identical line, the credential their masked `metadata_url` sibling was written to hide. New logSafeErrorField/LogSafeErrorText in internal/oauth; internal/transport's twin, which had drifted back to the name-rule-only scrubber, rejoins it. - `url_tried` survived raw four lines below a line this branch fixed, along with every `auth_server*` field in discovery.go — because the AST guard's regex was written from the sites already fixed. It now matches `_tried$` and the four auth-server names, and a second guard fails any log statement that carries a URL field and a raw zap.Error together. - contracts.OAuthFlowError was scrubbed field-by-field at seven construction sites, and one got it half right: emptyClientIDFlowError masked Details.ServerURL and set Details.DCRStatus.Error raw on the same struct, which is JSON-encoded straight to the REST caller. One scrubbedFlowError at construction now walks the whole struct, and a third guard requires every literal to go through it. - Client.logSafeURL, managed.Client.logSafeURL, the transport config, the CLI client and the four registry routes rendered with RedactURLQueryParams — the NAME rule only — while this branch's own new renderer deliberately does not, and says why. One exported oauth.LogSafeURL now backs all of them. Two spawn-rule corrections pull in opposite directions and both matter: - The command-string tokenizer split on whitespace only, while the string it is fed comes from Shellescape — so a credential containing a space arrived single-quoted, was split into four tokens, and had exactly its first fragment masked. On the `-c "<whole command line>"` form the original commit calls "the form that actually leaked". The split is now shell-aware (including the `'"'"'` idiom), falling back to whitespace-only when quotes do not balance, because swallowing the rest of a line into one token would be a new leak rather than a safe degradation. - The other direction: IsSensitiveKeyName matches SUBSTRINGS, so applying it to a spawn argv blanked `--max-tokens 4096` and `--public-key-path /etc/x`. The property that makes this redaction acceptable to an operator is that flag names, hosts and ordinary values survive, so the log-only rule now subtracts a short list of qualifier segments (max/min/num/public/…). It keeps the substring match rather than switching to segment EQUALITY, which would have silently un-masked `--apikey` and `--clientsecret`. Three findings were investigated rather than reflexively fixed; all three were genuine and all three are closed. The PRM's own authorization_servers list left createMetadataError unscrubbed beside siblings that were scrubbed. The token endpoint's non-200 BODY was embedded verbatim in the refresh error — provider free text that echoes request parameters, unbounded, logged and returned over REST. And the launched child's stdout/stderr went into the per-server log raw, which matters more than it looks: GET /api/v1/servers/{id}/logs serves that file, so an upstream echoing its own startup banner puts a credential mcpproxy handed it onto an HTTP surface. Finally, the design comment claiming the two-stage spawn composition is provably idempotent was false, and the pinning test used the one fixture that hid it: RedactSensitiveData's sensitiveParams sweep matches `[^&\s]+`, which DOES match the bullet marker, so `SLACK_TOKEN=••••` becomes `SLACK_TOKEN=***REDACTED***` while `API_KEY=••••` does not. The composition is disclosure-safe, not byte-idempotent; the comment now says so and both spellings are pinned. Diagnostics preserved throughout: flag names, env KEY names, hosts, paths, OAuth error codes, DCR status codes and `code_present` all survive.
… that feed every sink
The round-2 live run — a stdio server carrying an argv secret and printing a
token on startup, plus an http server with `?token=` and `?opaque=ghp_…` in its
URL — still put two credentials in the log after every fix above had landed.
Both were the same shape as finding B3 one level further out.
`sk-live-…` appeared 8 times because the stdio stderr PUMP
(core/monitoring.go) is a different code path from the launcher's loggerWriter.
It feeds THREE sinks from one string: main.log, the per-server log that
GET /api/v1/servers/{id}/logs serves, and the recent-stderr ring buffer that
connectStdio splices into the "did not respond to MCP initialize" error — which
is itself logged and returned to REST callers. Scrubbing the line once at the
top of the loop covers all three.
`ghp_…` appeared 15 times because redactURLCredentialsInError — the ONE seam
where a transport error enters mcpproxy, and whose own doc comment says
"redacting once, where the transport error enters mcpproxy, is what keeps a
future log site from re-leaking it" — ran the NAME rule only. `opaque` is not a
recognised parameter name, so the token walked through it and then through
managed.Client, upstream.Manager, the supervisor and the runtime, each of which
zap.Errors the string it is handed and none of which can be fixed from its own
file. ScrubUpstreamText there fixes all four.
Also registers the three new exported renderings with the fail-closed net's
discovery gate: LogSafeURL and LogSafeCallbackQuery are bound and probed;
StateFingerprint is recorded as not-a-rendering, with the reason — it keeps no
byte of its input, no read door publishes it and no write door consumes it, so
putting a per-value hash into MaskMarkers would be meaningless.
Live re-run across seven sinks (three log files, stdout, GET /api/v1/servers
and both GET /api/v1/servers/{id}/logs payloads): 0 occurrences of all five
planted secrets, while `--api-key`, `--max-tokens 4096`, `--public-key-path
/etc/ssl/pub.pem`, the SLACK_TOKEN key name, the upstream host and "connection
refused" all survive.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Both failures were POSIX assumptions in tests added by the #1158 round-2 work, and both are Windows-only. internal/oauth: the -c command-line fixtures were built with shellwrap.Shellescape, which switches on runtime.GOOS and emits cmd.exe double-quoting on Windows - so the sanity assertions looked for a POSIX quote and the ' idiom that were never going to be there. The tokenizer under test handles the POSIX form WrapWithUserShell produces, and a Windows core still reads per-server logs and configs a POSIX host wrote, so the fixture must be POSIX-quoted on every host. Added posixShellescape plus TestPosixShellescape_MatchesProduction, which pins it against the real Shellescape on POSIX - keeping the drift protection that building the fixture from production code was there to provide. internal/upstream/launcher: the banner tests spawned /bin/echo, which does not exist on Windows. Argv redaction in the banner is platform-independent, so rather than skipping the way this package's POSIX signal-semantics tests do, the interpreter is chosen per host and the expected command path travels with it. Verified: both packages pass on darwin, and GOOS=windows go vet is clean.
Deploying mcpproxy-docs with
|
| Latest commit: |
ad79d32
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ed8fb24b.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://claude-sec-1158-log-redactio.mcpproxy-docs.pages.dev |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 33627268136 --repo smart-mcp-proxy/mcpproxy-go
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1158.
Credentials were reaching logs, stdout and structured API responses through paths the #1148/#1157 sweep deliberately left alone. Verified live before and after: on
main, an argv secret appeared 12 times and a URL-query secret 47 times in a 20-second startup at plainDEBUG— no trace mode needed. On this branch, zero, across seven sinks.What was leaking
Info, on every login.config.gologged the whole callback query string and thecodeparameter directly. That is a single-use credential exchangeable for an access token, written tomain.log— andGET /api/v1/servers/{id}/logsserves log content over REST. Pre-existing on main.-e KEY=VALUEinjection, and the login-shell-c "<whole command line>"form that every macOS stdio server goes through.Info, unconditionally, on three near-duplicate emission paths —autoDetectResourcereturns the configured server URL verbatim, that becomesextraParams["resource"], and it was spliced into the authorize URL.Authorization/Cookieheaders, request and response bodies, SSE frames.ServerURLandURLCheckedserialized to REST callers.main.log, the REST-served per-server log, and the connect-error ring buffer from one string) andredactURLCredentialsInError, the single seam where transport errors enter mcpproxy, which ran the name-only rule so?opaque=ghp_…reached the log 15 times via four different callers.Approach
Everything routes through the redaction seams the earlier sweep established rather than new matchers, and is fixed at the seam rather than per call site, so
managed.Client,upstream.Manager, the supervisor and the runtime all inherit it.Three AST guards make the class hard to reintroduce: no raw
zap.Errorbeside a masked URL field, everyOAuthFlowErrorliteral throughscrubbedFlowError, and a widened URL-field-name regex.Diagnostics preserved
This was a real constraint, not an afterthought — masking that destroys debuggability gets turned off. Flag names, hosts, paths and parameter names survive;
--max-tokens 4096and--public-key-path /etc/ssl/pub.pemkeep their values while--api-keyloses its;statebecomes a stable 12-hex fingerprint so cross-line flow correlation still works; "connection refused" still classifies.Review
Cross-reviewed with
opencode(gpt-5.6-sol), which returned three blocking and four major findings on the first pass; all are closed, and its two false positives were re-derived and rejected with evidence. 34 tests, every one proven to fail against a permissively-neutered fix.