From ec96e4e2e39968855db0a824deadad720bf70b48 Mon Sep 17 00:00:00 2001 From: root Date: Mon, 1 Jun 2026 02:01:10 +1000 Subject: [PATCH] feat(api): pending-changes + audit routes Owner-only routes wired with an applyPendingChange dispatch helper covering page/project/task/ref/resource/source_doc create/update/delete. Approve and reject emit their own audit_log entries (actions already in the CHECK vocab) so the audit trail is self-contained. Documents a latent bug in security-followups.md: pending_changes.action CHECK constraint blocks 'upsert' / 'add_dependency' / 'remove_dependency' divertToPending paths in refs/resources routes when an agent at suggest tier hits them. Co-Authored-By: Claude Opus 4.7 --- docs/security-followups.md | 27 +++++ lib/api/index.js | 4 + lib/api/routes/audit.js | 45 ++++++++ lib/api/routes/pending_changes.js | 95 +++++++++++++++++ tests/api/audit.test.js | 86 +++++++++++++++ tests/api/pending_changes.test.js | 167 ++++++++++++++++++++++++++++++ 6 files changed, 424 insertions(+) create mode 100644 lib/api/routes/audit.js create mode 100644 lib/api/routes/pending_changes.js create mode 100644 tests/api/audit.test.js create mode 100644 tests/api/pending_changes.test.js diff --git a/docs/security-followups.md b/docs/security-followups.md index d8c31f8..f937f49 100644 --- a/docs/security-followups.md +++ b/docs/security-followups.md @@ -36,3 +36,30 @@ Before closing Plan 1, decide whether to: Owner-only authn (Plan 1) means there's only one tenant in practice today, so (b) is defensible for the alpha. (a) is the right long-term answer. + +## Migration 006 (audit + pending_changes) — Plan 2 finding + +**[HIGH] `pending_changes.action` CHECK constraint blocks extended actions emitted +by some routes.** The table's CHECK accepts only `('create','update','delete')`, +but the following routes call `divertToPending` with actions outside that set: + +- `lib/api/routes/refs.js` (`POST /refs/upsert`) → `action: 'upsert'` +- `lib/api/routes/resources.js` (`POST /resources/:id/dependencies`) → `action: 'add_dependency'` +- `lib/api/routes/resources.js` (`DELETE /resources/:id/dependencies/:dep_id`) → `action: 'remove_dependency'` + +Today the owner bearer token always lands in `allow` tier and these paths never +hit `divertToPending`. The bug surfaces only when an agent at `suggest` tier +calls one of these endpoints, in which case the `INSERT INTO pending_changes` +fails with a CHECK violation and the request 500s. + +**Mitigation options:** +1. Widen the CHECK constraint to include `'upsert'`, `'add_dependency'`, + `'remove_dependency'`. Add the matching dispatch arms in + `lib/api/routes/pending_changes.js::applyPendingChange`. +2. Normalize at `divertToPending` entry — collapse `'upsert'` → `'create'`, + reject the dependency variants with `405 Method Not Allowed for agents`. +3. Move dependency mutations to owner-only endpoints (remove `requireWrite` and + the suggest branch) since dependencies are infra-level wiring. + +Recommended: (1) for upsert (legitimate suggestion path) and (3) for +add_dependency / remove_dependency (infra wiring, owner-only is the right scope). diff --git a/lib/api/index.js b/lib/api/index.js index dcc2efc..0770afd 100644 --- a/lib/api/index.js +++ b/lib/api/index.js @@ -17,6 +17,8 @@ import { router as conversationsRouter } from './routes/conversations.js'; import { conversationsScopedRouter as messagesByConvRouter } from './routes/messages.js'; import { router as tagsRouter, entityScopedRouter as tagsByEntityRouter } from './routes/tags.js'; import { router as linksRouter } from './routes/links.js'; +import { router as pendingChangesRouter } from './routes/pending_changes.js'; +import { router as auditRouter } from './routes/audit.js'; export function mountApi(app) { const api = Router(); @@ -41,6 +43,8 @@ export function mountApi(app) { api.use('/conversations/:conversation_id/messages', messagesByConvRouter); api.use('/tags', tagsRouter); api.use('/links', linksRouter); + api.use('/pending-changes', pendingChangesRouter); + api.use('/audit', auditRouter); api.use('/:entity_type/:entity_id/tags', tagsByEntityRouter); api.use((_req, _res, next) => next(new NotFoundError('route not found'))); diff --git a/lib/api/routes/audit.js b/lib/api/routes/audit.js new file mode 100644 index 0000000..c270a56 --- /dev/null +++ b/lib/api/routes/audit.js @@ -0,0 +1,45 @@ +import { Router } from 'express'; +import { z } from 'zod'; +import * as repo from '../../db/repos/audit.js'; +import { parsePagination } from '../pagination.js'; +import { validate } from '../validate.js'; +import { requireOwner } from '../cap.js'; +import { asyncWrap } from '../errors.js'; + +const ENTITY_TYPES = ['space','project','task','page','ref','resource','source_doc','conversation']; +const ACTOR_KINDS = ['user','agent','cron','worker','system']; + +const entityParams = z.object({ + type: z.enum(ENTITY_TYPES), + id: z.string().uuid() +}); + +const actorQuery = z.object({ + actor_kind: z.enum(ACTOR_KINDS).optional(), + actor_id: z.string().uuid().optional(), + limit: z.string().optional(), + offset: z.string().optional() +}); + +export const router = Router(); +router.use(requireOwner); + +router.get('/entity/:type/:id', + validate({ params: entityParams }), + asyncWrap(async (req, res) => { + const { limit } = parsePagination(req); + res.json(await repo.listForEntity(req.params.type, req.params.id, { limit })); + }) +); + +router.get('/actor', + validate({ query: actorQuery }), + asyncWrap(async (req, res) => { + const { limit } = parsePagination(req); + res.json(await repo.listByActor({ + actor_kind: req.validatedQuery.actor_kind, + actor_id: req.validatedQuery.actor_id, + limit + })); + }) +); diff --git a/lib/api/routes/pending_changes.js b/lib/api/routes/pending_changes.js new file mode 100644 index 0000000..eda8a72 --- /dev/null +++ b/lib/api/routes/pending_changes.js @@ -0,0 +1,95 @@ +import { Router } from 'express'; +import { z } from 'zod'; +import * as pendingRepo from '../../db/repos/pending_changes.js'; +import * as pagesRepo from '../../db/repos/pages.js'; +import * as projectsRepo from '../../db/repos/projects.js'; +import * as tasksRepo from '../../db/repos/tasks.js'; +import * as refsRepo from '../../db/repos/refs.js'; +import * as resourcesRepo from '../../db/repos/resources.js'; +import * as sourceDocsRepo from '../../db/repos/source_docs.js'; +import * as audit from '../../db/repos/audit.js'; +import { parsePagination } from '../pagination.js'; +import { validate } from '../validate.js'; +import { requireOwner } from '../cap.js'; +import { NotFoundError, ConflictError, ValidationError, asyncWrap } from '../errors.js'; + +const REPOS = { + page: pagesRepo, + project: projectsRepo, + task: tasksRepo, + ref: refsRepo, + resource: resourcesRepo, + source_doc: sourceDocsRepo +}; + +// Action dispatch only supports the three CHECK-allowed actions on +// pending_changes. Extended actions like 'upsert' / 'add_dependency' / +// 'remove_dependency' are surfaced from a few routes but are blocked at +// the DB level today — see docs/security-followups.md. +export async function applyPendingChange(row, actor) { + const repo = REPOS[row.entity_type]; + if (!repo) throw new ValidationError(`unsupported entity_type for approval: ${row.entity_type}`); + switch (row.action) { + case 'create': { + const created = await repo.create(row.payload, actor); + return created.id; + } + case 'update': { + await repo.update(row.entity_id, row.payload, actor); + return row.entity_id; + } + case 'delete': { + await repo.del(row.entity_id, actor); + return row.entity_id; + } + default: + throw new ValidationError(`unsupported action for approval: ${row.action}`); + } +} + +const idParams = z.object({ id: z.string().uuid() }); + +export const router = Router(); +router.use(requireOwner); + +router.get('/', + asyncWrap(async (req, res) => { + const { limit } = parsePagination(req); + res.json(await pendingRepo.listPending({ limit })); + }) +); + +router.post('/:id/approve', + validate({ params: idParams }), + asyncWrap(async (req, res) => { + const row = await pendingRepo.getById(req.params.id); + if (!row) throw new NotFoundError('pending change not found'); + if (row.status !== 'pending') { + throw new ConflictError(`pending change is already ${row.status}`); + } + const entity_id = await applyPendingChange(row, req.actor); + const resolved = await pendingRepo.resolve(req.params.id, 'approved', req.actor?.id ?? 'owner'); + await audit.recordAudit( + req.actor, 'approve', row.entity_type, entity_id, + null, { pending_id: row.id, original_agent_id: row.agent_id, action: row.action } + ); + res.json({ ...resolved, entity_id }); + }) +); + +router.post('/:id/reject', + validate({ params: idParams }), + asyncWrap(async (req, res) => { + const row = await pendingRepo.getById(req.params.id); + if (!row) throw new NotFoundError('pending change not found'); + if (row.status !== 'pending') { + throw new ConflictError(`pending change is already ${row.status}`); + } + const resolved = await pendingRepo.resolve(req.params.id, 'rejected', req.actor?.id ?? 'owner'); + await audit.recordAudit( + req.actor, 'reject', row.entity_type, row.entity_id, + null, { pending_id: row.id, original_agent_id: row.agent_id, action: row.action } + ); + res.json(resolved); + }) +); diff --git a/tests/api/audit.test.js b/tests/api/audit.test.js new file mode 100644 index 0000000..0444076 --- /dev/null +++ b/tests/api/audit.test.js @@ -0,0 +1,86 @@ +import { describe, it, expect, beforeAll, beforeEach } from 'vitest'; +import request from 'supertest'; +import { setup } from './helpers.js'; +import * as spacesRepo from '../../lib/db/repos/spaces.js'; +import * as pagesRepo from '../../lib/db/repos/pages.js'; +import * as agentsRepo from '../../lib/db/repos/agents.js'; + +let app, ownerHeaders, space; +const owner = { kind: 'user', id: null }; + +beforeAll(async () => { ({ app, ownerHeaders } = await setup()); }); +beforeEach(async () => { + space = await spacesRepo.create( + { slug: `s-${Date.now()}-${Math.random().toString(36).slice(2,5)}`, name: 'S' }, owner + ); +}); + +describe('audit routes', () => { + it('GET /entity/:type/:id returns create + update entries ordered DESC', async () => { + const page = await pagesRepo.create( + { space_id: space.id, slug: 'pg-au', title: 'T1' }, owner + ); + await pagesRepo.update(page.id, { title: 'T2' }, owner); + const res = await request(app) + .get(`/api/audit/entity/page/${page.id}`).set(ownerHeaders); + expect(res.status).toBe(200); + expect(res.body.length).toBeGreaterThanOrEqual(2); + const actions = res.body.map(e => e.action); + expect(actions).toContain('create'); + expect(actions).toContain('update'); + }); + + it('GET /entity/:type/:id respects limit', async () => { + const page = await pagesRepo.create( + { space_id: space.id, slug: 'pg-lim', title: 'T1' }, owner + ); + for (const t of ['T2','T3','T4']) { + await pagesRepo.update(page.id, { title: t }, owner); + } + const res = await request(app) + .get(`/api/audit/entity/page/${page.id}?limit=2`).set(ownerHeaders); + expect(res.body.length).toBe(2); + }); + + it('GET /actor filters by actor_kind=user', async () => { + await pagesRepo.create({ space_id: space.id, slug: 'a-u', title: 'Au' }, owner); + const res = await request(app) + .get('/api/audit/actor?actor_kind=user').set(ownerHeaders); + expect(res.status).toBe(200); + expect(res.body.every(e => e.actor_kind === 'user')).toBe(true); + }); + + it('GET /actor with actor_id filters tightly', async () => { + const agent = await agentsRepo.create({ + slug: `au-agent-${Date.now()}`, name: 'A', kind: 'claude', model: 'sonnet', + capabilities: { read: true, write: true }, scopes: { page: true } + }, owner); + const actor = { kind: 'agent', id: agent.id }; + await pagesRepo.create({ space_id: space.id, slug: 'a-x', title: 'X' }, actor); + const res = await request(app) + .get(`/api/audit/actor?actor_kind=agent&actor_id=${agent.id}`).set(ownerHeaders); + expect(res.status).toBe(200); + expect(res.body.length).toBeGreaterThan(0); + expect(res.body.every(e => e.actor_id === agent.id)).toBe(true); + }); + + it('agent token → 403 on audit endpoints (owner-only)', async () => { + const agent = await agentsRepo.create({ + slug: `au-403-${Date.now()}`, name: 'A', kind: 'claude', model: 'sonnet', + capabilities: { read: true }, scopes: {} + }, owner); + const { token } = await agentsRepo.createToken(agent.id, 'test'); + const headers = { Authorization: `Bearer ${token}` }; + const e = await request(app) + .get('/api/audit/entity/page/00000000-0000-0000-0000-000000000000').set(headers); + expect(e.status).toBe(403); + const a = await request(app).get('/api/audit/actor').set(headers); + expect(a.status).toBe(403); + }); + + it('invalid entity_type → 400', async () => { + const res = await request(app) + .get('/api/audit/entity/widget/00000000-0000-0000-0000-000000000000').set(ownerHeaders); + expect(res.status).toBe(400); + }); +}); diff --git a/tests/api/pending_changes.test.js b/tests/api/pending_changes.test.js new file mode 100644 index 0000000..50e337c --- /dev/null +++ b/tests/api/pending_changes.test.js @@ -0,0 +1,167 @@ +import { describe, it, expect, beforeAll, beforeEach } from 'vitest'; +import request from 'supertest'; +import { setup } from './helpers.js'; +import * as spacesRepo from '../../lib/db/repos/spaces.js'; +import * as projectsRepo from '../../lib/db/repos/projects.js'; +import * as pagesRepo from '../../lib/db/repos/pages.js'; +import * as agentsRepo from '../../lib/db/repos/agents.js'; + +let app, ownerHeaders, space, project, page; +const owner = { kind: 'user', id: null }; + +async function mintSuggestAgent(slug) { + const a = await agentsRepo.create({ + slug, name: slug, kind: 'claude', model: 'sonnet', + capabilities: { read: true, suggest: true }, scopes: {} + }, owner); + const { token } = await agentsRepo.createToken(a.id, 'test'); + return { agent: a, headers: { Authorization: `Bearer ${token}` } }; +} + +beforeAll(async () => { ({ app, ownerHeaders } = await setup()); }); +beforeEach(async () => { + space = await spacesRepo.create( + { slug: `s-${Date.now()}-${Math.random().toString(36).slice(2,5)}`, name: 'S' }, owner + ); + project = await projectsRepo.create( + { space_id: space.id, slug: 'p', name: 'P' }, owner + ); + page = await pagesRepo.create( + { space_id: space.id, slug: 'pg', title: 'PG' }, owner + ); +}); + +describe('pending_changes routes', () => { + it('GET returns empty list initially', async () => { + const res = await request(app).get('/api/pending-changes').set(ownerHeaders); + expect(res.status).toBe(200); + expect(res.body).toEqual([]); + }); + + it('agent suggest → row appears in GET list', async () => { + const { headers } = await mintSuggestAgent(`sug-list-${Date.now()}`); + await request(app).post(`/api/spaces/${space.id}/pages`).set(headers) + .send({ slug: 'pp', title: 'Pending Page' }); + const list = await request(app).get('/api/pending-changes').set(ownerHeaders); + expect(list.body.length).toBe(1); + expect(list.body[0].entity_type).toBe('page'); + expect(list.body[0].action).toBe('create'); + }); + + it('approve happy path: page create through dispatch', async () => { + const { agent, headers } = await mintSuggestAgent(`sug-approve-${Date.now()}`); + const sug = await request(app).post(`/api/spaces/${space.id}/pages`).set(headers) + .send({ slug: 'new-page', title: 'New Page', body_md: 'hello' }); + expect(sug.status).toBe(202); + const changeId = sug.body.change_id; + + const ap = await request(app).post(`/api/pending-changes/${changeId}/approve`).set(ownerHeaders); + expect(ap.status).toBe(200); + expect(ap.body.status).toBe('approved'); + expect(ap.body.entity_id).toBeTruthy(); + + const created = await request(app).get(`/api/pages/${ap.body.entity_id}`).set(ownerHeaders); + expect(created.body.title).toBe('New Page'); + expect(created.body.slug).toBe('new-page'); + + const audit = await request(app) + .get(`/api/audit/entity/page/${ap.body.entity_id}`).set(ownerHeaders); + const create_entry = audit.body.find(e => e.action === 'create'); + expect(create_entry).toBeDefined(); + expect(create_entry.actor_kind).toBe('user'); + const approve_entry = audit.body.find(e => e.action === 'approve'); + expect(approve_entry).toBeDefined(); + expect(approve_entry.actor_kind).toBe('user'); + expect(approve_entry.diff).toBeTruthy(); + expect(approve_entry.diff.after.original_agent_id).toBe(agent.id); + }); + + it('approve dispatch: project create', async () => { + const { headers } = await mintSuggestAgent(`sug-proj-${Date.now()}`); + const sug = await request(app).post(`/api/spaces/${space.id}/projects`).set(headers) + .send({ slug: 'nproj', name: 'NewProj' }); + const ap = await request(app).post(`/api/pending-changes/${sug.body.change_id}/approve`).set(ownerHeaders); + expect(ap.status).toBe(200); + const proj = await request(app).get(`/api/projects/${ap.body.entity_id}`).set(ownerHeaders); + expect(proj.body.name).toBe('NewProj'); + }); + + it('approve dispatch: page update', async () => { + const { headers } = await mintSuggestAgent(`sug-upd-${Date.now()}`); + const sug = await request(app).patch(`/api/pages/${page.id}`).set(headers) + .send({ title: 'Renamed' }); + expect(sug.status).toBe(202); + const ap = await request(app).post(`/api/pending-changes/${sug.body.change_id}/approve`).set(ownerHeaders); + expect(ap.status).toBe(200); + expect(ap.body.entity_id).toBe(page.id); + const after = await request(app).get(`/api/pages/${page.id}`).set(ownerHeaders); + expect(after.body.title).toBe('Renamed'); + }); + + it('approve dispatch: page delete', async () => { + const { headers } = await mintSuggestAgent(`sug-del-${Date.now()}`); + const sug = await request(app).delete(`/api/pages/${page.id}`).set(headers); + expect(sug.status).toBe(202); + const ap = await request(app).post(`/api/pending-changes/${sug.body.change_id}/approve`).set(ownerHeaders); + expect(ap.status).toBe(200); + const after = await request(app).get(`/api/pages/${page.id}`).set(ownerHeaders); + expect(after.status).toBe(404); + }); + + it('reject leaves entity untouched and marks row rejected', async () => { + const { headers } = await mintSuggestAgent(`sug-rej-${Date.now()}`); + const sug = await request(app).post(`/api/spaces/${space.id}/pages`).set(headers) + .send({ slug: 'rej', title: 'Rejected' }); + const rj = await request(app).post(`/api/pending-changes/${sug.body.change_id}/reject`).set(ownerHeaders); + expect(rj.status).toBe(200); + expect(rj.body.status).toBe('rejected'); + const audit = await request(app) + .get(`/api/audit/actor?actor_kind=user`).set(ownerHeaders); + expect(audit.body.some(e => e.action === 'reject')).toBe(true); + }); + + it('approve missing → 404', async () => { + const res = await request(app) + .post('/api/pending-changes/00000000-0000-0000-0000-000000000000/approve') + .set(ownerHeaders); + expect(res.status).toBe(404); + }); + + it('reject missing → 404', async () => { + const res = await request(app) + .post('/api/pending-changes/00000000-0000-0000-0000-000000000000/reject') + .set(ownerHeaders); + expect(res.status).toBe(404); + }); + + it('double-approve → 409', async () => { + const { headers } = await mintSuggestAgent(`sug-dup-${Date.now()}`); + const sug = await request(app).post(`/api/spaces/${space.id}/pages`).set(headers) + .send({ slug: 'dup', title: 'Dup' }); + await request(app).post(`/api/pending-changes/${sug.body.change_id}/approve`).set(ownerHeaders); + const second = await request(app) + .post(`/api/pending-changes/${sug.body.change_id}/approve`).set(ownerHeaders); + expect(second.status).toBe(409); + }); + + it('reject-then-approve → 409', async () => { + const { headers } = await mintSuggestAgent(`sug-ra-${Date.now()}`); + const sug = await request(app).post(`/api/spaces/${space.id}/pages`).set(headers) + .send({ slug: 'ra', title: 'RA' }); + await request(app).post(`/api/pending-changes/${sug.body.change_id}/reject`).set(ownerHeaders); + const ap = await request(app).post(`/api/pending-changes/${sug.body.change_id}/approve`).set(ownerHeaders); + expect(ap.status).toBe(409); + }); + + it('agent token → 403 on pending-changes endpoints (owner-only)', async () => { + const { headers } = await mintSuggestAgent(`sug-403-${Date.now()}`); + const list = await request(app).get('/api/pending-changes').set(headers); + expect(list.status).toBe(403); + const ap = await request(app) + .post('/api/pending-changes/00000000-0000-0000-0000-000000000000/approve').set(headers); + expect(ap.status).toBe(403); + const rj = await request(app) + .post('/api/pending-changes/00000000-0000-0000-0000-000000000000/reject').set(headers); + expect(rj.status).toBe(403); + }); +});