diff --git a/backend/__tests__/service/tasks.source-ref-idempotency.test.js b/backend/__tests__/service/tasks.source-ref-idempotency.test.js new file mode 100644 index 00000000..e723e0b5 --- /dev/null +++ b/backend/__tests__/service/tasks.source-ref-idempotency.test.js @@ -0,0 +1,162 @@ +process.env.PG_HOST = ''; +process.env.JWT_SECRET = 'tasks-source-ref-idempotency-test-secret'; + +const express = require('express'); +const request = require('supertest'); +const Pod = require('../../models/Pod'); +const Task = require('../../models/Task'); +const User = require('../../models/User'); +const tasksApiRoutes = require('../../routes/tasksApi'); +const { + setupMongoDb, + closeMongoDb, + clearMongoDb, + generateTestToken, +} = require('../utils/testUtils'); + +describe('POST /api/v1/tasks/:podId sourceRef idempotency', () => { + let app; + let owner; + let pod; + let token; + + beforeAll(async () => { + await setupMongoDb(); + await Task.init(); + app = express(); + app.use(express.json()); + app.use('/api/v1/tasks', tasksApiRoutes); + }); + + beforeEach(async () => { + await clearMongoDb(); + owner = await User.create({ + username: `task-owner-${Date.now()}`, + email: `task-owner-${Date.now()}@test.com`, + password: 'Password123!', + verified: true, + }); + pod = await Pod.create({ + name: 'Task idempotency pod', + type: 'team', + createdBy: owner._id, + members: [owner._id], + }); + token = generateTestToken(owner._id); + }); + + afterAll(async () => { + await clearMongoDb(); + await closeMongoDb(); + }); + + const postTask = (body) => request(app) + .post(`/api/v1/tasks/${pod._id}`) + .set('Authorization', `Bearer ${token}`) + .send(body); + + const seedTask = (overrides = {}) => Task.create({ + podId: pod._id, + taskNum: 1, + taskId: 'TASK-001', + title: 'Existing task', + source: 'github', + sourceRef: 'GH#697', + updates: [{ + text: 'Seeded for test', + author: 'system', + authorId: null, + createdAt: new Date(), + }], + ...overrides, + }); + + it('returns an existing pending task through the pre-check', async () => { + const existing = await seedTask(); + + const response = await postTask({ + title: 'Retry of existing task', + sourceRef: 'GH#697', + }).expect(200); + + expect(response.body.alreadyExists).toBe(true); + expect(response.body.task._id).toBe(String(existing._id)); + expect(await Task.countDocuments({ podId: pod._id, sourceRef: 'GH#697' })).toBe(1); + }); + + it('keeps the pre-check reopen behavior for a completed task', async () => { + const existing = await seedTask({ status: 'done', completedAt: new Date() }); + + const response = await postTask({ + title: 'Still-open source issue', + sourceRef: 'GH#697', + assignee: 'codex', + }).expect(200); + + expect(response.body).toMatchObject({ + alreadyExists: false, + reopened: true, + task: { + _id: String(existing._id), + status: 'pending', + assignee: 'codex', + }, + }); + expect(await Task.countDocuments({ podId: pod._id, sourceRef: 'GH#697' })).toBe(1); + }); + + it('reconciles a sourceRef E11000 race as an idempotent 200', async () => { + const existing = await seedTask(); + const findOneSpy = jest.spyOn(Task, 'findOne'); + findOneSpy.mockImplementationOnce(() => Promise.resolve(null)); + + const response = await postTask({ + title: 'Concurrent retry', + sourceRef: 'GH#697', + }).expect(200); + + expect(response.body.alreadyExists).toBe(true); + expect(response.body.task._id).toBe(String(existing._id)); + expect(await Task.countDocuments({ podId: pod._id, sourceRef: 'GH#697' })).toBe(1); + }); + + it('does not misclassify a taskId E11000 as sourceRef idempotency', async () => { + await seedTask({ sourceRef: 'GH#different-source' }); + const findOneSpy = jest.spyOn(Task, 'findOne'); + // The sourceRef pre-check legitimately finds no match. Then force the + // nextTaskId read to miss the existing TASK-001 so Mongo raises E11000 + // from the *taskId* index while the request still carries a sourceRef. + findOneSpy.mockImplementationOnce(() => Promise.resolve(null)); + findOneSpy.mockImplementationOnce(() => ({ + sort: () => ({ + select: () => ({ + lean: () => Promise.resolve(null), + }), + }), + })); + jest.spyOn(console, 'error').mockImplementation(() => {}); + + const response = await postTask({ + title: 'Colliding task number', + sourceRef: 'GH#697', + }).expect(500); + + expect(response.body.alreadyExists).toBeUndefined(); + expect(response.body.error).toBe('Failed to create task'); + expect(await Task.countDocuments({ podId: pod._id, taskId: 'TASK-001' })).toBe(1); + expect(await Task.countDocuments({ podId: pod._id, sourceRef: 'GH#697' })).toBe(0); + }); + + it('rejects an operator-shaped sourceRef before it reaches Mongo', async () => { + const findOneSpy = jest.spyOn(Task, 'findOne'); + + const response = await postTask({ + title: 'Attempted query injection', + sourceRef: { $ne: null }, + }).expect(400); + + expect(response.body.error).toBe('sourceRef must be a string'); + expect(findOneSpy).not.toHaveBeenCalled(); + expect(await Task.countDocuments({ podId: pod._id })).toBe(0); + }); +}); diff --git a/backend/routes/tasksApi.ts b/backend/routes/tasksApi.ts index a914281a..c4cfa487 100644 --- a/backend/routes/tasksApi.ts +++ b/backend/routes/tasksApi.ts @@ -1,3 +1,6 @@ +import { createHash } from 'crypto'; +import type { Request, Response } from 'express'; +import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; // eslint-disable-next-line global-require const express = require('express'); // eslint-disable-next-line global-require @@ -39,6 +42,14 @@ function auth(req: AuthReq, res: Res, next: () => void) { const router: ReturnType = express.Router(); +const taskWriteRateLimitKey = (req: Request): string => { + const authHeader = req.get('Authorization') || req.get('x-auth-token'); + if (authHeader) { + return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; + } + return req.ip ? ipKeyGenerator(req.ip) : 'anon'; +}; + async function resolveAuthor(req: AuthReq): Promise { const agentInstance = req.user?.isBot ? (req.user.botMetadata?.instanceId || req.user.botMetadata?.agentName) : null; if (agentInstance) return agentInstance; @@ -104,11 +115,67 @@ router.get('/:podId', auth, async (req: AuthReq, res: Res) => { } }); -router.post('/:podId', auth, async (req: AuthReq, res: Res) => { +router.post('/:podId', rateLimit({ + windowMs: 60_000, + max: 20, + standardHeaders: true, + legacyHeaders: false, + keyGenerator: taskWriteRateLimitKey, + handler: (_req: Request, res: Response) => res.status(429).json({ error: 'rate limit exceeded: 20 task creates per 60s' }), +}), auth, async (req: AuthReq, res: Res) => { try { const { podId } = req.params || {}; const userId = req.userId || req.user?._id || req.agentUser?._id; - const { title, assignee, dep, depMockOk, parentTask, source, sourceRef, githubIssueNumber, githubIssueUrl, createGithubIssue } = (req.body || {}) as { title?: string; assignee?: string; dep?: string; depMockOk?: boolean; parentTask?: string; source?: string; sourceRef?: string; githubIssueNumber?: number; githubIssueUrl?: string; createGithubIssue?: boolean }; + const { + title: titleInput, + assignee: assigneeInput, + dep: depInput, + depMockOk: depMockOkInput, + parentTask: parentTaskInput, + source: sourceInput, + sourceRef: sourceRefInput, + githubIssueNumber: githubIssueNumberInput, + githubIssueUrl: githubIssueUrlInput, + createGithubIssue: createGithubIssueInput, + } = req.body || {}; + const stringInputs: Record = { + title: titleInput, + assignee: assigneeInput, + dep: depInput, + parentTask: parentTaskInput, + source: sourceInput, + sourceRef: sourceRefInput, + githubIssueUrl: githubIssueUrlInput, + }; + const invalidStringField = Object.entries(stringInputs) + .find(([, value]) => value !== undefined && typeof value !== 'string'); + if (invalidStringField) { + return res.status(400).json({ error: `${invalidStringField[0]} must be a string` }); + } + if (githubIssueNumberInput !== undefined + && (typeof githubIssueNumberInput !== 'number' + || !Number.isSafeInteger(githubIssueNumberInput) + || githubIssueNumberInput < 1)) { + return res.status(400).json({ error: 'githubIssueNumber must be a positive integer' }); + } + if (depMockOkInput !== undefined && typeof depMockOkInput !== 'boolean') { + return res.status(400).json({ error: 'depMockOk must be a boolean' }); + } + if (createGithubIssueInput !== undefined && typeof createGithubIssueInput !== 'boolean') { + return res.status(400).json({ error: 'createGithubIssue must be a boolean' }); + } + // Materialize primitives after runtime validation so Mongo never receives + // user-supplied query objects (for example, operator-shaped values). + const title = titleInput === undefined ? undefined : String(titleInput); + const assignee = assigneeInput === undefined ? undefined : String(assigneeInput); + const dep = depInput === undefined ? undefined : String(depInput); + const parentTask = parentTaskInput === undefined ? undefined : String(parentTaskInput); + const source = sourceInput === undefined ? undefined : String(sourceInput); + const sourceRef = sourceRefInput === undefined ? undefined : String(sourceRefInput); + const githubIssueUrl = githubIssueUrlInput === undefined ? undefined : String(githubIssueUrlInput); + const githubIssueNumber = githubIssueNumberInput === undefined ? undefined : Number(githubIssueNumberInput); + const depMockOk = depMockOkInput === true; + const createGithubIssue = createGithubIssueInput === true; if (!title) return res.status(400).json({ error: 'title is required' }); const access = await requirePodMember(podId || '', userId, { write: true }); if (access.error) return res.status(access.status || 500).json({ error: access.error }); @@ -151,7 +218,38 @@ router.post('/:podId', auth, async (req: AuthReq, res: Res) => { if (sourceRef) initUpdate.text = `Created by ${author} from ${sourceRef}${assignee ? ` · assigned to ${assignee}` : ''}`; if (ghNumber) initUpdate.text += ` · GH#${ghNumber}`; if (parentTask) initUpdate.text += ` · sub-task of ${parentTask}`; - const task = await Task.create({ podId, taskNum, taskId, title, assignee: assignee || null, dep: dep || null, depMockOk: !!depMockOk, parentTask: parentTask || null, source: source || (ghNumber ? 'github' : 'human'), sourceRef: sourceRef || (ghNumber ? `GH#${ghNumber}` : undefined), githubIssueNumber: ghNumber, githubIssueUrl: ghUrl, updates: [initUpdate] }); + let task; + try { + task = await Task.create({ podId, taskNum, taskId, title, assignee: assignee || null, dep: dep || null, depMockOk: !!depMockOk, parentTask: parentTask || null, source: source || (ghNumber ? 'github' : 'human'), sourceRef: sourceRef || (ghNumber ? `GH#${ghNumber}` : undefined), githubIssueNumber: ghNumber, githubIssueUrl: ghUrl, updates: [initUpdate] }); + } catch (createErr) { + const duplicate = createErr as { + code?: number; + keyPattern?: Record; + message?: string; + }; + const sourceRefIndexCollision = duplicate.code === 11000 + && !!sourceRef + && ( + ( + duplicate.keyPattern?.podId === 1 + && duplicate.keyPattern?.sourceRef === 1 + && duplicate.keyPattern?.taskId === undefined + ) + || duplicate.message?.includes('podId_1_sourceRef_1_partial') + ); + if (!sourceRefIndexCollision) throw createErr; + + // The pre-check can lose a race to another request. Re-read the winner + // only after Mongo identifies the sourceRef index as the collision. + // Do not reopen here: the concurrent winner has just created a fresh + // task, so this request is an idempotent replay of that creation. + const existing = await Task.findOne({ + podId: mongoose.Types.ObjectId.createFromHexString(podId || ''), + sourceRef, + }) as { toObject: () => unknown } | null; + if (!existing) throw createErr; + return res.json({ task: existing.toObject(), alreadyExists: true }); + } if (parentTask && GitHubAppService.isPatConfigured()) { try { const parent = await Task.findOne({ podId: mongoose.Types.ObjectId.createFromHexString(podId || ''), taskId: parentTask }).lean() as { githubIssueNumber?: number } | null;