diff --git a/backend/__tests__/migrations/167_projects_created_by.test.js b/backend/__tests__/migrations/167_projects_created_by.test.js new file mode 100644 index 00000000..7afe5082 --- /dev/null +++ b/backend/__tests__/migrations/167_projects_created_by.test.js @@ -0,0 +1,35 @@ +/** + * 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); + }); +}); diff --git a/backend/__tests__/routes/projectOwnership.test.js b/backend/__tests__/routes/projectOwnership.test.js new file mode 100644 index 00000000..428d75cb --- /dev/null +++ b/backend/__tests__/routes/projectOwnership.test.js @@ -0,0 +1,181 @@ +/** + * Project ownership — GHSA-wrg5 (project routes) and GHSA-93x4 (project email + * endpoints). + * + * Project routes authorized on generic events.view / events.edit with NO + * ownership check, so an editor could enumerate, read, update and aggregate + * projects belonging to other admins' events. The email endpoints keyed on an + * email_queue id alone, so any id could be previewed/resent/cancelled. + * + * `projects` had no owner column. It was added in migration 167 (backfilled + * from linked events) rather than relying only on the transitive + * events.project_id -> events.created_by path, because a brand-new EMPTY + * project has no linked event to infer an owner from — which is exactly where + * the create -> attach flow begins. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-projown-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'projown-test-secret'; + +const request = require('supertest'); +const express = require('express'); +const bcrypt = require('bcrypt'); +const jwt = require('jsonwebtoken'); + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +describe('project ownership (GHSA-wrg5 / GHSA-93x4)', () => { + let db; let cleanup; let app; + let editorToken; let superToken; let editorId; let superId; + let ownProjectId; let foreignProjectId; let foreignEventId; let foreignEmailId; + + const mkAdmin = async (username, roleName) => { + const role = await db('roles').where({ name: roleName }).first(); + const r = await db('admin_users').insert({ + username, + email: `${username}@example.com`, + password_hash: await bcrypt.hash('Passw0rd!', 4), + role_id: role.id, + is_active: 1, + created_at: new Date(), + updated_at: new Date(), + }).returning('id'); + const id = r[0]?.id ?? r[0]; + return { + id, + token: jwt.sign( + { id, username, type: 'admin', role: roleName, loginTime: Date.now() }, + process.env.JWT_SECRET, { expiresIn: '1h', issuer: 'picpeak-auth' }, + ), + }; + }; + + const mkProject = async (name, createdBy) => { + const r = await db('projects').insert({ + name, status: 'active', created_by: createdBy, + created_at: new Date(), updated_at: new Date(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + + await db('feature_flags').insert({ key: 'projects', value: 1 }) + .onConflict('key').merge({ value: 1 }); + + const editor = await mkAdmin('proj-editor', 'editor'); + const sup = await mkAdmin('proj-super', 'super_admin'); + editorToken = editor.token; editorId = editor.id; + superToken = sup.token; superId = sup.id; + + ownProjectId = await mkProject('own-project', editorId); + foreignProjectId = await mkProject('foreign-project', superId); + + // A foreign event linked to the foreign project, plus a queued email on it. + const ev = await db('events').insert({ + slug: 'foreign-ev', + event_type: 'wedding', + event_name: 'Foreign Event', + event_date: '2026-08-01', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + share_token: 'ftok', share_link: '/gallery/foreign-ev/ftok', + created_by: superId, + project_id: foreignProjectId, + expires_at: new Date(Date.now() + 7 * 864e5).toISOString(), + is_active: 1, is_archived: 0, is_draft: 0, + created_at: new Date().toISOString(), + }).returning('id'); + foreignEventId = ev[0]?.id ?? ev[0]; + + const em = await db('email_queue').insert({ + event_id: foreignEventId, + recipient_email: 'client@example.com', + email_type: 'gallery_created', + status: 'sent', + created_at: new Date().toISOString(), + }).returning('id'); + foreignEmailId = em[0]?.id ?? em[0]; + + app = express(); + app.use(express.json()); + app.use('/api/admin/projects', require('../../src/routes/adminProjects')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it('lists only the editor\'s own projects', async () => { + const res = await request(app) + .get('/api/admin/projects') + .set('Authorization', `Bearer ${editorToken}`); + + expect(res.status).toBe(200); + const names = (res.body.projects || res.body.data?.projects || []).map((p) => p.name); + expect(names).toContain('own-project'); + expect(names).not.toContain('foreign-project'); + }); + + it('refuses to read a foreign project', async () => { + const res = await request(app) + .get(`/api/admin/projects/${foreignProjectId}`) + .set('Authorization', `Bearer ${editorToken}`); + expect([403, 404]).toContain(res.status); + }); + + it('refuses to update or aggregate a foreign project', async () => { + const update = await request(app) + .put(`/api/admin/projects/${foreignProjectId}`) + .set('Authorization', `Bearer ${editorToken}`) + .send({ name: 'hijacked' }); + expect([403, 404]).toContain(update.status); + + const overview = await request(app) + .get(`/api/admin/projects/${foreignProjectId}/overview`) + .set('Authorization', `Bearer ${editorToken}`); + expect([403, 404]).toContain(overview.status); + + // And the name must not have changed. + const row = await db('projects').where({ id: foreignProjectId }).first(); + expect(row.name).toBe('foreign-project'); + }); + + it('refuses to attach a FOREIGN event to an owned project', async () => { + const res = await request(app) + .post(`/api/admin/projects/${ownProjectId}/events`) + .set('Authorization', `Bearer ${editorToken}`) + .send({ eventId: foreignEventId }); + + expect([403, 404]).toContain(res.status); + const ev = await db('events').where({ id: foreignEventId }).first(); + expect(ev.project_id).toBe(foreignProjectId); // still attached to its own + }); + + it('refuses to preview or act on a foreign queued email (GHSA-93x4)', async () => { + const preview = await request(app) + .get(`/api/admin/projects/email/${foreignEmailId}/preview`) + .set('Authorization', `Bearer ${editorToken}`); + expect([403, 404]).toContain(preview.status); + + const cancel = await request(app) + .post(`/api/admin/projects/email/${foreignEmailId}/cancel`) + .set('Authorization', `Bearer ${editorToken}`); + expect([403, 404]).toContain(cancel.status); + }); + + it('leaves super_admin unrestricted', async () => { + const res = await request(app) + .get(`/api/admin/projects/${foreignProjectId}`) + .set('Authorization', `Bearer ${superToken}`); + expect(res.status).toBe(200); + }); +}); diff --git a/backend/__tests__/routes/projectOwnershipEdgeCases.test.js b/backend/__tests__/routes/projectOwnershipEdgeCases.test.js new file mode 100644 index 00000000..12d1ca2e --- /dev/null +++ b/backend/__tests__/routes/projectOwnershipEdgeCases.test.js @@ -0,0 +1,115 @@ +/** + * Project ownership edge cases (GHSA-wrg5, codex round 2). + * + * The first predicate union'd "any linked event I can see" with the stored + * owner, which opened two holes: + * - a project owned by B containing ONE legacy ownerless event became + * readable by everyone (and /overview aggregates B's other events, + * invoices and emails); + * - migration 167 deliberately leaves multi-owner projects NULL, and a NULL + * owner was treated as "everyone's". + * The stored owner is now authoritative, and a NULL owner only derives access + * when EVERY linked event is accessible. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-projedge-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'projedge-test-secret'; + +const bcrypt3 = require('bcrypt'); +const { bootCrmDb: boot3, seedMinimal: seed3 } = require('../integration/helpers/crmDb'); + +describe('project ownership edge cases (GHSA-wrg5, round 2)', () => { + let db3; let cleanup3; let ownership; let editorA; let editorB; + + const mkAdmin3 = async (username, roleName) => { + const role = await db3('roles').where({ name: roleName }).first(); + const r = await db3('admin_users').insert({ + username, email: `${username}@example.com`, + password_hash: await bcrypt3.hash('Passw0rd!', 4), + role_id: role.id, is_active: 1, + created_at: new Date(), updated_at: new Date(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + const mkProject3 = async (name, createdBy) => { + const r = await db3('projects').insert({ + name, status: 'active', created_by: createdBy, + created_at: new Date(), updated_at: new Date(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + const mkEvent3 = async (slug, createdBy, projectId) => { + const r = await db3('events').insert({ + slug, event_type: 'wedding', event_name: slug, event_date: '2026-08-01', + host_email: 'h@e.com', admin_email: 'a@e.com', password_hash: 'x', + share_token: `t-${slug}`, share_link: `/g/${slug}/t-${slug}`, + created_by: createdBy, project_id: projectId, + expires_at: new Date(Date.now() + 864e5).toISOString(), + is_active: 1, is_archived: 0, is_draft: 0, + created_at: new Date().toISOString(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + + beforeAll(async () => { + ({ db: db3, cleanup: cleanup3 } = await boot3()); + await seed3(db3); + ownership = require('../../src/middleware/ownership'); + editorA = await mkAdmin3('edge-a', 'editor'); + editorB = await mkAdmin3('edge-b', 'editor'); + }, 120000); + + afterAll(async () => { if (cleanup3) await cleanup3(); }); + + it('one ownerless event in B\'s project does not expose it to A', async () => { + const pid = await mkProject3('b-project', editorB); + await mkEvent3('b-owned-ev', editorB, pid); + await mkEvent3('legacy-ev', null, pid); // ownerless legacy event + + const idsA = await ownership.ownedProjectIds({ id: editorA, roleName: 'editor' }); + expect(idsA).not.toContain(Number(pid)); + + const idsB = await ownership.ownedProjectIds({ id: editorB, roleName: 'editor' }); + expect(idsB).toContain(Number(pid)); + }); + + it('a mixed-owner project left NULL by migration 167 is not global', async () => { + const pid = await mkProject3('ambiguous', null); + await mkEvent3('mix-a-ev', editorA, pid); + await mkEvent3('mix-b-ev', editorB, pid); + + for (const who of [editorA, editorB]) { + const ids = await ownership.ownedProjectIds({ id: who, roleName: 'editor' }); + expect(ids).not.toContain(Number(pid)); + } + }); + + it('a NULL-owner project whose events are all mine IS mine', async () => { + const pid = await mkProject3('legacy-mine', null); + await mkEvent3('mine-ev', editorA, pid); + + const ids = await ownership.ownedProjectIds({ id: editorA, roleName: 'editor' }); + expect(ids).toContain(Number(pid)); + }); + + it('a project whose creator was deleted falls back to its events', async () => { + const ghost = await mkAdmin3('ghost-admin', 'editor'); + const pid = await mkProject3('orphaned', ghost); + await mkEvent3('orphan-ev', editorA, pid); + await db3('admin_users').where({ id: ghost }).del(); + + const ids = await ownership.ownedProjectIds({ id: editorA, roleName: 'editor' }); + expect(ids).toContain(Number(pid)); + }); + + it('super_admin stays unrestricted', async () => { + expect(await ownership.ownedProjectIds({ id: 1, roleName: 'super_admin' })).toBeNull(); + }); +}); diff --git a/backend/__tests__/services/projectDealLineageOwnership.test.js b/backend/__tests__/services/projectDealLineageOwnership.test.js new file mode 100644 index 00000000..e11a9e46 --- /dev/null +++ b/backend/__tests__/services/projectDealLineageOwnership.test.js @@ -0,0 +1,138 @@ +/** + * Deal-lineage ownership on project attach (GHSA-wrg5, codex round 3). + * + * requireProjectOwnership vets only the DESTINATION project. Attaching a quote + * cascades through linkDealToProject, which re-points every event the deal + * produced into that project — so an editor could create an empty project of + * their own, attach another admin's quote, and pull that admin's events (and + * the invoices, emails and gallery that roll up with them) into a project they + * own and can read via /:id/overview. An unassigned project offered no + * resistance either: it ADOPTS the deal's customer rather than rejecting it. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-deallineage-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'deallineage-test-secret'; + +const bcrypt = require('bcrypt'); +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +describe('linkDealToProject enforces lineage ownership (GHSA-wrg5, round 3)', () => { + let db; let cleanup; let projectService; + let editorA; let editorB; let superAdmin; + let customerId; + + const mkAdmin = async (username, roleName) => { + const role = await db('roles').where({ name: roleName }).first(); + const r = await db('admin_users').insert({ + username, email: `${username}@example.com`, + password_hash: await bcrypt.hash('Passw0rd!', 4), + role_id: role.id, is_active: 1, + created_at: new Date(), updated_at: new Date(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + const mkProject = async (name, createdBy) => { + const r = await db('projects').insert({ + name, status: 'active', created_by: createdBy, + created_at: new Date(), updated_at: new Date(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + const mkEvent = async (slug, createdBy) => { + const r = await db('events').insert({ + slug, event_type: 'wedding', event_name: slug, event_date: '2026-08-01', + host_email: 'h@e.com', admin_email: 'a@e.com', password_hash: 'x', + share_token: `t-${slug}`, share_link: `/g/${slug}/t-${slug}`, + created_by: createdBy, + expires_at: new Date(Date.now() + 864e5).toISOString(), + is_active: 1, is_archived: 0, is_draft: 0, + created_at: new Date().toISOString(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + const mkQuote = async (dealUuid, convertedEventId) => { + const r = await db('quotes').insert({ + quote_number: `Q-${dealUuid}`, + customer_account_id: customerId, + deal_uuid: dealUuid, + converted_event_id: convertedEventId, + status: 'accepted', + currency: 'EUR', + issue_date: '2026-08-01', + total_amount_minor: 1000, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + projectService = require('../../src/services/projectService'); + editorA = await mkAdmin('deal-a', 'editor'); + editorB = await mkAdmin('deal-b', 'editor'); + superAdmin = await mkAdmin('deal-root', 'super_admin'); + const c = await db('customer_accounts').first('id'); + customerId = c.id; + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it("refuses to move another admin's event into the caller's project", async () => { + const victimEvent = await mkEvent('victim-gala', editorB); + const quoteId = await mkQuote('deal-foreign', victimEvent); + const attackerProject = await mkProject('attacker-empty', editorA); + + await expect( + projectService.assignQuote(attackerProject, quoteId, { id: editorA, roleName: 'editor' }), + ).rejects.toMatchObject({ code: 'DEAL_EVENT_FORBIDDEN' }); + + // Nothing may be half-applied: neither the event nor the quote moved. + const ev = await db('events').where({ id: victimEvent }).first('project_id'); + expect(ev.project_id == null).toBe(true); + const q = await db('quotes').where({ id: quoteId }).first('project_id'); + expect(q.project_id == null).toBe(true); + }); + + it("allows the caller's own event through the same path", async () => { + const ownEvent = await mkEvent('own-gala', editorA); + const quoteId = await mkQuote('deal-own', ownEvent); + const project = await mkProject('attacker-own', editorA); + + await projectService.assignQuote(project, quoteId, { id: editorA, roleName: 'editor' }); + + const ev = await db('events').where({ id: ownEvent }).first('project_id'); + expect(Number(ev.project_id)).toBe(Number(project)); + }); + + it('leaves super_admin unrestricted', async () => { + const victimEvent = await mkEvent('root-gala', editorB); + const quoteId = await mkQuote('deal-root', victimEvent); + const project = await mkProject('root-project', superAdmin); + + await projectService.assignQuote(project, quoteId, { id: superAdmin, roleName: 'super_admin' }); + + const ev = await db('events').where({ id: victimEvent }).first('project_id'); + expect(Number(ev.project_id)).toBe(Number(project)); + }); + + it('resolves the role from a bare admin id (quote/contract create+update paths)', async () => { + // Those services thread `adminId`, not req.admin — the lookup must still + // scope them, and must fail closed rather than assume super_admin. + const victimEvent = await mkEvent('bare-gala', editorB); + const quoteId = await mkQuote('deal-bare', victimEvent); + const project = await mkProject('bare-project', editorA); + + await expect( + projectService.assignQuote(project, quoteId, { id: editorA }), + ).rejects.toMatchObject({ code: 'DEAL_EVENT_FORBIDDEN' }); + }); +}); diff --git a/backend/migrations/core/167_add_projects_created_by.js b/backend/migrations/core/167_add_projects_created_by.js new file mode 100644 index 00000000..c50a2609 --- /dev/null +++ b/backend/migrations/core/167_add_projects_created_by.js @@ -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')); + } +}; diff --git a/backend/src/middleware/ownership.js b/backend/src/middleware/ownership.js index 713d2b19..1a04e0b3 100644 --- a/backend/src/middleware/ownership.js +++ b/backend/src/middleware/ownership.js @@ -78,4 +78,88 @@ async function filterOwnedEventIds(admin, eventIds) { return { allowed, denied }; } -module.exports = { requireEventOwnership, filterOwnedEventIds, scopeEventsQuery }; +/** + * Knex subquery selecting the ids of projects `admin` may act on, or `null` + * when the caller is unrestricted (GHSA-wrg5). + * + * Rules, in priority order: + * 1. A project's STORED owner is authoritative. If `projects.created_by` is + * set to a live admin, only that admin (and super_admin) may act on it. + * Earlier this union'd in "any linked event I can see", which meant one + * legacy ownerless event inside another admin's project exposed the whole + * project — its other events, invoices and emails — through the overview. + * 2. Only when there is NO usable stored owner (NULL, or pointing at a + * deleted admin) do we derive from linked events, and then EVERY linked + * event must be accessible: a project the old unrestricted routes filled + * with several admins' events is ambiguous, and migration 167 deliberately + * leaves those NULL. Granting on "any" would have made exactly those + * mixed projects readable by everyone. + * 3. A project with no usable owner AND no linked events (an orphan — not + * creatable since createProject stamps created_by) stays super_admin-only. + * Failing closed beats failing open; a super_admin can reassign it. + * + * Returned as a subquery so callers avoid materialising an id list. + */ +function ownedProjectsSubquery(admin) { + if (admin?.roleName === 'super_admin') return null; + + const linkedEvents = () => db('events').select(db.raw('1')).whereRaw('events.project_id = projects.id'); + + return db('projects').select('projects.id').where((w) => { + w.where('projects.created_by', admin.id) + .orWhere((noOwner) => { + noOwner + // No usable stored owner: NULL, or a creator that no longer exists + // (hard-deleted admin) — otherwise that project would be locked away + // from everyone but super_admin forever. + .where((c) => c + .whereNull('projects.created_by') + .orWhereNotIn('projects.created_by', db('admin_users').select('id'))) + .whereExists(linkedEvents()) + .whereNotExists( + linkedEvents().whereNotNull('events.created_by').whereNot('events.created_by', admin.id), + ); + }); + }); +} + +/** + * Materialised form of ownedProjectsSubquery, for callers that need the ids + * themselves. `null` = unrestricted. + * + * @returns {Promise} + */ +async function ownedProjectIds(admin) { + const sub = ownedProjectsSubquery(admin); + if (sub === null) return null; + const rows = await sub; + return rows.map((r) => Number(r.id)); +} + +/** + * Middleware enforcing ownedProjectIds() on a :id project route. 404 (not 403) + * on a foreign project so the endpoint isn't an existence oracle — same + * posture filterOwnedEventIds takes for foreign-vs-missing ids. + */ +function requireProjectOwnership(req, res, next) { + const sub = ownedProjectsSubquery(req.admin); + if (sub === null) return next(); + const projectId = Number(req.params.id); + sub.clone() + .where('projects.id', projectId) + .first() + .then((row) => { + if (!row) return res.status(404).json({ error: 'Project not found' }); + next(); + }) + .catch(() => res.status(500).json({ error: 'Failed to verify project ownership' })); +} + +module.exports = { + requireEventOwnership, + filterOwnedEventIds, + scopeEventsQuery, + ownedProjectIds, + ownedProjectsSubquery, + requireProjectOwnership, +}; diff --git a/backend/src/routes/adminProjects.js b/backend/src/routes/adminProjects.js index 3c335e65..2c4abcc4 100644 --- a/backend/src/routes/adminProjects.js +++ b/backend/src/routes/adminProjects.js @@ -15,6 +15,7 @@ const { requirePermission, userHasAnyPermission } = require('../middleware/permi const { handleAsync, validateRequest, successResponse } = require('../utils/routeHelpers'); const projectService = require('../services/projectService'); const { db } = require('../database/db'); +const { ownedProjectsSubquery, requireProjectOwnership, filterOwnedEventIds } = require('../middleware/ownership'); const { ForbiddenError } = require('../utils/errors'); const router = express.Router(); @@ -65,10 +66,15 @@ router.get('/', requirePermission('events.view'), handleAsync(async (req, res) = bills: await userHasAnyPermission(req.admin.id, ['bills.view']), quotes: await userHasAnyPermission(req.admin.id, ['quotes.view']), }; + // Only the caller's projects (GHSA-wrg5). Passed as a SUBQUERY so a large + // project count can't hit the driver's bind-parameter limit; null means + // unrestricted. + const projectIds = ownedProjectsSubquery(req.admin); const projects = await projectService.listProjects({ search: req.query.q || '', status: req.query.status || null, perms, + projectIds, }); return successResponse(res, { projects }); })); @@ -88,7 +94,7 @@ router.post('/', ); // Detail -router.get('/:id', requirePermission('events.view'), [param('id').isInt({ min: 1 })], handleAsync(async (req, res) => { +router.get('/:id', requirePermission('events.view'), requireProjectOwnership, [param('id').isInt({ min: 1 })], handleAsync(async (req, res) => { validateRequest(req); const project = await projectService.getProjectById(parseInt(req.params.id, 10)); if (!project) return res.status(404).json({ error: 'Project not found' }); @@ -98,6 +104,7 @@ router.get('/:id', requirePermission('events.view'), [param('id').isInt({ min: 1 // Update router.put('/:id', requirePermission('events.edit'), + requireProjectOwnership, [ param('id').isInt({ min: 1 }), body('name').optional().isString().trim().isLength({ min: 1, max: 255 }), @@ -119,10 +126,20 @@ router.put('/:id', // Attach an event to the project router.post('/:id/events', requirePermission('events.edit'), + requireProjectOwnership, [param('id').isInt({ min: 1 }), body('eventId').isInt({ min: 1 })], handleAsync(async (req, res) => { validateRequest(req); - const result = await projectService.assignEvent(parseInt(req.params.id, 10), parseInt(req.body.eventId, 10)); + const eventId = parseInt(req.body.eventId, 10); + // Both sides must be the caller's (GHSA-wrg5): requireProjectOwnership + // covers the project, this covers the INCOMING event. Otherwise an editor + // could pull a foreign event into a project they own and then read that + // event's rolled-up documents through /:id/overview. + const { denied } = await filterOwnedEventIds(req.admin, [eventId]); + if (denied.length) { + return res.status(403).json({ error: 'That event is not yours to attach' }); + } + const result = await projectService.assignEvent(parseInt(req.params.id, 10), eventId); return successResponse(res, result, 200, 'Event attached to project'); }), ); @@ -132,12 +149,13 @@ router.post('/:id/events', // mutates a separately-permissioned document domain (GHSA-v4vw). router.post('/:id/quotes', requirePermission(['events.edit', 'quotes.manage'], { requireAll: true }), + requireProjectOwnership, [param('id').isInt({ min: 1 }), body('quoteId').isInt({ min: 1 })], handleAsync(async (req, res) => { validateRequest(req); const quoteId = parseInt(req.body.quoteId, 10); await assertCascadePermitted(req, 'quotes', quoteId, 'contracts', 'contracts.manage'); - const result = await projectService.assignQuote(parseInt(req.params.id, 10), quoteId); + const result = await projectService.assignQuote(parseInt(req.params.id, 10), quoteId, req.admin); return successResponse(res, result, 200, 'Quote attached to project'); }), ); @@ -146,18 +164,19 @@ router.post('/:id/quotes', // to events.edit (GHSA-v4vw). router.post('/:id/contracts', requirePermission(['events.edit', 'contracts.manage'], { requireAll: true }), + requireProjectOwnership, [param('id').isInt({ min: 1 }), body('contractId').isInt({ min: 1 })], handleAsync(async (req, res) => { validateRequest(req); const contractId = parseInt(req.body.contractId, 10); await assertCascadePermitted(req, 'contracts', contractId, 'quotes', 'quotes.manage'); - const result = await projectService.assignContract(parseInt(req.params.id, 10), contractId); + const result = await projectService.assignContract(parseInt(req.params.id, 10), contractId, req.admin); return successResponse(res, result, 200, 'Contract attached to project'); }), ); // The cockpit aggregation — doc types gated on the admin's own permissions -router.get('/:id/overview', requirePermission('events.view'), [param('id').isInt({ min: 1 })], handleAsync(async (req, res) => { +router.get('/:id/overview', requirePermission('events.view'), requireProjectOwnership, [param('id').isInt({ min: 1 })], handleAsync(async (req, res) => { validateRequest(req); const perms = { bills: await userHasAnyPermission(req.admin.id, ['bills.view']), @@ -168,8 +187,37 @@ router.get('/:id/overview', requirePermission('events.view'), [param('id').isInt return successResponse(res, overview); })); +/** + * These routes key on an `email_queue` id alone (GHSA-93x4) — nothing tied the + * row to a project or event the caller can see, so any admin holding + * `events.view` / `email.send` could preview, resend, cancel or retry ANY + * queued mail on the instance by walking ids. + * + * Scoped via `email_queue.event_id` → the caller's owned events. `event_id` is + * NULL for CRM document mail (quote/contract/invoice sends carry no event), and + * those rows have no ownable parent here, so a scoped caller is denied them + * rather than guessed into access. 404, not 403, so this isn't an id oracle. + */ +async function requireOwnedQueuedEmail(req, res, next) { + try { + if (req.admin?.roleName === 'super_admin') return next(); + const emailId = parseInt(req.params.emailId, 10); + const row = await db('email_queue').where({ id: emailId }).first('event_id'); + if (!row || !row.event_id) { + return res.status(404).json({ error: 'Email not found' }); + } + const { denied } = await filterOwnedEventIds(req.admin, [row.event_id]); + if (denied.length) { + return res.status(404).json({ error: 'Email not found' }); + } + return next(); + } catch (err) { + return next(err); + } +} + // Email preview — the ACTUAL sent HTML (or null for pre-rendered_html rows) -router.get('/email/:emailId/preview', requirePermission('events.view'), [param('emailId').isInt({ min: 1 })], handleAsync(async (req, res) => { +router.get('/email/:emailId/preview', requirePermission('events.view'), [param('emailId').isInt({ min: 1 })], requireOwnedQueuedEmail, handleAsync(async (req, res) => { validateRequest(req); const preview = await projectService.getEmailPreview(parseInt(req.params.emailId, 10)); return successResponse(res, preview); @@ -182,9 +230,9 @@ const emailAction = (fn) => handleAsync(async (req, res) => { const result = await projectService[fn](parseInt(req.params.emailId, 10), req.admin.id); return successResponse(res, result); }); -router.post('/email/:emailId/resend', requirePermission('email.send'), [param('emailId').isInt({ min: 1 })], emailAction('resendEmail')); -router.post('/email/:emailId/cancel', requirePermission('email.send'), [param('emailId').isInt({ min: 1 })], emailAction('cancelEmail')); -router.post('/email/:emailId/retry', requirePermission('email.send'), [param('emailId').isInt({ min: 1 })], emailAction('retryEmail')); -router.post('/email/:emailId/send-now', requirePermission('email.send'), [param('emailId').isInt({ min: 1 })], emailAction('sendEmailNow')); +router.post('/email/:emailId/resend', requirePermission('email.send'), [param('emailId').isInt({ min: 1 })], requireOwnedQueuedEmail, emailAction('resendEmail')); +router.post('/email/:emailId/cancel', requirePermission('email.send'), [param('emailId').isInt({ min: 1 })], requireOwnedQueuedEmail, emailAction('cancelEmail')); +router.post('/email/:emailId/retry', requirePermission('email.send'), [param('emailId').isInt({ min: 1 })], requireOwnedQueuedEmail, emailAction('retryEmail')); +router.post('/email/:emailId/send-now', requirePermission('email.send'), [param('emailId').isInt({ min: 1 })], requireOwnedQueuedEmail, emailAction('sendEmailNow')); module.exports = router; diff --git a/backend/src/services/contract/crud.js b/backend/src/services/contract/crud.js index f5d834f5..42170142 100644 --- a/backend/src/services/contract/crud.js +++ b/backend/src/services/contract/crud.js @@ -205,7 +205,7 @@ async function createContract(payload, adminId) { } const inserted = await trx('contracts').insert(row).returning('id'); if (row.project_id && row.deal_uuid) { - await require('../projectService').linkDealToProject(row.deal_uuid, row.project_id, trx); + await require('../projectService').linkDealToProject(row.deal_uuid, row.project_id, trx, { id: adminId }); } const contractId = typeof inserted[0] === 'object' ? inserted[0].id : inserted[0]; @@ -302,7 +302,7 @@ async function updateContract(id, payload, adminId) { // Cascade across the deal lineage (linked quote / event / invoices). if (updates.project_id) { const dealRow = await trx('contracts').where({ id }).select('deal_uuid').first(); - await require('../projectService').linkDealToProject(dealRow && dealRow.deal_uuid, updates.project_id, trx); + await require('../projectService').linkDealToProject(dealRow && dealRow.deal_uuid, updates.project_id, trx, { id: adminId }); } // Replace inclusions only when the caller sent an explicit list. diff --git a/backend/src/services/projectService.js b/backend/src/services/projectService.js index 057bd7ab..f470f5e0 100644 --- a/backend/src/services/projectService.js +++ b/backend/src/services/projectService.js @@ -34,7 +34,7 @@ function transformProject(p) { /** List projects with customer email + event count + rolled-up value. * `perms` gates which document types feed the value (matches the cockpit): * invoices need bills.view, quotes need quotes.view. */ -async function listProjects({ search = '', status = null, perms = {} } = {}) { +async function listProjects({ search = '', status = null, perms = {}, projectIds = null } = {}) { let q = db('projects') .leftJoin('customer_accounts', 'customer_accounts.id', 'projects.customer_account_id') .select( @@ -43,6 +43,11 @@ async function listProjects({ search = '', status = null, perms = {} } = {}) { db('events').count('* as c').whereRaw('events.project_id = projects.id').as('event_count'), ) .orderBy('projects.updated_at', 'desc'); + // Ownership allowlist (GHSA-wrg5). `null` = unrestricted; otherwise a knex + // SUBQUERY of allowed ids (a plain array also works). The subquery keeps a + // large project count off the driver's bind-parameter limit, and correctly + // yields no rows for an admin who owns nothing. + if (projectIds !== null) q = q.whereIn('projects.id', projectIds); if (status) q = q.where('projects.status', status); if (search) { q = q.where(function () { @@ -130,13 +135,21 @@ async function getProjectById(id) { async function createProject({ name, customerAccountId = null }, adminId) { if (!name || !String(name).trim()) throw new AppError('Project name is required', 400); - const inserted = await db('projects').insert({ + const row = { name: String(name).trim(), customer_account_id: customerAccountId || null, status: 'active', created_at: new Date(), updated_at: new Date(), - }).returning('id'); + }; + // Record the owner (GHSA-wrg5). adminId was already passed in and silently + // discarded, which left a brand-new empty project with no derivable owner — + // it has no linked events to infer one from yet. Guarded so an instance that + // has not run migration 167 still creates projects. + if (adminId && await hasColumnCached('projects', 'created_by')) { + row.created_by = adminId; + } + const inserted = await db('projects').insert(row).returning('id'); const id = (inserted[0] && typeof inserted[0] === 'object') ? inserted[0].id : inserted[0]; return getProjectById(id); } @@ -222,6 +235,26 @@ async function assignEvent(projectId, eventId) { return { projectId, eventId }; } +/** + * Resolve `actor.roleName`, looking it up when the caller only had an admin id + * to hand (the quote/contract create+update paths thread `adminId`, not the + * full req.admin). Fails CLOSED — an unresolvable role is treated as scoped, + * never as super_admin. + */ +async function isSuperAdmin(actor, conn = db) { + if (!actor) return false; + if (actor.roleName !== undefined) return actor.roleName === 'super_admin'; + try { + const row = await conn('admin_users') + .leftJoin('roles', 'roles.id', 'admin_users.role_id') + .where('admin_users.id', actor.id) + .first('roles.name as role_name'); + return row?.role_name === 'super_admin'; + } catch (err) { + return false; + } +} + /** * Cascade a project link across a whole deal's lineage. Given a deal_uuid, link * every quote + contract in that deal to the project, re-point every event the @@ -229,8 +262,12 @@ async function assignEvent(projectId, eventId) { * adopt the deal's customer onto the project when it has none. This is what * makes "drop a quote on an empty project" fill the cockpit with the linked * contract, event and invoices. Idempotent; pass a trx to run inside a txn. + * + * `actor` (req.admin) enables the ownership guard below and MUST be supplied by + * any admin-facing caller — route-level project ownership only vets the + * destination, while this function re-points the deal's events into it. */ -async function linkDealToProject(dealUuid, projectId, conn = db) { +async function linkDealToProject(dealUuid, projectId, conn = db, actor = null) { if (!dealUuid || !projectId) return; // Collect ALL the deal's customers across its quote/contract/invoice lineage @@ -273,6 +310,31 @@ async function linkDealToProject(dealUuid, projectId, conn = db) { throw new AppError('That belongs to a different customer than this project', 422, 'PROJECT_CUSTOMER_MISMATCH'); } + // Ownership of the LINEAGE, not just the destination (GHSA-wrg5). The route + // guard (requireProjectOwnership) only vets `projectId`; the writes below + // re-point every event this deal produced into it. Without this check an + // editor could create an empty project, 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. + // An unassigned project offers no resistance either, since it ADOPTS the + // deal's customer below rather than rejecting it. + // + // Events are the only ownership signal a deal carries: quotes/contracts have + // no created_by in this schema, so a deal whose lineage produced no event + // still cannot be attributed to an admin — a pre-existing property of the CRM + // model, not something this guard can close. + if (actor?.id && eventIds.size && !(await isSuperAdmin(actor, conn))) { + const ownable = await conn('events') + .whereIn('id', Array.from(eventIds)) + .andWhere((q) => q.whereNull('created_by').orWhere('created_by', actor.id)) + .pluck('id'); + if (ownable.length !== eventIds.size) { + throw new AppError( + 'That deal includes events that are not yours to move', 403, 'DEAL_EVENT_FORBIDDEN', + ); + } + } + // Cleared to write: link the deal's quotes/contracts, re-point its events so // invoices/emails/gallery roll up automatically. if (quotesHaveDeal && await hasColumnCached('quotes', 'project_id')) { @@ -294,7 +356,7 @@ async function linkDealToProject(dealUuid, projectId, conn = db) { /** Attach (or, with projectId=null, detach) a quote/contract to a project. * Attaching cascades the link across the deal lineage (see linkDealToProject). */ -async function assignDocument(table, projectId, documentId) { +async function assignDocument(table, projectId, documentId, actor = null) { if (!(await hasColumnCached(table, 'project_id'))) { throw new AppError('This instance has no project_id column yet — run migrations', 409); } @@ -317,15 +379,21 @@ async function assignDocument(table, projectId, documentId) { ) { throw new AppError('That belongs to a different customer than this project', 422, 'PROJECT_CUSTOMER_MISMATCH'); } - await db(table).where({ id: documentId }).update({ project_id: projectId || null }); + // Cascade FIRST, then stamp this document. linkDealToProject runs the + // lineage-ownership guard and throws before it writes anything, so a refused + // attach leaves no half-applied link behind — the other order committed the + // foreign document into the caller's project and only then refused the + // cascade. It already stamps this row's project_id via the deal_uuid sweep; + // the update below covers the standalone (no-deal) document. if (projectId && doc.deal_uuid) { - await linkDealToProject(doc.deal_uuid, projectId); + await linkDealToProject(doc.deal_uuid, projectId, db, actor); } + await db(table).where({ id: documentId }).update({ project_id: projectId || null }); return { projectId: projectId || null, documentId }; } -const assignQuote = (projectId, quoteId) => assignDocument('quotes', projectId, quoteId); -const assignContract = (projectId, contractId) => assignDocument('contracts', projectId, contractId); +const assignQuote = (projectId, quoteId, actor) => assignDocument('quotes', projectId, quoteId, actor); +const assignContract = (projectId, contractId, actor) => assignDocument('contracts', projectId, contractId, actor); /** * Project valuation — "newest stage wins per deal, cumulative across events". diff --git a/backend/src/services/quoteService.js b/backend/src/services/quoteService.js index 66c83291..fdc8f66d 100644 --- a/backend/src/services/quoteService.js +++ b/backend/src/services/quoteService.js @@ -612,7 +612,7 @@ async function createQuote(payload, adminId) { // quote with no contract/event yet — just adopts the customer onto an // empty project). if (row.project_id) { - await require('./projectService').linkDealToProject(row.deal_uuid, row.project_id, trx); + await require('./projectService').linkDealToProject(row.deal_uuid, row.project_id, trx, { id: adminId }); } if (totals.lineItems.length > 0) { @@ -751,7 +751,7 @@ async function updateQuote(id, payload, adminId) { // contract / event / invoices roll up into the same project automatically. if (updates.project_id) { const dealRow = await trx('quotes').where({ id }).select('deal_uuid').first(); - await require('./projectService').linkDealToProject(dealRow && dealRow.deal_uuid, updates.project_id, trx); + await require('./projectService').linkDealToProject(dealRow && dealRow.deal_uuid, updates.project_id, trx, { id: adminId }); } // Delete + reinsert keeps the editor flow simple: the frontend