fix(security): enforce project ownership on project + project-email routes (GHSA-wrg5, GHSA-93x4) (#960)

* fix(security): enforce project ownership (GHSA-wrg5, GHSA-93x4) Project routes authorized on generic events.view / events.edit with NO ownership check, so an editor-like admin could enumerate, read, update and aggregate projects belonging to other admins' events. The project email endpoints keyed on an email_queue id alone — any admin with events.view / email.send could preview, resend, cancel or retry ANY queued mail by walking ids. The earlier 'needs a migration, deferred' assessment was wrong in one direction and right in another: ownership IS derivable transitively via events.project_id -> events.created_by, but only for projects that already have a linked event. A brand-new EMPTY project has no derivable owner, which is exactly where the create -> attach flow starts. So migration 167 adds projects.created_by (backfilled from the single linked event owner, skipping ambiguous multi-owner projects) and createProject finally persists the adminId it was already being passed. - ownedProjectIds(): union of the stored owner and the transitive path, so pre-167 rows and new empty projects both resolve. Reads created_by defensively so an instance that hasn't run 167 falls back to the transitive rule instead of throwing. - requireProjectOwnership on detail/update/attach-event/attach-quote/ attach-contract/overview; list filtered by an id allowlist (empty array means 'owns nothing' and must return no rows, hence null-vs-[] care). - POST /:id/events also validates the INCOMING eventId — owning the project is not enough, or an editor could pull a foreign event in and read its rolled-up documents via /:id/overview. - Queued-email routes scoped via email_queue.event_id. CRM document mail has event_id NULL and no ownable parent here, so a scoped caller is denied rather than guessed into access. 404 (not 403) so it isn't an id oracle. Note: adminEmail.js:315/332 let any email.view/edit admin archive or delete any email_queue row — the same class, pre-existing and outside these two advisories. Left untouched and reported rather than silently widened. * fix(security): codex round 2 — make the stored project owner authoritative (GHSA-wrg5) The first predicate union'd 'any linked event I can see' with the stored owner, which opened two holes: - A project owned by admin B containing ONE legacy ownerless event became readable by every admin — and /:id/overview aggregates B's other events, invoices and emails, so a single legacy event exposed the whole project. - Migration 167 deliberately leaves multi-owner (ambiguous) projects NULL rather than guessing an owner. A NULL owner was then treated as 'everyone's', so exactly those mixed projects became globally accessible. Now: the stored created_by wins outright, and a project without a usable stored owner only derives access when EVERY linked event is accessible (and at least one exists). A created_by pointing at a hard-deleted admin degrades to 'no usable owner' so the project falls back to its events instead of being locked away — no ON DELETE SET NULL migration needed. A project with neither a usable owner nor linked events stays super_admin-only: failing closed beats failing open, and a super_admin can reassign it. Also returns a knex SUBQUERY rather than a materialised id list, so a large project count can't hit the driver's bind-parameter limit. * fix(security): codex round 3 — enforce deal-lineage ownership on project attach (GHSA-wrg5) requireProjectOwnership vets only the DESTINATION project, while attaching a quote or contract cascades through linkDealToProject — which re-points every event the deal produced into that project. An editor could therefore create an empty project of their own, attach another admin's quote, and pull that admin's events (plus the invoices, emails and gallery that roll up with them) into a project they own and can read via /:id/overview. The single-customer guard did not stand in the way: an unassigned project ADOPTS the deal's customer rather than rejecting it. linkDealToProject now refuses to move lineage events the actor cannot own, and assignDocument cascades BEFORE stamping the document so a refused attach leaves nothing half-applied (the old order committed the foreign document into the caller's project and only then declined the cascade). The quote/contract create+update paths, which reach the same cascade with an arbitrary project_id, thread their adminId through as well; isSuperAdmin() resolves the role for them and fails closed when it cannot. Events are the only ownership signal a deal carries — quotes and contracts have no created_by in this schema — so a lineage that produced no event still cannot be attributed. That is a property of the CRM model, noted in the code. Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me * docs(security): drop the stale ownership JSDoc left by the rebase (GHSA-wrg5) Rebasing onto main (which had gained scopeEventsQuery from #957) replayed the round-1 doc block above round-2's replacement, leaving a comment that describes the ORIGINAL union rule — "a project is the caller's when … it has at least one linked event they own" — directly above the code that deliberately no longer does that. That union is the hole round 2 closed; a comment asserting it is worse than none. Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me --------- Co-authored-by: Paul Nothaft <[email protected]>
2026-08-02 21:24:38 +02:00
parent 3fc6463873
commit 7c0c0a5b7f
10 changed files with 764 additions and 24 deletions
@@ -0,0 +1,71 @@
+/**
+ * Migration 167: give `projects` a first-class owner (GHSA-wrg5).
+ *
+ * Project routes authorize on generic `events.view` / `events.edit` only, with
+ * no ownership check, so an editor-like admin could enumerate, read, update
+ * and aggregate projects belonging to other admins' events.
+ *
+ * Ownership IS derivable transitively — `events.project_id` (migration 117)
+ * plus `events.created_by` (migration 060) — but only for projects that have
+ * at least one linked event. A freshly created, still-empty project has no
+ * derivable owner, which would leave a hole exactly where the create → attach
+ * flow starts. Storing the creator removes that ambiguity: projectService
+ * already receives `adminId` in createProject() and simply discarded it.
+ *
+ * Backfill uses the transitive path, which is well-defined here: migration 117
+ * created exactly one auto-project per pre-existing event, so those projects
+ * map 1:1 to an owning event. Projects with no linked event (or whose events
+ * are themselves ownerless legacy rows) stay NULL and are treated as
+ * unowned//legacy by the ownership helper — same convention the events table
+ * already uses for `created_by IS NULL`.
+ *
+ * down() drops the column; the derived data is reconstructible by re-running
+ * the same backfill, so nothing is lost irreversibly.
+ */
+
+exports.up = async function (knex) {
+  if (!(await knex.schema.hasTable('projects'))) return;
+
+  if (!(await knex.schema.hasColumn('projects', 'created_by'))) {
+    await knex.schema.alterTable('projects', (t) => {
+      // No FK constraint: admin_users rows can be removed, and orphaning a
+      // project would be worse than a dangling id (which reads as unowned).
+      t.integer('created_by').nullable();
+    });
+  }
+
+  // Backfill from the linked events, only where we can determine it
+  // unambiguously (every owning event agrees on a single non-null creator).
+  if (await knex.schema.hasColumn('events', 'project_id')
+    && await knex.schema.hasColumn('events', 'created_by')) {
+    const rows = await knex('events')
+      .whereNotNull('project_id')
+      .whereNotNull('created_by')
+      .select('project_id', 'created_by')
+      .groupBy('project_id', 'created_by');
+
+    const byProject = new Map();
+    for (const row of rows) {
+      const list = byProject.get(row.project_id) || [];
+      list.push(row.created_by);
+      byProject.set(row.project_id, list);
+    }
+
+    for (const [projectId, creators] of byProject) {
+      // Ambiguous (events from two different admins) → leave NULL rather than
+      // guess an owner and hand one admin authority over another's work.
+      if (creators.length !== 1) continue;
+      await knex('projects')
+        .where({ id: projectId })
+        .whereNull('created_by')
+        .update({ created_by: creators[0] });
+    }
+  }
+};
+
+exports.down = async function (knex) {
+  if (!(await knex.schema.hasTable('projects'))) return;
+  if (await knex.schema.hasColumn('projects', 'created_by')) {
+    await knex.schema.alterTable('projects', (t) => t.dropColumn('created_by'));
+  }
+};