From c591b2aed1f35be51306900ccf3ab7cd0e94d894 Mon Sep 17 00:00:00 2001 From: root Date: Mon, 1 Jun 2026 23:19:44 +1000 Subject: [PATCH] fix(pending): allow suggest-tier 'upsert' drafts; make dependency wiring owner-only The pending_changes.action CHECK only permitted create/update/delete, so a suggest-tier agent hitting POST /api/refs/upsert (or the resource dependency routes) 500'd on the INSERT (docs/security-followups.md HIGH finding). - migration 009: widen CHECK to include 'upsert' - applyPendingChange: dispatch 'upsert' -> refsRepo.upsertByExternal on approve - resources.js: add_dependency/remove_dependency are now owner-only (requireOwner), infra wiring is never diverted to pending_changes - tests/api/pending_extended_actions.test.js: regression coverage Full suite green (278 pass / 1 skip). Co-Authored-By: Claude Opus 4.8 --- docs/security-followups.md | 7 ++ lib/api/routes/pending_changes.js | 12 +- lib/api/routes/resources.js | 18 +-- .../migrations/009_pending_upsert_action.sql | 9 ++ tests/api/pending_extended_actions.test.js | 105 ++++++++++++++++++ 5 files changed, 133 insertions(+), 18 deletions(-) create mode 100644 lib/db/migrations/009_pending_upsert_action.sql create mode 100644 tests/api/pending_extended_actions.test.js diff --git a/docs/security-followups.md b/docs/security-followups.md index f937f49..d773593 100644 --- a/docs/security-followups.md +++ b/docs/security-followups.md @@ -39,6 +39,13 @@ defensible for the alpha. (a) is the right long-term answer. ## Migration 006 (audit + pending_changes) — Plan 2 finding +> **✅ RESOLVED 2026-06-01** (migration `009_pending_upsert_action.sql`). Applied the +> recommended fix: (1) `upsert` is now a valid `pending_changes.action` with an +> approval dispatch arm (`applyPendingChange` → `refsRepo.upsertByExternal`); (3) +> `add_dependency` / `remove_dependency` routes are now **owner-only** (`requireOwner`) +> and never divert to `pending_changes`. Regression coverage in +> `tests/api/pending_extended_actions.test.js`. Original finding kept below for context. + **[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: diff --git a/lib/api/routes/pending_changes.js b/lib/api/routes/pending_changes.js index eda8a72..d7e6676 100644 --- a/lib/api/routes/pending_changes.js +++ b/lib/api/routes/pending_changes.js @@ -22,10 +22,10 @@ const REPOS = { 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. +// Action dispatch for approving a pending_change. 'create'/'update'/'delete' +// plus 'upsert' (the suggestion path from POST /api/refs/upsert). Dependency +// mutations are owner-only and never reach pending_changes — 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}`); @@ -34,6 +34,10 @@ export async function applyPendingChange(row, actor) { const created = await repo.create(row.payload, actor); return created.id; } + case 'upsert': { + const row_ = await repo.upsertByExternal(row.payload, actor); + return row_.id; + } case 'update': { await repo.update(row.entity_id, row.payload, actor); return row.entity_id; diff --git a/lib/api/routes/resources.js b/lib/api/routes/resources.js index b667ff0..73071a8 100644 --- a/lib/api/routes/resources.js +++ b/lib/api/routes/resources.js @@ -5,7 +5,7 @@ import * as sourceDocs from '../../db/repos/source_docs.js'; import * as audit from '../../db/repos/audit.js'; import { validate } from '../validate.js'; import { NotFoundError, ValidationError, ConflictError, asyncWrap } from '../errors.js'; -import { requireWrite, divertToPending } from '../cap.js'; +import { requireWrite, requireOwner, divertToPending } from '../cap.js'; const RUNTIME = ['lxc', 'vm', 'docker', 'bare-metal']; const STATUSES = ['running', 'stopped', 'down', 'unknown']; @@ -103,18 +103,14 @@ router.delete('/:id', }) ); +// Dependency wiring is infra-level: owner-only, never diverted to pending_changes. router.post('/:id/dependencies', - requireWrite('resource'), + requireOwner, validate({ params: idParams, body: depBody }), asyncWrap(async (req, res) => { if (req.params.id === req.body.depends_on) { throw new ValidationError('resource cannot depend on itself'); } - if (req.capTier === 'suggest') { - return divertToPending(req, res, { - entity_type: 'resource', entity_id: req.params.id, action: 'add_dependency', payload: req.body - }); - } try { await repo.addDependency(req.params.id, req.body.depends_on, req.body.kind); res.status(201).json({ resource_id: req.params.id, depends_on: req.body.depends_on }); @@ -135,15 +131,9 @@ router.get('/:id/dependencies', ); router.delete('/:id/dependencies/:dep_id', - requireWrite('resource'), + requireOwner, validate({ params: depParams }), asyncWrap(async (req, res) => { - if (req.capTier === 'suggest') { - return divertToPending(req, res, { - entity_type: 'resource', entity_id: req.params.id, action: 'remove_dependency', - payload: { depends_on: req.params.dep_id } - }); - } await repo.removeDependency(req.params.id, req.params.dep_id); res.status(204).end(); }) diff --git a/lib/db/migrations/009_pending_upsert_action.sql b/lib/db/migrations/009_pending_upsert_action.sql new file mode 100644 index 0000000..b9f3bd3 --- /dev/null +++ b/lib/db/migrations/009_pending_upsert_action.sql @@ -0,0 +1,9 @@ +-- Allow the legitimate suggestion action 'upsert' on pending_changes. +-- A suggest-tier agent hitting POST /api/refs/upsert previously 500'd because +-- the inline CHECK only permitted create/update/delete (docs/security-followups.md). +-- 'add_dependency' / 'remove_dependency' are intentionally NOT added here: +-- those routes become owner-only and never divert to pending_changes. +ALTER TABLE pending_changes DROP CONSTRAINT IF EXISTS pending_changes_action_check; +ALTER TABLE pending_changes + ADD CONSTRAINT pending_changes_action_check + CHECK (action IN ('create','update','delete','upsert')); diff --git a/tests/api/pending_extended_actions.test.js b/tests/api/pending_extended_actions.test.js new file mode 100644 index 0000000..ff39202 --- /dev/null +++ b/tests/api/pending_extended_actions.test.js @@ -0,0 +1,105 @@ +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 agentsRepo from '../../lib/db/repos/agents.js'; +import * as refsRepo from '../../lib/db/repos/refs.js'; +import * as pendingChanges from '../../lib/db/repos/pending_changes.js'; + +// Regression coverage for docs/security-followups.md: +// the pending_changes.action CHECK previously rejected the extended actions +// 'upsert' / 'add_dependency' / 'remove_dependency', so a suggest-tier agent +// hitting those routes 500'd on the INSERT. Fix: 'upsert' is a legitimate +// suggestion path (widen CHECK + add an approval dispatch arm); dependency +// mutations are infra wiring and become owner-only. + +let app, ownerHeaders, space; +const owner = { kind: 'user', id: null }; + +async function mintAgent(slug, caps, scopes = {}) { + const a = await agentsRepo.create({ + slug, name: slug, kind: 'claude', model: 'sonnet', + capabilities: caps, scopes + }, owner); + const { token } = await agentsRepo.createToken(a.id, 'test'); + return { agent: a, headers: { Authorization: `Bearer ${token}` } }; +} + +async function makeResource(spaceId, slug) { + const res = await request(app).post(`/api/spaces/${spaceId}/resources`).set(ownerHeaders) + .send({ slug, name: slug, runtime_type: 'lxc' }); + return res.body; +} + +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('ref upsert as a suggest-tier draft', () => { + it('suggest-tier upsert diverts to a pending_change with action=upsert (not 500)', async () => { + const { agent, headers } = await mintAgent(`sug-up-${Date.now()}`, { read: true, suggest: true }); + const res = await request(app).post('/api/refs/upsert').set(headers).send({ + space_id: space.id, kind: 'url', source_url: 'https://x', title: 'X', + source_kind: 'karakeep', external_id: 'ext-1' + }); + expect(res.status).toBe(202); + expect(res.body.pending).toBe(true); + const change = await pendingChanges.getById(res.body.change_id); + expect(change.agent_id).toBe(agent.id); + expect(change.entity_type).toBe('ref'); + expect(change.action).toBe('upsert'); + expect(change.payload.external_id).toBe('ext-1'); + }); + + it('approving an upsert pending_change creates the ref via upsertByExternal', async () => { + const { headers } = await mintAgent(`sug-up2-${Date.now()}`, { read: true, suggest: true }); + const draft = await request(app).post('/api/refs/upsert').set(headers).send({ + space_id: space.id, kind: 'url', source_url: 'https://y', title: 'Y', + source_kind: 'karakeep', external_id: 'ext-2' + }); + expect(draft.status).toBe(202); + + const approve = await request(app) + .post(`/api/pending-changes/${draft.body.change_id}/approve`).set(ownerHeaders); + expect(approve.status).toBe(200); + + const list = await refsRepo.list({ space_id: space.id, kind: 'url', limit: 50, offset: 0 }); + const created = list.find(r => r.external_id === 'ext-2'); + expect(created).toBeTruthy(); + expect(created.title).toBe('Y'); + }); +}); + +describe('resource dependency mutations are owner-only', () => { + it('owner can add and remove a dependency', async () => { + const a = await makeResource(space.id, 'dep-a'); + const b = await makeResource(space.id, 'dep-b'); + const add = await request(app).post(`/api/resources/${a.id}/dependencies`).set(ownerHeaders) + .send({ depends_on: b.id }); + expect(add.status).toBe(201); + const del = await request(app).delete(`/api/resources/${a.id}/dependencies/${b.id}`).set(ownerHeaders); + expect(del.status).toBe(204); + }); + + it('an agent (even suggest tier) cannot add a dependency → 403, no pending_change', async () => { + const a = await makeResource(space.id, 'dep-c'); + const b = await makeResource(space.id, 'dep-d'); + const { headers } = await mintAgent(`sug-dep-${Date.now()}`, { read: true, suggest: true }); + const res = await request(app).post(`/api/resources/${a.id}/dependencies`).set(headers) + .send({ depends_on: b.id }); + expect(res.status).toBe(403); + const pending = await pendingChanges.listPending({ limit: 50 }); + expect(pending.some(p => p.action === 'add_dependency')).toBe(false); + }); + + it('an agent cannot remove a dependency → 403', async () => { + const a = await makeResource(space.id, 'dep-e'); + const b = await makeResource(space.id, 'dep-f'); + await request(app).post(`/api/resources/${a.id}/dependencies`).set(ownerHeaders) + .send({ depends_on: b.id }); + const { headers } = await mintAgent(`sug-dep2-${Date.now()}`, { read: true, suggest: true }); + const res = await request(app).delete(`/api/resources/${a.id}/dependencies/${b.id}`).set(headers); + expect(res.status).toBe(403); + }); +});