# 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).