[pull] main from formbricks:main

apps/web/app/.well-known/oauth-protected-resource/route.test.ts

@@ -12,7 +12,7 @@ vi.mock("@/modules/auth/lib/oauth-urls", async (importOriginal) => ({
   getMcpResourceUrl: () => "https://app.example.com/api/mcp",
 }));
 
-const { MCP_OAUTH_SCOPES } = await import("@/modules/auth/lib/oauth-urls");
+const { MCP_OAUTH_SCOPES, MCP_CHALLENGE_SCOPE } = await import("@/modules/auth/lib/oauth-urls");
 
 const createRequest = () =>
   new NextRequest("https://app.example.com/.well-known/oauth-protected-resource/api/mcp");
@@ -73,6 +73,27 @@ describe("OAuth protected resource metadata", () => {
     }
   });
 
+  test("the 401 WWW-Authenticate challenge scope matches scopes_supported exactly", async () => {
+    // The regression guard for ENG-2175. A client that hits the MCP 401 before fetching this
+    // document uses the challenge string as its DCR `scope`; the oauth-provider then validates
+    // /authorize against the client's REGISTERED scopes. If the challenge is narrower than what
+    // this document advertises, the client registers narrow, authorizes wide, and its first
+    // connect fails with `invalid_scope` — succeeding only on retry, once the metadata is cached.
+    // The provider validates authorize as a subset of the registered scopes, so the challenge must
+    // COVER this list; a wider challenge would also pass. Asserting equality is the stricter check,
+    // and it holds because both derive from MCP_PROTECTED_RESOURCE_SCOPES — so a drift in either
+    // direction is a deliberate change, not an accident.
+    const response = await GET(createRequest(), {
+      params: Promise.resolve({ resource: ["api", "mcp"] }),
+    });
+    const { scopes_supported: scopesSupported } = (await response.json()) as {
+      scopes_supported: string[];
+    };
+
+    expect(MCP_CHALLENGE_SCOPE.split(" ")).toEqual(scopesSupported);
+    expect(MCP_CHALLENGE_SCOPE).toContain("offline_access");
+  });
+
   test("returns 404 for unrelated protected resource metadata paths", async () => {
     const response = await GET(createRequest(), {
       params: Promise.resolve({ resource: ["api", "other"] }),

apps/web/app/api/client/[workspaceId]/responses/lib/response-error.integration.test.ts

@@ -0,0 +1,74 @@
+import { beforeEach, describe, expect, test } from "vitest";
+import { prisma } from "@formbricks/database";
+import { DatabaseError, InvalidInputError, UniqueConstraintError } from "@formbricks/types/errors";
+import { resetDb } from "@/integration/reset-db";
+import { handleClientResponseCreateError } from "./response-error";
+
+/**
+ * ENG-2174, against the REAL Prisma 7 + @prisma/adapter-pg stack.
+ *
+ * The adapter builds the P2002 column list by regex-scraping the Postgres error DETAIL
+ * (`Key ("surveyId", "singleUseId")=(…)`) and never unquotes it, so every camelCase column arrives
+ * wrapped in double quotes. The exact-equality checks in `response-error.ts` therefore never matched
+ * and a routine duplicate submission fell through to `DatabaseError` — a 500, plus a Sentry report,
+ * instead of the documented 409.
+ *
+ * The unit tests could not have caught this: they build the meta by hand, and every fixture in the
+ * repo passed unquoted names. Only a genuine violation produces the quoting, so this drives one.
+ */
+beforeEach(async () => {
+  await resetDb();
+});
+
+const createSurvey = async () => {
+  const organization = await prisma.organization.create({ data: { name: "ENG-2174 Org" } });
+  const workspace = await prisma.workspace.create({
+    data: { name: "ENG-2174 Workspace", organizationId: organization.id },
+  });
+  return prisma.survey.create({ data: { name: "ENG-2174 Survey", workspaceId: workspace.id } });
+};
+
+describe("handleClientResponseCreateError vs real Prisma 7 + adapter-pg (ENG-2174)", () => {
+  test("maps a duplicate (surveyId, singleUseId) to a 409, not a 500", async () => {
+    const survey = await createSurvey();
+    const singleUseId = "eng2174-single-use";
+    await prisma.response.create({ data: { surveyId: survey.id, singleUseId, data: {} } });
+
+    const error = await prisma.response
+      .create({ data: { surveyId: survey.id, singleUseId, data: {} } })
+      .catch((e) => e);
+
+    expect(error?.code).toBe("P2002");
+    // The premise of the bug: the adapter reports the columns quoted.
+    const rawFields = (
+      error?.meta as { driverAdapterError?: { cause?: { constraint?: { fields?: string[] } } } }
+    )?.driverAdapterError?.cause?.constraint?.fields;
+    expect(rawFields).toContain('"singleUseId"');
+
+    expect(() => handleClientResponseCreateError(error)).toThrow(UniqueConstraintError);
+    expect(() => handleClientResponseCreateError(error)).toThrow(
+      "Response already submitted for this single-use link"
+    );
+    // Guards the actual regression: before the fix this fell through to DatabaseError (500).
+    expect(() => handleClientResponseCreateError(error)).not.toThrow(DatabaseError);
+  });
+
+  test("maps a duplicate displayId to a 400, not a 500", async () => {
+    const survey = await createSurvey();
+    const display = await prisma.display.create({ data: { surveyId: survey.id } });
+    await prisma.response.create({ data: { surveyId: survey.id, displayId: display.id, data: {} } });
+
+    const error = await prisma.response
+      .create({ data: { surveyId: survey.id, displayId: display.id, data: {} } })
+      .catch((e) => e);
+
+    expect(error?.code).toBe("P2002");
+    const rawFields = (
+      error?.meta as { driverAdapterError?: { cause?: { constraint?: { fields?: string[] } } } }
+    )?.driverAdapterError?.cause?.constraint?.fields;
+    expect(rawFields).toContain('"displayId"');
+
+    expect(() => handleClientResponseCreateError(error, display.id)).toThrow(InvalidInputError);
+    expect(() => handleClientResponseCreateError(error, display.id)).not.toThrow(DatabaseError);
+  });
+});

apps/web/app/api/mcp/route.test.ts

@@ -43,23 +43,23 @@ vi.mock("@formbricks/database", () => ({
   },
 }));
 
-vi.mock("@/modules/auth/lib/oauth-urls", () => ({
-  // Must mirror the real MCP_RESOURCE_SCOPES: the route's minimum-scope gate and its WWW-Authenticate
-  // challenge are both derived from this list, so a short mock would test a world production doesn't have.
-  MCP_RESOURCE_SCOPES: [
-    "surveys:read",
-    "surveys:write",
-    "workflows:read",
-    "workflows:write",
-    "feedbackRecords:read",
-    "feedbackRecords:write",
-  ],
+// Only the env-dependent URL getters are mocked. The scope constants are the real ones: the route's
+// minimum-scope gate and its WWW-Authenticate challenge are both derived from them, so literals here
+// would test a world production doesn't have and mask scope drift (ENG-2175).
+vi.mock("@/modules/auth/lib/oauth-urls", async (importOriginal) => ({
+  ...(await importOriginal<typeof import("@/modules/auth/lib/oauth-urls")>()),
   getAuthIssuerUrl: () => "http://localhost/api/auth",
   getMcpOrigin: () => "http://localhost",
   getMcpProtectedResourceMetadataUrl: () => "http://localhost/.well-known/oauth-protected-resource/api/mcp",
   getMcpResourceUrl: () => "http://localhost/api/mcp",
 }));
 
+const { MCP_CHALLENGE_SCOPE } = await import("@/modules/auth/lib/oauth-urls");
+// The auth-params are comma-separated per RFC 9110 §11.6.1 (#8718): asserting the whole string is what
+// keeps the separator from regressing, since a strict client parser needs it to read `resource_metadata`.
+// The scope list interpolates the real constant, so this stays honest as the advertised scopes change.
+const EXPECTED_CHALLENGE = `Bearer resource_metadata="http://localhost/.well-known/oauth-protected-resource/api/mcp", scope="${MCP_CHALLENGE_SCOPE}"`;
+
 vi.mock("@/modules/api/lib/api-key-auth", () => ({
   authenticateApiKeyFromHeaders: vi.fn(),
   getBearerTokenFromHeaders: vi.fn((headers: Headers) => {
@@ -170,9 +170,7 @@ describe("POST /api/mcp", () => {
 
     expect(response.status).toBe(401);
     expect(response.headers.get("Content-Type")).toBe("application/problem+json");
-    expect(response.headers.get("WWW-Authenticate")).toBe(
-      'Bearer resource_metadata="http://localhost/.well-known/oauth-protected-resource/api/mcp", scope="surveys:read surveys:write workflows:read workflows:write feedbackRecords:read feedbackRecords:write"'
-    );
+    expect(response.headers.get("WWW-Authenticate")).toBe(EXPECTED_CHALLENGE);
     expect(applyIPRateLimit).toHaveBeenCalled();
   });
 
@@ -444,9 +442,7 @@ describe("POST /api/mcp", () => {
     expect(response.status).toBe(401);
     expect(authenticateApiKeyFromHeaders).not.toHaveBeenCalled();
     expect(applyIPRateLimit).toHaveBeenCalled();
-    expect(response.headers.get("WWW-Authenticate")).toBe(
-      'Bearer resource_metadata="http://localhost/.well-known/oauth-protected-resource/api/mcp", scope="surveys:read surveys:write workflows:read workflows:write feedbackRecords:read feedbackRecords:write"'
-    );
+    expect(response.headers.get("WWW-Authenticate")).toBe(EXPECTED_CHALLENGE);
   });
 
   test("blocks write tools for read-only OAuth tokens", async () => {

apps/web/app/api/v3/surveys/lib/operations.test.ts

@@ -320,6 +320,16 @@ describe("createV3SurveyResponse", () => {
 
     expect(response.status).toBe(201);
     expect(response.headers.get("Location")).toBe("/api/v3/surveys/survey_1");
+    // Negative control for the level, not just the check: validateV3Survey was moved across this
+    // same seam from "readWrite" to "read" because it writes nothing. Create does write, so it must
+    // stay at "readWrite" — without this, the same move here would pass the suite.
+    expect(vi.mocked(requireV3WorkspaceAccess)).toHaveBeenCalledWith(
+      authentication,
+      workspaceId,
+      "readWrite",
+      requestId,
+      instance
+    );
     expect(vi.mocked(createV3Survey)).toHaveBeenCalledWith(
       expect.objectContaining({
         workspaceId,
@@ -913,10 +923,12 @@ describe("validateV3Survey", () => {
     } as any);
 
     expect(response.status).toBe(200);
+    // "read", not "readWrite": validation writes nothing, and the MCP validate_survey tool is
+    // registered surveys:read. Raising this back to readWrite re-breaks that tool (ENG-2179).
     expect(vi.mocked(requireV3WorkspaceAccess)).toHaveBeenCalledWith(
       authentication,
       workspaceId,
-      "readWrite",
+      "read",
       requestId,
       instance
     );
@@ -953,10 +965,11 @@ describe("validateV3Survey", () => {
     } as any);
 
     expect(response.status).toBe(200);
+    // See the create-branch note above: the patch dry run is gated at "read" too.
     expect(vi.mocked(getAuthorizedV3Survey)).toHaveBeenCalledWith({
       surveyId: validSurveyId,
       authentication,
-      access: "readWrite",
+      access: "read",
       requestId,
       instance,
     });

apps/web/app/api/v3/surveys/lib/operations.ts

@@ -690,6 +690,16 @@ export async function patchV3SurveyResponse({
   }
 }
 
+/**
+ * Dry-run validation of a create or patch payload. Neither branch writes: the create branch is a
+ * pure function of the input, and the patch branch merges the payload into the loaded survey in
+ * memory. Both are therefore gated at `read`, not `readWrite`.
+ *
+ * That level is load-bearing for the MCP `validate_survey` tool, which is registered `surveys:read`
+ * (and annotated `readOnlyHint`). Requiring write here made a read-scoped agent 403 on a tool that
+ * mutates nothing (ENG-2179). If this is ever raised back to `readWrite`, that tool's declared scope
+ * has to move with it.
+ */
 export async function validateV3Survey({
   body,
   authentication,
@@ -710,7 +720,7 @@ export async function validateV3Survey({
         const authResult = await requireV3WorkspaceAccess(
           authentication,
           workspaceResult.data.workspaceId,
-          "readWrite",
+          "read",
           requestId,
           instance
         );
@@ -732,7 +742,7 @@ export async function validateV3Survey({
     const { survey, response } = await getAuthorizedV3Survey({
       surveyId: validationBody.surveyId,
       authentication,
-      access: "readWrite",
+      access: "read",
       requestId,
       instance,
     });

apps/web/lib/utils/prisma-constraint.integration.test.ts

@@ -48,4 +48,25 @@ describe("getUniqueConstraintFields vs real Prisma 7 + adapter-pg (ENG-1801)", (
     // The adapter reports the DB column name (`token_hash`), not the Prisma field name (`tokenHash`).
     expect(getUniqueConstraintFields(error)).toEqual(["token_hash"]);
   });
+
+  test("unquotes camelCase columns, which Postgres quotes in the error DETAIL (ENG-2174)", async () => {
+    const organization = await prisma.organization.create({ data: { name: "ENG-2174 quoting" } });
+    await prisma.workspace.create({ data: { name: "Duplicate", organizationId: organization.id } });
+
+    const error = await prisma.workspace
+      .create({ data: { name: "Duplicate", organizationId: organization.id } })
+      .catch((e) => e);
+
+    expect(error?.code).toBe("P2002");
+    // The premise: Postgres quotes any identifier that is not all-lowercase, and the adapter
+    // regex-scrapes the DETAIL without unquoting — so the raw meta carries the quotes.
+    const rawFields = (
+      error?.meta as { driverAdapterError?: { cause?: { constraint?: { fields?: string[] } } } }
+    )?.driverAdapterError?.cause?.constraint?.fields;
+    expect(rawFields).toContain('"organizationId"');
+
+    // ...and the helper hands back usable Prisma field names. Note `name` is lowercase and therefore
+    // never quoted, which is why callers that only read fields[0] kept working by luck.
+    expect(getUniqueConstraintFields(error)).toEqual(["organizationId", "name"]);
+  });
 });

apps/web/lib/utils/prisma-constraint.test.ts

@@ -54,4 +54,48 @@ describe("getUniqueConstraintFields", () => {
   test("filters out non-string entries defensively", () => {
     expect(getUniqueConstraintFields(legacyP2002(["email", null as unknown as string]))).toEqual(["email"]);
   });
+
+  // The adapter regex-scrapes the Postgres DETAIL, which quotes every identifier that isn't
+  // all-lowercase. Before this was handled, `includes("singleUseId")` never matched `'"singleUseId"'`
+  // and duplicate single-use responses fell through to a 500 instead of a 409 (ENG-2174).
+  describe("quoted identifiers (Postgres quote_identifier)", () => {
+    test("unquotes a fully quoted composite key", () => {
+      expect(getUniqueConstraintFields(adapterP2002(['"surveyId"', '"singleUseId"']))).toEqual([
+        "surveyId",
+        "singleUseId",
+      ]);
+    });
+
+    test("unquotes a single quoted column", () => {
+      expect(getUniqueConstraintFields(adapterP2002(['"displayId"']))).toEqual(["displayId"]);
+    });
+
+    test("handles a mixed list, leaving the bare lowercase column untouched", () => {
+      // ActionClass_name_workspaceId_key — callers that read fields[0] must be unaffected.
+      const fields = getUniqueConstraintFields(adapterP2002(["name", '"workspaceId"']));
+      expect(fields).toEqual(["name", "workspaceId"]);
+      expect(fields[0]).toBe("name");
+    });
+
+    test("leaves @map()ed snake_case columns unchanged (Postgres never quotes them)", () => {
+      expect(getUniqueConstraintFields(adapterP2002(["token_hash"]))).toEqual(["token_hash"]);
+    });
+
+    test("normalises the legacy shape too, so both shapes stay interchangeable", () => {
+      expect(getUniqueConstraintFields(legacyP2002(['"email"']))).toEqual(["email"]);
+      expect(getUniqueConstraintFields(legacyP2002(["email"]))).toEqual(["email"]);
+    });
+
+    test("only strips a matched outer pair", () => {
+      // A lone quote is not a pair; an inner quote is part of the identifier.
+      expect(getUniqueConstraintFields(adapterP2002(['"']))).toEqual(['"']);
+      expect(getUniqueConstraintFields(adapterP2002(['""']))).toEqual([""]);
+      expect(getUniqueConstraintFields(adapterP2002(['"a"b"']))).toEqual(['a"b']);
+    });
+
+    test("is idempotent — an already-bare name survives a second pass", () => {
+      const once = getUniqueConstraintFields(adapterP2002(['"surveyId"']));
+      expect(getUniqueConstraintFields(adapterP2002(once))).toEqual(once);
+    });
+  });
 });

apps/web/lib/utils/prisma-constraint.ts

@@ -14,6 +14,26 @@ const UNIQUE_CONSTRAINT_VIOLATION = "P2002";
 export const isUniqueConstraintError = (error: unknown): error is PrismaClientKnownRequestError =>
   error instanceof Prisma.PrismaClientKnownRequestError && error.code === UNIQUE_CONSTRAINT_VIOLATION;
 
+/**
+ * Strips one symmetric pair of double quotes from a column name.
+ *
+ * `@prisma/adapter-pg` derives the column list by regex-scraping the Postgres error DETAIL
+ * (`Key ("surveyId", "singleUseId")=(…)`) and never unquotes it. Postgres quotes any identifier
+ * `quote_identifier()` does not consider safe to leave bare — not all-lowercase, starting with a
+ * digit, containing anything outside `[a-z0-9_]`, or colliding with a keyword — so `singleUseId`
+ * arrives as `"singleUseId"` while `token_hash` arrives bare. (`quote_all_identifiers = on` quotes
+ * everything, which this also handles.)
+ *
+ * Only a matched outer pair is removed, so already-bare names pass through byte-identical. Applied
+ * to the legacy `meta.target` shape too: that engine does not quote, but running both branches
+ * through the same normaliser keeps the two interchangeable for callers and tests.
+ */
+const unquoteIdentifier = (field: string): string =>
+  field.length >= 2 && field.startsWith('"') && field.endsWith('"') ? field.slice(1, -1) : field;
+
+const toColumnNames = (fields: unknown[]): string[] =>
+  fields.filter((field): field is string => typeof field === "string").map(unquoteIdentifier);
+
 /**
  * Returns the column names involved in a P2002 unique-constraint violation.
  *
@@ -28,7 +48,8 @@ export const isUniqueConstraintError = (error: unknown): error is PrismaClientKn
  *
  * Security: only the structured column names are returned. Never surface `originalMessage`, the
  * constraint name, or any other raw `driverAdapterError.cause` string to a response or log — the
- * underlying Postgres unique-violation detail can contain the offending value (PII).
+ * underlying Postgres unique-violation detail can contain the offending value (PII). Stripping the
+ * quotes below is deliberately the *only* string processing done here, for the same reason.
  */
 export const getUniqueConstraintFields = (error: PrismaClientKnownRequestError): string[] => {
   const meta = error.meta as
@@ -41,13 +62,13 @@ export const getUniqueConstraintFields = (error: PrismaClientKnownRequestError):
   // Legacy / library-engine shape.
   const legacyTarget = meta?.target;
   if (Array.isArray(legacyTarget)) {
-    return legacyTarget.filter((field): field is string => typeof field === "string");
+    return toColumnNames(legacyTarget);
   }
 
   // Prisma 7 driver-adapter shape.
   const adapterFields = meta?.driverAdapterError?.cause?.constraint?.fields;
   if (Array.isArray(adapterFields)) {
-    return adapterFields.filter((field): field is string => typeof field === "string");
+    return toColumnNames(adapterFields);
   }
 
   return [];

apps/web/modules/auth/lib/oauth-urls.ts

@@ -71,3 +71,18 @@ export const MCP_PROTECTED_RESOURCE_SCOPES = [
   ...MCP_RESOURCE_SCOPES,
   "offline_access",
 ] as const satisfies readonly (typeof MCP_OAUTH_SCOPES)[number][];
+
+/**
+ * The `scope` advertised in the 401 `WWW-Authenticate` challenge from the MCP endpoint.
+ *
+ * A client that hits the 401 *before* fetching the protected-resource metadata uses this string as
+ * its Dynamic Client Registration `scope`, then authorizes with the scopes the metadata advertises.
+ * The oauth-provider validates an authorize request as a *subset* of the client's registered scopes,
+ * so the invariant is that this list must **cover** the metadata list. A narrower challenge means the
+ * client registers narrow, authorizes wide, and is rejected with `invalid_scope` — failing its first
+ * connect and succeeding only on retry, once the metadata is cached (ENG-2175).
+ *
+ * Derived from the same array so the two are identical, which satisfies the invariant by construction
+ * and leaves nothing to drift.
+ */
+export const MCP_CHALLENGE_SCOPE = MCP_PROTECTED_RESOURCE_SCOPES.join(" ");