Skip to content

feat(client): add credentials: 'include' toggle for cookie-auth servers - #1580

Closed
SarthakB11 wants to merge 1 commit into
modelcontextprotocol:v1/mainfrom
SarthakB11:feat/credentials-include-1454
Closed

feat(client): add credentials: 'include' toggle for cookie-auth servers#1580
SarthakB11 wants to merge 1 commit into
modelcontextprotocol:v1/mainfrom
SarthakB11:feat/credentials-include-1454

Conversation

@SarthakB11

Copy link
Copy Markdown

Title: feat(web): add "Send cookies" toggle for direct Streamable HTTP connections (closes #1454)

Closes #1454.

Summary

Adds a "Send cookies (credentials: include)" checkbox to the connection settings
panel. When enabled, the direct transport passes credentials: 'include' on its
fetch calls so the browser attaches cookies stored for the target origin. Feature is default-off and follows the same UI+localStorage pattern used in the recently-merged connection-settings additions (#1553, #1551).

The browser treats Cookie as a forbidden header, so JavaScript cannot set it
manually. credentials: 'include' is the only standards-compliant way to send
cookies on a cross-origin request. Without it, MCP servers that rely on
cookie-based auth or session routing are unreachable from the inspector UI
unless the built bundle is patched by hand.

Changes

  • New checkbox in the Sidebar connection settings, shown only when Connection
    Type is Direct (the option does not apply to the proxy transport, which
    forwards cookies server-side).
  • credentialsInclude plumbed through App into useConnection; both direct
    fetch call sites (SSE and Streamable HTTP) add credentials: 'include' only
    when the toggle is on.
  • State persists across sessions in localStorage under lastCredentialsInclude,
    matching the existing lastConnectionType persistence.
  • Tooltip notes the server-side requirements: Access-Control-Allow-Credentials: true, a non-wildcard Access-Control-Allow-Origin, and SameSite=None; Secure for cross-site cookies.

Behavior

  • Default: off. Fetch behavior is unchanged from today.
  • On: credentials: 'include' is added to the direct-connection fetch
    options.

Acceptance criteria

  • Toggle present in the connection settings UI for the Streamable HTTP
    transport.
  • Off leaves fetch behavior unchanged.
  • On passes credentials: 'include' to both direct-connection fetch sites.
  • Toggle state persists via localStorage.
  • Tooltip explains the required CORS response headers.

Test plan

  • npm test (client) passes; added two useConnection tests covering the enabled and unset cases for the direct transport
  • tsc --noEmit, eslint, and prettier --check clean on the changed files
  • Manual smoke: point inspector at a cookie-authenticated MCP server with the toggle on, confirm Cookie header sent and session established (screenshot / GIF happy to add on request)

Cookie is a forbidden request header, so JS cannot set it manually. The
only standards path to send cookies cross-origin is credentials: 'include'
on the fetch call. The inspector never set it, so cookie-auth MCP servers
were unreachable from the UI without patching the built bundle.

Add a client-side toggle in connection settings, persisted to
localStorage, applied at both SSE and streamable-http fetch call sites.
Off by default. Tooltip notes the server-side CORS requirements
(Access-Control-Allow-Credentials + non-wildcard origin).

Closes modelcontextprotocol#1454

Signed-off-by: SarthakB11 <sarthak.bhardwaj21b@iiitg.ac.in>
@SarthakB11

Copy link
Copy Markdown
Author

This follows the same connection-settings pattern as #1551/#1553 (both merged recently): a scoped, default-off toggle, not a V2-scope change. Happy to add the manual-smoke screenshot if that would help move review along.

@cliffhall
cliffhall changed the base branch from main to v1/main July 28, 2026 03:04
@cliffhall

Copy link
Copy Markdown
Member

Closing: reviewed and declined on security grounds.

Thank you for this contribution, and apologies for the long wait for a response.

This PR was included in a security review of the full v1 backlog before bulk-closing it (#1819). Unlike most of that backlog — which is closed simply because v1 is deprecated — this change was read closely and is being declined on its merits: as written, it would weaken a security control the Inspector's threat model depends on.

Specifically: a credentials: 'include' toggle allows browser cookies to be sent to arbitrary direct targets. It is default-off, but it adds a new risk surface.

The Inspector's backend spawns local processes and proxies outbound requests on the developer's behalf, so it defends in layers: session-token auth, origin validation, loopback-only binding, and an SSRF block-list on the proxy. A change that removes or widens any one of those is a net loss even when it fixes a real usability problem — and several PRs in this group were framed as fixes or improvements, which is exactly why we're recording the decision explicitly rather than closing silently.

We're labeling this closed-v1-security-declined so the decision is queryable and doesn't get re-proposed against v2 later. This is not an accusation of bad intent — in most cases the security consequence was a side effect of an unrelated goal.

If the underlying need is real, we'd like to solve it in a way that preserves those controls. Please open an issue against v2 describing the problem you hit (not the patch), and we'll look at it. Note that we accept external contributions as issues rather than pull requests — see CONTRIBUTORS.md.

@cliffhall cliffhall closed this Jul 31, 2026
@cliffhall cliffhall added closed-v1-deprecated Closed: v1 is deprecated and accepting security fixes only closed-v1-security-declined Closed: reviewed and declined on security grounds; do not re-propose labels Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

closed-v1-deprecated Closed: v1 is deprecated and accepting security fixes only closed-v1-security-declined Closed: reviewed and declined on security grounds; do not re-propose

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[enhancement] Add credentials: 'include' toggle to connection settings

2 participants