From 684dbdc6a4bb619bbdc9f4fe8c04846e9fdf70c6 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Fri, 13 Mar 2026 14:43:33 -0500 Subject: [PATCH] fix(gateway): enforce task and mission ownership --- .../src/__tests__/resource-ownership.test.ts | 48 +++++++++++++++ .../src/missions/missions.controller.ts | 5 +- apps/gateway/src/tasks/tasks.controller.ts | 53 ++++++++++++++--- .../task-mission-ownership-20260313.md | 58 +++++++++++++++++++ 4 files changed, 155 insertions(+), 9 deletions(-) create mode 100644 docs/scratchpads/task-mission-ownership-20260313.md diff --git a/apps/gateway/src/__tests__/resource-ownership.test.ts b/apps/gateway/src/__tests__/resource-ownership.test.ts index 8fbf4ee..b1718e3 100644 --- a/apps/gateway/src/__tests__/resource-ownership.test.ts +++ b/apps/gateway/src/__tests__/resource-ownership.test.ts @@ -86,4 +86,52 @@ describe('Resource ownership checks', () => { ForbiddenException, ); }); + + it('forbids creating a task with an unowned project', async () => { + const brain = createBrain(); + brain.projects.findById.mockResolvedValue({ id: 'project-1', ownerId: 'user-2' }); + const controller = new TasksController(brain as never); + + await expect( + controller.create( + { + title: 'Task', + projectId: 'project-1', + }, + { id: 'user-1' }, + ), + ).rejects.toBeInstanceOf(ForbiddenException); + }); + + it('forbids listing tasks for an unowned project', async () => { + const brain = createBrain(); + brain.projects.findById.mockResolvedValue({ id: 'project-1', ownerId: 'user-2' }); + const controller = new TasksController(brain as never); + + await expect( + controller.list({ id: 'user-1' }, 'project-1', undefined, undefined), + ).rejects.toBeInstanceOf(ForbiddenException); + }); + + it('lists only tasks for the current user owned projects when no filter is provided', async () => { + const brain = createBrain(); + brain.projects.findAll.mockResolvedValue([ + { id: 'project-1', ownerId: 'user-1' }, + { id: 'project-2', ownerId: 'user-2' }, + ]); + brain.missions.findAll.mockResolvedValue([{ id: 'mission-1', projectId: 'project-1' }]); + brain.tasks.findAll.mockResolvedValue([ + { id: 'task-1', projectId: 'project-1' }, + { id: 'task-2', missionId: 'mission-1' }, + { id: 'task-3', projectId: 'project-2' }, + ]); + const controller = new TasksController(brain as never); + + await expect( + controller.list({ id: 'user-1' }, undefined, undefined, undefined), + ).resolves.toEqual([ + { id: 'task-1', projectId: 'project-1' }, + { id: 'task-2', missionId: 'mission-1' }, + ]); + }); }); diff --git a/apps/gateway/src/missions/missions.controller.ts b/apps/gateway/src/missions/missions.controller.ts index 67aa01e..1671d45 100644 --- a/apps/gateway/src/missions/missions.controller.ts +++ b/apps/gateway/src/missions/missions.controller.ts @@ -36,7 +36,10 @@ export class MissionsController { } @Post() - async create(@Body() dto: CreateMissionDto) { + async create(@Body() dto: CreateMissionDto, @CurrentUser() user: { id: string }) { + if (dto.projectId) { + await this.getOwnedProject(dto.projectId, user.id, 'Mission'); + } return this.brain.missions.create({ name: dto.name, description: dto.description, diff --git a/apps/gateway/src/tasks/tasks.controller.ts b/apps/gateway/src/tasks/tasks.controller.ts index 3988a23..7796715 100644 --- a/apps/gateway/src/tasks/tasks.controller.ts +++ b/apps/gateway/src/tasks/tasks.controller.ts @@ -28,17 +28,48 @@ export class TasksController { @Get() async list( + @CurrentUser() user: { id: string }, @Query('projectId') projectId?: string, @Query('missionId') missionId?: string, @Query('status') status?: string, ) { - if (projectId) return this.brain.tasks.findByProject(projectId); - if (missionId) return this.brain.tasks.findByMission(missionId); - if (status) - return this.brain.tasks.findByStatus( - status as Parameters[0], - ); - return this.brain.tasks.findAll(); + if (projectId) { + await this.getOwnedProject(projectId, user.id, 'Task'); + return this.brain.tasks.findByProject(projectId); + } + if (missionId) { + await this.getOwnedMission(missionId, user.id, 'Task'); + return this.brain.tasks.findByMission(missionId); + } + + const [projects, missions, tasks] = await Promise.all([ + this.brain.projects.findAll(), + this.brain.missions.findAll(), + status + ? this.brain.tasks.findByStatus( + status as Parameters[0], + ) + : this.brain.tasks.findAll(), + ]); + + const ownedProjectIds = new Set( + projects.filter((project) => project.ownerId === user.id).map((project) => project.id), + ); + const ownedMissionIds = new Set( + missions + .filter( + (ownedMission) => + typeof ownedMission.projectId === 'string' && + ownedProjectIds.has(ownedMission.projectId), + ) + .map((ownedMission) => ownedMission.id), + ); + + return tasks.filter( + (task) => + (task.projectId ? ownedProjectIds.has(task.projectId) : false) || + (task.missionId ? ownedMissionIds.has(task.missionId) : false), + ); } @Get(':id') @@ -47,7 +78,13 @@ export class TasksController { } @Post() - async create(@Body() dto: CreateTaskDto) { + async create(@Body() dto: CreateTaskDto, @CurrentUser() user: { id: string }) { + if (dto.projectId) { + await this.getOwnedProject(dto.projectId, user.id, 'Task'); + } + if (dto.missionId) { + await this.getOwnedMission(dto.missionId, user.id, 'Task'); + } return this.brain.tasks.create({ title: dto.title, description: dto.description, diff --git a/docs/scratchpads/task-mission-ownership-20260313.md b/docs/scratchpads/task-mission-ownership-20260313.md new file mode 100644 index 0000000..0374f71 --- /dev/null +++ b/docs/scratchpads/task-mission-ownership-20260313.md @@ -0,0 +1,58 @@ +# Task Ownership Gap Fix Scratchpad + +## Metadata + +- Date: 2026-03-13 +- Worktree: `/home/jwoltje/src/mosaic-mono-v1-worktrees/fix-task-ownership` +- Branch: `fix/task-mission-ownership` +- Scope: Fix ownership checks in TasksController/MissionsController and extend gateway ownership tests +- Related tracker: worker task only; `docs/TASKS.md` is orchestrator-owned and left unchanged +- Budget assumption: no explicit token cap; keep scope limited to requested gateway permission fixes + +## Objective + +Close ownership gaps so task listing/creation and mission creation enforce project/mission ownership and reject cross-user access. + +## Acceptance Criteria + +1. TasksController `list()` enforces ownership for `projectId` and `missionId`, and does not return cross-user data when neither filter is provided. +2. TasksController `create()` rejects unowned `projectId` and `missionId` references. +3. MissionsController `create()` rejects unowned `projectId` references. +4. Gateway ownership tests cover forbidden task creation and forbidden task listing by unowned project. + +## Plan + +1. Inspect current controller and ownership test patterns. +2. Add failing permission tests first. +3. Patch controller methods with existing ownership helpers. +4. Run targeted gateway tests, then gateway typecheck/lint/full test. +5. Perform independent review, record evidence, then complete the requested git/PR workflow. + +## TDD Notes + +- Required: yes. This is auth/permission logic and a bugfix. +- Strategy: add failing tests in `resource-ownership.test.ts`, verify red, then implement minimal controller changes. + +## Verification Log + +- `pnpm --filter @mosaic/gateway test -- src/__tests__/resource-ownership.test.ts` + - Red: failed with 2 expected permission-path failures before controller changes. + - Green: passed after wiring ownership checks and adding owned-task filtering coverage. +- `pnpm --filter @mosaic/gateway typecheck` + - Pass on 2026-03-13 after fixing parameter ordering and mission project nullability. +- `pnpm --filter @mosaic/gateway lint` + - Pass on 2026-03-13. +- `pnpm --filter @mosaic/gateway test` + - Pass on 2026-03-13 with 3 test files and 23 tests passing. +- `pnpm format:check` + - Pass on 2026-03-13. + +## Review Log + +- Manual review: checked for auth regressions, cross-user list leakage, and dashboard behavior impact; kept unfiltered task list functional by filtering to owned projects/missions instead of returning an empty list. +- Automated review: `~/.config/mosaic/tools/codex/codex-code-review.sh --uncommitted` running/re-run for independent review evidence. + +## Risks / Blockers + +- Repository-wide Mosaic instructions require merge/issue closure, but the user explicitly instructed PR-only and no merge; follow the user instruction. +- `docs/TASKS.md` is orchestrator-owned and will not be edited from this worker task.