Skip to content

fix(server): bind MCP transport sessions to authenticated owners - #30591

Open
mahruskazi wants to merge 1 commit into
pingdotgg:mainfrom
mahruskazi:draft/mcp-transport-ownership
Open

mahruskazi wants to merge 1 commit into
pingdotgg:mainfrom
mahruskazi:draft/mcp-transport-ownership

Conversation

@mahruskazi

Copy link
Copy Markdown

Problem

MCP transport sessions are not bound to the authenticated namespace that initialized them. A different valid credential that knows a session ID can delete that transport and interrupt its owner.

Change

Bind each transport session to its initializing authenticated namespace. Reject foreign POST, GET, and DELETE requests with 404 before dispatch. Authenticate every new request so expired or revoked credentials cannot reuse an existing transport.

No session-count quota is introduced. Requests and streams accepted before a later revocation are not cancelled; ownership records for sessions never deleted remain until server restart, matching the transport state lifetime.

Scope and approval

This draft addresses transport-session ownership only. No prior maintainer approval is claimed; maintainer review and triage remain pending.

Verification

Recorded HTTP checks reproduce foreign-session DELETE against the base, then cover foreign POST/GET/DELETE denial, OAuth namespaces, owner DELETE, expiry, trusted refresh, revocation, and more than 64 reconnects on one credential. The prepared candidate passed vp run check:changed with 31 tests across 3 files. This publication preserves the tested tree. Repo-wide CI and live client reinitialization after a 404 have not been checked. Final independent review is pending.

Models: GPT-6.1 Sol and Claude Opus 5.5. Harnesses: Codex and Claude Code in T3 Code.

Prepared as a draft for maintainer review.
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Nov 4, 2026
@mahruskazi
mahruskazi marked this pull request as ready for review November 5, 2026 22:19
@coderabbitai

coderabbitai Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The MCP HTTP middleware now associates session IDs with request namespaces, rejects requests from invocations that do not own the session, and removes ownership after successful session deletion. Integration tests cover ownership, deletion, and authentication-state behavior.

Changes: MCP session ownership

Layer / File(s) Summary
Session ownership gate
apps/server/src/mcp/McpHttpServer.ts
The middleware maps MCP session IDs to request namespaces and returns 404 when the authenticated invocation does not own the supplied session ID.
Session lifecycle tracking and validation
apps/server/src/mcp/McpHttpServer.ts, apps/server/src/mcp/McpSessionOwnership.test.ts
The middleware records newly issued session IDs and removes their mappings after a DELETE response with status 204. The integration test checks ownership, deletion, repeated initialization, and authentication-state cases.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant AuthenticationMiddleware
  participant MCPServer
  Client->>AuthenticationMiddleware: Send request with mcp-session-id
  AuthenticationMiddleware->>AuthenticationMiddleware: Check session owner against invocation namespace
  AuthenticationMiddleware->>MCPServer: Dispatch request when ownership matches
  MCPServer-->>AuthenticationMiddleware: Return response
  AuthenticationMiddleware->>AuthenticationMiddleware: Record issued session ID or remove mapping after DELETE 204
Loading

Possibly related PRs

  • pingdotgg/t3code#30834: Both changes modify MCP request admission in McpHttpServer; this PR uses the authenticated invocation scope to enforce session ownership.

Suggested reviewers: juliusmarminge

Merge Risk: Minimal · up to 452fb

No merge-blocking issue was identified in session ownership or lifecycle tracking. Merge after normal checks.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly describes the session ownership change.
Description check Passed The description is mostly complete. The scope section does not link approval or explain why the fix qualifies without it.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 059804d and 452fb63.

Files selected for processing: 2

  • apps/server/src/mcp/McpHttpServer.ts
  • apps/server/src/mcp/McpSessionOwnership.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant