From c60472149506d16708cc854709d0801996334169 Mon Sep 17 00:00:00 2001 From: Den Delimarsky Date: Thu, 5 Feb 2026 20:59:40 +0000 Subject: [PATCH 1/3] Fix cross-session task leakage --- .../src/experimental/tasks/stores/inMemory.ts | 56 +++++++--- .../core/test/experimental/inMemory.test.ts | 105 ++++++++++++++++++ 2 files changed, 147 insertions(+), 14 deletions(-) diff --git a/packages/core/src/experimental/tasks/stores/inMemory.ts b/packages/core/src/experimental/tasks/stores/inMemory.ts index b7b20da5d..ff578ea9e 100644 --- a/packages/core/src/experimental/tasks/stores/inMemory.ts +++ b/packages/core/src/experimental/tasks/stores/inMemory.ts @@ -11,6 +11,7 @@ interface StoredTask { task: Task; request: Request; requestId: RequestId; + sessionId?: string; result?: Result; } @@ -30,7 +31,7 @@ export class InMemoryTaskStore implements TaskStore { return crypto.randomUUID().replaceAll('-', ''); } - async createTask(taskParams: CreateTaskOptions, requestId: RequestId, request: Request, _sessionId?: string): Promise { + async createTask(taskParams: CreateTaskOptions, requestId: RequestId, request: Request, sessionId?: string): Promise { // Generate a unique task ID const taskId = this.generateTaskId(); @@ -55,7 +56,8 @@ export class InMemoryTaskStore implements TaskStore { this.tasks.set(taskId, { task, request, - requestId + requestId, + sessionId }); // Schedule cleanup if ttl is specified @@ -72,13 +74,30 @@ export class InMemoryTaskStore implements TaskStore { return task; } - async getTask(taskId: string, _sessionId?: string): Promise { + /** + * Retrieves a stored task, enforcing session ownership when a sessionId is provided. + * Returns undefined if the task does not exist or belongs to a different session. + */ + private getStoredTask(taskId: string, sessionId?: string): StoredTask | undefined { const stored = this.tasks.get(taskId); + if (!stored) { + return undefined; + } + // Enforce session isolation: if a sessionId is provided and the task + // was created with a sessionId, they must match. + if (sessionId !== undefined && stored.sessionId !== undefined && stored.sessionId !== sessionId) { + return undefined; + } + return stored; + } + + async getTask(taskId: string, sessionId?: string): Promise { + const stored = this.getStoredTask(taskId, sessionId); return stored ? { ...stored.task } : null; } - async storeTaskResult(taskId: string, status: 'completed' | 'failed', result: Result, _sessionId?: string): Promise { - const stored = this.tasks.get(taskId); + async storeTaskResult(taskId: string, status: 'completed' | 'failed', result: Result, sessionId?: string): Promise { + const stored = this.getStoredTask(taskId, sessionId); if (!stored) { throw new Error(`Task with ID ${taskId} not found`); } @@ -110,8 +129,8 @@ export class InMemoryTaskStore implements TaskStore { } } - async getTaskResult(taskId: string, _sessionId?: string): Promise { - const stored = this.tasks.get(taskId); + async getTaskResult(taskId: string, sessionId?: string): Promise { + const stored = this.getStoredTask(taskId, sessionId); if (!stored) { throw new Error(`Task with ID ${taskId} not found`); } @@ -123,8 +142,8 @@ export class InMemoryTaskStore implements TaskStore { return stored.result; } - async updateTaskStatus(taskId: string, status: Task['status'], statusMessage?: string, _sessionId?: string): Promise { - const stored = this.tasks.get(taskId); + async updateTaskStatus(taskId: string, status: Task['status'], statusMessage?: string, sessionId?: string): Promise { + const stored = this.getStoredTask(taskId, sessionId); if (!stored) { throw new Error(`Task with ID ${taskId} not found`); } @@ -159,13 +178,22 @@ export class InMemoryTaskStore implements TaskStore { } } - async listTasks(cursor?: string, _sessionId?: string): Promise<{ tasks: Task[]; nextCursor?: string }> { + async listTasks(cursor?: string, sessionId?: string): Promise<{ tasks: Task[]; nextCursor?: string }> { const PAGE_SIZE = 10; - const allTaskIds = [...this.tasks.keys()]; + + // Filter tasks by session ownership before pagination + const filteredTaskIds = [...this.tasks.entries()] + .filter(([, stored]) => { + if (sessionId === undefined || stored.sessionId === undefined) { + return true; + } + return stored.sessionId === sessionId; + }) + .map(([taskId]) => taskId); let startIndex = 0; if (cursor) { - const cursorIndex = allTaskIds.indexOf(cursor); + const cursorIndex = filteredTaskIds.indexOf(cursor); if (cursorIndex === -1) { // Invalid cursor - throw error throw new Error(`Invalid cursor: ${cursor}`); @@ -174,13 +202,13 @@ export class InMemoryTaskStore implements TaskStore { } } - const pageTaskIds = allTaskIds.slice(startIndex, startIndex + PAGE_SIZE); + const pageTaskIds = filteredTaskIds.slice(startIndex, startIndex + PAGE_SIZE); const tasks = pageTaskIds.map(taskId => { const stored = this.tasks.get(taskId)!; return { ...stored.task }; }); - const nextCursor = startIndex + PAGE_SIZE < allTaskIds.length ? pageTaskIds.at(-1) : undefined; + const nextCursor = startIndex + PAGE_SIZE < filteredTaskIds.length ? pageTaskIds.at(-1) : undefined; return { tasks, nextCursor }; } diff --git a/packages/core/test/experimental/inMemory.test.ts b/packages/core/test/experimental/inMemory.test.ts index 65e0a99ae..a3130fd01 100644 --- a/packages/core/test/experimental/inMemory.test.ts +++ b/packages/core/test/experimental/inMemory.test.ts @@ -647,6 +647,111 @@ describe('InMemoryTaskStore', () => { }); }); + describe('session isolation', () => { + const baseRequest: Request = { method: 'tools/call', params: { name: 'demo' } }; + + it('should not allow session-b to list tasks created by session-a', async () => { + await store.createTask({}, 1, baseRequest, 'session-a'); + await store.createTask({}, 2, baseRequest, 'session-a'); + + const result = await store.listTasks(undefined, 'session-b'); + expect(result.tasks).toHaveLength(0); + }); + + it('should not allow session-b to read a task created by session-a', async () => { + const task = await store.createTask({}, 1, baseRequest, 'session-a'); + + const result = await store.getTask(task.taskId, 'session-b'); + expect(result).toBeNull(); + }); + + it('should not allow session-b to update a task created by session-a', async () => { + const task = await store.createTask({}, 1, baseRequest, 'session-a'); + + await expect( + store.updateTaskStatus(task.taskId, 'cancelled', undefined, 'session-b') + ).rejects.toThrow('not found'); + }); + + it('should not allow session-b to store a result on session-a task', async () => { + const task = await store.createTask({}, 1, baseRequest, 'session-a'); + + await expect( + store.storeTaskResult(task.taskId, 'completed', { content: [] }, 'session-b') + ).rejects.toThrow('not found'); + }); + + it('should not allow session-b to get the result of session-a task', async () => { + const task = await store.createTask({}, 1, baseRequest, 'session-a'); + await store.storeTaskResult(task.taskId, 'completed', { content: [{ type: 'text', text: 'secret' }] }, 'session-a'); + + await expect( + store.getTaskResult(task.taskId, 'session-b') + ).rejects.toThrow('not found'); + }); + + it('should allow the owning session to access its own tasks', async () => { + const task = await store.createTask({}, 1, baseRequest, 'session-a'); + + const retrieved = await store.getTask(task.taskId, 'session-a'); + expect(retrieved).toBeDefined(); + expect(retrieved?.taskId).toBe(task.taskId); + }); + + it('should list only tasks belonging to the requesting session', async () => { + await store.createTask({}, 1, baseRequest, 'session-a'); + await store.createTask({}, 2, baseRequest, 'session-b'); + await store.createTask({}, 3, baseRequest, 'session-a'); + + const resultA = await store.listTasks(undefined, 'session-a'); + expect(resultA.tasks).toHaveLength(2); + + const resultB = await store.listTasks(undefined, 'session-b'); + expect(resultB.tasks).toHaveLength(1); + }); + + it('should allow access when no sessionId is provided (backward compatibility)', async () => { + const task = await store.createTask({}, 1, baseRequest, 'session-a'); + + // No sessionId on read = no filtering + const retrieved = await store.getTask(task.taskId); + expect(retrieved).toBeDefined(); + }); + + it('should allow access when task was created without sessionId', async () => { + const task = await store.createTask({}, 1, baseRequest); + + // Any sessionId on read should still see the task + const retrieved = await store.getTask(task.taskId, 'session-b'); + expect(retrieved).toBeDefined(); + }); + + it('should paginate correctly within a session', async () => { + // Create 15 tasks for session-a, 5 for session-b + for (let i = 1; i <= 15; i++) { + await store.createTask({}, i, baseRequest, 'session-a'); + } + for (let i = 16; i <= 20; i++) { + await store.createTask({}, i, baseRequest, 'session-b'); + } + + // First page for session-a should have 10 + const page1 = await store.listTasks(undefined, 'session-a'); + expect(page1.tasks).toHaveLength(10); + expect(page1.nextCursor).toBeDefined(); + + // Second page for session-a should have 5 + const page2 = await store.listTasks(page1.nextCursor, 'session-a'); + expect(page2.tasks).toHaveLength(5); + expect(page2.nextCursor).toBeUndefined(); + + // session-b should only see its 5 + const resultB = await store.listTasks(undefined, 'session-b'); + expect(resultB.tasks).toHaveLength(5); + expect(resultB.nextCursor).toBeUndefined(); + }); + }); + describe('cleanup', () => { it('should clear all timers and tasks', async () => { await store.createTask({ ttl: 1000 }, 1, { From fc0957896a591d336bc4b9426a39e39c3b4b233d Mon Sep 17 00:00:00 2001 From: Den Delimarsky Date: Thu, 5 Feb 2026 21:11:31 +0000 Subject: [PATCH 2/3] Add changeset. --- .changeset/fix-task-session-isolation.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/fix-task-session-isolation.md diff --git a/.changeset/fix-task-session-isolation.md b/.changeset/fix-task-session-isolation.md new file mode 100644 index 000000000..722067337 --- /dev/null +++ b/.changeset/fix-task-session-isolation.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/core': patch +--- + +Fix InMemoryTaskStore to enforce session isolation. Previously, sessionId was accepted but ignored on all TaskStore methods, allowing any session to enumerate, read, and mutate tasks created by other sessions. The store now persists sessionId at creation time and enforces ownership on all reads and writes. From 2c60294dce51139417e719327693df124b56f936 Mon Sep 17 00:00:00 2001 From: Den Delimarsky Date: Thu, 5 Feb 2026 21:14:29 +0000 Subject: [PATCH 3/3] Fix prettier error. --- packages/core/test/experimental/inMemory.test.ts | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/packages/core/test/experimental/inMemory.test.ts b/packages/core/test/experimental/inMemory.test.ts index a3130fd01..ea6a3a8dc 100644 --- a/packages/core/test/experimental/inMemory.test.ts +++ b/packages/core/test/experimental/inMemory.test.ts @@ -668,26 +668,20 @@ describe('InMemoryTaskStore', () => { it('should not allow session-b to update a task created by session-a', async () => { const task = await store.createTask({}, 1, baseRequest, 'session-a'); - await expect( - store.updateTaskStatus(task.taskId, 'cancelled', undefined, 'session-b') - ).rejects.toThrow('not found'); + await expect(store.updateTaskStatus(task.taskId, 'cancelled', undefined, 'session-b')).rejects.toThrow('not found'); }); it('should not allow session-b to store a result on session-a task', async () => { const task = await store.createTask({}, 1, baseRequest, 'session-a'); - await expect( - store.storeTaskResult(task.taskId, 'completed', { content: [] }, 'session-b') - ).rejects.toThrow('not found'); + await expect(store.storeTaskResult(task.taskId, 'completed', { content: [] }, 'session-b')).rejects.toThrow('not found'); }); it('should not allow session-b to get the result of session-a task', async () => { const task = await store.createTask({}, 1, baseRequest, 'session-a'); await store.storeTaskResult(task.taskId, 'completed', { content: [{ type: 'text', text: 'secret' }] }, 'session-a'); - await expect( - store.getTaskResult(task.taskId, 'session-b') - ).rejects.toThrow('not found'); + await expect(store.getTaskResult(task.taskId, 'session-b')).rejects.toThrow('not found'); }); it('should allow the owning session to access its own tasks', async () => {