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 <noreply@anthropic.com>
This commit is contained in:
@@ -39,6 +39,13 @@ defensible for the alpha. (a) is the right long-term answer.
|
|||||||
|
|
||||||
## Migration 006 (audit + pending_changes) — Plan 2 finding
|
## 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
|
**[HIGH] `pending_changes.action` CHECK constraint blocks extended actions emitted
|
||||||
by some routes.** The table's CHECK accepts only `('create','update','delete')`,
|
by some routes.** The table's CHECK accepts only `('create','update','delete')`,
|
||||||
but the following routes call `divertToPending` with actions outside that set:
|
but the following routes call `divertToPending` with actions outside that set:
|
||||||
|
|||||||
@@ -22,10 +22,10 @@ const REPOS = {
|
|||||||
source_doc: sourceDocsRepo
|
source_doc: sourceDocsRepo
|
||||||
};
|
};
|
||||||
|
|
||||||
// Action dispatch only supports the three CHECK-allowed actions on
|
// Action dispatch for approving a pending_change. 'create'/'update'/'delete'
|
||||||
// pending_changes. Extended actions like 'upsert' / 'add_dependency' /
|
// plus 'upsert' (the suggestion path from POST /api/refs/upsert). Dependency
|
||||||
// 'remove_dependency' are surfaced from a few routes but are blocked at
|
// mutations are owner-only and never reach pending_changes — see
|
||||||
// the DB level today — see docs/security-followups.md.
|
// docs/security-followups.md.
|
||||||
export async function applyPendingChange(row, actor) {
|
export async function applyPendingChange(row, actor) {
|
||||||
const repo = REPOS[row.entity_type];
|
const repo = REPOS[row.entity_type];
|
||||||
if (!repo) throw new ValidationError(`unsupported entity_type for approval: ${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);
|
const created = await repo.create(row.payload, actor);
|
||||||
return created.id;
|
return created.id;
|
||||||
}
|
}
|
||||||
|
case 'upsert': {
|
||||||
|
const row_ = await repo.upsertByExternal(row.payload, actor);
|
||||||
|
return row_.id;
|
||||||
|
}
|
||||||
case 'update': {
|
case 'update': {
|
||||||
await repo.update(row.entity_id, row.payload, actor);
|
await repo.update(row.entity_id, row.payload, actor);
|
||||||
return row.entity_id;
|
return row.entity_id;
|
||||||
|
|||||||
@@ -5,7 +5,7 @@ import * as sourceDocs from '../../db/repos/source_docs.js';
|
|||||||
import * as audit from '../../db/repos/audit.js';
|
import * as audit from '../../db/repos/audit.js';
|
||||||
import { validate } from '../validate.js';
|
import { validate } from '../validate.js';
|
||||||
import { NotFoundError, ValidationError, ConflictError, asyncWrap } from '../errors.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 RUNTIME = ['lxc', 'vm', 'docker', 'bare-metal'];
|
||||||
const STATUSES = ['running', 'stopped', 'down', 'unknown'];
|
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',
|
router.post('/:id/dependencies',
|
||||||
requireWrite('resource'),
|
requireOwner,
|
||||||
validate({ params: idParams, body: depBody }),
|
validate({ params: idParams, body: depBody }),
|
||||||
asyncWrap(async (req, res) => {
|
asyncWrap(async (req, res) => {
|
||||||
if (req.params.id === req.body.depends_on) {
|
if (req.params.id === req.body.depends_on) {
|
||||||
throw new ValidationError('resource cannot depend on itself');
|
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 {
|
try {
|
||||||
await repo.addDependency(req.params.id, req.body.depends_on, req.body.kind);
|
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 });
|
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',
|
router.delete('/:id/dependencies/:dep_id',
|
||||||
requireWrite('resource'),
|
requireOwner,
|
||||||
validate({ params: depParams }),
|
validate({ params: depParams }),
|
||||||
asyncWrap(async (req, res) => {
|
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);
|
await repo.removeDependency(req.params.id, req.params.dep_id);
|
||||||
res.status(204).end();
|
res.status(204).end();
|
||||||
})
|
})
|
||||||
|
|||||||
9
lib/db/migrations/009_pending_upsert_action.sql
Normal file
9
lib/db/migrations/009_pending_upsert_action.sql
Normal file
@@ -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'));
|
||||||
105
tests/api/pending_extended_actions.test.js
Normal file
105
tests/api/pending_extended_actions.test.js
Normal file
@@ -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);
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user