Skip to content

Bind MCP auth grants to the server URL to require re-consent on URL change#320165

Merged
TylerLeonhardt merged 2 commits into
mainfrom
tyler/cold-shrimp
Jun 5, 2026
Merged

Bind MCP auth grants to the server URL to require re-consent on URL change#320165
TylerLeonhardt merged 2 commits into
mainfrom
tyler/cold-shrimp

Conversation

@TylerLeonhardt
Copy link
Copy Markdown
Member

Problem

An MCP auth grant was previously keyed only by the server id. Because a server's id can stay the same while its url changes, re-pointing an HTTP MCP server at a new endpoint (or editing mcp.json to swap the URL under an existing id) would silently reuse a token the user had consented to release for the original endpoint. That means a token scoped/intended for https://trusted.example.com/mcp could be sent to https://evil.example.com/mcp without any re-prompt.

Fix

Bind each grant to the URL it was made for:

  • AllowedMcpServer now stores an optional url (the URL the grant was made for; undefined for stdio servers, which have no URL).
  • isAccessAllowed(...) takes an optional mcpServerUrl. When provided, the token is only released if the server's current URL still matches the stored one; otherwise it returns undefined (re-prompt). Omitting the argument preserves the old id-only behavior for inspection (e.g. the management UI).
  • MainThreadMcp passes the HTTP server's current URL through on the access checks and persists it when access is granted.

URL comparison semantics

URLs are compared by their canonical WHATWG URL form so that:

  • Cosmetic origin differences don't force re-consent — host case (SERVER.EXAMPLE.COM), default port, encoding, and a root trailing slash (foo.com vs foo.com/).
  • Meaningful endpoint differences are preserved — a path trailing slash (foo.com/a vs foo.com/a/) is treated as a different endpoint and re-prompts.

Behavior notes

  • Legacy grants stored before URL binding (no url) re-prompt once when a URL is supplied — intentional, since we can't prove they were consented for the current endpoint.
  • Management toggles that omit the URL do not clear the stored binding (the url is only overwritten when one is provided).
  • Stdio servers (no URL on either side) and product.json trusted servers are unaffected.

Tests

Adds a isAccessAllowed URL binding (security) suite covering match, cosmetic-equivalence, path-slash difference, URL change, legacy grants, inspection-without-URL, stdio, trusted-server bypass, and the management-toggle-preserves-binding case. Full AuthenticationMcpAccessService suite passes (42/42).


Opening as a draft for review.

…hange

An MCP auth grant was previously keyed only by server id, so re-pointing a server at a new URL (while keeping the same id) would silently reuse a token the user consented to for the original endpoint. Store the URL the grant was made for and only release the token when the server's current URL still matches, otherwise re-prompt. Cosmetic origin differences (host case, default port, root trailing slash) are normalized via WHATWG URL so they don't force spurious re-consent, while a path trailing slash is preserved as a meaningful difference. Stdio servers (no URL) and product.json trusted servers are unaffected.
Copilot AI review requested due to automatic review settings June 5, 2026 19:31
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR strengthens MCP authentication by binding stored auth grants to the MCP server URL (for HTTP transports), ensuring users are re-prompted if a server keeps the same id but is repointed to a different endpoint.

Changes:

  • Extend AllowedMcpServer with an optional url binding and update isAccessAllowed(...) to optionally enforce URL matching via canonical WHATWG URL comparison.
  • Update MainThreadMcp to pass the current HTTP server URL into access checks and to persist the URL when granting access.
  • Add a dedicated test suite covering URL binding behaviors, including cosmetic equivalence and legacy grants.
Show a summary per file
File Description
src/vs/workbench/services/authentication/test/browser/authenticationQueryServiceMocks.ts Updates test mock signature to accept an optional MCP server URL.
src/vs/workbench/services/authentication/test/browser/authenticationMcpAccessService.test.ts Adds a URL-binding-focused test suite for isAccessAllowed.
src/vs/workbench/services/authentication/browser/authenticationMcpAccessService.ts Implements URL canonical comparison, stores URL binding on grants, and enforces URL match when provided.
src/vs/workbench/api/browser/mainThreadMcp.ts Threads the MCP server’s current HTTP URL through access checks and persists it when granting access.

Copilot's findings

  • Files reviewed: 4/4 changed files
  • Comments generated: 4

Address PR review: the optional mcpServerUrl conflated id-only inspection with the HTTP token-release gate. Split into isAccessAllowed (id only, used by the management/query API) and isAccessAllowedForUrl (URL required, used by the HTTP gate in mainThreadMcp). Auth is HTTP-only, so the gate now narrows the launch to HTTP and always passes the URL. Align the test mock with production semantics (canonical URL equality + legacy allowed===undefined => true) by reusing the exported urlsEqual helper.
@TylerLeonhardt TylerLeonhardt marked this pull request as ready for review June 5, 2026 20:27
@TylerLeonhardt TylerLeonhardt merged commit 8131d06 into main Jun 5, 2026
25 checks passed
@TylerLeonhardt TylerLeonhardt deleted the tyler/cold-shrimp branch June 5, 2026 20:35
@vs-code-engineering vs-code-engineering Bot added this to the 1.124.0 milestone Jun 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants