Paul Nothaft
fecc18cbc8
* 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 (cherry picked from commit 688e318850db1b5f4ea2a4ae3c0fcf0fc137620d) * docs(security): drop the stale ownership JSDoc left by the rebase (GHSA-wrg5) Rebasing onto stable (which had gained scopeEventsQuery from #963) 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 <paul@MacStudio-von-Paul.local>
36 lines
2.0 KiB
JavaScript
36 lines
2.0 KiB
JavaScript
/**
|
|
* Migration 167 (projects.created_by) — idempotent on re-run, reversible,
|
|
* and backfills the owner from a project's single linked event (GHSA-wrg5).
|
|
*/
|
|
const path=require('path'), fs=require('fs'), os=require('os');
|
|
process.env.NODE_ENV='test';
|
|
process.env.TEST_DATABASE_PATH=path.join(fs.mkdtempSync(path.join(os.tmpdir(),'picpeak-mig167-')),'db.sqlite');
|
|
process.env.JWT_SECRET='mig';
|
|
const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb');
|
|
const mig = require('../../migrations/core/167_add_projects_created_by');
|
|
describe('migration 167', () => {
|
|
let db, cleanup;
|
|
beforeAll(async()=>{ ({db,cleanup}=await bootCrmDb()); await seedMinimal(db); },120000);
|
|
afterAll(async()=>{ if(cleanup) await cleanup(); });
|
|
it('is idempotent on re-run and reversible', async () => {
|
|
await mig.up(db); // already applied by boot; must no-op
|
|
await mig.up(db); // and again
|
|
expect(await db.schema.hasColumn('projects','created_by')).toBe(true);
|
|
await mig.down(db);
|
|
expect(await db.schema.hasColumn('projects','created_by')).toBe(false);
|
|
await mig.up(db); // re-apply cleanly
|
|
expect(await db.schema.hasColumn('projects','created_by')).toBe(true);
|
|
});
|
|
it('backfills created_by from a single linked event owner', async () => {
|
|
const p = await db('projects').insert({name:'bf',status:'active',created_at:new Date(),updated_at:new Date()}).returning('id');
|
|
const pid = p[0]?.id ?? p[0];
|
|
await db('events').insert({slug:'bf-ev',event_type:'wedding',event_name:'bf',event_date:'2026-08-01',
|
|
host_email:'h@e.com',admin_email:'a@e.com',password_hash:'x',share_token:'t1',share_link:'/g/bf-ev/t1',
|
|
created_by: 4242, project_id: pid, expires_at:new Date(Date.now()+864e5).toISOString(),
|
|
is_active:1,is_archived:0,is_draft:0,created_at:new Date().toISOString()});
|
|
await mig.up(db);
|
|
const row = await db('projects').where({id:pid}).first();
|
|
expect(row.created_by).toBe(4242);
|
|
});
|
|
});
|