diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 5e382cc35ec1..26f64191af69 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -421,13 +421,23 @@ reviews: "Open gaps". - A row names a behaviour with no verification method, or a method with no observed outcome. - - A `unit` or `e2e` row does not name the test or spec it rests on. A `manual` row has no - test to name and passes without one. - - A row claims `red on main` or `mutation` without a command or a mutated `file:line` a - reviewer could rerun. + - A `unit` or `e2e` row names no test or spec. The name may sit in the row or in the + `Rerun:` line above the table, which carries it once for every row — but a bare suite + command such as `pnpm test` names nothing, so those rows name their own. A `manual` row + has no test to name and passes without one. + - A row claims `red on main` and gives nothing to rerun: neither the `Rerun:` line nor a + command of its own where that differs. + - A row claims `mutation` and does not name the mutated `file:line` a reviewer would edit + to turn it red. - The diff changes rendered UI and the description attaches no screenshot or screencast. - The "Open gaps" subsection is absent. An explicit "none" passes. + Content inside a `
` fold counts as present. The template caps the description at + 350 words outside folds and folds the overflow rather than dropping it — long media, + command logs, and Coverage rows past the sixth — while the Coverage table, Open gaps and + Breaking changes stay visible themselves. A collapsed fold is evidence, not a gap: do not + ask for a longer description, ask for the missing behaviour. + PASS if every behaviour the diff changes is either covered by a row stating how it was verified and what was observed, or listed under "Open gaps" with a reason. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index ccf34cca58f3..2cc726e679e9 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,25 +1,41 @@ - - - - - - - + + + + + + Fixes ENG- ## What & why - + + +**Was:** `` + +**Now:** `` + +## Where to look + + + +- ## Breaking changes @@ -67,68 +83,49 @@ None ## Migrations & env - + - none ## How this was tested - -**Coverage** +pass. `Rerun:` carries the command once, so no row below repeats it. --> - +Rerun: `` - +**Coverage** - + | Behaviour | How | Outcome | | --- | --- | --- | -| | unit (red on main) / unit (mutation) / unit (guard) / e2e / manual | | +| | | | **Open gaps** - + - none **Risks** - + - none --- - + > [!NOTE] > **AI model used** — ``, reasoning effort ``. diff --git a/AGENTS.md b/AGENTS.md index a3082b4f2f48..910f8c14f023 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -212,6 +212,15 @@ where the behavior only exists once browser, survey bundle, and server are wired The spec filenames in `apps/web/playwright/` are the inventory of covered areas — check there before concluding an area has no spec. +The PR's Coverage table names each row's level with one of five words, and the first three are claims a +reviewer can check: `unit (red on main)` fails against the old code, so it proves the bug existed; +`unit (mutation)` only fails if you break the fix, because the code under test is new; `unit (guard)` +passes either way, protecting against a future regression; `e2e` and `manual` say where the check ran. Every +`unit` and `e2e` row names the test or spec it rests on, in the row or in a `Rerun:` line that names +it — a bare `pnpm test` names nothing. A `unit (red on main)` row is rerunnable from the +`Rerun:` command, or names its own where that differs; a `unit (mutation)` row names the mutated +`file:line` a reviewer edits to turn it red. + This raises a floor as well as lowering a ceiling. Every feature area ships a happy-path E2E, and an area with none is a gap rather than a saving (Dashboards and Workflows are the current examples — ENG-2314). A bug fix inside a feature that already has one almost never needs a second spec — the level still follows @@ -298,6 +307,10 @@ Commits follow a lightweight Conventional Commit format (`fix:`, `chore:`, `feat Every PR must use `.github/pull_request_template.md` and follow its inline guidance — the template is the source of truth for PR structure. The ticket line at the top is the only place a magic word (`Fixes`, `Ref`, `Closes`) may sit next to a ticket id: Linear and GitHub scan the whole body, so the same pair written in prose — inside backticks too — links and closes that ticket as well. When you need to name the convention in prose, write it without a resolvable id. All QA for a change happens on its own PR before review: the creator shows that every behaviour the diff changes is covered, and lists what is not under `Open gaps`; the reviewer challenges that list and asks for the missing coverage. There is no separate release QA pass per PR — release review only looks for problems arising from the interplay of several changes. Fill every section from the actual diff on PR open, and re-update it in the same turn on every change (new commits, scope or review fixes) so it never drifts — treat a stale section as a bug. +**A PR description is read, not filed.** Keep the whole thing under 350 words outside `
` folds — one screen — with lists of at most three bullets of at most twenty words and a Coverage table of at most six rows. Open `## What & why` with a `**Was:**` / `**Now:**` pair: one plain sentence for how it behaved before, one for what happens now. User-visible effect first, mechanism second, written for a colleague who has not read the ticket. Under `## Where to look`, link the one to three places that carry the risk so a reviewer can spot-check the code without reading all of it. Detail that does not fit goes into a fold rather than being dropped — the evidence stays in the PR, out of the reviewer's way. Four things never belong at any length: blame archaeology, a defence of a choice nobody questioned or of what you deliberately did not do, commentary on how strong your own tests are, and anything the `Rerun:` line already carries. + +The agent note names the exact model id the vendor serves — `claude-opus-5`, `gpt-5.1-codex` — not the harness it runs in; Claude Code, Codex CLI and Cursor are harnesses, so name one in parentheses only when it adds something (`claude-opus-5 (Claude Code 2.1.237)`). The reasoning level is whatever knob that vendor exposes, in its own units: an effort level (`max`, `high`), a thinking budget (`32k tokens`), or `n/a`. Read both out of the tool, never from memory — Claude Code reports them in `/status` or as the session's `model` and `effort_level`, Codex CLI in `/model` or its startup line. A value you cannot look up is `unknown`, never a guess and never the harness name standing in for the model. + The checkbox under `## Breaking changes` is a decision you own, not a formality: judge the diff against the template's list of breaking changes and tick it (`- [x]`) when one applies, leave it unticked when none does. The template also lists what is **not** breaking — purely additive changes, and anything internal to this repo that no external consumer reaches — and an uncertain call is an unticked box with a line of reasoning, never a defensive tick. It is the only input to the `breaking-change` label, which feeds the release notes and the self-hoster migration guide, so a wrong answer either invents a migration entry or hides one. Re-check it whenever the diff grows. `pr-label-sync.yml` reads nothing but the tick, so the prose below the checkbox cannot change the label — but it is not free-form either: the CodeRabbit `Breaking changes match the diff` check compares the tick against the diff and expects a ticked box to document each breaking change, so explain your answer there in whatever shape fits (table or prose). ## Next.js Documentation diff --git a/apps/web/lib/survey/service.test.ts b/apps/web/lib/survey/service.test.ts index 67565da0396f..bef47b3f1bb1 100644 --- a/apps/web/lib/survey/service.test.ts +++ b/apps/web/lib/survey/service.test.ts @@ -1,4 +1,5 @@ import { prisma } from "@/lib/__mocks__/database"; +import { createId } from "@paralleldrive/cuid2"; import { beforeEach, describe, expect, test, vi } from "vitest"; import { testInputValidation } from "vitestSetup"; import { ActionClass, Prisma, Survey } from "@formbricks/database/prisma"; @@ -11,7 +12,12 @@ import { ResourceNotFoundError, ValidationError, } from "@formbricks/types/errors"; -import { TBaseFilters, TSegment } from "@formbricks/types/segment"; +import { + MAX_SEGMENT_FILTERS_PER_TREE, + MAX_SEGMENT_SURVEYS, + TBaseFilters, + TSegment, +} from "@formbricks/types/segment"; import { TSurveyFollowUp } from "@formbricks/types/surveys/follow-up"; import { TSurvey, TSurveyCreateInput, TSurveyQuestionTypeEnum } from "@formbricks/types/surveys/types"; import { getActionClasses } from "@/lib/actionClass/service"; @@ -524,6 +530,210 @@ describe("Tests for updateSurvey", () => { expect(prisma.segment.update).not.toHaveBeenCalled(); }); + // ENG-2305: the draft save (skipValidation=true) deliberately skips full semantic validation, + // but the filter tree BOUNDS must hold unconditionally — an over-bounds tree persisted through + // a draft would break every consumer that parses the row back (publish validation, clone, + // evaluation). + test("rejects an over-bounds segment filter tree even when validation is skipped (draft save)", async () => { + prisma.survey.findUnique.mockResolvedValueOnce({ ...mockSurveyOutput, status: "draft" }); + prisma.segment.findUnique.mockResolvedValueOnce({ + workspaceId: updateSurveyInput.workspaceId, + } as any); // segment belongs to the survey's workspace (passes the segment guard) + + // One node over the tree-wide cap. Junk-shaped on purpose: the bounds walk counts raw nodes + // without requiring valid filter shapes, exactly what an unvalidated draft can carry. + const overBoundsFilters = Array.from({ length: MAX_SEGMENT_FILTERS_PER_TREE + 1 }, (_, index) => ({ + id: `f_${index}`, + connector: index === 0 ? null : "and", + resource: {}, + })); + + await expect( + updateSurveyInternal( + { + ...updateSurveyInput, + status: "draft", + type: "app", + segment: { + id: "clownsegment000000000001", + title: "seg", + description: null, + isPrivate: false, + filters: overBoundsFilters, + workspaceId: updateSurveyInput.workspaceId, + surveys: [], + createdAt: new Date(), + updatedAt: new Date(), + }, + } as any, + true + ) + ).rejects.toThrow(InvalidInputError); + + expect(prisma.segment.update).not.toHaveBeenCalled(); + expect(prisma.survey.update).not.toHaveBeenCalled(); + }); + + // Locks the draft UX the bounds check must not regress: a half-built (semantically invalid, + // within-bounds) filter tree still saves when validation is skipped. + test("still saves a draft whose segment filters are within bounds but semantically invalid", async () => { + prisma.survey.findUnique.mockResolvedValueOnce({ ...mockSurveyOutput, status: "draft" }); + prisma.segment.findUnique.mockResolvedValueOnce({ + workspaceId: updateSurveyInput.workspaceId, + } as any); + prisma.survey.update.mockResolvedValueOnce({ ...mockSurveyOutput, status: "draft" } as any); + + // Fails ZSegmentFilters (junk leaf shape, non-cuid id) but is far inside every tree bound. + const halfBuiltFilters = [{ id: "f1", connector: null, resource: { root: { type: "attribute" } } }]; + + await updateSurveyInternal( + { + ...updateSurveyInput, + status: "draft", + type: "app", + segment: { + id: "clownsegment000000000001", + title: "seg", + description: null, + isPrivate: false, + filters: halfBuiltFilters, + workspaceId: updateSurveyInput.workspaceId, + surveys: [], + createdAt: new Date(), + updatedAt: new Date(), + }, + } as any, + true + ); + + expect(prisma.segment.update).toHaveBeenCalledWith( + expect.objectContaining({ + where: { id: "clownsegment000000000001" }, + data: expect.objectContaining({ filters: halfBuiltFilters }), + }) + ); + }); + + // ENG-2305 sibling of the filter-tree gate: on the draft path segment.surveys is just as + // unvalidated as the filter tree (ZSurveyDraft.segment is an untyped record), so the id-format + // rule and MAX_SEGMENT_SURVEYS cap must hold unconditionally — BEFORE the ids drive the batched + // workspace lookup. An over-limit or junk draft must perform no survey queries at all. + test("rejects an over-cap segment.surveys list on the draft path without any survey lookup", async () => { + prisma.survey.findUnique.mockResolvedValueOnce({ ...mockSurveyOutput, status: "draft" }); + prisma.segment.findUnique.mockResolvedValueOnce({ + workspaceId: updateSurveyInput.workspaceId, + } as any); + + // Every id is a valid cuid2 — only the cap can reject this list. + const overCapSurveys = Array.from({ length: MAX_SEGMENT_SURVEYS + 1 }, () => createId()); + + await expect( + updateSurveyInternal( + { + ...updateSurveyInput, + status: "draft", + type: "app", + segment: { + id: "clownsegment000000000001", + title: "seg", + description: null, + isPrivate: false, + filters: [], + workspaceId: updateSurveyInput.workspaceId, + surveys: overCapSurveys, + createdAt: new Date(), + updatedAt: new Date(), + }, + } as any, + true + ) + ).rejects.toThrow(InvalidInputError); + + expect(prisma.survey.findMany).not.toHaveBeenCalled(); + expect(prisma.segment.update).not.toHaveBeenCalled(); + expect(prisma.survey.update).not.toHaveBeenCalled(); + }); + + test("rejects a non-id junk string in segment.surveys on the draft path without any survey lookup", async () => { + prisma.survey.findUnique.mockResolvedValueOnce({ ...mockSurveyOutput, status: "draft" }); + prisma.segment.findUnique.mockResolvedValueOnce({ + workspaceId: updateSurveyInput.workspaceId, + } as any); + + await expect( + updateSurveyInternal( + { + ...updateSurveyInput, + status: "draft", + type: "app", + segment: { + id: "clownsegment000000000001", + title: "seg", + description: null, + isPrivate: false, + filters: [], + workspaceId: updateSurveyInput.workspaceId, + surveys: ["not-a-survey-id"], + createdAt: new Date(), + updatedAt: new Date(), + }, + } as any, + true + ) + ).rejects.toThrow(InvalidInputError); + + expect(prisma.survey.findMany).not.toHaveBeenCalled(); + expect(prisma.segment.update).not.toHaveBeenCalled(); + expect(prisma.survey.update).not.toHaveBeenCalled(); + }); + + // Guard test locking the happy path: a within-cap, well-formed surveys list still flows into + // the ownership lookup and connects exactly as before. + test("still runs the ownership lookup for a within-cap segment.surveys list on the draft path", async () => { + prisma.survey.findUnique.mockResolvedValueOnce({ ...mockSurveyOutput, status: "draft" }); + prisma.segment.findUnique.mockResolvedValueOnce({ + workspaceId: updateSurveyInput.workspaceId, + } as any); + const connectedSurveyId = createId(); + prisma.survey.findMany.mockResolvedValueOnce([ + { id: connectedSurveyId, workspaceId: updateSurveyInput.workspaceId }, + ] as any); + prisma.survey.update.mockResolvedValueOnce({ ...mockSurveyOutput, status: "draft" } as any); + + await updateSurveyInternal( + { + ...updateSurveyInput, + status: "draft", + type: "app", + segment: { + id: "clownsegment000000000001", + title: "seg", + description: null, + isPrivate: false, + filters: [], + workspaceId: updateSurveyInput.workspaceId, + surveys: [connectedSurveyId], + createdAt: new Date(), + updatedAt: new Date(), + }, + } as any, + true + ); + + expect(prisma.survey.findMany).toHaveBeenCalledWith({ + where: { id: { in: [connectedSurveyId] } }, + select: { id: true, workspaceId: true }, + }); + expect(prisma.segment.update).toHaveBeenCalledWith( + expect.objectContaining({ + where: { id: "clownsegment000000000001" }, + data: expect.objectContaining({ + surveys: { connect: [{ id: connectedSurveyId }] }, + }), + }) + ); + }); + // Archived surveys are read-only on every write path that flows through updateSurveyInternal // (editor save, summary status dropdown, v1/v3 update) — not just the v3 API layer. test("rejects updating an archived survey and does not write", async () => { diff --git a/apps/web/lib/survey/service.ts b/apps/web/lib/survey/service.ts index f9e1dc45ae0f..54d7cf3810e3 100644 --- a/apps/web/lib/survey/service.ts +++ b/apps/web/lib/survey/service.ts @@ -10,7 +10,13 @@ import { OperationNotAllowedError, ResourceNotFoundError, } from "@formbricks/types/errors"; -import { TBaseFilters, ZSegmentFilters } from "@formbricks/types/segment"; +import { + MAX_SEGMENT_SURVEYS, + TBaseFilters, + ZSegmentFilters, + ZSegmentSurveyIds, + getSegmentFilterTreeBoundsViolation, +} from "@formbricks/types/segment"; import { TSurveyBlock } from "@formbricks/types/surveys/blocks"; import { TSurvey, TSurveyCreateInput, ZSurvey, ZSurveyCreateInput } from "@formbricks/types/surveys/types"; import { scheduleFeedbackSourceReconciliation } from "@/lib/feedback-source/mapping-reconciliation"; @@ -428,12 +434,32 @@ export const updateSurveyInternal = async ( // if the survey body has type other than "app" but has a private segment, we delete that segment, and if it has a public segment, we disconnect from to the survey if (segment) { if (type === "app") { + // ENG-2305: tree bounds are enforced UNCONDITIONALLY — the draft save (skipValidation) + // deliberately skips full semantic validation so half-built filters can be saved, but an + // over-bounds tree persisted through it would break every consumer that parses the row + // back (publish validation, clone, evaluation). + const boundsViolation = getSegmentFilterTreeBoundsViolation(segment.filters); + if (boundsViolation) { + throw new InvalidInputError(boundsViolation); + } + // parse the segment filters: const parsedFilters = ZSegmentFilters.safeParse(segment.filters); if (!skipValidation && !parsedFilters.success) { throw new InvalidInputError("Invalid user segment filters"); } + // ENG-2305 sibling of the filter-tree gate above: on the draft path (skipValidation) + // segment.surveys reaches this point unvalidated — ZSurveyDraft.segment is an untyped + // record, so neither the ZId format rule nor the MAX_SEGMENT_SURVEYS cap has applied. Both + // must hold unconditionally BEFORE the ids drive the batched workspace lookup below; the + // validated (non-draft) path re-checks the same schema it already passed, a no-op. + if (segment.surveys && !ZSegmentSurveyIds.safeParse(segment.surveys).success) { + throw new InvalidInputError( + `Invalid segment surveys: at most ${MAX_SEGMENT_SURVEYS} valid survey ids are allowed` + ); + } + // 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 diff --git a/apps/web/modules/ee/contacts/segments/lib/helper.test.ts b/apps/web/modules/ee/contacts/segments/lib/helper.test.ts index be6357599f39..324bcd9ed72d 100644 --- a/apps/web/modules/ee/contacts/segments/lib/helper.test.ts +++ b/apps/web/modules/ee/contacts/segments/lib/helper.test.ts @@ -1,6 +1,10 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; import { InvalidInputError } from "@formbricks/types/errors"; -import { TBaseFilters, TSegmentWithSurveyRefs } from "@formbricks/types/segment"; +import { + MAX_SEGMENT_SURVEY_INTERACTION_IDS_PER_TREE, + TBaseFilters, + TSegmentWithSurveyRefs, +} from "@formbricks/types/segment"; import { assertSurveyInteractionSurveyIds, checkForRecursiveSegmentFilter, @@ -336,4 +340,82 @@ describe("assertSurveyInteractionSurveyIds", () => { assertSurveyInteractionSurveyIds(buildSpecificFilter(["s1", "s2"]), "workspace-1") ).rejects.toThrow(new InvalidInputError("Survey not found in workspace: s2")); }); + + test("deduplicates ids collected across filters before querying", async () => { + const filters = [ + ...buildSpecificFilter(["s1", "s2"]), + ...buildSpecificFilter(["s2", "s1"]), + ] as unknown as TBaseFilters; + mockSurveyFindMany.mockResolvedValue([{ id: "s1" }, { id: "s2" }]); + + await expect(assertSurveyInteractionSurveyIds(filters, "workspace-1")).resolves.toBeUndefined(); + + expect(mockSurveyFindMany).toHaveBeenCalledTimes(1); + expect(mockSurveyFindMany).toHaveBeenCalledWith({ + where: { id: { in: ["s1", "s2"] }, workspaceId: "workspace-1" }, + select: { id: true }, + }); + }); + + test("splits an oversized id list into bounded sequential batches, all workspace-scoped", async () => { + const surveyIds = Array.from({ length: 450 }, (_, index) => `s_${index}`); + mockSurveyFindMany.mockImplementation(async ({ where }: any) => + where.id.in.map((id: string) => ({ id })) + ); + + await expect( + assertSurveyInteractionSurveyIds(buildSpecificFilter(surveyIds), "workspace-1") + ).resolves.toBeUndefined(); + + // 450 ids at a batch size of 200 -> 200 / 200 / 50, each query scoped to the workspace. + const batchSizes = mockSurveyFindMany.mock.calls.map(([args]: any) => args.where.id.in.length); + expect(batchSizes).toEqual([200, 200, 50]); + for (const [args] of mockSurveyFindMany.mock.calls) { + expect((args as any).where.workspaceId).toBe("workspace-1"); + } + }); + + test("throws on the first missing id in collection order and stops querying further batches", async () => { + const surveyIds = Array.from({ length: 450 }, (_, index) => `s_${index}`); + // Two ids in the second batch are foreign/unknown; the earlier one must win, and the third + // batch must never be queried. + mockSurveyFindMany.mockImplementation(async ({ where }: any) => + where.id.in.filter((id: string) => id !== "s_205" && id !== "s_210").map((id: string) => ({ id })) + ); + + await expect( + assertSurveyInteractionSurveyIds(buildSpecificFilter(surveyIds), "workspace-1") + ).rejects.toThrow(new InvalidInputError("Survey not found in workspace: s_205")); + + expect(mockSurveyFindMany).toHaveBeenCalledTimes(2); + }); + + test("a parsed-tree-max payload (the tree-wide id cap) resolves in exactly five batches", async () => { + // Callers pass ZSegmentFilters-parsed trees, so the largest total this guard can receive is + // MAX_SEGMENT_SURVEY_INTERACTION_IDS_PER_TREE ids — pin that worst case: ceil(1000/200) = 5 + // sequential batches, no more. + const filterCount = MAX_SEGMENT_SURVEY_INTERACTION_IDS_PER_TREE / 100; + const filters = Array.from({ length: filterCount }, (_, filterIndex) => ({ + id: `f_${filterIndex}`, + connector: filterIndex === 0 ? null : "and", + resource: { + id: `si_${filterIndex}`, + root: { type: "surveyInteraction" }, + qualifier: { operator: "haveSeen" }, + value: { + surveyScope: "specific", + surveyIds: Array.from({ length: 100 }, (_, idIndex) => `s_${filterIndex * 100 + idIndex}`), + within: { amount: 1, unit: "months" }, + }, + }, + })) as unknown as TBaseFilters; + mockSurveyFindMany.mockImplementation(async ({ where }: any) => + where.id.in.map((id: string) => ({ id })) + ); + + await expect(assertSurveyInteractionSurveyIds(filters, "workspace-1")).resolves.toBeUndefined(); + + const batchSizes = mockSurveyFindMany.mock.calls.map(([args]: any) => args.where.id.in.length); + expect(batchSizes).toEqual([200, 200, 200, 200, 200]); + }); }); diff --git a/apps/web/modules/ee/contacts/segments/lib/helper.ts b/apps/web/modules/ee/contacts/segments/lib/helper.ts index 94852f15a6dc..ec22a5987614 100644 --- a/apps/web/modules/ee/contacts/segments/lib/helper.ts +++ b/apps/web/modules/ee/contacts/segments/lib/helper.ts @@ -2,7 +2,10 @@ import { prisma } from "@formbricks/database"; import { InvalidInputError } from "@formbricks/types/errors"; import { TBaseFilters, TSegmentSurveyInteractionFilter } from "@formbricks/types/segment"; import { getSegment } from "@/modules/ee/contacts/segments/lib/segments"; -import { isResourceFilter } from "@/modules/ee/contacts/segments/lib/utils"; +import { + SURVEY_WORKSPACE_LOOKUP_BATCH_SIZE, + isResourceFilter, +} from "@/modules/ee/contacts/segments/lib/utils"; /** * Checks if a segment filter contains a recursive reference to itself @@ -66,25 +69,29 @@ export const collectSurveyIdsFromSegmentFilters = (filters: TBaseFilters): strin * Ensures every survey referenced by a "specific" survey-interaction filter belongs to the given * workspace. This is the tenancy guard for interaction filters — the runtime evaluation query is * already workspace-scoped, but we reject unknown/foreign ids at write time to avoid persisting - * dead references. + * dead references. The deduplicated ids are looked up in bounded batches, sequentially: each batch + * is checked before the next query runs, so the first missing id (in collection order) still + * rejects, and no further queries are issued after a rejection. Callers pass ZSegmentFilters-parsed + * trees, so the total is already capped (MAX_SEGMENT_SURVEY_INTERACTION_IDS_PER_TREE — a handful of + * batches at most); the chunking is defense in depth for each query's parameter payload and for any + * future caller that skips the parse. * @throws {InvalidInputError} When a referenced survey is not found in the workspace */ export const assertSurveyInteractionSurveyIds = async (filters: TBaseFilters, workspaceId: string) => { const surveyIds = Array.from(new Set(collectSurveyIdsFromSegmentFilters(filters))); - if (surveyIds.length === 0) { - return; - } - - const foundSurveys = await prisma.survey.findMany({ - where: { id: { in: surveyIds }, workspaceId }, - select: { id: true }, - }); + for (let i = 0; i < surveyIds.length; i += SURVEY_WORKSPACE_LOOKUP_BATCH_SIZE) { + const batch = surveyIds.slice(i, i + SURVEY_WORKSPACE_LOOKUP_BATCH_SIZE); + const foundSurveys = await prisma.survey.findMany({ + where: { id: { in: batch }, workspaceId }, + select: { id: true }, + }); - const foundIds = new Set(foundSurveys.map((survey) => survey.id)); - const missingId = surveyIds.find((id) => !foundIds.has(id)); + const foundIds = new Set(foundSurveys.map((survey) => survey.id)); + const missingId = batch.find((id) => !foundIds.has(id)); - if (missingId) { - throw new InvalidInputError(`Survey not found in workspace: ${missingId}`); + if (missingId) { + throw new InvalidInputError(`Survey not found in workspace: ${missingId}`); + } } }; diff --git a/apps/web/modules/ee/contacts/segments/lib/segment-schema.test.ts b/apps/web/modules/ee/contacts/segments/lib/segment-schema.test.ts index 0ada141d1338..75a3b0ad753b 100644 --- a/apps/web/modules/ee/contacts/segments/lib/segment-schema.test.ts +++ b/apps/web/modules/ee/contacts/segments/lib/segment-schema.test.ts @@ -1,7 +1,10 @@ import { createId } from "@paralleldrive/cuid2"; import { describe, expect, test } from "vitest"; import { + MAX_SEGMENT_FILTERS_PER_TREE, + MAX_SEGMENT_FILTER_DEPTH, MAX_SEGMENT_SURVEYS, + MAX_SEGMENT_SURVEY_INTERACTION_IDS_PER_TREE, type TBaseFilters, type TSurveyInteractionOperator, ZSegmentCreateInput, @@ -126,6 +129,149 @@ describe("segment schema validation", () => { }); }); +describe("segment filter tree bounds", () => { + const attributeLeaf = (connector: "and" | null) => ({ + id: createId(), + connector, + resource: { + id: createId(), + root: { type: "attribute" as const, contactAttributeKey: "email" }, + value: "user@example.com", + qualifier: { operator: "equals" as const }, + }, + }); + + const flatTree = (length: number) => + Array.from({ length }, (_, index) => attributeLeaf(index === 0 ? null : "and")); + + // One group node per level around a single innermost leaf: depth === node count. Built with a + // loop on purpose — a recursive builder would blow the stack on the deep trees this exercises. + const leftSpine = (depth: number): unknown[] => { + let filters: unknown[] = [attributeLeaf(null)]; + for (let level = 1; level < depth; level++) { + filters = [{ id: createId(), connector: null, resource: filters }]; + } + return filters; + }; + + const interactionLeaf = (connector: "and" | null, surveyIdCount: number) => ({ + id: createId(), + connector, + resource: { + id: createId(), + root: { type: "surveyInteraction" as const }, + qualifier: { operator: "haveSeen" as const }, + value: { + surveyScope: "specific" as const, + surveyIds: Array.from({ length: surveyIdCount }, () => createId()), + within: { amount: 1, unit: "months" as const }, + }, + }, + }); + + const interactionTree = (filterCount: number, surveyIdsPerFilter: number) => + Array.from({ length: filterCount }, (_, index) => + interactionLeaf(index === 0 ? null : "and", surveyIdsPerFilter) + ); + + test("accepts a flat tree with exactly the maximum number of filters", () => { + const result = ZSegmentFilters.safeParse(flatTree(MAX_SEGMENT_FILTERS_PER_TREE)); + + expect(result.success).toBe(true); + }); + + test("rejects a flat tree one filter over the cap", () => { + const result = ZSegmentFilters.safeParse(flatTree(MAX_SEGMENT_FILTERS_PER_TREE + 1)); + + expect(result.success).toBe(false); + expect(result.error?.issues[0]?.message).toBe( + `Too many filters: a segment supports at most ${MAX_SEGMENT_FILTERS_PER_TREE} filters in total` + ); + }); + + test("rejects an over-cap tree hidden behind nesting (small arrays at every level)", () => { + // Each wrap keeps the per-level array at 25 entries (24 leaves + 1 nested group) and the depth + // within MAX_SEGMENT_FILTER_DEPTH, so any per-level `.max()` (and the depth bound) would pass — + // only the whole-tree node bound catches the total. + let filters: unknown[] = flatTree(25); + let nodeCount = 25; + while (nodeCount <= MAX_SEGMENT_FILTERS_PER_TREE) { + filters = [...flatTree(24), { id: createId(), connector: "and" as const, resource: filters }]; + nodeCount += 25; + } + + const result = ZSegmentFilters.safeParse(filters); + + expect(result.success).toBe(false); + expect(result.error?.issues[0]?.message).toBe( + `Too many filters: a segment supports at most ${MAX_SEGMENT_FILTERS_PER_TREE} filters in total` + ); + }); + + test("accepts a nested tree under the cap", () => { + const filters = [...flatTree(3), { id: createId(), connector: "and" as const, resource: flatTree(5) }]; + + const result = ZSegmentFilters.safeParse(filters); + + expect(result.success).toBe(true); + }); + + test("rejects an over-cap tree submitted through a segment update", () => { + const result = ZSegmentUpdateInput.safeParse({ + filters: flatTree(MAX_SEGMENT_FILTERS_PER_TREE + 1), + }); + + expect(result.success).toBe(false); + }); + + test("a deep left-spine tree within the node cap fails with a clean depth issue, not a RangeError", () => { + // Regression: depth === MAX_SEGMENT_FILTERS_PER_TREE nodes is within the node cap, but deep + // enough that the recursive parse alone would overflow the call stack (safeParse does not catch + // RangeError). The bounds gate must reject it BEFORE the recursive parse ever runs. + const result = ZSegmentFilters.safeParse(leftSpine(MAX_SEGMENT_FILTERS_PER_TREE)); + + expect(result.success).toBe(false); + expect(result.error?.issues[0]?.message).toBe( + `Segment filters are nested too deeply: at most ${MAX_SEGMENT_FILTER_DEPTH} levels are supported` + ); + }); + + test("accepts a tree nested exactly at the depth limit", () => { + const result = ZSegmentFilters.safeParse(leftSpine(MAX_SEGMENT_FILTER_DEPTH)); + + expect(result.success).toBe(true); + }); + + test("rejects a tree nested one level over the depth limit", () => { + const result = ZSegmentFilters.safeParse(leftSpine(MAX_SEGMENT_FILTER_DEPTH + 1)); + + expect(result.success).toBe(false); + expect(result.error?.issues[0]?.message).toBe( + `Segment filters are nested too deeply: at most ${MAX_SEGMENT_FILTER_DEPTH} levels are supported` + ); + }); + + test("accepts survey-interaction filters totalling exactly the tree-wide id cap", () => { + // 10 filters x 100 ids: each filter at its own per-filter cap, tree total exactly at the bound. + const result = ZSegmentFilters.safeParse( + interactionTree(10, MAX_SEGMENT_SURVEY_INTERACTION_IDS_PER_TREE / 10) + ); + + expect(result.success).toBe(true); + }); + + test("rejects survey-interaction filters totalling one id over the tree-wide cap", () => { + // 11 filters x 91 ids = 1001: every filter is under the per-filter cap (100), so only the + // tree-wide total can catch it. + const result = ZSegmentFilters.safeParse(interactionTree(11, 91)); + + expect(result.success).toBe(false); + expect(result.error?.issues[0]?.message).toBe( + `Too many surveys referenced: survey-interaction filters may reference at most ${MAX_SEGMENT_SURVEY_INTERACTION_IDS_PER_TREE} surveys in total` + ); + }); +}); + describe("survey interaction filter value validation", () => { test("accepts any-survey scope with empty surveyIds", () => { const result = ZSegmentSurveyInteractionFilterValue.safeParse({ 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 d046ddda00c8..ac062824be9e 100644 --- a/apps/web/modules/ee/contacts/segments/lib/segments.test.ts +++ b/apps/web/modules/ee/contacts/segments/lib/segments.test.ts @@ -256,6 +256,40 @@ describe("Segment Service Tests", () => { expect(result.size).toBe(0); expect(prisma.survey.findMany).not.toHaveBeenCalled(); }); + + test("deduplicates ids before querying", async () => { + vi.mocked(prisma.survey.findMany).mockResolvedValue([{ id: "survey1" }] as any); + + const result = await getExistingWorkspaceSurveyIds("ws_1", ["survey1", "survey1", "survey1"]); + + expect(result).toEqual(new Set(["survey1"])); + expect(prisma.survey.findMany).toHaveBeenCalledTimes(1); + expect(prisma.survey.findMany).toHaveBeenCalledWith({ + where: { workspaceId: "ws_1", id: { in: ["survey1"] } }, + select: { id: true }, + }); + }); + + test("splits an oversized id list into bounded batches and still returns only the found subset", async () => { + const surveyIds = Array.from({ length: 450 }, (_, index) => `survey_${index}`); + // survey_449 (last batch) is foreign/unknown — everything else belongs to the workspace. + vi.mocked(prisma.survey.findMany).mockImplementation((async ({ where }: any) => + where.id.in.filter((id: string) => id !== "survey_449").map((id: string) => ({ id }))) as any); + + const result = await getExistingWorkspaceSurveyIds("ws_1", surveyIds); + + // 450 ids at a batch size of 200 -> 200 / 200 / 50, each query scoped to the workspace. + const batchSizes = vi + .mocked(prisma.survey.findMany) + .mock.calls.map(([args]: any) => args.where.id.in.length); + expect(batchSizes).toEqual([200, 200, 50]); + for (const [args] of vi.mocked(prisma.survey.findMany).mock.calls) { + expect((args as any).where.workspaceId).toBe("ws_1"); + } + expect(result.size).toBe(449); + expect(result.has("survey_0")).toBe(true); + expect(result.has("survey_449")).toBe(false); + }); }); describe("createSegment", () => { diff --git a/apps/web/modules/ee/contacts/segments/lib/segments.ts b/apps/web/modules/ee/contacts/segments/lib/segments.ts index 59555b03b4e7..90e24c347407 100644 --- a/apps/web/modules/ee/contacts/segments/lib/segments.ts +++ b/apps/web/modules/ee/contacts/segments/lib/segments.ts @@ -35,7 +35,11 @@ import { } from "@formbricks/types/segment"; import { getSurvey } from "@/lib/survey/service"; import { validateInputs } from "@/lib/utils/validate"; -import { isResourceFilter, searchForAttributeKeyInSegment } from "@/modules/ee/contacts/segments/lib/utils"; +import { + SURVEY_WORKSPACE_LOOKUP_BATCH_SIZE, + isResourceFilter, + searchForAttributeKeyInSegment, +} from "@/modules/ee/contacts/segments/lib/utils"; import { isSameDay, subtractTimeUnit } from "./date-utils"; import { combineFilterResults, evaluateSurveyInteractionFilterInMemory } from "./filter/survey-interaction"; @@ -177,18 +181,28 @@ export const getSurveyRefsForWorkspace = reactCache( * {@link getSurveyRefsForWorkspace} (which caps at {@link SURVEY_FILTER_REF_LIMIT} for the picker), * this queries by the exact ids being validated, so it stays correct for workspaces with more than * that many surveys — a referenced-but-valid survey outside the picker's recent window is no longer - * wrongly rejected. Returns an empty set for an empty input without hitting the DB. + * wrongly rejected. Ids are deduplicated and looked up in bounded sequential batches (ENG-2305), + * mirroring {@link getSurveyWorkspaceIdMap}. Returns an empty set for an empty input without + * hitting the DB. */ export const getExistingWorkspaceSurveyIds = reactCache( async (workspaceId: string, surveyIds: string[]): Promise> => { validateInputs([workspaceId, ZId], [surveyIds, z.array(ZId)]); - if (surveyIds.length === 0) return new Set(); + const uniqueSurveyIds = Array.from(new Set(surveyIds)); + const existingSurveyIds = new Set(); + if (uniqueSurveyIds.length === 0) return existingSurveyIds; try { - const surveys = await prisma.survey.findMany({ - where: { workspaceId, id: { in: surveyIds } }, - select: { id: true }, - }); - return new Set(surveys.map((survey) => survey.id)); + for (let i = 0; i < uniqueSurveyIds.length; i += SURVEY_WORKSPACE_LOOKUP_BATCH_SIZE) { + const batch = uniqueSurveyIds.slice(i, i + SURVEY_WORKSPACE_LOOKUP_BATCH_SIZE); + const surveys = await prisma.survey.findMany({ + where: { workspaceId, id: { in: batch } }, + select: { id: true }, + }); + for (const survey of surveys) { + existingSurveyIds.add(survey.id); + } + } + return existingSurveyIds; } catch (error) { if (error instanceof Prisma.PrismaClientKnownRequestError) { throw new DatabaseError(error.message); @@ -415,12 +429,6 @@ export const resetSegmentInSurvey = async (surveyId: string): Promise } }; -// Upper bound on ids per `IN (...)` lookup below. ZSegment/ZSegmentUpdateInput already cap what a -// client can submit (MAX_SEGMENT_SURVEYS), but this helper is also reached with ids read back from -// the database (updateSurveyInternal's skipValidation path, rows persisted before the cap existed), -// so it bounds the query itself instead of trusting the caller's array length. -const SURVEY_WORKSPACE_LOOKUP_BATCH_SIZE = 200; - /** * Batched lookup of the owning workspace for a set of surveys, used to keep segment↔survey links * within one workspace (ENG-1920). Ids are deduplicated and queried in bounded batches — one query diff --git a/apps/web/modules/ee/contacts/segments/lib/utils.ts b/apps/web/modules/ee/contacts/segments/lib/utils.ts index 099475cd43be..ec16ff62b38f 100644 --- a/apps/web/modules/ee/contacts/segments/lib/utils.ts +++ b/apps/web/modules/ee/contacts/segments/lib/utils.ts @@ -24,6 +24,17 @@ export const isResourceFilter = (resource: TSegmentFilter | TBaseFilters): resou return (resource as TSegmentFilter).root !== undefined; }; +/** + * Upper bound on ids per `IN (...)` survey lookup in the segment write path (segments.ts and + * helper.ts). The Zod boundary already bounds what a client can submit (MAX_SEGMENT_SURVEYS, + * MAX_SEGMENT_SURVEY_INTERACTION_IDS_PER_TREE), so batching is defense in depth: it keeps each + * query's SQL parameter payload flat for within-cap totals (a handful of batches at most) and + * holds for callers whose arrays never went through those schemas (the survey editor's draft-save + * path). Batches run sequentially — the point is to cap per-query and concurrent database work, + * not to fan it out. + */ +export const SURVEY_WORKSPACE_LOOKUP_BATCH_SIZE = 200; + export const convertOperatorToText = (operator: TAllOperators, t: TFunction) => { switch (operator) { case "equals": diff --git a/packages/types/segment.ts b/packages/types/segment.ts index c3c611a789b7..aded7bc69119 100644 --- a/packages/types/segment.ts +++ b/packages/types/segment.ts @@ -367,16 +367,6 @@ export interface TBaseFilter { export type TBaseFilters = TBaseFilter[]; -export const ZBaseFilter: z.ZodType = z.lazy(() => - z.object({ - id: ZId, - connector: ZSegmentConnector, - resource: z.union([ZSegmentFilter, ZBaseFilters]), - }) -); - -export const ZBaseFilters: z.ZodType = z.lazy(() => z.array(ZBaseFilter)); - // here again, we refine the filters to make sure that the filters are valid const refineFilters = (filters: TBaseFilters): boolean => { let result = true; @@ -396,20 +386,131 @@ const refineFilters = (filters: TBaseFilters): boolean => { return result; }; +/** + * Maximum number of filter nodes — leaf filters plus nested groups — across the WHOLE recursive + * filter tree. The tree schema has no per-level length cap, so a per-level `.max()` alone would be + * bypassable by nesting; only a total bound keeps the tree size itself from being an unbounded + * payload (ENG-2305, sibling of ENG-2004). The same schema also parses trees read back from the + * database (clone, publish validation, segment editor), so the cap is deliberately generous — + * orders of magnitude above anything the segment editor produces — to never brick a pre-existing + * segment on read. + */ +export const MAX_SEGMENT_FILTERS_PER_TREE = 1000; + +/** + * Maximum nesting depth of the filter tree. The recursive Zod parse (and every recursive consumer + * of a parsed tree) grows the JS call stack with nesting depth and overflows around depth ~1000 in + * practice — a RangeError that `safeParse` does NOT catch, so no post-parse `.refine` can guard + * against it; only a pre-parse check can (see ZSegmentFilters). The editor produces single-digit + * depth; 50 leaves ~20x headroom below the measured stack limit. + */ +export const MAX_SEGMENT_FILTER_DEPTH = 50; + +/** + * Maximum total surveyIds across every survey-interaction filter in the tree. The per-filter cap + * (100) times the node cap would still admit 100k ids in one request — hundreds of sequential + * batched lookups, each holding a pooled DB connection. 1000 total ids means at most a handful of + * batches at write time, and is far beyond any tree the editor produces. + */ +export const MAX_SEGMENT_SURVEY_INTERACTION_IDS_PER_TREE = 1000; + +const countLeafSurveyInteractionIds = (resource: unknown): number => { + if (typeof resource !== "object" || resource === null) return 0; + const { root, value } = resource as { root?: unknown; value?: unknown }; + if (typeof root !== "object" || root === null) return 0; + if ((root as { type?: unknown }).type !== "surveyInteraction") return 0; + if (typeof value !== "object" || value === null) return 0; + const surveyIds = (value as { surveyIds?: unknown }).surveyIds; + return Array.isArray(surveyIds) ? surveyIds.length : 0; +}; + +/** + * Measures the raw (untrusted, unparsed) filter tree in one linear pass: total nodes, max nesting + * depth, and total survey-interaction surveyIds. Iterative (explicit stack) on purpose — this walk + * is what protects the recursive parse from stack overflow, so it must not recurse itself. Junk + * shapes are simply not counted; shape errors are the schema's job. + */ +const measureSegmentFilterTree = ( + raw: unknown +): { nodes: number; depth: number; surveyInteractionIds: number } => { + let nodes = 0; + let depth = 0; + let surveyInteractionIds = 0; + + const pending: { group: unknown[]; level: number }[] = []; + if (Array.isArray(raw)) { + pending.push({ group: raw, level: 1 }); + } + + let entry = pending.pop(); + while (entry) { + const { group, level } = entry; + depth = Math.max(depth, level); + for (const node of group) { + nodes += 1; + const resource = + typeof node === "object" && node !== null ? (node as { resource?: unknown }).resource : undefined; + if (Array.isArray(resource)) { + pending.push({ group: resource, level: level + 1 }); + } else { + surveyInteractionIds += countLeafSurveyInteractionIds(resource); + } + } + entry = pending.pop(); + } + + return { nodes, depth, surveyInteractionIds }; +}; + +/** + * Returns a human-readable message when the raw tree exceeds any bound, or null when it is within + * all of them. Shared by ZSegmentFilters (rejects at the schema boundary, before the recursive + * parse) and the survey draft-save path, which skips full semantic validation but must never + * persist an over-bounds tree. + */ +export const getSegmentFilterTreeBoundsViolation = (raw: unknown): string | null => { + const { nodes, depth, surveyInteractionIds } = measureSegmentFilterTree(raw); + + if (depth > MAX_SEGMENT_FILTER_DEPTH) { + return `Segment filters are nested too deeply: at most ${MAX_SEGMENT_FILTER_DEPTH} levels are supported`; + } + if (nodes > MAX_SEGMENT_FILTERS_PER_TREE) { + return `Too many filters: a segment supports at most ${MAX_SEGMENT_FILTERS_PER_TREE} filters in total`; + } + if (surveyInteractionIds > MAX_SEGMENT_SURVEY_INTERACTION_IDS_PER_TREE) { + return `Too many surveys referenced: survey-interaction filters may reference at most ${MAX_SEGMENT_SURVEY_INTERACTION_IDS_PER_TREE} surveys in total`; + } + return null; +}; + // The filters can be nested, so we need to use z.lazy to define the type // more on recusrsive types -> https://zod.dev/?id=recursive-types -export const ZSegmentFilters: z.ZodType = z +const ZSegmentFiltersInner: z.ZodType = z .array( z.object({ id: ZId, connector: ZSegmentConnector, - resource: z.union([ZSegmentFilter, z.lazy(() => ZSegmentFilters)]), + resource: z.union([ZSegmentFilter, z.lazy(() => ZSegmentFiltersInner)]), }) ) .refine(refineFilters, { error: "Invalid filters applied", }); +// Bounds gate + recursive parse. The bounds MUST run on the raw value before the recursive parse: +// a deep enough tree overflows the call stack inside the parse itself, throwing a RangeError that +// even safeParse does not catch — so a post-parse refine could never see it. `.pipe` short-circuits +// on failure, so the recursive inner schema never sees a tree that failed the bounds check. +export const ZSegmentFilters: z.ZodType = z + .unknown() + .superRefine((raw, ctx) => { + const violation = getSegmentFilterTreeBoundsViolation(raw); + if (violation) { + ctx.addIssue({ code: "custom", message: violation }); + } + }) + .pipe(ZSegmentFiltersInner); + const ZRequiredSegmentFilters = ZSegmentFilters.refine((filters) => filters.length > 0, { error: "At least one filter is required", }); @@ -425,7 +526,9 @@ export const MAX_SEGMENT_SURVEYS = 500; // `ZId` (cuid2) also keeps non-id junk from reaching the database. Ownership is still enforced at // write time — every id must resolve to a survey in the segment's workspace (ENG-1749/ENG-1920). -const ZSegmentSurveyIds = z.array(ZId).max(MAX_SEGMENT_SURVEYS); +// Exported so the survey draft-save path (which skips the segment schemas) can enforce the exact +// same rule before the ids drive its batched workspace lookup (ENG-2305). +export const ZSegmentSurveyIds = z.array(ZId).max(MAX_SEGMENT_SURVEYS); export const ZSegment = z.object({ id: z.string(),