Void-Homelab/docs/security-followups.md

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