Revert "fix: filter out client_secret when storing OAuth client information"

[pcarleton]

This reverts commit 36b9acc which filtered out client_secret from being populated in local browser storage in response to a CodeQL advisory.

Situation

In OAuth there is a notion of a “client id” and a “client secret”, and also a “public” client and a “confidential” client:

• Client id: an identifier for a client that can be shared publicly, useful for scoping access tokens to a single client, and as a key to reference the rest of the registered client’s metadata (client name, redirect URI’s, logos etc.). This ID is generally not treated as sensitive, and can be displayed in UI’s etc.
• Client secret: an additional secret that is NOT public, and is expected to be kept private to a backend component. Leaking a client secret is problematic when an attacker can abuse it to e.g. get a refresh token, since using a refresh token does not require controlling the redirect_uri destination.
• Confidential client: A client that can be expected to keep a client secret confidential, e.g. a backend service.
• Public client: A client that cannot be expected to keep a client secret confidential, e.g. a single page javascript application. Refresh tokens from public clients are discouraged, and require additional mitigations.
  • Note: An application that would normally have to be a public client due to having its source code inspectable, and therefore any embedded secret available, could be considered confidential if using DCR to get a per-instance client secret. This relies on the application instance being able to keep a secret confidential from other instances.

Additionally, there are access tokens and refresh tokens:

• Access Tokens: can be used directly in the Authorization header to gain access to a service
• Refresh tokens: can be used (often in conjunction with the client secret) to gain a new access token

In Inspector:

• we have historically allowed the inspector to be treated as a confidential or public client when registering via DCR
• We have stored client information (as well as access tokens and refresh tokens) in sessionStorage.

Complication

A recent Github CodeQL rule indicated that our plaintext storage in localStorage was insecure. It looked like client_secret was unused, so we removed it from local storage here. This broke people who were relying on client_secret from their servers.

The attack vector we’re concerned about is an XSS attack or malicious chrome extension could read the local storage, and use it to gain access to an MCP server and abuse a user’s data. This is applicable to the following tokens:

1. Access token: usable directly
2. Refresh token (+ client secret): usable to get a new access token

This is not particularly concerning for:

1. Client ID: this is expected to be public
2. Client secret: this is a gray area, but generally an attacker could just re-register, so stealing this would not be particularly valuable in the current set up.

Question

What is the right set of tradeoffs for storing tokens in MCP Inspector that balances developer convenience with risk of abuse from attackers?

Options

1. Continue using plaintext sessionStorage for all tokens
   • sessionStorage means closing a tab loses your stored tokens, so they’re generally short lived
2. Encrypt tokens in sessionStorage
3. Use cookies
4. Store sensitive values server side.

Options (1) or (4) seem most viable.

(2) adds a hurdle, but not fundamentally more secure (XSS could also decrypt the tokens)
(3) requires the server to set the cookies anyway, so is mostly just a more complicated version of (4).

Given the current issue folks are blocked on is client_secret not being available, and adding it back doesn’t materially change our security posture (since access_tokens are still there in plaintext), I’m proposing with this PR that we go with (1) (i.e. back to the way things were) and then work towards (4).

Appendix

PR addressing CodeQL concern re: plaintext storage: #715

Discord thread from Cliff discussing issue in general:
https://discord.com/channels/1358869848138059966/1419078117590564904/1420187984438890548

Cliff’s PR implementing encryption in local storage, w/ key available via /config endpoint #822

Issue where users noticed the regression from not supporting client_secret any more:
#726

How Has This Been Tested?

Tested with the example given in #726

Breaking Changes

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

[github-actions]

🎭 Playwright E2E Test Results

✅  24 passed

Details

24 tests across 3 suites
 33.1 seconds
 b72e3c1
ℹ️  Test Environment: Ubuntu Latest, Node.js v22.19.0
Browsers: Chromium, Firefox

📊 View Detailed HTML Report (download artifacts)

[cliffhall]

LGTM! 👍