c591b2aed1
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>
73 lines
3.9 KiB
Markdown
73 lines
3.9 KiB
Markdown
# Security Follow-ups for Plan 1
|
|
|
|
Items flagged by the security review plugin that were deferred at the user's request
|
|
("ease up on the security plugin") and are to be addressed in a consolidated pass
|
|
before Plan 1 is declared complete.
|
|
|
|
## Migration 005 (cross-cutting / polymorphic tables)
|
|
|
|
The polymorphic shape (`entity_type` text + `entity_id` uuid, no composite FK to a
|
|
parent) was an approved design decision in `docs/superpowers/specs/2026-05-31-void-v2-design.md`.
|
|
That decision trades referential integrity at the DB layer for one shared join table
|
|
across entity types. The plugin's findings are accurate but require revisiting the
|
|
design choice — not a one-line fix.
|
|
|
|
- **[HIGH] entity_tags / entity_links / attachments lack `space_id`.**
|
|
Cross-tenant linkage is possible at the DB layer. Mitigation options:
|
|
1. Add `space_id` NOT NULL to each polymorphic row; require callers to filter.
|
|
2. Per-type junction tables (e.g. `page_tags`, `task_links`) with real FKs.
|
|
|
|
- **[MEDIUM] `tags.name UNIQUE` is global, not per-space.** Cross-tenant collision
|
|
creates a side-channel for tag-name discovery. Fix: `UNIQUE(space_id, name)`.
|
|
|
|
- **[MEDIUM] No cascade on parent delete for `attachments` / `entity_tags`.**
|
|
Polymorphic FKs cannot be expressed in standard SQL. Fix options as above, or
|
|
a deletion trigger per parent type.
|
|
|
|
## Decision pending
|
|
|
|
Before closing Plan 1, decide whether to:
|
|
- (a) **Tighten now**: add `space_id` columns + per-type composite FKs to `entity_tags`,
|
|
`entity_links`, `attachments`, and a per-space UNIQUE on `tags`. Cost: ~1-2 hrs and
|
|
invalidates the polymorphic claim in the spec.
|
|
- (b) **Document and defer**: accept the tradeoff for Plan 1, gate access at the
|
|
REST/MCP layer (where `space_id` IS available from the actor's session), and
|
|
revisit when multi-tenant becomes load-bearing.
|
|
|
|
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
|
|
|
|
> **✅ 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:
|
|
|
|
- `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).
|