Skip to content

fix(auth): redact secrets in Debug output for StoredCredentials and StoredAuthorizationState#744

Open
peatey wants to merge 8 commits intomodelcontextprotocol:mainfrom
peatey:issue-741-cred
Open

fix(auth): redact secrets in Debug output for StoredCredentials and StoredAuthorizationState#744
peatey wants to merge 8 commits intomodelcontextprotocol:mainfrom
peatey:issue-741-cred

Conversation

@peatey
Copy link

@peatey peatey commented Mar 11, 2026

Summary

Fixes #741.

StoredCredentials and StoredAuthorizationState both derived #[derive(Debug)], causing secret fields — OAuth access/refresh tokens, PKCE verifiers, and CSRF tokens — to be printed in plaintext via any {:?} formatter, including log calls and anyhow/thiserror error chains.

  • Removes Debug from the derive macros on both types
  • Implements Debug manually, printing [REDACTED] for sensitive fields and leaving non-secret fields (e.g. client_id, created_at) visible
  • Adds regression tests asserting that {:?} formatting does not emit plaintext secrets

Approach notes

OAuthTokenResponse comes from the oauth2 crate and cannot be wrapped directly, so StoredCredentials uses a manual Debug impl that redacts the entire token_response field.

The secrecy crate was considered for StoredAuthorizationState (wrapping pkce_verifier/csrf_token in SecretString), but secrecy intentionally blocks Serialize on Secret<T> unless the inner type implements the SerializableSecret marker trait — which String does not, and orphan rules prevent adding it. Since both types are Serialize + Deserialize for use with custom CredentialStore/StateStore backends, field-type changes would break serialization. Manual Debug impls are the correct approach for both types.

Known scope limitation

The fields pkce_verifier: String and csrf_token: String on StoredAuthorizationState remain pub. This fix protects against accidentally formatting the struct via {:?}, but not against a caller extracting the field and formatting it directly (e.g. format!("{:?}", state.pkce_verifier)). This is an inherent limitation of the manual-Debug approach vs. type-level wrapping, and is noted here for reviewer awareness.

Test plan

  • cargo test -p rmcp --features auth passes
  • cargo clippy -p rmcp --features auth clean (pre-existing unused import warning unrelated to this change)
  • test_stored_authorization_state_debug_redacts_secrets — asserts {:?} output does not contain raw verifier/csrf values and does contain [REDACTED]
  • test_stored_credentials_debug_redacts_token_response — asserts {:?} output does not contain raw access token and does contain [REDACTED]

peatey and others added 2 commits March 10, 2026 15:55
…toredAuthorizationState

Removes `Debug` from the derive macros on `StoredCredentials` and
`StoredAuthorizationState` and replaces them with manual `Debug` impls
that print `[REDACTED]` for sensitive fields (access/refresh tokens,
PKCE verifiers, and CSRF tokens), preventing accidental credential
leakage via `{:?}` formatters, log calls, and error chains.

Fixes modelcontextprotocol#741

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds regression tests for the fix in the previous commit, verifying
that `{:?}` formatting of `StoredAuthorizationState` and
`StoredCredentials` does not emit plaintext secrets.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@peatey peatey requested a review from a team as a code owner March 11, 2026 02:06
@github-actions github-actions bot added T-core Core library changes T-transport Transport layer changes labels Mar 11, 2026
Copy link
Member

@DaleSeo DaleSeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, @peatey! Just a couple of small suggestions below. Also, please ensure the code is formatted to pass the check.

- Remove redundant VendorExtraTokenFields from use super:: in
  test_stored_credentials_debug_redacts_token_response (already
  imported at module scope)
- Add assert!(debug_output.contains("created_at")) to
  test_stored_authorization_state_debug_redacts_secrets to verify
  non-secret fields remain visible in Debug output
- Run cargo fmt

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Dale Seo <5466341+DaleSeo@users.noreply.github.com>
@peatey
Copy link
Author

peatey commented Mar 12, 2026

ok fmt now run (its in the workflow so not sure what happened there, sorry) - issues addressed.

@DaleSeo
Copy link
Member

DaleSeo commented Mar 12, 2026

Hey @jamadeo, could you help approve/merge this PR? I've fixed the formatting issue on the web, but now it seems like someone else needs to approve it since I've added commits technically. 😂

@DaleSeo DaleSeo requested a review from jamadeo March 12, 2026 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-core Core library changes T-transport Transport layer changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug - [derive(Debug)] cred leak

2 participants