fweinberger/fix sensitive local storage #715
Merged
+8
−1
Conversation
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
🎭 Playwright E2E Test Results✅ 24 passed Details24 tests across 3 suites 📊 View Detailed HTML Report (download artifacts) |
felixweinberger
commented
Aug 14, 2025
felixweinberger
force-pushed
the
fweinberger/fix-sensitive-local-storage
branch
from
18c6450 to
b8f5d2c
Compare
felixweinberger
force-pushed
the
fweinberger/fix-sensitive-local-storage
branch
from
b8f5d2c to
36b9acc
Compare
olaservo
added a commit
to olaservo/inspector
that referenced
this pull request
Aug 20, 2025
…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]>
9 tasks
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.
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_secretpotentially 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 becuThis PR filters out the
client_secretbefore storage of OAuth information to the sessionStorage for improved security.More details:
⏺ When client_secret Gets Supplied
Based on my investigation:
Conclusion
The client_secret:
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 testand these still passed.Breaking Changes
None.
Types of changes
Checklist
Additional context