Skip to content

fix(mcp): redact secrets from OAuth failure logs - #1976

Merged
Aaronontheweb merged 5 commits into
netclaw-dev:devfrom
Aaronontheweb:fix/mcp-oauth-log-redaction
Aug 17, 2026
Merged

fix(mcp): redact secrets from OAuth failure logs#1976
Aaronontheweb merged 5 commits into
netclaw-dev:devfrom
Aaronontheweb:fix/mcp-oauth-log-redaction

Conversation

@Aaronontheweb

@Aaronontheweb Aaronontheweb commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Problem

A review of #1969 raised a question: could the new OAuth diagnostic logging
print a secret? The new code does not. It logs only booleans and field
names, never the raw token or client secret value.

The review found a separate, pre-existing gap. McpClientManager logs nine
OAuth-related exceptions directly through ILogger. A token or DCR exchange
failure can carry the provider's raw HTTP error body inside the exception
message, and that body can echo back the client secret or token the request
sent. A log sink prints the full exception text, so a secret could leave the
daemon log. This risk grows when OTLP export sends logs off the box.

A second gap made the fix harder than "add a redact call": SecretOutputRedactor
already redacts secrets in JSON key form ("client_secret": "..."), but its
env-style regex (key=value text) did not match compound keys like
client_secret or refresh_token. Only the JSON regex matched those keys. A
form-urlencoded OAuth error body (client_secret=...&grant_type=...) uses
exactly the compound-key shape the env-style regex missed.

A follow-up check against #1970 (which touches McpOAuthCredentialStore)
found the same shape of gap there: its credentials-file load path also logs
a raw exception. The secrets file is decrypted before that callback runs, so
a malformed-JSON or decryption failure could in principle echo a fragment of
plaintext credential content into the log. #1970's own diff adds no logging
and does not introduce this; it is pre-existing in the file it touches.

Change

  • Fix SecretOutputRedactor's env-style regex to match the same compound
    key set the JSON regex already matches.
  • Add SecretOutputRedactor.RedactForLogging, a shared helper that swaps in
    a redacted summary only when an exception's rendered text matches a secret
    pattern. The common case (network errors, cancellations, disposal
    failures) keeps its original exception instance, type, and full stack
    trace untouched.
  • Route all nine _logger.LogWarning(ex, ...) / _logger.LogError(ex, ...)
    call sites in McpClientManager.cs through it, plus the two in
    McpOAuthCredentialStore.cs.
  • Add regression tests for the redactor gap and the logging helper.

An OAuth token or DCR exchange failure can carry the provider's raw
error body inside the exception message. That body can echo back the
client secret or token the request sent. McpClientManager logged nine
such exceptions directly, and a log sink can print the full exception
text, so a secret could leave the daemon log.

Add a RedactForLogging helper. It swaps in a redacted summary only
when the exception text matches a secret pattern. Most exceptions
keep their original type and full stack trace, because most carry no
secret-shaped text.

This also fixes a gap in SecretOutputRedactor: the env-style regex
did not match compound keys like client_secret or refresh_token, only
the JSON regex did. A form-urlencoded OAuth error body uses exactly
those compound keys.
@Aaronontheweb Aaronontheweb added security Security-related changes bug Something isn't working mcp Model context protocol server / client issues. labels Aug 17, 2026
@Aaronontheweb
Aaronontheweb enabled auto-merge (squash) August 17, 2026 21:34
PR netclaw-dev#1970 changes only in-memory identity fallback logic and adds no
logging, so it does not introduce a leak on its own. But an audit for
the same issue found McpOAuthCredentialStore has the same shape of
gap: it also logs a raw exception in its credentials-file load path.
The secrets file is decrypted before that callback runs, so a
malformed-JSON or decryption exception could in principle echo a
fragment of plaintext credential content into the log.

Move RedactForLogging out of McpClientManager and into
SecretOutputRedactor, so both McpClientManager and
McpOAuthCredentialStore route their exception logging through the
same shared helper.
@Aaronontheweb
Aaronontheweb merged commit de0cb53 into netclaw-dev:dev Aug 17, 2026
17 checks passed
@Aaronontheweb Aaronontheweb mentioned this pull request Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working mcp Model context protocol server / client issues. security Security-related changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant