Skip to content

fweinberger/fix sensitive local storage #715

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
cliffhall merged 1 commit into from
Aug 14, 2025
Merged

fweinberger/fix sensitive local storage #715

cliffhall merged 1 commit into from
Aug 14, 2025

Conversation

@felixweinberger
Copy link
Contributor

@felixweinberger felixweinberger commented Aug 14, 2025 •
edited
Loading

Motivation and Context

CodeQL pointed out we were storing some OAuth information unencrypted in SessionStorage which is not ideal: https://github.com/modelcontextprotocol/inspector/security/code-scanning/8

Not all of this information is sensitive, but the client_secret potentially is. We're actually unnecessarily storing this, as it's not used at any point after storage - we're storing it by default right now becu

This PR filters out the client_secret before storage of OAuth information to the sessionStorage for improved security.

More details:

Details From @claude:

⏺ When client_secret Gets Supplied

Based on my investigation:

  1. During Dynamic Client Registration (DCR)
  • When registerClient() is called, it sends a POST request to the OAuth server's registration endpoint
  • The server responds with OAuthClientInformationFullSchema which MAY include client_secret
  • Whether it includes client_secret depends on the OAuth server's policy:
    • Public clients (SPAs, mobile apps): No client_secret
    • Confidential clients (server apps): Includes client_secret
  1. When client_secret is Used
  • During token exchange (exchangeAuthorization()), the SDK uses client_secret if present
  • It applies different authentication methods based on what the server supports:
    • client_secret_basic: Sends as HTTP Basic Auth header
    • client_secret_post: Sends in POST body
    • none: Public client, no secret needed
  1. The Problem
  • The MCP Inspector is a browser-based SPA (Single Page Application)
  • SPAs should be public clients (no client_secret)
  • BUT some OAuth servers might incorrectly return a client_secret anyway
  • We store whatever the server returns, including client_secret if present
  1. Why It's Not Actually Used After Storage
  • The client_secret is only needed during exchangeAuthorization()
  • The SDK reads it from the clientInformation object passed to it
  • After token exchange, the client_secret is never needed again
  • We only store OAuth tokens for future use, not the client credentials

Conclusion

The client_secret:

  1. Comes from the OAuth server during dynamic client registration
  2. Is used once during token exchange
  3. Doesn't need to be stored after that
  4. Shouldn't even exist for browser-based apps (but might due to misconfigured servers)

The simplest fix is to filter it out when storing, since it's not needed after the initial OAuth flow completes.

How Has This Been Tested?

Ran inspector to do some smoke testing with local MCP servers, OAuth still worked, including on refresh.
Ran tests with npm run test and these still passed.

Breaking Changes

None.

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

Additional context

@felixweinberger felixweinberger marked this pull request as draft August 14, 2025 14:32
@github-actions
Copy link

github-actions bot commented Aug 14, 2025 •
edited
Loading

🎭 Playwright E2E Test Results

✅  24 passed

Details

24 tests across 3 suites
 35.8 seconds
 36b9acc
ℹ️  Test Environment: Ubuntu Latest, Node.js v22.18.0
Browsers: Chromium, Firefox

📊 View Detailed HTML Report (download artifacts)

felixweinberger
felixweinberger commented Aug 14, 2025
View reviewed changes
@felixweinberger felixweinberger force-pushed the fweinberger/fix-sensitive-local-storage branch from 18c6450 to b8f5d2c Compare August 14, 2025 16:03
@felixweinberger felixweinberger force-pushed the fweinberger/fix-sensitive-local-storage branch from b8f5d2c to 36b9acc Compare August 14, 2025 16:04
@felixweinberger felixweinberger marked this pull request as ready for review August 14, 2025 16:08
cliffhall
cliffhall approved these changes Aug 14, 2025
View reviewed changes
Copy link
Member

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@cliffhall cliffhall merged commit c5047ca into main Aug 14, 2025
8 checks passed
@cliffhall cliffhall deleted the fweinberger/fix-sensitive-local-storage branch August 14, 2025 17:23
@olaservo olaservo mentioned this pull request Aug 20, 2025
Closed
olaservo added a commit to olaservo/inspector that referenced this pull request Aug 20, 2025
@olaservo @claude
Fix OAuth token endpoint authentication method regression (issue mode…
57b8653 
…lcontextprotocol#726)

This commit resolves a regression introduced in PR modelcontextprotocol#715 where the OAuth flow
always chose client_secret_post regardless of the server's
token_endpoint_auth_methods_supported setting.

## Problem
- PR modelcontextprotocol#715 removed client_secret from sessionStorage for security reasons
- This broke the MCP SDK's ability to determine the correct authentication method
- OAuth flows with servers requiring client_secret_basic would fail

## Solution
- Added temporary in-memory storage for client_secret during OAuth flow
- client_secret is stored temporarily when saveClientInformation() is called
- client_secret is provided to the SDK when clientInformation() is called
- client_secret is cleared from memory after successful token exchange or clear()
- client_secret is never persisted to sessionStorage (maintains security)

## Testing
- Added comprehensive tests for temporary client_secret handling
- Verified client_secret is available during OAuth flow
- Verified client_secret is not stored in sessionStorage
- Verified client_secret is cleared after token exchange
- All existing tests continue to pass

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@olaservo olaservo mentioned this pull request Aug 20, 2025
Closed
9 tasks
@pmig pmig mentioned this pull request Aug 25, 2025
Closed
@cliffhall cliffhall mentioned this pull request Sep 20, 2025
Closed
9 tasks
@cliffhall
Copy link
Member

cliffhall commented Sep 22, 2025

We're actually unnecessarily storing this, as it's not used at any point after storage

Turns out we actually are. It is retrieved and sent to the /token endpoint in the case of a client_secret_basic or client_secret_post challenge. See #722 for the discussion on this. It may not be required after all, but it does get retrieved and sent after the redirect, and if it is in place, it fixes the problem in 722's sample server. The question is whether that server is actually implemented correctly. Just placing this note here for reference. I think it will ultimately not be needed and that servers must offer PKCE code challenge/verifier and client_id only at token request time.

@pcarleton pcarleton mentioned this pull request Sep 24, 2025
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cliffhall cliffhall cliffhall approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@felixweinberger @cliffhall