diff --git a/packages/desktop/packages/session-tools-core/src/handlers/submit-plan.test.ts b/packages/desktop/packages/session-tools-core/src/handlers/submit-plan.test.ts new file mode 100644 index 00000000000..9cf9e9d13bf --- /dev/null +++ b/packages/desktop/packages/session-tools-core/src/handlers/submit-plan.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it } from 'bun:test'; +import { join } from 'node:path'; +import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { handleSubmitPlan } from './submit-plan.ts'; +import type { SessionToolContext } from '../context.ts'; + +function createCtx( + plansFolderPath: string, + opts: { + exists?: boolean; + readFile?: string; + onRead?: (path: string) => void; + onSubmitted?: (path: string) => void; + } = {} +): SessionToolContext { + return { + sessionId: 'session-123', + workspacePath: join('/tmp', 'workspace'), + get sourcesPath() { + return join(this.workspacePath, 'sources'); + }, + get skillsPath() { + return join(this.workspacePath, 'skills'); + }, + plansFolderPath, + callbacks: { + onPlanSubmitted: (path: string) => opts.onSubmitted?.(path), + onAuthRequest: () => {}, + }, + fs: { + exists: () => opts.exists ?? true, + readFile: (path: string) => { + opts.onRead?.(path); + return opts.readFile ?? '# Plan'; + }, + readFileBuffer: () => Buffer.from(opts.readFile ?? '# Plan'), + writeFile: () => {}, + isDirectory: () => false, + readdir: () => [], + stat: () => ({ size: 0, isDirectory: () => false }), + }, + loadSourceConfig: () => null, + }; +} + +describe('handleSubmitPlan', () => { + const plansFolderPath = join('/tmp', 'workspace', 'sessions', 'session-123', 'plans'); + + it('submits a plan inside the session plans directory', async () => { + const submitted: string[] = []; + const planPath = join(plansFolderPath, 'plan.md'); + const ctx = createCtx(plansFolderPath, { + onSubmitted: (path) => submitted.push(path), + }); + + const result = await handleSubmitPlan(ctx, { planPath }); + + expect(result.isError).toBe(false); + expect(submitted).toEqual([planPath]); + }); + + it('rejects sibling paths that share the plans directory prefix', async () => { + const readAttempts: string[] = []; + const submitted: string[] = []; + const siblingPlanPath = join(`${plansFolderPath}-other`, 'plan.md'); + const ctx = createCtx(plansFolderPath, { + onRead: (path) => readAttempts.push(path), + onSubmitted: (path) => submitted.push(path), + }); + + const result = await handleSubmitPlan(ctx, { planPath: siblingPlanPath }); + + expect(result.isError).toBe(true); + expect(result.content[0]?.text).toContain('session plans directory'); + expect(readAttempts).toEqual([]); + expect(submitted).toEqual([]); + }); + + it('rejects paths that escape the plans directory through a symlink', async () => { + if (process.platform === 'win32') { + return; + } + + const rootDir = mkdtempSync(join(tmpdir(), 'submit-plan-boundary-')); + try { + const realPlansDir = join(rootDir, 'plans'); + const outsideDir = join(rootDir, 'outside'); + mkdirSync(realPlansDir, { recursive: true }); + mkdirSync(outsideDir, { recursive: true }); + writeFileSync(join(outsideDir, 'plan.md'), '# outside'); + symlinkSync(outsideDir, join(realPlansDir, 'escape-link'), 'dir'); + + const readAttempts: string[] = []; + const submitted: string[] = []; + const ctx = createCtx(realPlansDir, { + onRead: (path) => readAttempts.push(path), + onSubmitted: (path) => submitted.push(path), + }); + + const result = await handleSubmitPlan(ctx, { + planPath: join(realPlansDir, 'escape-link', 'plan.md'), + }); + + expect(result.isError).toBe(true); + expect(result.content[0]?.text).toContain('session plans directory'); + expect(readAttempts).toEqual([]); + expect(submitted).toEqual([]); + } finally { + rmSync(rootDir, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/desktop/packages/session-tools-core/src/handlers/submit-plan.ts b/packages/desktop/packages/session-tools-core/src/handlers/submit-plan.ts index 258a0fd91b2..74f27d753b8 100644 --- a/packages/desktop/packages/session-tools-core/src/handlers/submit-plan.ts +++ b/packages/desktop/packages/session-tools-core/src/handlers/submit-plan.ts @@ -8,6 +8,7 @@ import type { SessionToolContext } from '../context.ts'; import type { ToolResult } from '../types.ts'; import { successResponse, errorResponse } from '../response.ts'; +import { isPathWithinDirectory } from '../runtime/path-security.ts'; export interface SubmitPlanArgs { planPath: string; @@ -27,6 +28,12 @@ export async function handleSubmitPlan( ): Promise { const { planPath } = args; + if (!isPathWithinDirectory(planPath, ctx.plansFolderPath)) { + return errorResponse( + `Plan file must be inside the session plans directory: ${ctx.plansFolderPath}` + ); + } + // Verify the file exists if (!ctx.fs.exists(planPath)) { return errorResponse( diff --git a/packages/desktop/packages/shared/src/agent/__tests__/session-scoped-tools-path-boundary.test.ts b/packages/desktop/packages/shared/src/agent/__tests__/session-scoped-tools-path-boundary.test.ts new file mode 100644 index 00000000000..5892da7dd0e --- /dev/null +++ b/packages/desktop/packages/shared/src/agent/__tests__/session-scoped-tools-path-boundary.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from 'bun:test'; +import { join } from 'node:path'; +import { + getSessionPlansDir, + isPathInPlansDir, +} from '../session-scoped-tools.ts'; + +describe('session-scoped plan path helpers', () => { + const workspacePath = join('/tmp', 'workspace'); + const sessionId = 'session-123'; + + it('allows the plans directory itself', () => { + const plansDir = getSessionPlansDir(workspacePath, sessionId); + + expect(isPathInPlansDir(plansDir, workspacePath, sessionId)).toBe(true); + }); + + it('allows child paths inside the plans directory', () => { + const plansDir = getSessionPlansDir(workspacePath, sessionId); + const planPath = join(plansDir, 'plan.md'); + + expect(isPathInPlansDir(planPath, workspacePath, sessionId)).toBe(true); + }); + + it('rejects sibling paths that share the plans directory prefix', () => { + const plansDir = getSessionPlansDir(workspacePath, sessionId); + const siblingPlanPath = join(`${plansDir}-other`, 'plan.md'); + + expect(isPathInPlansDir(siblingPlanPath, workspacePath, sessionId)).toBe(false); + }); +}); diff --git a/packages/desktop/packages/shared/src/agent/session-scoped-tools.ts b/packages/desktop/packages/shared/src/agent/session-scoped-tools.ts index 8d0a6aa380d..7f3790578d7 100644 --- a/packages/desktop/packages/shared/src/agent/session-scoped-tools.ts +++ b/packages/desktop/packages/shared/src/agent/session-scoped-tools.ts @@ -17,7 +17,7 @@ import { getSessionPlansPath, getSessionPath } from '../sessions/storage.ts'; import { DOC_REFS } from '../docs/index.ts'; -import { basename } from 'node:path'; +import { basename, isAbsolute, relative, sep } from 'node:path'; import { createLocalMcpServer, localTool, type LocalTool } from '../mcp/local-tools.ts'; // Import from session-tools-core: registry + schemas + base descriptions @@ -140,7 +140,13 @@ export function getSessionPlansDir(workspacePath: string, sessionId: string): st */ export function isPathInPlansDir(path: string, workspacePath: string, sessionId: string): boolean { const plansDir = getSessionPlansDir(workspacePath, sessionId); - return path.startsWith(plansDir); + const relativePath = relative(plansDir, path); + return ( + relativePath === '' || + (!relativePath.startsWith(`..${sep}`) && + relativePath !== '..' && + !isAbsolute(relativePath)) + ); } // ============================================================