diff --git a/.taskmaster/tasks/tasks.json b/.taskmaster/tasks/tasks.json index bc4452c..1e48a80 100644 --- a/.taskmaster/tasks/tasks.json +++ b/.taskmaster/tasks/tasks.json @@ -3075,9 +3075,9 @@ "130", "148" ], - "status": "pending", + "status": "done", "subtasks": [], - "updatedAt": "2026-08-08T08:55:58.627Z" + "updatedAt": "2026-08-15T22:10:49.721Z" }, { "id": "152", @@ -3461,9 +3461,9 @@ ], "metadata": { "version": "1.0.0", - "lastModified": "2026-08-14T23:05:31.792Z", + "lastModified": "2026-08-15T22:10:49.722Z", "taskCount": 177, - "completedCount": 136, + "completedCount": 137, "tags": [ "master" ] diff --git a/src/db/prisma/migrations/20260815090000_add_merge_commit_and_webhook_secret/migration.sql b/src/db/prisma/migrations/20260815090000_add_merge_commit_and_webhook_secret/migration.sql new file mode 100644 index 0000000..e693b7f --- /dev/null +++ b/src/db/prisma/migrations/20260815090000_add_merge_commit_and_webhook_secret/migration.sql @@ -0,0 +1,6 @@ +-- AlterTable +ALTER TABLE "Task" ADD COLUMN "mergeCommit" TEXT; + +-- AlterTable +ALTER TABLE "GitProvider" ADD COLUMN "webhookSecretKey" TEXT; + diff --git a/src/db/prisma/schema.prisma b/src/db/prisma/schema.prisma index ac46a7c..7e4e636 100644 --- a/src/db/prisma/schema.prisma +++ b/src/db/prisma/schema.prisma @@ -1047,6 +1047,10 @@ model Task { headCommit String? prUrl String? prNumber Int? + // The commit the merge produced. CI runs against this, so a failing status + // webhook can find exactly the task that caused it instead of guessing at + // "whatever merged most recently". + mergeCommit String? // Free-form reviewer comment attached at the accept gate. reviewNote String? @db.Text ownerId String @@ -1256,21 +1260,26 @@ enum SessionState { // is a non-migration change (same reasoning as ResourceProposal.resourceType). model GitProvider { - id String @id @default(cuid()) - name String @unique - description String @default("") + id String @id @default(cuid()) + name String @unique + description String @default("") // Driver key: gitea | (future) github | gitlab - type String @default("gitea") + type String @default("gitea") // Instance base URL, e.g. https://mysources.co.uk — never hardcoded anywhere. - url String + url String // API credential, by reference. Same shape as Llm.apiKeySecret*; the value is // resolved through SecretService and never stored or returned here. - credSecretId String? - credSecretKey String? - ownerId String - version Int @default(1) - createdAt DateTime @default(now()) - updatedAt DateTime @updatedAt + credSecretId String? + credSecretKey String? + // Key (in the same Secret) holding this forge's webhook signing secret. + // Required before the webhook endpoint accepts anything from this provider — + // an unauthenticated webhook that can reopen work is an attack surface, so + // the route fails closed when this is unset. + webhookSecretKey String? + ownerId String + version Int @default(1) + createdAt DateTime @default(now()) + updatedAt DateTime @updatedAt cred Secret? @relation(fields: [credSecretId], references: [id]) owner User @relation(fields: [ownerId], references: [id], onDelete: Cascade) diff --git a/src/mcpd/src/main.ts b/src/mcpd/src/main.ts index bb62d48..9e948be 100644 --- a/src/mcpd/src/main.ts +++ b/src/mcpd/src/main.ts @@ -114,6 +114,7 @@ import { registerEpicTaskRoutes } from './routes/epictasks.js'; import { registerTaskRoutes } from './routes/tasks.js'; import { registerCodeRepoRoutes } from './routes/repos.js'; import { registerGitProviderRoutes } from './routes/gitproviders.js'; +import { registerGitWebhookRoutes } from './routes/git-webhooks.js'; import { registerSkillRoutes } from './routes/skills.js'; import { registerGitBackupRoutes } from './routes/git-backup.js'; import { PromptService } from './services/prompt.service.js'; @@ -643,6 +644,13 @@ async function main(): Promise { registerTaskRoutes(app, workTaskService, epicTaskRepo); registerCodeRepoRoutes(app, codeRepoService, projectRepo); registerGitProviderRoutes(app, gitProviderService, projectRepo); + registerGitWebhookRoutes(app, { + providers: gitProviderRepo, + secretRepo, + secrets: secretService, + tasks: workTaskRepo, + taskService: workTaskService, + }); // ── Git-based backup ── const gitBackup = new GitBackupService(prisma); diff --git a/src/mcpd/src/middleware/url-permission.ts b/src/mcpd/src/middleware/url-permission.ts index dcd684e..1ce5483 100644 --- a/src/mcpd/src/middleware/url-permission.ts +++ b/src/mcpd/src/middleware/url-permission.ts @@ -36,6 +36,11 @@ export function mapUrlToPermission(method: string, url: string): PermissionCheck return { kind: 'operation', operation: SET_OWN_PASSWORD_OPERATION }; } + // Git webhooks authenticate by HMAC signature, not by a user token — the + // forge has no mcpctl identity. Skipping RBAC here is safe only because the + // route itself fails closed without a configured signing secret. + if (url.startsWith('/api/v1/webhooks/git/')) return { kind: 'skip' }; + // Operations (non-resource endpoints) if (segment === 'backup') return { kind: 'operation', operation: 'backup' }; if (segment === 'restore') return { kind: 'operation', operation: 'restore' }; diff --git a/src/mcpd/src/repositories/task.repository.ts b/src/mcpd/src/repositories/task.repository.ts index 14dd9ea..2fd8ea9 100644 --- a/src/mcpd/src/repositories/task.repository.ts +++ b/src/mcpd/src/repositories/task.repository.ts @@ -31,6 +31,7 @@ export interface TaskUpdateInput { headCommit?: string; prUrl?: string; prNumber?: number; + mergeCommit?: string; reviewNote?: string; } @@ -61,6 +62,8 @@ export interface EventCreateInput { export interface ITaskRepository { findAll(filters?: TaskFilters): Promise; + /** Find the task a merge produced this commit for — the webhook's entry point. */ + findByMergeCommit(sha: string): Promise; findById(id: string): Promise; findByName(name: string): Promise; findByNameAndEpicTask(name: string, epicTaskId: string): Promise; @@ -98,6 +101,10 @@ export class TaskRepository implements ITaskRepository { }); } + async findByMergeCommit(sha: string): Promise { + return this.prisma.task.findFirst({ where: { mergeCommit: sha }, include: TASK_INCLUDE }); + } + async findById(id: string): Promise { return this.prisma.task.findUnique({ where: { id }, include: TASK_INCLUDE }); } diff --git a/src/mcpd/src/routes/git-webhooks.ts b/src/mcpd/src/routes/git-webhooks.ts new file mode 100644 index 0000000..4290678 --- /dev/null +++ b/src/mcpd/src/routes/git-webhooks.ts @@ -0,0 +1,142 @@ +import { createHmac, timingSafeEqual } from 'node:crypto'; + +import type { FastifyInstance } from 'fastify'; + +import type { IGitProviderRepository } from '../repositories/git-provider.repository.js'; +import type { ISecretRepository } from '../repositories/interfaces.js'; +import type { ITaskRepository } from '../repositories/task.repository.js'; +import type { SecretService } from '../services/secret.service.js'; +import type { TaskService } from '../services/task.service.js'; + +/** + * Post-merge failure → reopen the task that caused it. + * + * The design's last loop: a task reaches `merged`, CI then fails on the merge + * commit, and the same task comes back as `reopened` rather than the breakage + * going unnoticed because the board already said done. + * + * Two things make this safe rather than a liability: + * + * 1. **It fails closed.** The endpoint is unauthenticated by nature — a forge + * calls it — so it accepts nothing unless the provider has a configured + * webhook signing secret and the signature verifies. No secret, no effect. + * 2. **It matches precisely.** A failing status carries the commit sha, and the + * accept path recorded exactly which commit each merge produced. No guessing + * at "whatever merged most recently", which would reopen innocent work. + */ + +interface GiteaStatusEvent { + sha?: string; + state?: string; + context?: string; + description?: string; + target_url?: string; + repository?: { full_name?: string }; +} + +/** Failing states across forges. Anything else is ignored. */ +const FAILED = new Set(['failure', 'error']); + +export interface GitWebhookDeps { + providers: IGitProviderRepository; + secretRepo: ISecretRepository; + secrets: SecretService; + tasks: ITaskRepository; + taskService: TaskService; +} + +export function registerGitWebhookRoutes(app: FastifyInstance, deps: GitWebhookDeps): void { + // Raw body is required: a signature is over the exact bytes the forge sent, + // and re-serializing a parsed object changes them. + app.addContentTypeParser( + 'application/json', + { parseAs: 'string' }, + (req, body, done) => { + (req as unknown as { rawBody?: string }).rawBody = body as string; + try { + done(null, JSON.parse(body as string)); + } catch (err) { + done(err as Error, undefined); + } + }, + ); + + app.post<{ Params: { provider: string } }>( + '/api/v1/webhooks/git/:provider', + async (request, reply) => { + const provider = await deps.providers.findByName(request.params.provider); + if (provider === null) { + // Deliberately vague: this endpoint is public, and confirming which + // provider names exist is free reconnaissance. + reply.code(404); + return { error: 'unknown webhook' }; + } + + if (provider.webhookSecretKey === null || provider.credSecretId === null) { + reply.code(403); + return { + error: 'webhook_not_configured', + message: `GitProvider '${provider.name}' has no webhook signing secret, so its webhooks are refused.`, + }; + } + + const secret = await deps.secretRepo.findById(provider.credSecretId); + if (secret === null) { + reply.code(403); + return { error: 'webhook_not_configured', message: 'Signing secret no longer resolvable.' }; + } + const signingKey = await deps.secrets.resolve(secret.name, provider.webhookSecretKey); + + const raw = (request as unknown as { rawBody?: string }).rawBody ?? ''; + const sent = String(request.headers['x-gitea-signature'] ?? request.headers['x-hub-signature-256'] ?? '') + .replace(/^sha256=/, ''); + if (!verify(raw, signingKey, sent)) { + reply.code(401); + return { error: 'bad_signature' }; + } + + const body = request.body as GiteaStatusEvent; + const state = String(body.state ?? '').toLowerCase(); + if (!FAILED.has(state)) { + // Success and pending are the common case — acknowledge and do nothing. + return { ok: true, action: 'ignored', reason: `state '${state}' is not a failure` }; + } + + const sha = body.sha; + if (sha === undefined || sha === '') { + return { ok: true, action: 'ignored', reason: 'no commit sha in payload' }; + } + + const task = await deps.tasks.findByMergeCommit(sha); + if (task === null) { + // A failing build on a commit mcpctl did not merge is somebody else's + // business — not an error, and not something to guess about. + return { ok: true, action: 'ignored', reason: `no task merged commit ${sha.slice(0, 8)}` }; + } + if (task.state !== 'merged') { + return { ok: true, action: 'ignored', reason: `task '${task.name}' is '${task.state}', not 'merged'` }; + } + + const why = [ + `CI failed on the merge commit ${sha.slice(0, 8)}`, + body.context !== undefined ? ` (${body.context})` : '', + body.description !== undefined ? `: ${body.description}` : '', + ].join(''); + + await deps.taskService.reopen(task.id, why, { type: 'system' }); + + return { ok: true, action: 'reopened', task: task.name, reason: why }; + }, + ); +} + +/** Constant-time compare of the forge's HMAC against ours. */ +function verify(rawBody: string, key: string, sent: string): boolean { + if (sent === '') return false; + const expected = createHmac('sha256', key).update(rawBody).digest('hex'); + const a = Buffer.from(expected, 'utf8'); + const b = Buffer.from(sent, 'utf8'); + // timingSafeEqual throws on length mismatch, which would itself leak length. + if (a.length !== b.length) return false; + return timingSafeEqual(a, b); +} diff --git a/src/mcpd/src/services/git/gitea.driver.ts b/src/mcpd/src/services/git/gitea.driver.ts index 58c6c50..a227ab6 100644 --- a/src/mcpd/src/services/git/gitea.driver.ts +++ b/src/mcpd/src/services/git/gitea.driver.ts @@ -41,6 +41,7 @@ interface GiteaPull { state?: string; merged?: boolean; mergeable?: boolean; + merge_commit_sha?: string; head?: { sha?: string; ref?: string }; base?: { ref?: string }; } @@ -234,6 +235,7 @@ function toPullRequest(p: GiteaPull): PullRequest { head: p.head?.ref ?? '', base: p.base?.ref ?? '', mergeable: p.mergeable ?? null, + mergeCommitSha: p.merge_commit_sha ?? null, }; } diff --git a/src/mcpd/src/services/git/types.ts b/src/mcpd/src/services/git/types.ts index f5111ba..f4879cb 100644 --- a/src/mcpd/src/services/git/types.ts +++ b/src/mcpd/src/services/git/types.ts @@ -69,6 +69,8 @@ export interface PullRequest { base: string; /** Forge's own verdict on whether it can merge cleanly. Null when unknown. */ mergeable: boolean | null; + /** Commit the merge produced. Null until merged — CI runs against this. */ + mergeCommitSha: string | null; } export interface MergeOptions { diff --git a/src/mcpd/src/services/task-merger.ts b/src/mcpd/src/services/task-merger.ts index f966b44..95aa679 100644 --- a/src/mcpd/src/services/task-merger.ts +++ b/src/mcpd/src/services/task-merger.ts @@ -22,7 +22,7 @@ export class GitTaskMerger implements TaskMerger { private readonly mergeMethod: 'merge' | 'squash' | 'rebase' = 'merge', ) {} - async mergeForTask(task: Task, expectHeadSha: string | null): Promise<{ merged: boolean; detail: string }> { + async mergeForTask(task: Task, expectHeadSha: string | null): Promise<{ merged: boolean; detail: string; mergeCommit?: string }> { if (task.prNumber === null) { return { merged: true, detail: 'no pull request attached — approval recorded only' }; } @@ -58,9 +58,21 @@ export class GitTaskMerger implements TaskMerger { ...(expectHeadSha !== null ? { expectHeadSha } : {}), }, ); + // Re-read to capture what the merge produced. Best-effort: the merge + // already succeeded, so failing to read the sha must not fail the accept — + // it only costs the webhook its precise match later. + let mergeCommit: string | undefined; + try { + const after = await driver.getPullRequest(ctx, { owner: slug.owner, repo: slug.repo, number: task.prNumber }); + mergeCommit = after.mergeCommitSha ?? undefined; + } catch { + mergeCommit = undefined; + } + return { merged: true, detail: `merged ${slug.owner}/${slug.repo}#${String(task.prNumber)} via ${provider.name}`, + ...(mergeCommit !== undefined ? { mergeCommit } : {}), }; } catch (err: unknown) { if (err instanceof MergeStaleError) { diff --git a/src/mcpd/src/services/task.service.ts b/src/mcpd/src/services/task.service.ts index b72f29e..361b6fc 100644 --- a/src/mcpd/src/services/task.service.ts +++ b/src/mcpd/src/services/task.service.ts @@ -78,7 +78,7 @@ export interface TaskMerger { * @param expectHeadSha merge only if the PR head is still this commit — * the human approved that commit, not whatever landed after. */ - mergeForTask(task: Task, expectHeadSha: string | null): Promise<{ merged: boolean; detail: string }>; + mergeForTask(task: Task, expectHeadSha: string | null): Promise<{ merged: boolean; detail: string; mergeCommit?: string }>; } export class TaskService { @@ -301,6 +301,11 @@ export class TaskService { throw Object.assign(new Error(result.detail), { statusCode: 409 }); } mergeDetail = result.detail; + // Record what the merge produced: a failing CI webhook arrives with this + // sha, and it is the only precise way back to the task that caused it. + if (result.mergeCommit !== undefined) { + await this.repo.update(id, { mergeCommit: result.mergeCommit }); + } } return this.transition( diff --git a/src/mcpd/tests/git-webhooks.test.ts b/src/mcpd/tests/git-webhooks.test.ts new file mode 100644 index 0000000..4ca81da --- /dev/null +++ b/src/mcpd/tests/git-webhooks.test.ts @@ -0,0 +1,149 @@ +import { createHmac } from 'node:crypto'; + +import type { GitProvider, Secret, Task } from '@prisma/client'; +import Fastify, { type FastifyInstance } from 'fastify'; +import { describe, it, expect, vi, afterEach } from 'vitest'; + +import { errorHandler } from '../src/middleware/error-handler.js'; +import { mapUrlToPermission } from '../src/middleware/url-permission.js'; +import type { IGitProviderRepository } from '../src/repositories/git-provider.repository.js'; +import type { ISecretRepository } from '../src/repositories/interfaces.js'; +import type { ITaskRepository } from '../src/repositories/task.repository.js'; +import { registerGitWebhookRoutes } from '../src/routes/git-webhooks.js'; +import type { SecretService } from '../src/services/secret.service.js'; +import type { TaskService } from '../src/services/task.service.js'; + +const SECRET = 'hunter2'; +const SHA = 'deadbeefcafe1234'; + +let app: FastifyInstance; +afterEach(async () => { if (app) await app.close(); }); + +function build(opts: { + provider?: Partial | null; + task?: Partial | null; +} = {}) { + const reopen = vi.fn(async () => ({})); + const providerRow = opts.provider === null ? null : { + id: 'gp1', name: 'mysources', type: 'gitea', + credSecretId: 'sec1', credSecretKey: 'TOKEN', webhookSecretKey: 'WEBHOOK', + ...opts.provider, + } as GitProvider; + + const taskRow = opts.task === null ? null + : ({ id: 't1', name: 'wire-cli', state: 'merged', mergeCommit: SHA, ...opts.task } as Task); + + const instance = Fastify(); + instance.setErrorHandler(errorHandler); + registerGitWebhookRoutes(instance, { + providers: { findByName: vi.fn(async () => providerRow) } as unknown as IGitProviderRepository, + secretRepo: { findById: vi.fn(async () => ({ id: 'sec1', name: 'gitea-token' } as Secret)) } as unknown as ISecretRepository, + secrets: { resolve: vi.fn(async () => SECRET) } as unknown as SecretService, + tasks: { findByMergeCommit: vi.fn(async () => taskRow) } as unknown as ITaskRepository, + taskService: { reopen } as unknown as TaskService, + }); + return { instance, reopen }; +} + +const sign = (body: string, key = SECRET) => createHmac('sha256', key).update(body).digest('hex'); + +async function post(instance: FastifyInstance, payload: unknown, sig?: string) { + const body = JSON.stringify(payload); + return instance.inject({ + method: 'POST', + url: '/api/v1/webhooks/git/mysources', + headers: { 'content-type': 'application/json', 'x-gitea-signature': sig ?? sign(body) }, + payload: body, + }); +} + +const failure = { sha: SHA, state: 'failure', context: 'ci/build', description: 'tests failed' }; + +describe('git webhook — fails closed', () => { + it('refuses when the provider has no signing secret configured', async () => { + const b = build({ provider: { webhookSecretKey: null } }); app = b.instance; + const res = await post(b.instance, failure); + expect(res.statusCode).toBe(403); + expect(b.reopen).not.toHaveBeenCalled(); + }); + + it('refuses an unsigned request', async () => { + const b = build(); app = b.instance; + const res = await post(b.instance, failure, ''); + expect(res.statusCode).toBe(401); + expect(b.reopen).not.toHaveBeenCalled(); + }); + + it('refuses a wrong signature', async () => { + const b = build(); app = b.instance; + const res = await post(b.instance, failure, sign(JSON.stringify(failure), 'wrong-key')); + expect(res.statusCode).toBe(401); + expect(b.reopen).not.toHaveBeenCalled(); + }); + + it('refuses a signature over different bytes than were sent', async () => { + // Signing a re-serialized object rather than the raw body is the classic + // way this check silently stops working. + const b = build(); app = b.instance; + const res = await post(b.instance, failure, sign('{"sha":"other"}')); + expect(res.statusCode).toBe(401); + }); + + it('does not confirm which provider names exist', async () => { + const b = build({ provider: null }); app = b.instance; + const res = await post(b.instance, failure); + expect(res.statusCode).toBe(404); + expect(JSON.stringify(res.json())).not.toContain('mysources'); + }); + + it('is skipped by the RBAC hook — it authenticates by signature', () => { + expect(mapUrlToPermission('POST', '/api/v1/webhooks/git/mysources')).toEqual({ kind: 'skip' }); + }); +}); + +describe('git webhook — reopening', () => { + it('reopens the task whose merge produced the failing commit', async () => { + const b = build(); app = b.instance; + const res = await post(b.instance, failure); + + expect(res.statusCode).toBe(200); + expect(res.json()).toMatchObject({ action: 'reopened', task: 'wire-cli' }); + expect(b.reopen).toHaveBeenCalledWith('t1', expect.stringContaining('deadbeef'), { type: 'system' }); + expect(b.reopen.mock.calls[0]?.[1]).toContain('ci/build'); + }); + + it('ignores a passing status', async () => { + const b = build(); app = b.instance; + const res = await post(b.instance, { ...failure, state: 'success' }); + expect(res.json()).toMatchObject({ action: 'ignored' }); + expect(b.reopen).not.toHaveBeenCalled(); + }); + + it('ignores a failure on a commit mcpctl did not merge', async () => { + // Somebody else's build breaking is not mcpctl's business, and guessing at + // a task would reopen innocent work. + const b = build({ task: null }); app = b.instance; + const res = await post(b.instance, failure); + expect(res.json().reason).toMatch(/no task merged commit deadbeef/); + expect(b.reopen).not.toHaveBeenCalled(); + }); + + it('ignores a task that has already moved on from merged', async () => { + const b = build({ task: { state: 'reopened' } }); app = b.instance; + const res = await post(b.instance, failure); + expect(res.json().reason).toMatch(/is 'reopened', not 'merged'/); + expect(b.reopen).not.toHaveBeenCalled(); + }); + + it('ignores a payload with no sha', async () => { + const b = build(); app = b.instance; + const res = await post(b.instance, { state: 'failure' }); + expect(res.json().reason).toMatch(/no commit sha/); + }); + + it('treats error the same as failure', async () => { + const b = build(); app = b.instance; + await post(b.instance, { ...failure, state: 'error' }); + expect(b.reopen).toHaveBeenCalled(); + }); +});