diff --git a/backend/__tests__/services/projectDealLineageOwnership.test.js b/backend/__tests__/services/projectDealLineageOwnership.test.js index e11a9e46..4a5ad422 100644 --- a/backend/__tests__/services/projectDealLineageOwnership.test.js +++ b/backend/__tests__/services/projectDealLineageOwnership.test.js @@ -135,4 +135,95 @@ describe('linkDealToProject enforces lineage ownership (GHSA-wrg5, round 3)', () projectService.assignQuote(project, quoteId, { id: editorA }), ).rejects.toMatchObject({ code: 'DEAL_EVENT_FORBIDDEN' }); }); + + // The lineage guard above only fires once a deal has produced an event. The + // quote/contract create+update paths call linkDealToProject with a + // body-supplied projectId and NO route-level ownership guard, so a brand-new + // deal (eventIds empty) skipped every check and wrote into a foreign project. + describe('destination ownership (codex review follow-up)', () => { + it('refuses a foreign project even when the deal has no events yet', async () => { + const victimProject = await mkProject('victim-destination', editorB); + const quoteId = await mkQuote('deal-no-events', null); + + await expect( + projectService.linkDealToProject('deal-no-events', victimProject, db, { id: editorA }), + ).rejects.toMatchObject({ code: 'PROJECT_NOT_FOUND' }); + + const q = await db('quotes').where({ id: quoteId }).first('project_id'); + expect(q.project_id == null).toBe(true); + }); + + it('refuses an OWNERLESS project with no events (the escalation path)', async () => { + // created_by NULL + no linked events is exactly the shape that would let + // the caller claim the project via ownedProjectsSubquery's second branch + // once their quote converts to an event. + const orphan = await mkProject('orphan-destination', null); + await mkQuote('deal-orphan', null); + + await expect( + projectService.linkDealToProject('deal-orphan', orphan, db, { id: editorA }), + ).rejects.toMatchObject({ code: 'PROJECT_NOT_FOUND' }); + }); + + it("still allows the caller's own project with no events", async () => { + const own = await mkProject('own-destination', editorA); + const quoteId = await mkQuote('deal-own-dest', null); + + await projectService.linkDealToProject('deal-own-dest', own, db, { id: editorA }); + + const q = await db('quotes').where({ id: quoteId }).first('project_id'); + expect(Number(q.project_id)).toBe(Number(own)); + }); + + it('refuses a foreign project when the deal_uuid is NULL (codex round 1)', async () => { + // deal_uuid is nullable (migration 107) and quoteService.update passes the + // EXISTING row's value, so a legacy quote reaches linkDealToProject with + // null. The old `if (!dealUuid || !projectId) return` bailed before the + // guard — while the caller had already written project_id onto its row. + const victimProject = await mkProject('victim-nulldeal', editorB); + + await expect( + projectService.linkDealToProject(null, victimProject, db, { id: editorA }), + ).rejects.toMatchObject({ code: 'PROJECT_NOT_FOUND' }); + }); + + it('still no-ops on a NULL deal_uuid pointed at the caller-s own project', async () => { + // The destination is vetted, then it returns without cascading — there is + // no lineage to move. + const own = await mkProject('own-nulldeal', editorA); + await expect( + projectService.linkDealToProject(null, own, db, { id: editorA }), + ).resolves.toBeUndefined(); + }); + + it('does not leak customer association through the error code', async () => { + // The customer check used to run first, so a foreign project whose + // customer differed answered 422 PROJECT_CUSTOMER_MISMATCH while an + // unknown id answered 404 — enough to enumerate projects and infer their + // customer. Both must now be indistinguishable to a scoped caller. + const foreignWithCustomer = await mkProject('victim-customer', editorB); + await db('projects').where({ id: foreignWithCustomer }).update({ customer_account_id: customerId }); + await mkQuote('deal-oracle', null); + + await expect( + projectService.linkDealToProject('deal-oracle', foreignWithCustomer, db, { id: editorA }), + ).rejects.toMatchObject({ code: 'PROJECT_NOT_FOUND' }); + + await expect( + projectService.linkDealToProject('deal-oracle', 999999, db, { id: editorA }), + ).rejects.toMatchObject({ code: 'PROJECT_NOT_FOUND' }); + }); + + it('leaves super_admin unrestricted on a foreign destination', async () => { + const victimProject = await mkProject('root-destination', editorB); + const quoteId = await mkQuote('deal-root-dest', null); + + await projectService.linkDealToProject('deal-root-dest', victimProject, db, { + id: superAdmin, roleName: 'super_admin', + }); + + const q = await db('quotes').where({ id: quoteId }).first('project_id'); + expect(Number(q.project_id)).toBe(Number(victimProject)); + }); + }); }); diff --git a/backend/src/services/projectService.js b/backend/src/services/projectService.js index a0334dc2..bdcc88a0 100644 --- a/backend/src/services/projectService.js +++ b/backend/src/services/projectService.js @@ -268,7 +268,61 @@ async function isSuperAdmin(actor, conn = db) { * destination, while this function re-points the deal's events into it. */ async function linkDealToProject(dealUuid, projectId, conn = db, actor = null) { - if (!dealUuid || !projectId) return; + if (!projectId) return; + + // Ownership of the DESTINATION. `attachDocumentToProject` reaches here behind + // requireProjectOwnership, but the quote/contract create+update paths do not: + // adminQuotes.js / adminContracts.js take `projectId` straight from the body + // behind `quotes.manage` / `contracts.manage`, which are permissions, not + // ownership. So the destination has to be vetted here, at the one choke point + // every caller shares, rather than relying on a route guard three of the four + // callers never had. + // + // Without it a scoped admin could point a new quote at a project they do not + // own: the lineage check below is skipped when the deal has produced no event + // yet (`eventIds.size` is 0), and an unassigned project ADOPTS the deal's + // customer instead of rejecting it. That writes their document into another + // admin's cockpit, and on an OWNERLESS project (created_by IS NULL — legacy + // rows migration 167's backfill could not attribute) it escalates: once the + // quote converts to an event, that event becomes the project's only linked + // event, which is exactly the condition ownedProjectsSubquery's second branch + // grants ownership on — handing the caller read access to whatever documents + // were already attached there. + // + // Mirrors ownedProjectsSubquery (middleware/ownership.js) rather than calling + // it, because that helper binds the module-level `db` and this runs inside the + // caller's transaction. + if (actor?.id && !(await isSuperAdmin(actor, conn))) { + const owned = await conn('projects') + .where({ id: projectId }) + .where((w) => { + w.where('created_by', actor.id) + .orWhere((noOwner) => { + noOwner + .where((c) => c + .whereNull('created_by') + .orWhereNotIn('created_by', conn('admin_users').select('id'))) + .whereExists( + conn('events').select(conn.raw('1')).whereRaw('events.project_id = projects.id'), + ) + .whereNotExists( + conn('events').select(conn.raw('1')).whereRaw('events.project_id = projects.id') + .whereNotNull('events.created_by').whereNot('events.created_by', actor.id), + ); + }); + }) + .first('id'); + if (!owned) { + throw new AppError('Project not found', 404, 'PROJECT_NOT_FOUND'); + } + } + + // Nothing to cascade without a deal, but the destination above still had + // to be vetted: every caller writes `project_id` onto its own row BEFORE + // calling us, and `deal_uuid` is nullable (migration 107). A legacy quote + // with no deal would otherwise return here having bypassed the check while + // its foreign project link stood. + if (!dealUuid) return; // Collect ALL the deal's customers across its quote/contract/invoice lineage // AND every event it converted into — BEFORE mutating anything, so a link @@ -310,9 +364,8 @@ async function linkDealToProject(dealUuid, projectId, conn = db, actor = null) { 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 + // Ownership of the LINEAGE, not just the destination (GHSA-wrg5). 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.