From cdbd37c7cec9a96953a35fa053d1735c60bd30a3 Mon Sep 17 00:00:00 2001 From: Tiago <1585571+xernobyl@users.noreply.github.com> Date: Tue, 21 Jul 2026 12:50:50 +0000 Subject: [PATCH] fix: enforce organization/workspace scoping across API keys, surveys, quotas, and integrations (ENG-1749) (#8596) --- .../google-sheets/actions.test.ts | 59 +++++++ .../integrations/google-sheets/actions.ts | 9 + apps/web/app/api/v1/auth.test.ts | 4 +- apps/web/app/api/v1/management/me/route.ts | 17 +- apps/web/lib/survey/service.test.ts | 164 +++++++++++++++++- apps/web/lib/survey/service.ts | 100 ++++++++--- apps/web/lib/utils/resolve-client-id.test.ts | 4 +- apps/web/lib/utils/resolve-client-id.ts | 8 +- apps/web/modules/api/lib/api-key-auth.test.ts | 62 ++++++- apps/web/modules/api/lib/api-key-auth.ts | 25 ++- .../auth/tests/authenticate-request.test.ts | 2 + .../contact-attribute-keys/route.ts | 2 +- .../management/lib/workspace-resolver.test.ts | 93 ++++++++++ .../v2/management/lib/workspace-resolver.ts | 13 +- .../api/v2/management/webhooks/route.ts | 2 +- .../api/v2/management/contacts/bulk/route.ts | 2 +- .../api/v2/management/contacts/route.ts | 2 +- .../ee/contacts/segments/actions.test.ts | 103 +++++++++++ .../modules/ee/contacts/segments/actions.ts | 18 +- .../ee/contacts/segments/lib/segments.test.ts | 32 ++++ .../ee/contacts/segments/lib/segments.ts | 20 +++ apps/web/modules/ee/quotas/lib/quotas.test.ts | 15 +- apps/web/modules/ee/quotas/lib/quotas.ts | 6 +- .../settings/api-keys/lib/api-key.ts | 27 ++- .../settings/api-keys/lib/api-keys.test.ts | 31 +++- .../settings/api-keys/types/api-keys.ts | 2 +- .../workspaces/settings/lib/workspace.test.ts | 13 +- .../workspaces/settings/lib/workspace.ts | 5 +- 28 files changed, 777 insertions(+), 63 deletions(-) create mode 100644 apps/web/app/(app)/workspaces/[workspaceId]/settings/workspace/integrations/google-sheets/actions.test.ts create mode 100644 apps/web/modules/api/v2/management/lib/workspace-resolver.test.ts create mode 100644 apps/web/modules/ee/contacts/segments/actions.test.ts diff --git a/apps/web/app/(app)/workspaces/[workspaceId]/settings/workspace/integrations/google-sheets/actions.test.ts b/apps/web/app/(app)/workspaces/[workspaceId]/settings/workspace/integrations/google-sheets/actions.test.ts new file mode 100644 index 000000000000..447e373958f4 --- /dev/null +++ b/apps/web/app/(app)/workspaces/[workspaceId]/settings/workspace/integrations/google-sheets/actions.test.ts @@ -0,0 +1,59 @@ +import { beforeEach, describe, expect, test, vi } from "vitest"; +import { OperationNotAllowedError } from "@formbricks/types/errors"; + +const mocks = vi.hoisted(() => ({ + checkAuthorizationUpdated: vi.fn(), + getOrganizationIdFromWorkspaceId: vi.fn(), + getSpreadsheetNameById: vi.fn(), +})); + +vi.mock("@/lib/utils/action-client", () => ({ + authenticatedActionClient: { + inputSchema: vi.fn(() => ({ action: vi.fn((fn) => fn) })), + }, +})); + +vi.mock("@/lib/utils/action-client/action-client-middleware", () => ({ + checkAuthorizationUpdated: mocks.checkAuthorizationUpdated, +})); + +vi.mock("@/lib/utils/helper", () => ({ + getOrganizationIdFromWorkspaceId: mocks.getOrganizationIdFromWorkspaceId, +})); + +vi.mock("@/lib/googleSheet/service", () => ({ + getSpreadsheetNameById: mocks.getSpreadsheetNameById, + validateGoogleSheetsConnection: vi.fn(), +})); + +vi.mock("@/lib/integration/service", () => ({ getIntegrationByType: vi.fn() })); + +const { getSpreadsheetNameByIdAction } = await import("./actions"); + +const call = (googleSheetIntegration: unknown, workspaceId: string) => + (getSpreadsheetNameByIdAction as unknown as (args: unknown) => Promise)({ + ctx: { user: { id: "user1" } }, + parsedInput: { googleSheetIntegration, workspaceId, spreadsheetId: "sheet1" }, + }); + +describe("getSpreadsheetNameByIdAction — ENG-1921 cross-workspace integration hijack", () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.checkAuthorizationUpdated.mockResolvedValue(undefined); + mocks.getOrganizationIdFromWorkspaceId.mockResolvedValue("org1"); + mocks.getSpreadsheetNameById.mockResolvedValue("My Sheet"); + }); + + test("rejects an integration whose workspaceId differs from the authorized workspace", async () => { + await expect(call({ workspaceId: "victim-ws", config: { data: [] } }, "attacker-ws")).rejects.toThrow( + OperationNotAllowedError + ); + expect(mocks.getSpreadsheetNameById).not.toHaveBeenCalled(); + }); + + test("allows an integration for the authorized workspace", async () => { + const result = await call({ workspaceId: "attacker-ws", config: { data: [] } }, "attacker-ws"); + expect(mocks.getSpreadsheetNameById).toHaveBeenCalled(); + expect(result).toBe("My Sheet"); + }); +}); diff --git a/apps/web/app/(app)/workspaces/[workspaceId]/settings/workspace/integrations/google-sheets/actions.ts b/apps/web/app/(app)/workspaces/[workspaceId]/settings/workspace/integrations/google-sheets/actions.ts index 791a91d3e459..aafaf82fba84 100644 --- a/apps/web/app/(app)/workspaces/[workspaceId]/settings/workspace/integrations/google-sheets/actions.ts +++ b/apps/web/app/(app)/workspaces/[workspaceId]/settings/workspace/integrations/google-sheets/actions.ts @@ -2,6 +2,7 @@ import { z } from "zod"; import { ZId } from "@formbricks/types/common"; +import { OperationNotAllowedError } from "@formbricks/types/errors"; import { TIntegrationGoogleSheets, ZIntegrationGoogleSheets, @@ -69,6 +70,14 @@ export const getSpreadsheetNameByIdAction = authenticatedActionClient ], }); + // ENG-1921: googleSheetIntegration is fully client-supplied and carries its own workspaceId, + // which is used downstream to upsert the integration (incl. OAuth tokens) on the token-refresh + // path. Reject it unless it targets the authorized workspace, otherwise a caller could + // create/overwrite another tenant's integration. + if (parsedInput.googleSheetIntegration.workspaceId !== parsedInput.workspaceId) { + throw new OperationNotAllowedError("Integration does not belong to the specified workspace"); + } + const integrationData = structuredClone(parsedInput.googleSheetIntegration); integrationData.config.data.forEach((data) => { data.createdAt = new Date(data.createdAt); diff --git a/apps/web/app/api/v1/auth.test.ts b/apps/web/app/api/v1/auth.test.ts index d3e8f7da7bc1..338c5dbff6eb 100644 --- a/apps/web/app/api/v1/auth.test.ts +++ b/apps/web/app/api/v1/auth.test.ts @@ -30,7 +30,7 @@ describe("getApiKeyWithPermissions", () => { { workspaceId: "workspace-1", permission: "manage" as const, - workspace: { id: "workspace-1", name: "Workspace 1" }, + workspace: { id: "workspace-1", name: "Workspace 1", organizationId: "org-id" }, }, ], }; @@ -112,7 +112,7 @@ describe("authenticateRequest", () => { { workspaceId: "workspace-1", permission: "manage" as const, - workspace: { id: "workspace-1", name: "Workspace 1" }, + workspace: { id: "workspace-1", name: "Workspace 1", organizationId: "org-id" }, }, ], }; diff --git a/apps/web/app/api/v1/management/me/route.ts b/apps/web/app/api/v1/management/me/route.ts index 225c07fa0719..361478858205 100644 --- a/apps/web/app/api/v1/management/me/route.ts +++ b/apps/web/app/api/v1/management/me/route.ts @@ -18,6 +18,7 @@ const apiKeySelect = { workspace: { select: { id: true, + organizationId: true, legacyEnvironmentId: true, createdAt: true, updatedAt: true, @@ -40,6 +41,7 @@ type ApiKeyData = { permission: string; workspace: { id: string; + organizationId: string; legacyEnvironmentId: string | null; createdAt: Date; updatedAt: Date; @@ -52,11 +54,20 @@ type ApiKeyData = { const validateApiKey = async (apiKey: string): Promise => { const v2Parsed = parseApiKeyV2(apiKey); - if (v2Parsed) { - return validateV2ApiKey(v2Parsed); + const apiKeyData = v2Parsed ? await validateV2ApiKey(v2Parsed) : await validateLegacyApiKey(apiKey); + if (!apiKeyData) { + return null; } - return validateLegacyApiKey(apiKey); + // ENG-1749: drop workspace permissions outside the key's organization (defense-in-depth, + // mirroring authenticateApiKeyFromHeaders) so a pre-fix cross-org row can't leak workspace + // metadata through this legacy route. + return { + ...apiKeyData, + apiKeyWorkspaces: apiKeyData.apiKeyWorkspaces.filter( + (workspacePermission) => workspacePermission.workspace.organizationId === apiKeyData.organizationId + ), + }; }; const validateV2ApiKey = async (v2Parsed: { secret: string }): Promise => { diff --git a/apps/web/lib/survey/service.test.ts b/apps/web/lib/survey/service.test.ts index 4e4830665199..e507e6249592 100644 --- a/apps/web/lib/survey/service.test.ts +++ b/apps/web/lib/survey/service.test.ts @@ -320,6 +320,21 @@ describe("Tests for updateSurvey", () => { expect(updatedSurvey).toEqual(mockTransformedSurveyOutput); }); + test("does not persist workspaceId or id from the payload on update — pinned to the existing survey (ENG-1749)", async () => { + prisma.survey.findUnique.mockResolvedValueOnce(mockSurveyOutput); + prisma.survey.update.mockResolvedValueOnce(mockSurveyOutput); + + await updateSurvey(updateSurveyInput); + + const updateArg = vi.mocked(prisma.survey.update).mock.calls.at(-1)?.[0]; + // workspaceId/id are the survey's tenant anchors: they must come from the existing record, + // never the client payload, so an authorized editor cannot re-point their survey to another + // workspace/organization on update. + expect(updateArg?.data).not.toHaveProperty("workspaceId"); + expect(updateArg?.data).not.toHaveProperty("id"); + expect(updateArg?.where).toEqual({ id: updateSurveyInput.id }); + }); + // Note: Language handling tests (for languages.length > 0 fix) are covered in // apps/web/modules/survey/editor/lib/survey.test.ts where we have better control // over the test mocks. The key fix ensures languages.length > 0 (not > 1) is used. @@ -358,6 +373,145 @@ describe("Tests for updateSurvey", () => { prisma.survey.update.mockRejectedValue(new Error(mockErrorMessage)); await expect(updateSurvey(updateSurveyInput)).rejects.toThrow(Error); }); + + // ENG-1749 sibling: the update path must not update/delete a segment from another workspace + // even when the caller is authorized for the survey being updated. + test("rejects a segment that belongs to another workspace", async () => { + prisma.survey.findUnique.mockResolvedValueOnce(mockSurveyOutput); + prisma.segment.findUnique.mockResolvedValueOnce({ + id: "clseg123456789012345678901", + title: "Segment", + description: null, + isPrivate: true, + filters: [], + workspaceId: "clotherworkspace1234567890", + createdAt: new Date(), + updatedAt: new Date(), + }); + + await expect( + updateSurvey({ + ...updateSurveyInput, + segment: { + id: "clseg123456789012345678901", + title: "Segment", + description: null, + isPrivate: true, + filters: [], + workspaceId: updateSurveyInput.workspaceId, + surveys: [], + createdAt: new Date(), + updatedAt: new Date(), + }, + }) + ).rejects.toThrow(ResourceNotFoundError); + + expect(prisma.segment.delete).not.toHaveBeenCalled(); + expect(prisma.survey.update).not.toHaveBeenCalled(); + }); + + // ENG-1749 sibling: the update path (incl. drafts, skipValidation=true) must not link a language + // from another workspace. The guard resolves the language's workspace from the DB, so a request + // that LIES about language.workspaceId (claiming this survey's workspace) is still rejected. + test("rejects a language whose real (DB) workspace differs, even when the input claims otherwise", async () => { + prisma.survey.findUnique.mockResolvedValueOnce(mockSurveyOutput); + prisma.language.findMany.mockResolvedValueOnce([ + { id: "cllangforeign00000000001", workspaceId: "clforeignws0000000000001" }, + ] as any); + + await expect( + updateSurveyInternal( + { + ...updateSurveyInput, + languages: [ + { + language: { + id: "cllangforeign00000000001", + createdAt: new Date(), + updatedAt: new Date(), + code: "de", + alias: null, + workspaceId: updateSurveyInput.workspaceId, // the lie: claims the survey's own workspace + }, + default: true, + enabled: true, + }, + ], + }, + true + ) + ).rejects.toThrow(ResourceNotFoundError); + + expect(prisma.language.findMany).toHaveBeenCalledWith({ + where: { id: { in: ["cllangforeign00000000001"] } }, + select: { id: true, workspaceId: true }, + }); + expect(prisma.survey.update).not.toHaveBeenCalled(); + }); + + test("rejects a language id that does not exist (absent from the DB result)", async () => { + prisma.survey.findUnique.mockResolvedValueOnce(mockSurveyOutput); + prisma.language.findMany.mockResolvedValueOnce([] as any); + + await expect( + updateSurveyInternal( + { + ...updateSurveyInput, + languages: [ + { + language: { + id: "cllangmissing0000000001", + createdAt: new Date(), + updatedAt: new Date(), + code: "de", + alias: null, + workspaceId: updateSurveyInput.workspaceId, + }, + default: true, + enabled: true, + }, + ], + }, + true + ) + ).rejects.toThrow(ResourceNotFoundError); + expect(prisma.survey.update).not.toHaveBeenCalled(); + }); + + // ENG-1749/ENG-1920: the app-survey segment block connects segment.surveys by id; a survey from + // another workspace must not be connectable (would re-point that survey's targeting). + test("rejects connecting a survey from another workspace to the segment (app survey update)", async () => { + prisma.survey.findUnique.mockResolvedValueOnce(mockSurveyOutput); // getSurvey → current survey (own workspace) + prisma.segment.findUnique.mockResolvedValueOnce({ + workspaceId: updateSurveyInput.workspaceId, + } as any); // segment.id belongs to the survey's workspace (passes the segment guard) + prisma.survey.findMany.mockResolvedValueOnce([ + { id: "clvictimsurvey0000000001", workspaceId: "clforeignws0000000000001" }, + ] as any); // the connected survey is in ANOTHER workspace + + await expect( + updateSurveyInternal( + { + ...updateSurveyInput, + type: "app", + segment: { + id: "clownsegment000000000001", + title: "seg", + description: null, + isPrivate: false, + filters: [], + workspaceId: updateSurveyInput.workspaceId, + surveys: ["clvictimsurvey0000000001"], + createdAt: new Date(), + updatedAt: new Date(), + }, + } as any, + true + ) + ).rejects.toThrow(InvalidInputError); + + expect(prisma.segment.update).not.toHaveBeenCalled(); + }); }); }); @@ -784,6 +938,9 @@ describe("Tests for createSurvey", () => { test("creates survey languages from validated language inputs", async () => { vi.mocked(getOrganizationByWorkspaceId).mockResolvedValueOnce(mockOrganizationOutput); + prisma.language.findMany.mockResolvedValueOnce([ + { id: "cllang12345678901234567890", workspaceId: mockWorkspaceId }, + ] as any); prisma.survey.create.mockResolvedValueOnce({ ...mockSurveyOutput, }); @@ -938,6 +1095,11 @@ describe("Tests for createSurvey", () => { }); test("rejects survey languages from a different workspace", async () => { + // The DB says this language belongs to another workspace, regardless of what the input claims. + prisma.language.findMany.mockResolvedValueOnce([ + { id: "cllang12345678901234567890", workspaceId: "clotherworkspace0000000000" }, + ] as any); + await expect( createSurvey(mockWorkspaceId, { ...mockCreateSurveyInput, @@ -949,7 +1111,7 @@ describe("Tests for createSurvey", () => { id: "cllang12345678901234567890", code: "en-US", alias: null, - workspaceId: "clotherworkspace0000000000", + workspaceId: mockWorkspaceId, createdAt: new Date(), updatedAt: new Date(), }, diff --git a/apps/web/lib/survey/service.ts b/apps/web/lib/survey/service.ts index 17381cdba105..360d17393451 100644 --- a/apps/web/lib/survey/service.ts +++ b/apps/web/lib/survey/service.ts @@ -12,6 +12,7 @@ import { getOrganizationByWorkspaceId, subscribeOrganizationMembersToSurveyResponses, } from "@/lib/organization/service"; +import { getSurveyWorkspaceIdMap } from "@/modules/ee/contacts/segments/lib/segments"; import { handleTriggerUpdates } from "@/modules/survey/lib/trigger-updates"; import { isSurveySchedulingDue, @@ -284,14 +285,38 @@ export const updateSurveyInternal = async ( const surveyId = updatedSurvey.id; let data: any = {}; - const actionClasses = await getActionClasses(updatedSurvey.workspaceId); const currentSurvey = await getSurvey(surveyId); if (!currentSurvey) { throw new ResourceNotFoundError("Survey", surveyId); } - const { triggers, segment, questions, languages, type, followUps, ...surveyData } = updatedSurvey; + // ENG-1749: workspaceId and id are the survey's tenant anchors. Always resolve the workspace from + // the existing survey (never the client payload), and strip workspaceId/id from the update below, + // so an authorized editor cannot re-point their own survey into another workspace/organization. + const actionClasses = await getActionClasses(currentSurvey.workspaceId); + + const { + triggers, + segment, + questions, + languages, + type, + followUps, + workspaceId: _workspaceId, + id: _id, + ...surveyData + } = updatedSurvey; + + // ENG-1749 sibling: the segment block below updates/deletes by segment.id directly. Ensure the + // segment belongs to this survey's workspace so a caller cannot mutate or delete another + // tenant's segment by supplying its id. Mirrors the create path guard. + await assertSurveySegmentBelongsToWorkspace(currentSurvey.workspaceId, segment); + + // ENG-1749 sibling: the languages block below links languages by language.id. Ensure every + // referenced language belongs to this survey's workspace so a caller cannot attach another + // tenant's language. Mirrors the create path guard (covers drafts too — runs before validation). + await assertSurveyLanguagesBelongToWorkspace(currentSurvey.workspaceId, languages); if (!skipValidation) { checkForInvalidImagesInQuestions(questions); @@ -373,22 +398,34 @@ export const updateSurveyInternal = async ( throw new InvalidInputError("Invalid user segment filters"); } + // ENG-1749/ENG-1920: the connected survey ids are client-supplied; ensure each belongs to + // this survey's workspace before re-pointing it to the segment (a foreign id would hijack + // another tenant's survey targeting). Done outside the try below, which masks errors as a + // generic Error and would otherwise hide this rejection. + if (segment.surveys && segment.surveys.length > 0) { + const workspaceBySurveyId = await getSurveyWorkspaceIdMap(segment.surveys); + if ( + !segment.surveys.every( + (surveyId) => workspaceBySurveyId.get(surveyId) === currentSurvey.workspaceId + ) + ) { + throw new InvalidInputError("Survey and segment are not in the same workspace"); + } + } + try { - // update the segment: - let updatedInput: Prisma.SegmentUpdateInput = { - ...segment, - surveys: undefined, + // Update only the segment's own mutable fields — never mass-assign workspaceId/id/ + // timestamps from the client-supplied segment object (ENG-1749). + const updatedInput: Prisma.SegmentUpdateInput = { + title: segment.title, + description: segment.description, + isPrivate: segment.isPrivate, + filters: segment.filters, + ...(segment.surveys + ? { surveys: { connect: segment.surveys.map((surveyId) => ({ id: surveyId })) } } + : {}), }; - if (segment.surveys) { - updatedInput = { - ...segment, - surveys: { - connect: segment.surveys.map((surveyId) => ({ id: surveyId })), - }, - }; - } - await prisma.segment.update({ where: { id: segment.id }, data: updatedInput, @@ -436,7 +473,7 @@ export const updateSurveyInternal = async ( } } else if (type === "app") { if (!currentSurvey.segment) { - const workspaceId = updatedSurvey.workspaceId; + const workspaceId = currentSurvey.workspaceId; await prisma.survey.update({ where: { id: surveyId, @@ -630,20 +667,35 @@ const validateSurveyCreateDataMedia = ( return data; }; -const assertSurveyLanguagesBelongToWorkspace = ( +const assertSurveyLanguagesBelongToWorkspace = async ( workspaceId: string, - languages: TSurveyCreateInput["languages"] -): void => { - for (const surveyLanguage of languages ?? []) { - if (surveyLanguage.language.workspaceId !== workspaceId) { - throw new ResourceNotFoundError("Language", surveyLanguage.language.id); + languages: Array<{ language: { id: string } }> | null | undefined +): Promise => { + const languageIds = [...new Set((languages ?? []).map((surveyLanguage) => surveyLanguage.language.id))]; + if (languageIds.length === 0) { + return; + } + + // ENG-1749: resolve each language's real owning workspace from the DB rather than trusting the + // caller-supplied language.workspaceId — the survey payload is client-controlled, so an attacker + // could otherwise claim a foreign language belongs to this workspace. A single batched query + // avoids a per-language fan-out; an unknown id is absent from the map and thus rejected. + const dbLanguages = await prisma.language.findMany({ + where: { id: { in: languageIds } }, + select: { id: true, workspaceId: true }, + }); + const workspaceByLanguageId = new Map(dbLanguages.map((language) => [language.id, language.workspaceId])); + + for (const languageId of languageIds) { + if (workspaceByLanguageId.get(languageId) !== workspaceId) { + throw new ResourceNotFoundError("Language", languageId); } } }; const assertSurveySegmentBelongsToWorkspace = async ( workspaceId: string, - segment: TSurveyCreateInput["segment"] + segment: { id?: string | null } | null | undefined ): Promise => { if (!segment?.id) { return; @@ -671,7 +723,7 @@ export const createSurvey = async ( try { const { createdBy, languages, segment, followUps, styling, ...restSurveyBody } = parsedSurveyBody; - assertSurveyLanguagesBelongToWorkspace(parsedWorkspaceId, languages); + await assertSurveyLanguagesBelongToWorkspace(parsedWorkspaceId, languages); await assertSurveySegmentBelongsToWorkspace(parsedWorkspaceId, segment); // An app survey can never be shown without a trigger, so block creating one directly in a diff --git a/apps/web/lib/utils/resolve-client-id.test.ts b/apps/web/lib/utils/resolve-client-id.test.ts index 04dc9c3a15e1..1f2d2cb2434c 100644 --- a/apps/web/lib/utils/resolve-client-id.test.ts +++ b/apps/web/lib/utils/resolve-client-id.test.ts @@ -29,7 +29,7 @@ describe("resolveClientApiIds", () => { }); expect(prisma.workspace.findFirst).toHaveBeenCalledWith({ where: { OR: [{ id: "ws-456" }, { legacyEnvironmentId: "ws-456" }] }, - select: { id: true }, + select: { id: true, organizationId: true }, }); }); @@ -42,7 +42,7 @@ describe("resolveClientApiIds", () => { expect(prisma.workspace.findFirst).toHaveBeenCalledTimes(1); expect(prisma.workspace.findFirst).toHaveBeenCalledWith({ where: { OR: [{ id: "env-old-123" }, { legacyEnvironmentId: "env-old-123" }] }, - select: { id: true }, + select: { id: true, organizationId: true }, }); }); diff --git a/apps/web/lib/utils/resolve-client-id.ts b/apps/web/lib/utils/resolve-client-id.ts index 6be08d67359d..f6d8c0f9c0ed 100644 --- a/apps/web/lib/utils/resolve-client-id.ts +++ b/apps/web/lib/utils/resolve-client-id.ts @@ -9,12 +9,14 @@ export type TResolvedClientIds = { /** * Finds a workspace by its primary id or by legacyEnvironmentId in a single query. * Both columns have unique indexes so the query planner will use index scans. - * Returns the workspace id if found, null otherwise. + * Returns the workspace id and its owning organizationId if found, null otherwise. */ -export const findWorkspaceByIdOrLegacyEnvId = async (id: string): Promise<{ id: string } | null> => { +export const findWorkspaceByIdOrLegacyEnvId = async ( + id: string +): Promise<{ id: string; organizationId: string } | null> => { return await prisma.workspace.findFirst({ where: { OR: [{ id }, { legacyEnvironmentId: id }] }, - select: { id: true }, + select: { id: true, organizationId: true }, }); }; diff --git a/apps/web/modules/api/lib/api-key-auth.test.ts b/apps/web/modules/api/lib/api-key-auth.test.ts index 2134912cf728..0f7d133621c5 100644 --- a/apps/web/modules/api/lib/api-key-auth.test.ts +++ b/apps/web/modules/api/lib/api-key-auth.test.ts @@ -1,5 +1,15 @@ -import { describe, expect, test } from "vitest"; -import { getApiKeyFromHeaders, getBearerTokenFromHeaders } from "./api-key-auth"; +import { beforeEach, describe, expect, test, vi } from "vitest"; +import { + authenticateApiKeyFromHeaders, + getApiKeyFromHeaders, + getBearerTokenFromHeaders, +} from "./api-key-auth"; + +const mocks = vi.hoisted(() => ({ getApiKeyWithPermissions: vi.fn() })); + +vi.mock("@/modules/organization/settings/api-keys/lib/api-key", () => ({ + getApiKeyWithPermissions: mocks.getApiKeyWithPermissions, +})); describe("api-key-auth helpers", () => { test("prefers x-api-key over bearer authorization", () => { @@ -38,3 +48,51 @@ describe("api-key-auth helpers", () => { expect(getBearerTokenFromHeaders(headers)).toBe("opaque_service_token"); }); }); + +describe("authenticateApiKeyFromHeaders — ENG-1749 cross-org permission filter", () => { + const headers = new Headers({ "x-api-key": "fbk_secret" }); + + const apiKeyData = (workspaces: unknown[]) => ({ + id: "key1", + organizationId: "org-self", + organizationAccess: { accessControl: { read: false, write: false } }, + apiKeyWorkspaces: workspaces, + }); + + const ws = (id: string, organizationId: string) => ({ + permission: "manage", + workspaceId: id, + workspace: { id, name: id, organizationId }, + }); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + test("drops workspace permissions whose workspace is in another organization", async () => { + mocks.getApiKeyWithPermissions.mockResolvedValue( + apiKeyData([ws("ws-own", "org-self"), ws("ws-victim", "org-other")]) + ); + + const auth = await authenticateApiKeyFromHeaders(headers); + + expect(auth?.workspacePermissions).toEqual([ + { permission: "manage", workspaceId: "ws-own", workspaceName: "ws-own" }, + ]); + }); + + test("returns null when only cross-org permissions remain", async () => { + mocks.getApiKeyWithPermissions.mockResolvedValue(apiKeyData([ws("ws-victim", "org-other")])); + + expect(await authenticateApiKeyFromHeaders(headers)).toBeNull(); + }); + + test("keeps a cross-org-only key for org-scoped routes but with no workspace permissions", async () => { + mocks.getApiKeyWithPermissions.mockResolvedValue(apiKeyData([ws("ws-victim", "org-other")])); + + const auth = await authenticateApiKeyFromHeaders(headers, { allowOrganizationOnlyApiKey: true }); + + expect(auth).not.toBeNull(); + expect(auth?.workspacePermissions).toEqual([]); + }); +}); diff --git a/apps/web/modules/api/lib/api-key-auth.ts b/apps/web/modules/api/lib/api-key-auth.ts index d7026fbdc1b1..aee57d9f967d 100644 --- a/apps/web/modules/api/lib/api-key-auth.ts +++ b/apps/web/modules/api/lib/api-key-auth.ts @@ -52,19 +52,32 @@ export const authenticateApiKeyFromHeaders = async ( return null; } + // ENG-1749 defense-in-depth: only honor workspace permissions whose workspace belongs to the + // key's own organization. API keys are org-scoped, so a permission on a foreign workspace is + // always illegitimate (it can only exist from a pre-fix bug/exploit). Filtering here — the single + // point where the permission list is built — protects every consumer, including the read/list + // routes that authorize off this list directly rather than through resolveBodyIdsV2. + const workspacePermissions = (apiKeyData.apiKeyWorkspaces ?? []) + .filter( + (workspacePermission) => workspacePermission.workspace.organizationId === apiKeyData.organizationId + ) + .map((workspacePermission) => ({ + permission: workspacePermission.permission, + workspaceId: workspacePermission.workspaceId, + workspaceName: workspacePermission.workspace.name, + })); + // Reject org-only API keys for routes that require workspace-scoped permissions // (those routes opt in via allowOrganizationOnlyApiKey when an org-only key is acceptable). - if (!options.allowOrganizationOnlyApiKey && (apiKeyData.apiKeyWorkspaces?.length ?? 0) === 0) { + // Uses the filtered list so a key left with only cross-org (now-dropped) permissions is treated + // as having no workspace access. + if (!options.allowOrganizationOnlyApiKey && workspacePermissions.length === 0) { return null; } return { type: "apiKey", - workspacePermissions: (apiKeyData.apiKeyWorkspaces ?? []).map((workspacePermission) => ({ - permission: workspacePermission.permission, - workspaceId: workspacePermission.workspaceId, - workspaceName: workspacePermission.workspace.name, - })), + workspacePermissions, apiKeyId: apiKeyData.id, organizationId: apiKeyData.organizationId, organizationAccess: apiKeyData.organizationAccess, diff --git a/apps/web/modules/api/v2/auth/tests/authenticate-request.test.ts b/apps/web/modules/api/v2/auth/tests/authenticate-request.test.ts index 113f5de99b18..91f54b8c4ed4 100644 --- a/apps/web/modules/api/v2/auth/tests/authenticate-request.test.ts +++ b/apps/web/modules/api/v2/auth/tests/authenticate-request.test.ts @@ -40,6 +40,7 @@ describe("authenticateRequest", () => { workspace: { id: "workspace-id-1", name: "Workspace 1", + organizationId: "org-id", }, }, { @@ -49,6 +50,7 @@ describe("authenticateRequest", () => { workspace: { id: "workspace-id-2", name: "Workspace 2", + organizationId: "org-id", }, }, ], diff --git a/apps/web/modules/api/v2/management/contact-attribute-keys/route.ts b/apps/web/modules/api/v2/management/contact-attribute-keys/route.ts index 92b3f6e0f5ae..522d2d6ec0a3 100644 --- a/apps/web/modules/api/v2/management/contact-attribute-keys/route.ts +++ b/apps/web/modules/api/v2/management/contact-attribute-keys/route.ts @@ -43,7 +43,7 @@ export const POST = async (request: NextRequest) => body: ZContactAttributeKeyCreateInput, }, bodyTransform: async (body, auth) => { - const resolved = await resolveBodyIdsV2(body, auth.workspacePermissions, "POST"); + const resolved = await resolveBodyIdsV2(body, auth, "POST"); if (!resolved.ok) throw resolved.error; return { ...body, ...resolved.data }; }, diff --git a/apps/web/modules/api/v2/management/lib/workspace-resolver.test.ts b/apps/web/modules/api/v2/management/lib/workspace-resolver.test.ts new file mode 100644 index 000000000000..5e1db9b5ba09 --- /dev/null +++ b/apps/web/modules/api/v2/management/lib/workspace-resolver.test.ts @@ -0,0 +1,93 @@ +import { beforeEach, describe, expect, test, vi } from "vitest"; +import { ApiKeyPermission } from "@formbricks/database/prisma"; +import { findWorkspaceByIdOrLegacyEnvId } from "@/lib/utils/resolve-client-id"; +import { resolveBodyIdsV2 } from "./workspace-resolver"; + +vi.mock("server-only", () => ({})); + +vi.mock("@/lib/utils/resolve-client-id", () => ({ + findWorkspaceByIdOrLegacyEnvId: vi.fn(), +})); + +const auth = (organizationId: string, workspaceId: string, permission: ApiKeyPermission) => ({ + organizationId, + workspacePermissions: [{ workspaceId, permission }], +}); + +describe("resolveBodyIdsV2", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + test("returns bad_request when no workspaceId/environmentId is provided", async () => { + const result = await resolveBodyIdsV2({}, auth("org1", "ws1", ApiKeyPermission.manage), "POST"); + + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error.type).toBe("bad_request"); + expect(findWorkspaceByIdOrLegacyEnvId).not.toHaveBeenCalled(); + }); + + test("returns not_found when the workspace does not exist", async () => { + vi.mocked(findWorkspaceByIdOrLegacyEnvId).mockResolvedValueOnce(null); + + const result = await resolveBodyIdsV2( + { workspaceId: "ws-missing" }, + auth("org1", "ws-missing", ApiKeyPermission.manage), + "POST" + ); + + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error.type).toBe("not_found"); + }); + + // ENG-1749 defense-in-depth: a permission row for a workspace in another organization must not + // grant access, even though hasPermission alone would match on workspaceId. + test("returns forbidden when the workspace belongs to a different organization", async () => { + vi.mocked(findWorkspaceByIdOrLegacyEnvId).mockResolvedValueOnce({ + id: "victim-ws", + organizationId: "victim-org", + }); + + const result = await resolveBodyIdsV2( + { workspaceId: "victim-ws" }, + // Attacker's key carries a (illegitimate) manage permission on the victim workspace. + auth("attacker-org", "victim-ws", ApiKeyPermission.manage), + "POST" + ); + + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error.type).toBe("forbidden"); + }); + + test("returns forbidden when the key lacks permission for an in-org workspace", async () => { + vi.mocked(findWorkspaceByIdOrLegacyEnvId).mockResolvedValueOnce({ + id: "ws1", + organizationId: "org1", + }); + + const result = await resolveBodyIdsV2( + { workspaceId: "ws1" }, + auth("org1", "ws1", ApiKeyPermission.read), // read cannot POST (needs write) + "POST" + ); + + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error.type).toBe("forbidden"); + }); + + test("resolves the workspaceId for a same-org workspace the key can access", async () => { + vi.mocked(findWorkspaceByIdOrLegacyEnvId).mockResolvedValueOnce({ + id: "ws1", + organizationId: "org1", + }); + + const result = await resolveBodyIdsV2( + { workspaceId: "ws1" }, + auth("org1", "ws1", ApiKeyPermission.manage), + "POST" + ); + + expect(result.ok).toBe(true); + if (result.ok) expect(result.data).toEqual({ workspaceId: "ws1" }); + }); +}); diff --git a/apps/web/modules/api/v2/management/lib/workspace-resolver.ts b/apps/web/modules/api/v2/management/lib/workspace-resolver.ts index df804fb740e6..bfb6d3c0c2e1 100644 --- a/apps/web/modules/api/v2/management/lib/workspace-resolver.ts +++ b/apps/web/modules/api/v2/management/lib/workspace-resolver.ts @@ -1,4 +1,4 @@ -import { TAPIKeyWorkspacePermission } from "@formbricks/types/auth"; +import { TAuthenticationApiKey } from "@formbricks/types/auth"; import { Result, err, ok } from "@formbricks/types/error-handlers"; import { findWorkspaceByIdOrLegacyEnvId } from "@/lib/utils/resolve-client-id"; import { ApiErrorResponseV2 } from "@/modules/api/v2/types/api-error"; @@ -17,7 +17,7 @@ type HttpMethod = "GET" | "POST" | "PUT" | "PATCH" | "DELETE"; */ export const resolveBodyIdsV2 = async ( body: { workspaceId?: string; environmentId?: string }, - permissions: TAPIKeyWorkspacePermission[], + authentication: Pick, method: HttpMethod ): Promise> => { const rawId = body.workspaceId ?? body.environmentId; @@ -31,7 +31,14 @@ export const resolveBodyIdsV2 = async ( }); } - if (!hasPermission(permissions, workspace.id, method)) { + // ENG-1749 defense-in-depth: even if the API key somehow carries a permission row for this + // workspace, refuse it unless the workspace belongs to the key's own organization. This is a + // second, independent tenant boundary behind the create-time check in createApiKey. + if (workspace.organizationId !== authentication.organizationId) { + return err({ type: "forbidden" }); + } + + if (!hasPermission(authentication.workspacePermissions, workspace.id, method)) { return err({ type: "forbidden" }); } diff --git a/apps/web/modules/api/v2/management/webhooks/route.ts b/apps/web/modules/api/v2/management/webhooks/route.ts index 9496abe0553d..1f5ec597ac43 100644 --- a/apps/web/modules/api/v2/management/webhooks/route.ts +++ b/apps/web/modules/api/v2/management/webhooks/route.ts @@ -44,7 +44,7 @@ export const POST = async (request: NextRequest) => body: ZWebhookCreateInput, }, bodyTransform: async (body, auth) => { - const resolved = await resolveBodyIdsV2(body, auth.workspacePermissions, "POST"); + const resolved = await resolveBodyIdsV2(body, auth, "POST"); if (!resolved.ok) throw resolved.error; return { ...body, ...resolved.data }; }, diff --git a/apps/web/modules/ee/contacts/api/v2/management/contacts/bulk/route.ts b/apps/web/modules/ee/contacts/api/v2/management/contacts/bulk/route.ts index 6b48a5affa9d..82031ad5bfc0 100644 --- a/apps/web/modules/ee/contacts/api/v2/management/contacts/bulk/route.ts +++ b/apps/web/modules/ee/contacts/api/v2/management/contacts/bulk/route.ts @@ -14,7 +14,7 @@ export const PUT = async (request: Request) => body: ZContactBulkUploadRequest, }, bodyTransform: async (body, auth) => { - const resolved = await resolveBodyIdsV2(body, auth.workspacePermissions, "PUT"); + const resolved = await resolveBodyIdsV2(body, auth, "PUT"); if (!resolved.ok) throw resolved.error; return { ...body, ...resolved.data }; }, diff --git a/apps/web/modules/ee/contacts/api/v2/management/contacts/route.ts b/apps/web/modules/ee/contacts/api/v2/management/contacts/route.ts index 2ca1c607cbf2..898d035e122e 100644 --- a/apps/web/modules/ee/contacts/api/v2/management/contacts/route.ts +++ b/apps/web/modules/ee/contacts/api/v2/management/contacts/route.ts @@ -15,7 +15,7 @@ export const POST = async (request: NextRequest) => body: ZContactCreateRequest, }, bodyTransform: async (body, auth) => { - const resolved = await resolveBodyIdsV2(body, auth.workspacePermissions, "POST"); + const resolved = await resolveBodyIdsV2(body, auth, "POST"); if (!resolved.ok) throw resolved.error; return { ...body, ...resolved.data }; }, diff --git a/apps/web/modules/ee/contacts/segments/actions.test.ts b/apps/web/modules/ee/contacts/segments/actions.test.ts new file mode 100644 index 000000000000..c73a63e01d20 --- /dev/null +++ b/apps/web/modules/ee/contacts/segments/actions.test.ts @@ -0,0 +1,103 @@ +import { beforeEach, describe, expect, test, vi } from "vitest"; +import { InvalidInputError } from "@formbricks/types/errors"; + +const mocks = vi.hoisted(() => ({ + checkAuthorizationUpdated: vi.fn(), + getOrganizationIdFromSegmentId: vi.fn(), + getWorkspaceIdFromSegmentId: vi.fn(), + getWorkspaceIdFromSurveyId: vi.fn(), + getSurveyWorkspaceIdMap: vi.fn(), + getIsContactsEnabled: vi.fn(), + getOrganization: vi.fn(), + updateSegment: vi.fn(), + getSegment: vi.fn(), +})); + +vi.mock("@/lib/utils/action-client", () => ({ + authenticatedActionClient: { + inputSchema: vi.fn(() => ({ action: vi.fn((fn) => fn) })), + }, +})); + +vi.mock("@/modules/ee/audit-logs/lib/handler", () => ({ + withAuditLogging: vi.fn((_eventName, _objectType, fn) => fn), +})); + +vi.mock("@/lib/utils/action-client/action-client-middleware", () => ({ + checkAuthorizationUpdated: mocks.checkAuthorizationUpdated, +})); + +vi.mock("@/lib/utils/helper", () => ({ + getOrganizationIdFromContactAttributeKeyId: vi.fn(), + getOrganizationIdFromSegmentId: mocks.getOrganizationIdFromSegmentId, + getOrganizationIdFromSurveyId: vi.fn(), + getOrganizationIdFromWorkspaceId: vi.fn(), + getWorkspaceIdFromContactAttributeKeyId: vi.fn(), + getWorkspaceIdFromSegmentId: mocks.getWorkspaceIdFromSegmentId, + getWorkspaceIdFromSurveyId: mocks.getWorkspaceIdFromSurveyId, +})); + +vi.mock("@/modules/ee/license-check/lib/utils", () => ({ + getIsContactsEnabled: mocks.getIsContactsEnabled, +})); + +vi.mock("@/modules/ee/contacts/segments/lib/segments", () => ({ + cloneSegment: vi.fn(), + createSegment: vi.fn(), + deleteSegment: vi.fn(), + getSegment: mocks.getSegment, + getSurveyWorkspaceIdMap: mocks.getSurveyWorkspaceIdMap, + resetSegmentInSurvey: vi.fn(), + updateSegment: mocks.updateSegment, +})); + +vi.mock("@/lib/survey/service", () => ({ loadNewSegmentInSurvey: vi.fn() })); +vi.mock("@/lib/organization/service", () => ({ getOrganization: mocks.getOrganization })); +vi.mock("@/lib/posthog", () => ({ capturePostHogEvent: vi.fn() })); +vi.mock("@/modules/ee/contacts/lib/contact-attributes", () => ({ getDistinctAttributeValues: vi.fn() })); +vi.mock("@/modules/ee/contacts/segments/lib/helper", () => ({ checkForRecursiveSegmentFilter: vi.fn() })); + +// Import after mocks so the action client / audit wrappers are the passthrough versions. +const { updateSegmentAction } = await import("./actions"); + +const callUpdate = (data: Record) => + (updateSegmentAction as unknown as (args: unknown) => Promise)({ + ctx: { user: { id: "user1" }, auditLoggingCtx: {} }, + parsedInput: { segmentId: "seg1", data }, + }); + +describe("updateSegmentAction — ENG-1920 cross-workspace survey re-point", () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.checkAuthorizationUpdated.mockResolvedValue(undefined); + mocks.getOrganizationIdFromSegmentId.mockResolvedValue("org1"); + mocks.getWorkspaceIdFromSegmentId.mockResolvedValue("ws-segment"); + mocks.getIsContactsEnabled.mockResolvedValue(true); + mocks.getOrganization.mockResolvedValue({ id: "org1" }); + }); + + test("rejects a survey that belongs to another workspace", async () => { + mocks.getSurveyWorkspaceIdMap.mockResolvedValue(new Map([["victim-survey", "ws-other"]])); + + await expect(callUpdate({ surveys: ["victim-survey"] })).rejects.toThrow(InvalidInputError); + expect(mocks.updateSegment).not.toHaveBeenCalled(); + }); + + test("rejects a survey id that does not resolve to any workspace (uniform rejection, no oracle)", async () => { + mocks.getSurveyWorkspaceIdMap.mockResolvedValue(new Map()); + + await expect(callUpdate({ surveys: ["nonexistent-survey"] })).rejects.toThrow(InvalidInputError); + expect(mocks.updateSegment).not.toHaveBeenCalled(); + }); + + test("allows surveys in the segment's own workspace", async () => { + mocks.getSurveyWorkspaceIdMap.mockResolvedValue(new Map([["own-survey", "ws-segment"]])); + mocks.getSegment.mockResolvedValue({ id: "seg1" }); + mocks.updateSegment.mockResolvedValue({ id: "seg1", surveys: [] }); + + await callUpdate({ surveys: ["own-survey"] }); + + expect(mocks.updateSegment).toHaveBeenCalledWith("seg1", { surveys: ["own-survey"] }); + expect(mocks.getSurveyWorkspaceIdMap).toHaveBeenCalledWith(["own-survey"]); + }); +}); diff --git a/apps/web/modules/ee/contacts/segments/actions.ts b/apps/web/modules/ee/contacts/segments/actions.ts index 55fcf3bc8292..ef9b49243080 100644 --- a/apps/web/modules/ee/contacts/segments/actions.ts +++ b/apps/web/modules/ee/contacts/segments/actions.ts @@ -26,6 +26,7 @@ import { createSegment, deleteSegment, getSegment, + getSurveyWorkspaceIdMap, resetSegmentInSurvey, updateSegment, } from "@/modules/ee/contacts/segments/lib/segments"; @@ -116,6 +117,7 @@ const ZUpdateSegmentAction = z.object({ export const updateSegmentAction = authenticatedActionClient.inputSchema(ZUpdateSegmentAction).action( withAuditLogging("updated", "segment", async ({ ctx, parsedInput }) => { const organizationId = await getOrganizationIdFromSegmentId(parsedInput.segmentId); + const segmentWorkspaceId = await getWorkspaceIdFromSegmentId(parsedInput.segmentId); await checkAuthorizationUpdated({ userId: ctx.user.id, organizationId, @@ -127,13 +129,27 @@ export const updateSegmentAction = authenticatedActionClient.inputSchema(ZUpdate { type: "workspaceTeam", minPermission: "readWrite", - workspaceId: await getWorkspaceIdFromSegmentId(parsedInput.segmentId), + workspaceId: segmentWorkspaceId, }, ], }); await checkAdvancedTargetingPermission(organizationId); + // ENG-1920: the surveys are connected to the segment by id alone, so ensure every survey + // belongs to the segment's workspace — otherwise a caller could re-point another tenant's + // survey to their segment. A single batched lookup avoids fanning out a query per survey id + // over the caller-controlled array; an unknown id is absent from the map and thus rejected. + if (parsedInput.data.surveys && parsedInput.data.surveys.length > 0) { + const surveyWorkspaceIdMap = await getSurveyWorkspaceIdMap(parsedInput.data.surveys); + const allInSegmentWorkspace = parsedInput.data.surveys.every( + (surveyId) => surveyWorkspaceIdMap.get(surveyId) === segmentWorkspaceId + ); + if (!allInSegmentWorkspace) { + throw new InvalidInputError("Survey and segment are not in the same workspace"); + } + } + const { filters } = parsedInput.data; if (filters) { const parsedFilters = ZSegmentFilters.safeParse(filters); diff --git a/apps/web/modules/ee/contacts/segments/lib/segments.test.ts b/apps/web/modules/ee/contacts/segments/lib/segments.test.ts index 0046fbe0b3f9..97c11ce7b93c 100644 --- a/apps/web/modules/ee/contacts/segments/lib/segments.test.ts +++ b/apps/web/modules/ee/contacts/segments/lib/segments.test.ts @@ -22,6 +22,7 @@ import { getSegment, getSegments, getSegmentsByAttributeKey, + getSurveyWorkspaceIdMap, resetSegmentInSurvey, selectSegment, transformPrismaSegment, @@ -42,6 +43,7 @@ vi.mock("@formbricks/database", () => ({ }, survey: { update: vi.fn(), + findMany: vi.fn(), }, $transaction: vi.fn((callback) => callback(prisma)), // Mock transaction to execute the callback }, @@ -167,6 +169,36 @@ describe("Segment Service Tests", () => { }); }); + describe("getSurveyWorkspaceIdMap", () => { + test("builds an id→workspaceId map, selecting only id and workspaceId", async () => { + vi.mocked(prisma.survey.findMany).mockResolvedValue([ + { id: "survey1", workspaceId: "ws-a" }, + { id: "survey2", workspaceId: "ws-b" }, + ] as any); + + const result = await getSurveyWorkspaceIdMap(["survey1", "survey2"]); + + expect(result).toEqual( + new Map([ + ["survey1", "ws-a"], + ["survey2", "ws-b"], + ]) + ); + expect(prisma.survey.findMany).toHaveBeenCalledWith({ + where: { id: { in: ["survey1", "survey2"] } }, + select: { id: true, workspaceId: true }, + }); + }); + + test("returns an empty map when no surveys match", async () => { + vi.mocked(prisma.survey.findMany).mockResolvedValue([] as any); + + const result = await getSurveyWorkspaceIdMap(["missing"]); + + expect(result.size).toBe(0); + }); + }); + describe("createSegment", () => { test("should create a segment without surveyId", async () => { vi.mocked(prisma.segment.create).mockResolvedValue(mockSegmentPrisma); diff --git a/apps/web/modules/ee/contacts/segments/lib/segments.ts b/apps/web/modules/ee/contacts/segments/lib/segments.ts index 557e69bce9f2..4b2631e3d196 100644 --- a/apps/web/modules/ee/contacts/segments/lib/segments.ts +++ b/apps/web/modules/ee/contacts/segments/lib/segments.ts @@ -348,6 +348,26 @@ export const resetSegmentInSurvey = async (surveyId: string): Promise } }; +// Batched lookup of the owning workspace for a set of surveys, used to keep segment↔survey links +// within one workspace (ENG-1920). A single query avoids fanning out one lookup per survey id over +// a caller-controlled, unbounded array. Missing ids are simply absent from the map. +export const getSurveyWorkspaceIdMap = reactCache( + async (surveyIds: string[]): Promise> => { + try { + const surveys = await prisma.survey.findMany({ + where: { id: { in: surveyIds } }, + select: { id: true, workspaceId: true }, + }); + return new Map(surveys.map((survey) => [survey.id, survey.workspaceId])); + } catch (error) { + if (error instanceof Prisma.PrismaClientKnownRequestError) { + throw new DatabaseError(error.message); + } + throw error; + } + } +); + export const updateSegment = async (segmentId: string, data: TSegmentUpdateInput): Promise => { validateInputs([segmentId, ZId], [data, ZSegmentUpdateInput]); diff --git a/apps/web/modules/ee/quotas/lib/quotas.test.ts b/apps/web/modules/ee/quotas/lib/quotas.test.ts index eece6a163b84..f967485ed575 100644 --- a/apps/web/modules/ee/quotas/lib/quotas.test.ts +++ b/apps/web/modules/ee/quotas/lib/quotas.test.ts @@ -193,12 +193,25 @@ describe("Quota Service", () => { const result = await updateQuota(updateInput, mockQuotaId); expect(result).toEqual(updatedQuota); + // surveyId is immutable on update (ENG-1749) and is stripped from the persisted data. + const { surveyId: _omittedSurveyId, ...expectedData } = updateInput; expect(prisma.surveyQuota.update).toHaveBeenCalledWith({ where: { id: mockQuotaId }, - data: updateInput, + data: expectedData, }); }); + // ENG-1749 sibling: updateQuota must not re-point a quota to a different (potentially + // cross-tenant) survey. Even when a foreign surveyId is supplied, it must not be persisted. + test("never persists a caller-supplied surveyId", async () => { + vi.mocked(prisma.surveyQuota.update).mockResolvedValue(mockQuota); + + await updateQuota({ ...updateInput, surveyId: "clvictimsurvey0000000000001" }, mockQuotaId); + + const arg = vi.mocked(prisma.surveyQuota.update).mock.calls[0][0]; + expect(arg.data).not.toHaveProperty("surveyId"); + }); + test("should throw DatabaseError when quota not found", async () => { const prismaError = new Prisma.PrismaClientKnownRequestError("Record not found", { code: "P2015", diff --git a/apps/web/modules/ee/quotas/lib/quotas.ts b/apps/web/modules/ee/quotas/lib/quotas.ts index 3ae15d8d322e..c987043c3825 100644 --- a/apps/web/modules/ee/quotas/lib/quotas.ts +++ b/apps/web/modules/ee/quotas/lib/quotas.ts @@ -72,9 +72,13 @@ export const createQuota = async (quota: TSurveyQuotaInput): Promise => { try { + // ENG-1749: surveyId is the quota's tenant anchor, set at creation and immutable thereafter. + // Persisting a caller-supplied surveyId here would let an authorized quota owner re-point their + // quota onto another tenant's survey, so it is stripped from the update payload. + const { surveyId: _surveyId, ...quotaData } = quota; const updatedQuota = await prisma.surveyQuota.update({ where: { id }, - data: quota, + data: quotaData, }); return updatedQuota; diff --git a/apps/web/modules/organization/settings/api-keys/lib/api-key.ts b/apps/web/modules/organization/settings/api-keys/lib/api-key.ts index 7ef0ed6c39b9..d6e49e0c1fdd 100644 --- a/apps/web/modules/organization/settings/api-keys/lib/api-key.ts +++ b/apps/web/modules/organization/settings/api-keys/lib/api-key.ts @@ -6,10 +6,11 @@ import { ApiKey, ApiKeyPermission, Prisma } from "@formbricks/database/prisma"; import { logger } from "@formbricks/logger"; import { TOrganizationAccess } from "@formbricks/types/api-key"; import { ZId } from "@formbricks/types/common"; -import { DatabaseError } from "@formbricks/types/errors"; +import { DatabaseError, OperationNotAllowedError } from "@formbricks/types/errors"; import { CONTROL_HASH } from "@/lib/constants"; import { hashSecret, hashSha256, parseApiKeyV2, verifySecret } from "@/lib/crypto"; import { validateInputs } from "@/lib/utils/validate"; +import { getWorkspacesByOrganizationId } from "@/modules/organization/settings/api-keys/lib/workspaces"; import { TApiKeyCreateInput, TApiKeyUpdateInput, @@ -61,6 +62,10 @@ export const getApiKeyWithPermissions = reactCache( select: { id: true, name: true, + // ENG-1749: needed so the auth layer can drop any workspace permission whose + // workspace is outside the key's organization (defense-in-depth for the read path, + // which authorizes off the permission list rather than resolveBodyIdsV2). + organizationId: true, }, }, }, @@ -161,6 +166,24 @@ export const createApiKey = async ( ): Promise => { validateInputs([organizationId, ZId], [apiKeyData, ZApiKeyCreateInput]); try { + // ENG-1749: an API key is created at the organization level but carries per-workspace + // permissions. Reject any workspace that does not belong to the authorized organization, + // otherwise a caller could mint a key scoped to another tenant's workspace (cross-tenant + // BOLA). Validate before generating/hashing the secret so illegitimate input does no work. + const { workspacePermissions, organizationAccess, ...apiKeyDataWithoutPermissions } = apiKeyData; + if (workspacePermissions && workspacePermissions.length > 0) { + const orgWorkspaceIds = new Set( + (await getWorkspacesByOrganizationId(organizationId)).map((workspace) => workspace.id) + ); + for (const { workspaceId } of workspacePermissions) { + if (!orgWorkspaceIds.has(workspaceId)) { + throw new OperationNotAllowedError( + `Workspace ${workspaceId} does not belong to organization ${organizationId}` + ); + } + } + } + // Generate a secure random secret (32 bytes base64url) const secret = randomBytes(32).toString("base64url"); @@ -171,8 +194,6 @@ export const createApiKey = async ( // 2. bcrypt hash const hashedKey = await hashSecret(secret, 12); - const { workspacePermissions, organizationAccess, ...apiKeyDataWithoutPermissions } = apiKeyData; - // Create the API key const result = await prisma.apiKey.create({ data: { diff --git a/apps/web/modules/organization/settings/api-keys/lib/api-keys.test.ts b/apps/web/modules/organization/settings/api-keys/lib/api-keys.test.ts index 0461c4a8a2dc..fed8a0892c92 100644 --- a/apps/web/modules/organization/settings/api-keys/lib/api-keys.test.ts +++ b/apps/web/modules/organization/settings/api-keys/lib/api-keys.test.ts @@ -1,7 +1,7 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; import { prisma } from "@formbricks/database"; import { ApiKey, ApiKeyPermission, Prisma } from "@formbricks/database/prisma"; -import { DatabaseError } from "@formbricks/types/errors"; +import { DatabaseError, OperationNotAllowedError } from "@formbricks/types/errors"; import { TApiKeyWithEnvironmentPermission } from "../types/api-keys"; import { createApiKey, @@ -10,6 +10,7 @@ import { getApiKeysWithEnvironmentPermissions, updateApiKey, } from "./api-key"; +import { getWorkspacesByOrganizationId } from "./workspaces"; const mockApiKey: ApiKey = { id: "apikey123", @@ -52,6 +53,10 @@ vi.mock("@formbricks/database", () => ({ }, })); +vi.mock("./workspaces", () => ({ + getWorkspacesByOrganizationId: vi.fn(), +})); + vi.mock("crypto", async () => { const actual = await vi.importActual("crypto"); return { @@ -344,7 +349,7 @@ describe("API Key Management", () => { apiKeyWorkspaces: { include: { workspace: { - select: { id: true, name: true }, + select: { id: true, name: true, organizationId: true }, }, }, }, @@ -366,7 +371,7 @@ describe("API Key Management", () => { apiKeyWorkspaces: { include: { workspace: { - select: { id: true, name: true }, + select: { id: true, name: true, organizationId: true }, }, }, }, @@ -452,6 +457,9 @@ describe("API Key Management", () => { }); test("creates an API key with environment permissions successfully", async () => { + vi.mocked(getWorkspacesByOrganizationId).mockResolvedValueOnce([ + { id: "workspace123", name: "Workspace 123" }, + ]); vi.mocked(prisma.apiKey.create).mockResolvedValueOnce(mockApiKeyWithEnvironments); const result = await createApiKey("org123", "user123", { @@ -463,6 +471,23 @@ describe("API Key Management", () => { expect(prisma.apiKey.create).toHaveBeenCalled(); }); + test("rejects a workspace permission for a workspace outside the organization (ENG-1749)", async () => { + // The organization owns only "own-workspace"; the caller attempts to scope the key to a + // victim organization's workspace. This must be refused before any key is persisted. + vi.mocked(getWorkspacesByOrganizationId).mockResolvedValueOnce([ + { id: "own-workspace", name: "Own Workspace" }, + ]); + + await expect( + createApiKey("org123", "user123", { + ...mockApiKeyData, + workspacePermissions: [{ workspaceId: "victim-workspace", permission: ApiKeyPermission.manage }], + }) + ).rejects.toThrow(OperationNotAllowedError); + expect(getWorkspacesByOrganizationId).toHaveBeenCalledWith("org123"); + expect(prisma.apiKey.create).not.toHaveBeenCalled(); + }); + test("rejects create input with duplicate workspaceId", async () => { await expect( createApiKey("org123", "user123", { diff --git a/apps/web/modules/organization/settings/api-keys/types/api-keys.ts b/apps/web/modules/organization/settings/api-keys/types/api-keys.ts index 5006273d8633..b1b6b1810cad 100644 --- a/apps/web/modules/organization/settings/api-keys/types/api-keys.ts +++ b/apps/web/modules/organization/settings/api-keys/types/api-keys.ts @@ -59,7 +59,7 @@ const ZApiKeyWorkspaceWithWorkspace = z.object({ apiKeyId: z.string(), workspaceId: z.string(), permission: ZApiKeyPermission, - workspace: ZWorkspace.pick({ id: true, name: true }), + workspace: ZWorkspace.pick({ id: true, name: true, organizationId: true }), }); const ZApiKey = z.object({ diff --git a/apps/web/modules/workspaces/settings/lib/workspace.test.ts b/apps/web/modules/workspaces/settings/lib/workspace.test.ts index 201ae004758c..2eec03bfc247 100644 --- a/apps/web/modules/workspaces/settings/lib/workspace.test.ts +++ b/apps/web/modules/workspaces/settings/lib/workspace.test.ts @@ -4,12 +4,13 @@ import { Prisma } from "@formbricks/database/prisma"; import { logger } from "@formbricks/logger"; import { StorageErrorCode } from "@formbricks/storage"; import { DatabaseError, InvalidInputError, ValidationError } from "@formbricks/types/errors"; +import { TWorkspace } from "@formbricks/types/workspace"; import { deleteFilesByWorkspaceId } from "@/modules/storage/service"; import { createWorkspace, deleteWorkspace, updateWorkspace } from "./workspace"; vi.mock("server-only", () => ({})); -const baseWorkspace = { +const baseWorkspace: TWorkspace = { id: "p1", createdAt: new Date(), updatedAt: new Date(), @@ -77,7 +78,7 @@ describe("workspace lib", () => { describe("updateWorkspace", () => { test("updates workspace and revalidates cache", async () => { - vi.mocked(prisma.workspace.update).mockResolvedValueOnce(baseWorkspace as any); + vi.mocked(prisma.workspace.update).mockResolvedValueOnce(baseWorkspace); const result = await updateWorkspace("p1", { name: "Workspace 1", }); @@ -102,6 +103,14 @@ describe("workspace lib", () => { const result = await updateWorkspace("p1", { name: "Workspace 1" }); expect(result).toEqual({ ...baseWorkspace, id: 123 }); }); + + // ENG-1919: a workspace must not be moved to another organization via update. + test("never persists a caller-supplied organizationId", async () => { + vi.mocked(prisma.workspace.update).mockResolvedValueOnce(baseWorkspace); + await updateWorkspace("p1", { name: "Workspace 1", organizationId: "attacker-target-org" }); + const arg = vi.mocked(prisma.workspace.update).mock.calls[0][0]; + expect(arg.data).not.toHaveProperty("organizationId"); + }); }); describe("createWorkspace", () => { diff --git a/apps/web/modules/workspaces/settings/lib/workspace.ts b/apps/web/modules/workspaces/settings/lib/workspace.ts index 672c6af57fc6..515169ed630d 100644 --- a/apps/web/modules/workspaces/settings/lib/workspace.ts +++ b/apps/web/modules/workspaces/settings/lib/workspace.ts @@ -77,7 +77,10 @@ export const updateWorkspace = async ( inputWorkspace: TWorkspaceUpdateInput ): Promise => { validateInputs([workspaceId, ZId], [inputWorkspace, ZWorkspaceUpdateInput]); - const { ...data } = inputWorkspace; + // ENG-1919: organizationId is the workspace's tenant anchor, set at creation and immutable on + // update. Persisting a caller-supplied organizationId here would let an authorized workspace + // owner move their workspace (and all its data) into another organization, so it is stripped. + const { organizationId: _organizationId, ...data } = inputWorkspace; let updatedWorkspace; try { updatedWorkspace = await prisma.workspace.update({