-
Notifications
You must be signed in to change notification settings - Fork 1.8k
client: preserve OAuth discovery metadata across browser redirects #1816
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.
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
Closed
windro-xdd
wants to merge
35
commits into
modelcontextprotocol:main
from
windro-xdd:fix/oauth-redirect-preserve-resource-metadata
Closed
Changes from 1 commit
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
45c5a0c
client: persist interactive OAuth metadata across redirects
5276439
fix(stdio): always set windowsHide on Windows, not just in Electron (…
felixweinberger 6711ed9
feat(client): add reconnectionScheduler to StreamableHTTPClientTransp…
felixweinberger 9d08392
SEP-2207: Refresh token guidance (#1523)
wdawson 9d924b1
docs: note type vs interface for structuredContent (#1784)
felixweinberger 48aba0d
fix(core): add explicit | undefined to Transport interface optional p…
felixweinberger d0505c1
chore(v2): pnpm audit fix (#1789)
felixweinberger 9efecc2
ci: format fetch-spec-types output with prettier (#1782)
felixweinberger 4aec5f7
Private key jwt scopes (#1443)
NSeydoux 045c62a
feat!: remove WebSocketClientTransport (#1783)
felixweinberger d99f3ee
fix: continue OAuth metadata discovery on 5xx responses (#1632)
matantsach a39a9eb
test: add compile-time key-parity assertions for spec type checks (#1…
rechedev9 d6a02c8
fix(core): ensure standardSchemaToJsonSchema emits type:object (#1796)
felixweinberger 8822c96
fix(examples): return 404 for unknown session IDs, 400 for missing (#…
felixweinberger 89fb094
fix(core): consolidate per-request cleanup in _requestWithSchema (#1790)
felixweinberger fcde488
chore: drop zod from peerDependencies (kept as direct dependency) (#1…
felixweinberger 9bc9abc
Fix: Handle error responses in Streamable HTTP SSE streams (#1390)
DePasqualeOrg f73a5af
Add _meta support to registerPrompt (#1629)
chughtapan 2fd7f5f
`v2`: Web standards Request object in ctx (#1822)
KKonstantinov 5f32a90
fix(core): make fromJsonSchema() use runtime-aware default validator …
KKonstantinov 81e4b2a
Adds Fastify Middleware for v2 (#1536)
andyfleming 0fabc27
chore: enter alpha prerelease mode (#1823)
felixweinberger 689148d
fix(server): propagate negotiated protocol version to transport (#1660)
rechedev9 babaa50
chore(ci): remove dead publish job from main.yml (#1829)
felixweinberger 38d6cd2
chore(fastify): remove stray packageManager field from subpackage (#1…
felixweinberger 54fa96e
Version Packages (alpha) (#1420)
github-actions[bot] 53fb84b
fix(ci): split release.yml into version + publish jobs (#1836)
felixweinberger 424cbae
v2 tsdown fix (#1840)
KKonstantinov 0021561
Version Packages (alpha) (#1841)
github-actions[bot] 866c08d
fix(core): allow additional JSON Schema properties in elicitInput req…
felixweinberger 1eb3123
fix(client): preserve custom Accept headers in StreamableHTTPClientTr…
nielskaspers 653c5d0
Rewrite `docs/server.md` as a code-heavy, prose-light how-to guide (#…
jonathanhefner df4b6cc
fix: prevent stack overflow in transport close with re-entrancy guard…
claygeo a408ca7
feat(client): add OAuthProvider for preserving discovery state across…
317fec5
merge: resolve streamableHttp.ts conflict, keep OAuth implementation
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 StreamableHTTPClientTransport is missing the sessionStorage persistence that this PR claims to implement for it. The PR description states "persist interactive OAuth metadata (resource_metadata URL and scope) in browser sessionStorage for both Streamable HTTP and SSE client transports", but only SSEClientTransport received the implementation. In a real browser redirect flow, the new StreamableHTTPClientTransport instance will start with undefined _resourceMetadataUrl and _scope and must redo full OAuth discovery on every post-redirect finishAuth() call — unlike SSEClientTransport, which restores from sessionStorage. The test named "persists interactive auth metadata across transport recreation before finishAuth" passes only because all discovery responses are mocked; the sessionStorage mock set up in beforeEach is never exercised by StreamableHTTPClientTransport.
Extended reasoning...
What the bug is
The PR title and description explicitly state it persists OAuth discovery state "for both Streamable HTTP and SSE client transports", but StreamableHTTPClientTransport has zero sessionStorage implementation. A grep for "sessionStorage", "_saveInteractiveAuthState", "_loadInteractiveAuthState", or "_clearInteractiveAuthState" in packages/client/src/client/streamableHttp.ts returns no results in this PR. The feature is only half-implemented.
The specific code path that triggers it
SSEClientTransport (sse.ts) gained all three lifecycle methods:
_loadInteractiveAuthState()to restore persisted metadata_saveInteractiveAuthState()after extracting WWW-Authenticate params_clearInteractiveAuthState()to prevent stale statestreamableHttp.ts received none of these. The file was changed in this PR for reconnection, Accept header merging, and error-response handling — but the OAuth state persistence that is the core stated goal is absent.
Why existing code does not prevent it
The test "persists interactive auth metadata across transport recreation before finishAuth" in streamableHttp.test.ts is misleadingly named. The test sets up a sessionStorage mock in beforeEach (matching the SSE test), but StreamableHTTPClientTransport never reads from or writes to sessionStorage. The test works because the mock fetch provides four successive responses: 401 (first transport), resource metadata, auth server metadata, and token exchange (second transport). The second transport finishAuth() succeeds by doing full fresh discovery from the mocked data — not by restoring persisted state. This gives a false impression that the feature was implemented and tested.
Impact
In production browser redirect flows without mocked network calls:
How to fix it
StreamableHTTPClientTransport needs the same three private methods that SSEClientTransport has, with an appropriate storage key prefix (e.g. "mcp:oauth:interactive:streamablehttp:"). The constructor should call
_loadInteractiveAuthState(), the 401-handling path should call_saveInteractiveAuthState()after extracting WWW-Authenticate params, andfinishAuth()should call_clearInteractiveAuthState()after successful authorization.Step-by-step proof
_resourceMetadataUrland_scopein memory. Without_saveInteractiveAuthState(), nothing is written to sessionStorage._loadInteractiveAuthState(),_resourceMetadataUrlis undefined and_scopeis undefined.finishAuth('auth-code')callsauth()withresourceMetadataUrl: undefined, forcing a full re-discovery network call.