From 0c8ad6bbedb00ba443c20c7ab00b58925d6b9b5c Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Tue, 4 Aug 2026 16:36:12 +0200 Subject: [PATCH] fix(security): vet the destination project when linking a deal (#991) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit linkDealToProject re-points a deal's quotes, contracts and events into `projectId`. Its lineage guard vets the SOURCE events and its comment assumed the route had vetted the destination — true only for attachDocumentToProject. quoteService.create/update and contract crud.create/update take `projectId` straight from the request body behind quotes.manage / contracts.manage, which are permissions, not ownership; adminQuotes.js and adminContracts.js carry no ownership guard at all. The lineage guard did not cover it: it is skipped when the deal has produced no event yet, which is the state of a newly created quote, and an unassigned destination ADOPTS the deal's customer rather than rejecting it. A scoped admin could therefore write into another admin's project, and on an OWNERLESS project (created_by IS NULL — legacy rows migration 167 could not attribute) escalate to a read: once the quote converts to an event it becomes the project's only linked event, which is the condition ownedProjectsSubquery's second branch grants ownership on. Vetted at the service choke point all four callers share, ahead of both the null-deal early return (callers write project_id before calling, and deal_uuid is nullable) and the customer check (whose 422 vs 404 was an enumeration oracle). 404 PROJECT_NOT_FOUND throughout. super_admin unaffected. --- .../projectDealLineageOwnership.test.js | 91 +++++++++++++++++++ backend/src/services/projectService.js | 61 ++++++++++++- 2 files changed, 148 insertions(+), 4 deletions(-) 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.