History

safeguard against session id leaking

Rhys Sullivan committed 6 months ago

Commit 54139751 parent c5475223 files · +108 −15Browse files

Unified · Split

3 files
  1. apps/cloud/src/mcp-flow.test.ts +45 −4
  2. apps/cloud/src/mcp-session.ts +33 −1
  3. apps/cloud/src/mcp.ts +30 −10
apps/cloud/src/mcp-flow.test.ts+45−4
@@ -251,10 +251,11 @@ describe("/mcp unknown session id", () => { · 250 unmodified lines
251describe("/mcp notification responses", () => {251describe("/mcp notification responses", () => {
252 it("returns 202 with an empty body for notifications/initialized", async () => {252 it("returns 202 with an empty body for notifications/initialized", async () => {
253 const orgId = nextOrgId();253 const orgId = nextOrgId();
254 const accountId = nextAccountId();
254 await seedOrg(orgId);255 await seedOrg(orgId);
255 256
256 const initializeResponse = await mcpPost({257 const initializeResponse = await mcpPost({
257 bearer: makeTestBearer(nextAccountId(), orgId),258 bearer: makeTestBearer(accountId, orgId),
258 body: INITIALIZE_REQUEST,259 body: INITIALIZE_REQUEST,
259 });260 });
260 expect(initializeResponse.status).toBe(200);261 expect(initializeResponse.status).toBe(200);
@@ -262,7 +263,7 @@ describe("/mcp notification responses", () => { · 1 unmodified line
262 expect(sessionId).toBeTruthy();263 expect(sessionId).toBeTruthy();
263 264
264 const notificationResponse = await mcpPost({265 const notificationResponse = await mcpPost({
265 bearer: makeTestBearer(nextAccountId(), orgId),266 bearer: makeTestBearer(accountId, orgId),
266 sessionId,267 sessionId,
267 body: INITIALIZED_NOTIFICATION,268 body: INITIALIZED_NOTIFICATION,
268 });269 });
@@ -276,10 +277,11 @@ describe("/mcp notification responses", () => { · 7 unmodified lines
276describe("/mcp session restore", () => {277describe("/mcp session restore", () => {
277 it("restores an initialized SDK transport from durable storage", async () => {278 it("restores an initialized SDK transport from durable storage", async () => {
278 const orgId = nextOrgId();279 const orgId = nextOrgId();
280 const accountId = nextAccountId();
279 await seedOrg(orgId);281 await seedOrg(orgId);
280 282
281 const initializeResponse = await mcpPost({283 const initializeResponse = await mcpPost({
282 bearer: makeTestBearer(nextAccountId(), orgId),284 bearer: makeTestBearer(accountId, orgId),
283 body: INITIALIZE_REQUEST,285 body: INITIALIZE_REQUEST,
284 });286 });
285 expect(initializeResponse.status).toBe(200);287 expect(initializeResponse.status).toBe(200);
@@ -295,7 +297,7 @@ describe("/mcp session restore", () => { · 9 unmodified lines
295 });297 });
296 298
297 const response = await mcpPost({299 const response = await mcpPost({
298 bearer: makeTestBearer(nextAccountId(), orgId),300 bearer: makeTestBearer(accountId, orgId),
299 sessionId,301 sessionId,
300 body: TOOLS_LIST_REQUEST,302 body: TOOLS_LIST_REQUEST,
301 });303 });
@@ -307,6 +309,45 @@ describe("/mcp session restore", () => { · 5 unmodified lines
307 expect(body.jsonrpc).toBe("2.0");309 expect(body.jsonrpc).toBe("2.0");
308 expect(body.result?.tools?.some((tool) => tool.name === "execute")).toBe(true);310 expect(body.result?.tools?.some((tool) => tool.name === "execute")).toBe(true);
309 }, 15_000);311 }, 15_000);
312
313 it("reproduces cross-account session reuse via leaked mcp-session-id", async () => {
314 const victimOrgId = nextOrgId();
315 const attackerOrgId = nextOrgId();
316 const victimAccountId = nextAccountId();
317 const attackerAccountId = nextAccountId();
318 await seedOrg(victimOrgId);
319
320 const initializeResponse = await mcpPost({
321 bearer: makeTestBearer(victimAccountId, victimOrgId),
322 body: INITIALIZE_REQUEST,
323 });
324 expect(initializeResponse.status).toBe(200);
325 const sessionId = initializeResponse.headers.get("mcp-session-id");
326 expect(sessionId).toBeTruthy();
327
328 const ns = env.MCP_SESSION;
329 const stub = ns.get(ns.idFromString(sessionId!));
330 await runInDurableObject(stub, async (instance) => {
331 await Effect.runPromise(
332 (instance as unknown as { closeRuntime: () => Effect.Effect<void> }).closeRuntime(),
333 );
334 });
335
336 const response = await mcpPost({
337 bearer: makeTestBearer(attackerAccountId, attackerOrgId),
338 sessionId,
339 body: TOOLS_LIST_REQUEST,
340 });
341
342 expect(response.status).toBe(403);
343 const body = (await response.json()) as {
344 readonly jsonrpc: string;
345 readonly error?: { readonly code: number; readonly message: string };
346 };
347 expect(body.jsonrpc).toBe("2.0");
348 expect(body.error?.code).toBe(-32003);
349 expect(body.error?.message).toMatch(/does not belong/i);
350 }, 15_000);
310});351});
311 352
312describe("McpSessionDO alarm lifecycle", () => {353describe("McpSessionDO alarm lifecycle", () => {
apps/cloud/src/mcp-session.ts+33−1
@@ -55,6 +55,8 @@ const LONG_LIVED_DB_MAX_LIFETIME_SECONDS = 120; · 54 unmodified lines
55const TRANSPORT_STATE_KEY = "transport";55const TRANSPORT_STATE_KEY = "transport";
56const SESSION_META_KEY = "session-meta";56const SESSION_META_KEY = "session-meta";
57const LAST_ACTIVITY_KEY = "last-activity-ms";57const LAST_ACTIVITY_KEY = "last-activity-ms";
58const INTERNAL_ACCOUNT_ID_HEADER = "x-executor-mcp-account-id";
59const INTERNAL_ORGANIZATION_ID_HEADER = "x-executor-mcp-organization-id";
58 60
59// ---------------------------------------------------------------------------61// ---------------------------------------------------------------------------
60// Errors62// Errors
@@ -74,6 +76,9 @@ const jsonRpcError = (status: number, code: number, message: string) => · 13 unmodified lines
74 headers: { "content-type": "application/json" },76 headers: { "content-type": "application/json" },
75 });77 });
76 78
79const sessionOwnerMismatch = () =>
80 jsonRpcError(403, -32003, "MCP session does not belong to the current bearer");
81
77// W3C propagation across the worker→DO boundary. mcp.ts injects the worker's82// W3C propagation across the worker→DO boundary. mcp.ts injects the worker's
78// `traceparent` and forwards incoming `tracestate` / `baggage` headers on83// `traceparent` and forwards incoming `tracestate` / `baggage` headers on
79// forwarded requests (and as a second arg to `init()`). We parse the context84// forwarded requests (and as a second arg to `init()`). We parse the context
@@ -371,6 +376,25 @@ export class McpSessionDO extends DurableObject { · 291 unmodified lines
371 );376 );
372 }377 }
373 378
379 private validateSessionOwner(request: Request): Effect.Effect<Response | null> {
380 return Effect.gen(this, function* () {
381 const sessionMeta = yield* this.loadSessionMeta();
382 if (!sessionMeta) return null;
383
384 const accountId = request.headers.get(INTERNAL_ACCOUNT_ID_HEADER);
385 const organizationId = request.headers.get(INTERNAL_ORGANIZATION_ID_HEADER);
386 const matches =
387 accountId === sessionMeta.userId &&
388 organizationId === sessionMeta.organizationId;
389
390 yield* Effect.annotateCurrentSpan({
391 "mcp.session.owner_match": matches,
392 });
393
394 return matches ? null : sessionOwnerMismatch();
395 }).pipe(Effect.withSpan("mcp.session.validate_owner"));
396 }
397
374 private resolveAndStoreSessionMeta(token: McpSessionInit) {398 private resolveAndStoreSessionMeta(token: McpSessionInit) {
375 const self = this;399 const self = this;
376 return Effect.gen(function* () {400 return Effect.gen(function* () {
@@ -504,6 +528,14 @@ export class McpSessionDO extends DurableObject { · 127 unmodified lines
504 }528 }
505 529
506 private dispatchRequest(request: Request): Effect.Effect<Response> {530 private dispatchRequest(request: Request): Effect.Effect<Response> {
531 return Effect.gen(this, function* () {
532 const ownerError = yield* this.validateSessionOwner(request);
533 if (ownerError) return ownerError;
534 return yield* this.dispatchAuthorizedRequest(request);
535 });
536 }
537
538 private dispatchAuthorizedRequest(request: Request): Effect.Effect<Response> {
507 if (!this.initialized || !this.transport) {539 if (!this.initialized || !this.transport) {
508 if (request.method === "DELETE") {540 if (request.method === "DELETE") {
509 return this.clearSessionState().pipe(541 return this.clearSessionState().pipe(
@@ -514,7 +546,7 @@ export class McpSessionDO extends DurableObject { · 4 unmodified lines
514 return Effect.gen(this, function* () {546 return Effect.gen(this, function* () {
515 const restored = yield* this.restoreRuntimeFromStorage(request);547 const restored = yield* this.restoreRuntimeFromStorage(request);
516 if (restored === "restored") {548 if (restored === "restored") {
517 return yield* this.dispatchRequest(request);549 return yield* this.dispatchAuthorizedRequest(request);
518 }550 }
519 return jsonRpcError(404, -32001, "Session timed out due to inactivity — please reconnect");551 return jsonRpcError(404, -32001, "Session timed out due to inactivity — please reconnect");
520 });552 });
apps/cloud/src/mcp.ts+30−10
@@ -32,6 +32,8 @@ const RESOURCE_ORIGIN = env.MCP_RESOURCE_ORIGIN ?? "https://executor.sh"; · 31 unmodified lines
32const jwks = createRemoteJWKSet(new URL(`${AUTHKIT_DOMAIN}/oauth2/jwks`));32const jwks = createRemoteJWKSet(new URL(`${AUTHKIT_DOMAIN}/oauth2/jwks`));
33 33
34const BEARER_PREFIX = "Bearer ";34const BEARER_PREFIX = "Bearer ";
35const INTERNAL_ACCOUNT_ID_HEADER = "x-executor-mcp-account-id";
36const INTERNAL_ORGANIZATION_ID_HEADER = "x-executor-mcp-organization-id";
35 37
36const CORS_ALLOW_ORIGIN = { "access-control-allow-origin": "*" } as const;38const CORS_ALLOW_ORIGIN = { "access-control-allow-origin": "*" } as const;
37 39
@@ -544,6 +546,13 @@ const withPropagationHeaders = ( · 506 unmodified lines
544 return new Request(request, { headers });546 return new Request(request, { headers });
545};547};
546 548
549const withVerifiedIdentityHeaders = (request: Request, token: VerifiedToken): Request => {
550 const headers = new Headers(request.headers);
551 headers.set(INTERNAL_ACCOUNT_ID_HEADER, token.accountId);
552 headers.set(INTERNAL_ORGANIZATION_ID_HEADER, token.organizationId ?? "");
553 return new Request(request, { headers });
554};
555
547const withMcpResponseHeaders = (response: Response): Response => {556const withMcpResponseHeaders = (response: Response): Response => {
548 const headers = new Headers(response.headers);557 const headers = new Headers(response.headers);
549 headers.set("access-control-allow-origin", "*");558 headers.set("access-control-allow-origin", "*");
@@ -560,12 +569,20 @@ const withMcpResponseHeaders = (response: Response): Response => { · 10 unmodified lines
560 * with `HttpServerResponse.raw` lets streaming bodies (SSE) pass through569 * with `HttpServerResponse.raw` lets streaming bodies (SSE) pass through
561 * `HttpApp.toWebHandler`'s conversion unchanged.570 * `HttpApp.toWebHandler`'s conversion unchanged.
562 */571 */
563const forwardToExistingSession = (request: Request, sessionId: string, peek: boolean) =>572const forwardToExistingSession = (
573 request: Request,
574 sessionId: string,
575 peek: boolean,
576 token: VerifiedToken,
577) =>
564 Effect.gen(function* () {578 Effect.gen(function* () {
565 const ns = env.MCP_SESSION;579 const ns = env.MCP_SESSION;
566 const stub = ns.get(ns.idFromString(sessionId));580 const stub = ns.get(ns.idFromString(sessionId));
567 const propagation = yield* currentPropagationHeaders(request);581 const propagation = yield* currentPropagationHeaders(request);
568 const propagated = withPropagationHeaders(request, propagation);582 const propagated = withPropagationHeaders(
583 withVerifiedIdentityHeaders(request, token),
584 propagation,
585 );
569 const raw = yield* Effect.promise(586 const raw = yield* Effect.promise(
570 () => stub.handleRequest(propagated) as Promise<Response>,587 () => stub.handleRequest(propagated) as Promise<Response>,
571 ).pipe(588 ).pipe(
@@ -588,7 +605,7 @@ const dispatchPost = (request: Request, token: VerifiedToken) => · 16 unmodified lines
588 }605 }
589 606
590 const sessionId = request.headers.get("mcp-session-id");607 const sessionId = request.headers.get("mcp-session-id");
591 if (sessionId) return yield* forwardToExistingSession(request, sessionId, true);608 if (sessionId) return yield* forwardToExistingSession(request, sessionId, true, token);
592 609
593 const ns = env.MCP_SESSION;610 const ns = env.MCP_SESSION;
594 const stub = ns.get(ns.newUniqueId());611 const stub = ns.get(ns.newUniqueId());
@@ -600,7 +617,10 @@ const dispatchPost = (request: Request, token: VerifiedToken) => · 5 unmodified lines
600 attributes: { "mcp.request.session_id_present": false },617 attributes: { "mcp.request.session_id_present": false },
601 }),618 }),
602 );619 );
603 const propagated = withPropagationHeaders(request, propagation);620 const propagated = withPropagationHeaders(
621 withVerifiedIdentityHeaders(request, token),
622 propagation,
623 );
604 const raw = yield* Effect.promise(624 const raw = yield* Effect.promise(
605 () => stub.handleRequest(propagated) as Promise<Response>,625 () => stub.handleRequest(propagated) as Promise<Response>,
606 ).pipe(626 ).pipe(
@@ -615,16 +635,16 @@ const dispatchPost = (request: Request, token: VerifiedToken) => · 8 unmodified lines
615 return HttpServerResponse.raw(withMcpResponseHeaders(annotated));635 return HttpServerResponse.raw(withMcpResponseHeaders(annotated));
616 });636 });
617 637
618const dispatchGet = (request: Request) => {638const dispatchGet = (request: Request, token: VerifiedToken) => {
619 const sessionId = request.headers.get("mcp-session-id");639 const sessionId = request.headers.get("mcp-session-id");
620 if (!sessionId) return Effect.succeed(jsonRpcError(400, -32000, "mcp-session-id header required for SSE"));640 if (!sessionId) return Effect.succeed(jsonRpcError(400, -32000, "mcp-session-id header required for SSE"));
621 return forwardToExistingSession(request, sessionId, false);641 return forwardToExistingSession(request, sessionId, false, token);
622};642};
623 643
624const dispatchDelete = (request: Request) => {644const dispatchDelete = (request: Request, token: VerifiedToken) => {
625 const sessionId = request.headers.get("mcp-session-id");645 const sessionId = request.headers.get("mcp-session-id");
626 if (!sessionId) return Effect.succeed(HttpServerResponse.empty({ status: 204 }));646 if (!sessionId) return Effect.succeed(HttpServerResponse.empty({ status: 204 }));
627 return forwardToExistingSession(request, sessionId, true);647 return forwardToExistingSession(request, sessionId, true, token);
628};648};
629 649
630// ---------------------------------------------------------------------------650// ---------------------------------------------------------------------------
@@ -680,9 +700,9 @@ export const mcpApp: Effect.Effect< · 49 unmodified lines
680 case "POST":700 case "POST":
681 return yield* dispatchPost(request, token);701 return yield* dispatchPost(request, token);
682 case "GET":702 case "GET":
683 return yield* dispatchGet(request);703 return yield* dispatchGet(request, token);
684 case "DELETE":704 case "DELETE":
685 return yield* dispatchDelete(request);705 return yield* dispatchDelete(request, token);
686 default:706 default:
687 return jsonRpcError(405, -32001, "Method not allowed");707 return jsonRpcError(405, -32001, "Method not allowed");
688 }708 }