fix(security): vet the destination project when linking a deal (stable) (#992)
Backport of #991. stable carried the identical code path and the same missing guards. A scoped admin could point a quote or contract at a project they do not own — the quote/contract create+update paths pass a body-supplied projectId with no ownership check, and linkDealToProject's lineage guard is skipped when the deal has no event yet. On an ownerless project this escalated to a read once the quote converted to an event. Vetted at the service choke point, ahead of both the null-deal early return and the customer check. 404 PROJECT_NOT_FOUND throughout. super_admin unaffected.
This commit is contained in:
Paul Nothaft
@@ -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));
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user