diff --git a/apps/web/app/.well-known/oauth-protected-resource/route.test.ts b/apps/web/app/.well-known/oauth-protected-resource/route.test.ts index a139b2b6aa8e..5b8d3b9a978e 100644 --- a/apps/web/app/.well-known/oauth-protected-resource/route.test.ts +++ b/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"] }), diff --git a/apps/web/app/api/client/[workspaceId]/responses/lib/response-error.integration.test.ts b/apps/web/app/api/client/[workspaceId]/responses/lib/response-error.integration.test.ts new file mode 100644 index 000000000000..aa813ef457bc --- /dev/null +++ b/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); + }); +}); diff --git a/apps/web/app/api/mcp/route.test.ts b/apps/web/app/api/mcp/route.test.ts index 3ed4312af999..6e89d7f4bff5 100644 --- a/apps/web/app/api/mcp/route.test.ts +++ b/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()), 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 () => { diff --git a/apps/web/app/api/v3/surveys/lib/operations.test.ts b/apps/web/app/api/v3/surveys/lib/operations.test.ts index c97d7209c547..fcdbf1341679 100644 --- a/apps/web/app/api/v3/surveys/lib/operations.test.ts +++ b/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, }); diff --git a/apps/web/app/api/v3/surveys/lib/operations.ts b/apps/web/app/api/v3/surveys/lib/operations.ts index 8b358bd6eaf9..d8d5b3fd3d62 100644 --- a/apps/web/app/api/v3/surveys/lib/operations.ts +++ b/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, }); diff --git a/apps/web/lib/utils/prisma-constraint.integration.test.ts b/apps/web/lib/utils/prisma-constraint.integration.test.ts index 1dae19618f7c..7b7296942ed1 100644 --- a/apps/web/lib/utils/prisma-constraint.integration.test.ts +++ b/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"]); + }); }); diff --git a/apps/web/lib/utils/prisma-constraint.test.ts b/apps/web/lib/utils/prisma-constraint.test.ts index 4e84d0aa27af..e31c7beace13 100644 --- a/apps/web/lib/utils/prisma-constraint.test.ts +++ b/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); + }); + }); }); diff --git a/apps/web/lib/utils/prisma-constraint.ts b/apps/web/lib/utils/prisma-constraint.ts index 75f29587dd2b..6beb6a3add73 100644 --- a/apps/web/lib/utils/prisma-constraint.ts +++ b/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 []; diff --git a/apps/web/modules/auth/lib/oauth-urls.ts b/apps/web/modules/auth/lib/oauth-urls.ts index 27814c613d7f..744e13371f3e 100644 --- a/apps/web/modules/auth/lib/oauth-urls.ts +++ b/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(" "); diff --git a/apps/web/modules/ee/audit-logs/lib/handler.test.ts b/apps/web/modules/ee/audit-logs/lib/handler.test.ts index 24c69d2c2a41..c1b39cad9557 100644 --- a/apps/web/modules/ee/audit-logs/lib/handler.test.ts +++ b/apps/web/modules/ee/audit-logs/lib/handler.test.ts @@ -89,7 +89,9 @@ const baseEventParams = { status: "success" as TAuditStatus, oldObject: { foo: "bar" }, newObject: { foo: "baz" }, - apiUrl: "/api/test", + // Absolute: the schema validates apiUrl with z.url(). This file mocks the service out, so a bare + // path would pass here while being dropped in production (see service.test.ts). + apiUrl: "http://localhost:3000/api/test", }; const fullUser = { diff --git a/apps/web/modules/ee/audit-logs/lib/service.test.ts b/apps/web/modules/ee/audit-logs/lib/service.test.ts index 3f2d2eca6957..9f54faabeda9 100644 --- a/apps/web/modules/ee/audit-logs/lib/service.test.ts +++ b/apps/web/modules/ee/audit-logs/lib/service.test.ts @@ -61,6 +61,43 @@ describe("logAuditEvent", () => { await logAuditEvent(validEvent); expect(logger.error).toHaveBeenCalled(); }); + + // `apiUrl` is validated with z.url(), and a validation failure is swallowed into a logger.error + // rather than surfaced — so a producer passing a malformed value silently drops the whole audit + // event. That is exactly how every MCP mutation went unaudited (ENG-2173). + describe("apiUrl", () => { + beforeEach(() => { + getIsAuditLogsEnabled.mockResolvedValue(true); + }); + + test("drops the event when apiUrl is a bare path", async () => { + await logAuditEvent({ ...validEvent, apiUrl: "/api/mcp" } as any); + expect(logger.audit).not.toHaveBeenCalled(); + expect(logger.error).toHaveBeenCalled(); + }); + + test("logs the event when apiUrl is absolute", async () => { + const event = { ...validEvent, apiUrl: "http://localhost:3000/api/mcp" }; + await logAuditEvent(event); + expect(logger.audit).toHaveBeenCalledWith(event); + expect(logger.error).not.toHaveBeenCalled(); + }); + + test("logs the event when apiUrl is omitted (optional, unlike a malformed one)", async () => { + await logAuditEvent(validEvent); + expect(logger.audit).toHaveBeenCalledWith(validEvent); + expect(logger.error).not.toHaveBeenCalled(); + }); + + // Ties the MCP producer to this schema: the exact value the MCP tools pass must be one this + // validator accepts, which is the invariant that broke. + test("accepts the apiUrl the MCP tools actually pass", async () => { + const { getMcpResourceUrl } = await import("@/modules/auth/lib/oauth-urls"); + await logAuditEvent({ ...validEvent, apiUrl: getMcpResourceUrl() }); + expect(logger.audit).toHaveBeenCalled(); + expect(logger.error).not.toHaveBeenCalled(); + }); + }); }); describe("logAuditEvent export", () => { diff --git a/apps/web/modules/mcp/auth.test.ts b/apps/web/modules/mcp/auth.test.ts index 4dc7ae72ff86..c40c981789c1 100644 --- a/apps/web/modules/mcp/auth.test.ts +++ b/apps/web/modules/mcp/auth.test.ts @@ -45,27 +45,11 @@ vi.mock("@/modules/core/rate-limit/helpers", () => ({ applyRateLimit: vi.fn().mockResolvedValue(undefined), })); -vi.mock("@/modules/auth/lib/oauth-urls", () => ({ - MCP_OAUTH_SCOPES: [ - "openid", - "profile", - "email", - "offline_access", - "surveys:read", - "surveys:write", - "workflows:read", - "workflows:write", - "feedbackRecords:read", - "feedbackRecords:write", - ], - MCP_RESOURCE_SCOPES: [ - "surveys:read", - "surveys:write", - "workflows:read", - "workflows:write", - "feedbackRecords:read", - "feedbackRecords:write", - ], +// Only the env-dependent URL getters are stubbed; the scope constants come from the real module. +// Re-declaring them here would assert the mock against itself and mask scope drift — which is how +// the challenge list silently diverged from the protected-resource metadata (ENG-2175). +vi.mock("@/modules/auth/lib/oauth-urls", async (importOriginal) => ({ + ...(await importOriginal()), getAuthIssuerUrl: vi.fn(() => "https://app.example.com/api/auth"), getMcpOrigin: vi.fn(() => "https://app.example.com"), getMcpProtectedResourceMetadataUrl: vi.fn( @@ -83,6 +67,16 @@ vi.mock("@formbricks/logger", () => ({ }, })); +// Imported dynamically so it resolves against the partially-mocked module above rather than being +// hoisted past it. +const { MCP_CHALLENGE_SCOPE } = await import("@/modules/auth/lib/oauth-urls"); + +// Two comma-separated quoted auth-params, per the `#auth-param` list grammar in RFC 9110 §11.6.1 (#8718). +// Cases that care about the challenge's *shape* assert this rather than the whole string, so they do not +// have to be rewritten every time the advertised scope list changes; the exact value is pinned once, from +// the real constant, in app/api/mcp/route.test.ts. +const CHALLENGE_GRAMMAR = /^Bearer resource_metadata="[^"]+", scope="[^"]+"$/; + const apiKeyAuth = { type: "apiKey" as const, apiKeyId: "key_1", @@ -124,10 +118,11 @@ describe("authenticateMcpRequest", () => { if (!result.ok) { expect(result.response.status).toBe(401); // The challenge must advertise every resource scope so clients request them at consent and can - // reach the write tools (advertising only read is why write was unreachable — ENG-1055 QA). - expect(result.response.headers.get("WWW-Authenticate")).toContain( - 'scope="surveys:read surveys:write workflows:read workflows:write feedbackRecords:read feedbackRecords:write"' - ); + // reach the write tools (advertising only read is why write was unreachable — ENG-1055 QA), + // plus offline_access so a client that registers from this string can still be granted a + // refresh token (ENG-2175). Asserted against the real constant, not a literal. + expect(result.response.headers.get("WWW-Authenticate")).toContain(`scope="${MCP_CHALLENGE_SCOPE}"`); + expect(MCP_CHALLENGE_SCOPE).toContain("offline_access"); expect(await result.response.json()).toMatchObject({ code: "not_authenticated", detail: "API key or OAuth access token required", @@ -347,9 +342,13 @@ describe("authenticateMcpRequest", () => { if (!result.ok) { expect(result.response.status).toBe(401); // Auth-params are comma-separated (RFC 9110 `#auth-param`), so a strict client parser can read - // `resource_metadata` out of the challenge instead of choking on the whole tail. - expect(result.response.headers.get("WWW-Authenticate")).toBe( - 'Bearer resource_metadata="https://app.example.com/.well-known/oauth-protected-resource/api/mcp", scope="surveys:read surveys:write workflows:read workflows:write feedbackRecords:read feedbackRecords:write"' + // `resource_metadata` out of the challenge instead of choking on the whole tail. What this case is + // really about is the metadata URL being built from the configured origin, so assert that plus the + // grammar — not the scope list, which is pinned from the real constant elsewhere. + const challenge = result.response.headers.get("WWW-Authenticate"); + expect(challenge).toMatch(CHALLENGE_GRAMMAR); + expect(challenge).toContain( + 'resource_metadata="https://app.example.com/.well-known/oauth-protected-resource/api/mcp"' ); expect(await result.response.json()).toMatchObject({ detail: "Invalid OAuth access token", @@ -380,6 +379,10 @@ describe("authenticateMcpRequest", () => { if (!result.ok) { expect(result.response.status).toBe(403); expect(result.response.headers.get("WWW-Authenticate")).toContain('error="insufficient_scope"'); + // Deliberately the RESOURCE scopes, not the challenge scope: RFC 6750 `scope` names what the + // resource requires, and offline_access is not one of those. If this ever needs offline_access + // added, the baseline auth gate has been widened and MCP is accepting a token that grants no + // resource access. expect(result.response.headers.get("WWW-Authenticate")).toContain( 'scope="surveys:read surveys:write workflows:read workflows:write feedbackRecords:read feedbackRecords:write"' ); @@ -387,6 +390,28 @@ describe("authenticateMcpRequest", () => { expect(applyRateLimit).not.toHaveBeenCalled(); }); + // The inverse of the challenge fix: offline_access is advertised so clients can obtain a refresh + // token, but it grants no resource access, so it must never satisfy the baseline gate on its own. + test("rejects an OAuth bearer token scoped only to offline_access", async () => { + verifyAccessTokenMock.mockResolvedValue({ + sub: "user_1", + client_id: "client_2", + scope: "openid profile email offline_access", + }); + + const result = await authenticateMcpRequest( + createRequest("http://localhost/api/mcp", { + authorization: "Bearer oauth_access_token", + }) + ); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.response.status).toBe(403); + expect(result.response.headers.get("WWW-Authenticate")).toContain('error="insufficient_scope"'); + } + }); + // Any single resource scope is enough to authenticate: a feedbackRecords-only grant is a legitimate // MCP client and must not be turned away for lacking surveys:read (per-tool guards still apply). test.each([["feedbackRecords:read"], ["surveys:write"]])( @@ -442,9 +467,13 @@ describe("authenticateMcpRequest", () => { if (!result.ok) { expect(result.response.status).toBe(401); // Auth-params are comma-separated (RFC 9110 `#auth-param`), so a strict client parser can read - // `resource_metadata` out of the challenge instead of choking on the whole tail. - expect(result.response.headers.get("WWW-Authenticate")).toBe( - 'Bearer resource_metadata="https://app.example.com/.well-known/oauth-protected-resource/api/mcp", scope="surveys:read surveys:write workflows:read workflows:write feedbackRecords:read feedbackRecords:write"' + // `resource_metadata` out of the challenge instead of choking on the whole tail. What this case is + // really about is the metadata URL being built from the configured origin, so assert that plus the + // grammar — not the scope list, which is pinned from the real constant elsewhere. + const challenge = result.response.headers.get("WWW-Authenticate"); + expect(challenge).toMatch(CHALLENGE_GRAMMAR); + expect(challenge).toContain( + 'resource_metadata="https://app.example.com/.well-known/oauth-protected-resource/api/mcp"' ); expect(await result.response.json()).toMatchObject({ detail: "Invalid OAuth access token", diff --git a/apps/web/modules/mcp/auth.ts b/apps/web/modules/mcp/auth.ts index 4ee228e90282..b483be20427d 100644 --- a/apps/web/modules/mcp/auth.ts +++ b/apps/web/modules/mcp/auth.ts @@ -19,6 +19,7 @@ import { parseApiKeyV2 } from "@/lib/crypto"; import { authenticateApiKeyFromHeaders, getBearerTokenFromHeaders } from "@/modules/api/lib/api-key-auth"; import { auth } from "@/modules/auth/lib/auth"; import { + MCP_CHALLENGE_SCOPE, MCP_RESOURCE_SCOPES, getAuthIssuerUrl, getMcpOrigin, @@ -37,16 +38,6 @@ const QUERY_CREDENTIAL_PARAMS = new Set([ "authorization", ]); -// Minimum grant required to authenticate against the MCP server at all: at least ONE resource scope. -// Any single one is enough — a token granted only `feedbackRecords:read` is a legitimate MCP client and -// must not be rejected here for lacking `surveys:read`. Which tools it can actually call is enforced -// per-tool by guardMcpScopes at call time. -const MCP_MINIMUM_SCOPES = MCP_RESOURCE_SCOPES; -// Scopes advertised in the 401 WWW-Authenticate challenge. Clients build their DCR + authorize -// requests from this, so it must list every resource scope (read + write) or clients only ever -// request read and can never reach the write tools. Actual write access is still gated downstream -// by the user's workspace permissions in the v3 layer. -const MCP_CHALLENGE_SCOPE = MCP_RESOURCE_SCOPES.join(" "); const oauthResourceClient = oauthProviderResourceClient(auth); export type TMcpAuthInfo = AuthInfo & { @@ -373,14 +364,24 @@ async function authenticateMcpOAuthBearer( }; } - if (!hasAnyMcpScope(authInfo, MCP_MINIMUM_SCOPES)) { + // Minimum grant required to authenticate against the MCP server at all: at least ONE *resource* + // scope. Any single one is enough — a token granted only `feedbackRecords:read` is a legitimate + // MCP client and must not be rejected here for lacking `surveys:read`. Which tools it can actually + // call is enforced per-tool by guardMcpScopes at call time. + // + // Deliberately NOT MCP_CHALLENGE_SCOPE / MCP_PROTECTED_RESOURCE_SCOPES: those include + // `offline_access`, and an any-of gate over that list would let a token holding *only* + // `offline_access` — which grants no resource access at all — authenticate to the MCP server. + // Same reason the insufficient_scope challenge below advertises only the resource scopes: RFC 6750 + // `scope` names the scopes *required* for the resource, and `offline_access` is not one of them. + if (!hasAnyMcpScope(authInfo, MCP_RESOURCE_SCOPES)) { log.warn({ statusCode: 403, clientId: authInfo.clientId }, "MCP OAuth token missing every MCP scope"); return { ok: false, requestId, response: withInsufficientScopeChallenge( problemForbidden(requestId, "OAuth token does not include the required MCP scope", instance), - [...MCP_MINIMUM_SCOPES] + [...MCP_RESOURCE_SCOPES] ), }; } diff --git a/apps/web/modules/mcp/constants.ts b/apps/web/modules/mcp/constants.ts index ccc814a35b66..195a4af465df 100644 --- a/apps/web/modules/mcp/constants.ts +++ b/apps/web/modules/mcp/constants.ts @@ -1,4 +1,9 @@ +/** + * The MCP endpoint path. Correct for the RFC 9457 problem-document `instance` member, which is a + * URI *reference* — relative is expected there. + */ export const MCP_API_ROUTE = "/api/mcp" as const; + // Names the whole XM Suite surface, not just surveys — the server also exposes workspace discovery and // feedback records. Client config keys are user-chosen, so this is a display name only. export const MCP_SERVER_NAME = "formbricks-xm-suite" as const; diff --git a/apps/web/modules/mcp/tools/feedback-records.test.ts b/apps/web/modules/mcp/tools/feedback-records.test.ts index d40c84e8300b..a6751ef6c08a 100644 --- a/apps/web/modules/mcp/tools/feedback-records.test.ts +++ b/apps/web/modules/mcp/tools/feedback-records.test.ts @@ -20,8 +20,15 @@ import { successListResponse, successResponse, } from "@/app/api/v3/lib/response"; +import { UNKNOWN_DATA } from "@/modules/ee/audit-logs/types/audit-log"; import { registerFeedbackRecordTools } from "./feedback-records"; +// Asserted as a shape, not by calling getMcpResourceUrl() here: comparing production's value with +// itself would still pass if it regressed to the bare path "/api/mcp" — the ENG-2173 bug. The +// invariant that matters is that the audit apiUrl is absolute, because the audit schema validates it +// with z.url() and drops the whole event otherwise. +const ABSOLUTE_MCP_AUDIT_URL = expect.stringMatching(/^https?:\/\/[^/]+\/api\/mcp$/); + vi.mock("@/app/api/v3/feedbackRecords/lib/operations", () => ({ countV3FeedbackRecords: vi.fn(), createV3FeedbackRecord: vi.fn(), @@ -237,7 +244,12 @@ describe("create_feedback_record", () => { await tools.get("create_feedback_record")!.handler(body, { authInfo }); - expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "created", "feedbackRecord", "/api/mcp"); + expect(buildV3AuditLog).toHaveBeenCalledWith( + apiKeyAuth, + "created", + "feedbackRecord", + ABSOLUTE_MCP_AUDIT_URL + ); expect(createV3FeedbackRecord).toHaveBeenCalledWith( expect.objectContaining({ workspaceId, body, authentication: apiKeyAuth }) ); @@ -299,7 +311,12 @@ describe("delete_feedback_record", () => { const result = await tools.get("delete_feedback_record")!.handler(input, { authInfo }); - expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "deleted", "feedbackRecord", "/api/mcp"); + expect(buildV3AuditLog).toHaveBeenCalledWith( + apiKeyAuth, + "deleted", + "feedbackRecord", + ABSOLUTE_MCP_AUDIT_URL + ); expect(deleteV3FeedbackRecord).toHaveBeenCalledWith( expect.objectContaining({ workspaceId, @@ -445,6 +462,11 @@ describe("count_feedback_records", () => { }); describe("create_feedback_records", () => { + // buildAuditLogBaseObject seeds `targetId` with the UNKNOWN_DATA placeholder and the batch path + // branches on it, so the mock has to carry it. Omitting it (as these tests used to) makes a plain + // truthiness check pass here while matching every entry in production — which is how the batch path + // came to emit success events for records the Hub had rejected. + const makeAuditLog = () => ({ status: "failure", targetId: UNKNOWN_DATA }) as any; const records = [ { source_type: "call_notes", field_id: "note", field_type: "text", value_text: "one" }, { source_type: "call_notes", field_id: "note", field_type: "text", value_text: "two" }, @@ -465,7 +487,7 @@ describe("create_feedback_records", () => { test("queues one success audit event per created record", async () => { const built: any[] = []; vi.mocked(buildV3AuditLog).mockImplementation(() => { - const auditLog = { status: "failure" } as any; + const auditLog = makeAuditLog(); built.push(auditLog); return auditLog; }); @@ -482,12 +504,16 @@ describe("create_feedback_records", () => { expect(queueV3AuditLog).toHaveBeenCalledTimes(1); expect(built[0].status).toBe("success"); expect(built[1].status).toBe("failure"); + // The rejected record keeps the placeholder, so it must never be reported as a creation: an + // audit trail that invents records is worse than one with gaps. + expect(built[1].targetId).toBe(UNKNOWN_DATA); + expect(vi.mocked(queueV3AuditLog).mock.calls[0][0]).toBe(built[0]); }); test("still queues a failure event when the batch operation throws, and rethrows", async () => { const built: any[] = []; vi.mocked(buildV3AuditLog).mockImplementation(() => { - const auditLog = { status: "failure" } as any; + const auditLog = makeAuditLog(); built.push(auditLog); return auditLog; }); @@ -503,7 +529,7 @@ describe("create_feedback_records", () => { test("queues a single failure event when nothing was created", async () => { const built: any[] = []; vi.mocked(buildV3AuditLog).mockImplementation(() => { - const auditLog = { status: "failure" } as any; + const auditLog = makeAuditLog(); built.push(auditLog); return auditLog; }); @@ -533,7 +559,12 @@ describe("update_feedback_record", () => { await tools.get("update_feedback_record")!.handler(input, { authInfo }); - expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "updated", "feedbackRecord", "/api/mcp"); + expect(buildV3AuditLog).toHaveBeenCalledWith( + apiKeyAuth, + "updated", + "feedbackRecord", + ABSOLUTE_MCP_AUDIT_URL + ); expect(updateV3FeedbackRecord).toHaveBeenCalledWith( expect.objectContaining({ workspaceId, feedbackRecordId: recordId, body: input, auditLog }) ); diff --git a/apps/web/modules/mcp/tools/feedback-records.ts b/apps/web/modules/mcp/tools/feedback-records.ts index 0ecb1aa015ac..62ed6cc860b4 100644 --- a/apps/web/modules/mcp/tools/feedback-records.ts +++ b/apps/web/modules/mcp/tools/feedback-records.ts @@ -16,10 +16,13 @@ import { } from "@/app/api/v3/feedbackRecords/lib/operations"; import { buildV3AuditLog, queueV3AuditLog } from "@/app/api/v3/lib/audit"; import type { TV3AuditLog, TV3Authentication } from "@/app/api/v3/lib/types"; +import { getMcpResourceUrl } from "@/modules/auth/lib/oauth-urls"; +import { UNKNOWN_DATA } from "@/modules/ee/audit-logs/types/audit-log"; import { MCP_API_ROUTE } from "@/modules/mcp/constants"; import { getMcpAuthentication, getMcpRequestId } from "../auth"; import { responseToMcpToolResult } from "../errors"; import { guardMcpScopes } from "./guard-scopes"; +import { runMcpMutation } from "./run-mcp-mutation"; import { type TMcpCountFeedbackRecordsInput, type TMcpCreateFeedbackRecordInput, @@ -86,31 +89,14 @@ function writeHandler( return scopeError; } - const authentication = getMcpAuthentication(extra.authInfo); - const log = logger.withContext({ requestId, workspaceId: input.workspaceId }); - const auditLog = buildV3AuditLog(authentication, action, "feedbackRecord", MCP_API_ROUTE); - - try { - const response = await run(input, authentication, requestId, auditLog); - - if (auditLog) { - if (response.ok) { - auditLog.status = "success"; - } else { - auditLog.eventId = requestId; - } - } - - await queueV3AuditLog(auditLog, requestId, log); - return await responseToMcpToolResult(response, requestId); - } catch (error) { - if (auditLog) { - auditLog.eventId = requestId; - await queueV3AuditLog(auditLog, requestId, log); - } - - throw error; - } + // The scope gate above is the only part that differs from the survey/workflow tools, which get + // theirs from registerScopedTool; the audit lifecycle itself is shared. + return await runMcpMutation( + extra, + { action, resource: "feedbackRecord", logContext: { workspaceId: input.workspaceId } }, + ({ authentication, requestId: mutationRequestId, auditLog }) => + run(input, authentication, mutationRequestId, auditLog) + ); }; } @@ -261,13 +247,19 @@ export function registerFeedbackRecordTools(server: McpServer): void { // attribute a creation to the wrong record. `buildV3AuditLog` returns undefined for every record or // none (it only depends on whether auditing is enabled), so the holes are all-or-nothing. const auditLogs = input.records.map(() => - buildV3AuditLog(authentication, "created", "feedbackRecord", MCP_API_ROUTE) + buildV3AuditLog(authentication, "created", "feedbackRecord", getMcpResourceUrl()) ); const queueOutcome = async () => { - const stamped = auditLogs.filter((auditLog) => auditLog?.targetId); + // `targetId` must be compared against the placeholder, not just tested for truthiness: + // buildAuditLogBaseObject seeds it with UNKNOWN_DATA ("unknown"), so a truthy check matches + // every entry — including records the Hub rejected — and would emit a success event + // asserting a creation that never happened, with targetId "unknown" and no newObject. + // Only createV3FeedbackRecords overwrites it, and only for records it actually created. + const stamped = auditLogs.filter( + (auditLog): auditLog is TV3AuditLog => !!auditLog && auditLog.targetId !== UNKNOWN_DATA + ); for (const auditLog of stamped) { - if (!auditLog) continue; auditLog.status = "success"; await queueV3AuditLog(auditLog, requestId, log); } diff --git a/apps/web/modules/mcp/tools/run-mcp-mutation.ts b/apps/web/modules/mcp/tools/run-mcp-mutation.ts index d9be6ee669d0..8c745b398a2a 100644 --- a/apps/web/modules/mcp/tools/run-mcp-mutation.ts +++ b/apps/web/modules/mcp/tools/run-mcp-mutation.ts @@ -2,7 +2,7 @@ import type { AuthInfo } from "@modelcontextprotocol/sdk/server/auth/types.js"; import type { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; import { logger } from "@formbricks/logger"; import { buildV3AuditLog, queueV3AuditLog } from "@/app/api/v3/lib/audit"; -import { MCP_API_ROUTE } from "@/modules/mcp/constants"; +import { getMcpResourceUrl } from "@/modules/auth/lib/oauth-urls"; import { getMcpAuthentication, getMcpRequestId } from "../auth"; import { responseToMcpToolResult } from "../errors"; @@ -37,7 +37,12 @@ export async function runMcpMutation( const requestId = getMcpRequestId(extra.authInfo); const authentication = getMcpAuthentication(extra.authInfo); const log = logger.withContext({ requestId, ...logContext }); - const auditLog = buildV3AuditLog(authentication, action, resource, MCP_API_ROUTE); + // getMcpResourceUrl(), NOT MCP_API_ROUTE: the audit schema validates apiUrl with z.url(), and + // logAuditEvent catches the failure and downgrades it to a logger.error — so a bare path silently + // drops the whole event, which is why no MCP mutation was ever audited (ENG-2173). This is the same + // absolute URL the OAuth protected-resource metadata advertises, so the two agree by construction + // and it stays correct on a sub-path deployment (WEBAPP_URL=https://host/formbricks). + const auditLog = buildV3AuditLog(authentication, action, resource, getMcpResourceUrl()); try { const response = await run({ authentication, requestId, auditLog }); diff --git a/apps/web/modules/mcp/tools/surveys.test.ts b/apps/web/modules/mcp/tools/surveys.test.ts index 6133576ad3e6..e306d3c9a874 100644 --- a/apps/web/modules/mcp/tools/surveys.test.ts +++ b/apps/web/modules/mcp/tools/surveys.test.ts @@ -19,6 +19,12 @@ import { } from "@/app/api/v3/surveys/lib/operations"; import { buildListSurveysSearchParams, registerSurveyTools } from "./surveys"; +// Asserted as a shape, not by calling getMcpResourceUrl() here: comparing production's value with +// itself would still pass if it regressed to the bare path "/api/mcp" — the ENG-2173 bug. The +// invariant that matters is that the audit apiUrl is absolute, because the audit schema validates it +// with z.url() and drops the whole event otherwise. +const ABSOLUTE_MCP_AUDIT_URL = expect.stringMatching(/^https?:\/\/[^/]+\/api\/mcp$/); + vi.mock("@/app/api/v3/surveys/lib/operations", () => ({ createV3SurveyResponseFromRawInput: vi.fn(), deleteV3Survey: vi.fn(), @@ -347,7 +353,7 @@ describe("registerSurveyTools", () => { const result = await tools.get("create_survey")!.handler(createBody, { authInfo }); - expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "created", "survey", "/api/mcp"); + expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "created", "survey", ABSOLUTE_MCP_AUDIT_URL); expect(createV3SurveyResponseFromRawInput).toHaveBeenCalledWith({ body: createBody, authentication: apiKeyAuth, @@ -408,7 +414,7 @@ describe("registerSurveyTools", () => { const result = await tools.get("patch_survey")!.handler(patchInput, { authInfo }); - expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "updated", "survey", "/api/mcp"); + expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "updated", "survey", ABSOLUTE_MCP_AUDIT_URL); expect(patchV3SurveyResponse).toHaveBeenCalledWith({ surveyId: "clxx1234567890123456789012", body: { @@ -503,6 +509,9 @@ describe("registerSurveyTools", () => { }); }); + // Covers the MCP scope gate only — validateV3SurveyFromRawInput is mocked here, so this passed + // even while the real v3 operation demanded readWrite and 403'd every read-scoped caller + // (ENG-2179). The v3 gate itself is asserted in app/api/v3/surveys/lib/operations.test.ts. test("validate_survey allows patch validations for read-only OAuth scopes", async () => { const { tools } = createToolServer(); vi.mocked(validateV3SurveyFromRawInput).mockResolvedValue( diff --git a/apps/web/modules/mcp/tools/surveys.ts b/apps/web/modules/mcp/tools/surveys.ts index f5b0bef5e75b..edda001fd4e5 100644 --- a/apps/web/modules/mcp/tools/surveys.ts +++ b/apps/web/modules/mcp/tools/surveys.ts @@ -166,8 +166,8 @@ export function registerSurveyTools(server: McpServer): void { async (input: TMcpValidateSurveyInput, extra) => { const requestId = getMcpRequestId(extra.authInfo); // validate_survey never persists changes (readOnlyHint) — a dry-run validation of a create or - // patch payload only needs read access. The actual write permission is enforced by the v3 layer - // when create_survey / patch_survey run. + // patch payload only needs read access, and validateV3Survey gates at "read" to match. The + // actual write permission is enforced when create_survey / patch_survey run. const response = await validateV3SurveyFromRawInput({ body: input, authentication: getMcpAuthentication(extra.authInfo), diff --git a/apps/web/modules/mcp/tools/workflows.test.ts b/apps/web/modules/mcp/tools/workflows.test.ts index a2e11cc9e8cf..f90056efb129 100644 --- a/apps/web/modules/mcp/tools/workflows.test.ts +++ b/apps/web/modules/mcp/tools/workflows.test.ts @@ -15,6 +15,12 @@ import { registerWorkflowTools, } from "./workflows"; +// Asserted as a shape, not by calling getMcpResourceUrl() here: comparing production's value with +// itself would still pass if it regressed to the bare path "/api/mcp" — the ENG-2173 bug. The +// invariant that matters is that the audit apiUrl is absolute, because the audit schema validates it +// with z.url() and drops the whole event otherwise. +const ABSOLUTE_MCP_AUDIT_URL = expect.stringMatching(/^https?:\/\/[^/]+\/api\/mcp$/); + vi.mock("@/app/api/v3/workflows/lib/context", () => ({ buildWorkflowApiContext: vi.fn(() => ({ __ctx: true })), workflowsHandlers: { @@ -294,7 +300,7 @@ describe("registerWorkflowTools", () => { const result = await tools.get("create_workflow")!.handler(body, { authInfo }); - expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "created", "workflow", "/api/mcp"); + expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "created", "workflow", ABSOLUTE_MCP_AUDIT_URL); expect(buildWorkflowApiContext).toHaveBeenCalledWith(apiKeyAuth, "req_tool", "/api/mcp", auditLog); const callArg = vi.mocked(workflowsHandlers.create).mock.calls[0][0]; expect(callArg.ctx).toEqual({ __ctx: true }); @@ -314,7 +320,7 @@ describe("registerWorkflowTools", () => { await tools.get("enable_workflow")!.handler({ workflowId: WORKFLOW_ID }, { authInfo }); - expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "updated", "workflow", "/api/mcp"); + expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "updated", "workflow", ABSOLUTE_MCP_AUDIT_URL); expect(workflowsHandlers.enable).toHaveBeenCalledWith({ ctx: { __ctx: true }, params: { workflowId: WORKFLOW_ID }, @@ -331,7 +337,7 @@ describe("registerWorkflowTools", () => { const result = await tools.get("delete_workflow")!.handler({ workflowId: WORKFLOW_ID }, { authInfo }); - expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "deleted", "workflow", "/api/mcp"); + expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "deleted", "workflow", ABSOLUTE_MCP_AUDIT_URL); expect(auditLog.status).toBe("success"); expect(result.structuredContent).toEqual({ requestId: "req_tool" }); }); @@ -396,7 +402,7 @@ describe("registerWorkflowTools", () => { .get("patch_workflow")! .handler({ workflowId: WORKFLOW_ID, data: { name: "Renamed" } }, { authInfo }); - expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "updated", "workflow", "/api/mcp"); + expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "updated", "workflow", ABSOLUTE_MCP_AUDIT_URL); const callArg = vi.mocked(workflowsHandlers.patch).mock.calls[0][0]; expect(callArg.params).toEqual({ workflowId: WORKFLOW_ID }); expect(JSON.parse(await callArg.req.text())).toEqual({ name: "Renamed" }); @@ -414,7 +420,7 @@ describe("registerWorkflowTools", () => { await tools.get("duplicate_workflow")!.handler({ workflowId: WORKFLOW_ID, name: "Copy" }, { authInfo }); - expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "created", "workflow", "/api/mcp"); + expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "created", "workflow", ABSOLUTE_MCP_AUDIT_URL); const callArg = vi.mocked(workflowsHandlers.duplicate).mock.calls[0][0]; expect(callArg.params).toEqual({ workflowId: WORKFLOW_ID }); expect(JSON.parse(await callArg.req.text())).toEqual({ name: "Copy" }); @@ -431,7 +437,7 @@ describe("registerWorkflowTools", () => { await tools.get("archive_workflow")!.handler({ workflowId: WORKFLOW_ID }, { authInfo }); - expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "updated", "workflow", "/api/mcp"); + expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "updated", "workflow", ABSOLUTE_MCP_AUDIT_URL); expect(workflowsHandlers.archive).toHaveBeenCalledWith({ ctx: { __ctx: true }, params: { workflowId: WORKFLOW_ID }, @@ -449,7 +455,7 @@ describe("registerWorkflowTools", () => { await tools.get("unarchive_workflow")!.handler({ workflowId: WORKFLOW_ID }, { authInfo }); - expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "updated", "workflow", "/api/mcp"); + expect(buildV3AuditLog).toHaveBeenCalledWith(apiKeyAuth, "updated", "workflow", ABSOLUTE_MCP_AUDIT_URL); expect(workflowsHandlers.unarchive).toHaveBeenCalledWith({ ctx: { __ctx: true }, params: { workflowId: WORKFLOW_ID }, diff --git a/apps/web/modules/mcp/tools/workflows.ts b/apps/web/modules/mcp/tools/workflows.ts index 98fb07556bb0..f1d549a4700a 100644 --- a/apps/web/modules/mcp/tools/workflows.ts +++ b/apps/web/modules/mcp/tools/workflows.ts @@ -242,7 +242,9 @@ export function registerWorkflowTools(server: McpServer): void { "No run is persisted and no side effects occur; the response reports { ok, problems }.", ].join(" "), annotations: { - // Dry-run: validates + mock-executes with all side effects suppressed, so no world mutation. + // Dry-run: validates the definition and resolves its trigger references. Nothing is + // executed and no run is created, so the handler gates at "read" to match the + // workflows:read scope declared below. readOnlyHint: true, destructiveHint: false, idempotentHint: true, diff --git a/apps/web/modules/mcp/tools/workspaces.ts b/apps/web/modules/mcp/tools/workspaces.ts index bf69ea2c276a..5725b84a690d 100644 --- a/apps/web/modules/mcp/tools/workspaces.ts +++ b/apps/web/modules/mcp/tools/workspaces.ts @@ -9,7 +9,7 @@ import { type TMcpListWorkspacesInput, ZMcpListWorkspacesInput } from "./schemas export function registerWorkspaceTools(server: McpServer): void { // list_workspaces is the workspaceId-discovery prerequisite for the survey, workflow AND // feedback-record tools, so it gates on ANY resource read scope rather than a single one. auth.ts's - // baseline is now "at least one resource scope" (MCP_MINIMUM_SCOPES), so a workflows-only or + // baseline is now "at least one resource scope" (MCP_RESOURCE_SCOPES), so a workflows-only or // feedbackRecords-only token is a legitimate client and must still be able to discover its // workspaceId. The result is derived from the caller's own memberships/key grants, so admitting any // read scope exposes nothing extra. diff --git a/charts/formbricks/README.md b/charts/formbricks/README.md index f5c163a5ecbb..6dc0df20d000 100644 --- a/charts/formbricks/README.md +++ b/charts/formbricks/README.md @@ -259,22 +259,22 @@ taxonomy: The `taxonomy-vertex-secret` secret must contain `TAXONOMY_GOOGLE_CLOUD_CREDENTIALS_JSON` with service-account JSON that can call Vertex AI. -### Hub and Taxonomy metrics and structured logs +## Hub and Taxonomy metrics and structured logs -Hub and the taxonomy service can export OpenTelemetry metrics over OTLP/HTTP. Configure the standard `OTEL_*` -environment variables through the existing `hub.env` and `taxonomy.env` maps; no chart-specific collector values -are required. The example below uses a SigNoz collector in the `signoz` namespace and labels both services as -`production`. Replace the collector DNS name and `deployment.environment` with values matching each cluster and -environment: +Hub and the taxonomy service can export OpenTelemetry metrics over OTLP/HTTP, and Hub can additionally export +traces. Configure the standard `OTEL_*` environment variables through the existing `hub.env` and `taxonomy.env` +maps; no chart-specific collector values are required. The example below uses a SigNoz collector in the `signoz` +namespace and labels both services as `production`. Replace the collector DNS name and `deployment.environment` +with values matching each cluster and environment: ```yaml hub: env: LOG_FORMAT: json OTEL_METRICS_EXPORTER: otlp + OTEL_TRACES_EXPORTER: otlp OTEL_EXPORTER_OTLP_PROTOCOL: http/protobuf OTEL_EXPORTER_OTLP_ENDPOINT: http://signoz-otel-collector.signoz.svc.cluster.local:4318 - OTEL_SERVICE_NAME: formbricks-hub OTEL_RESOURCE_ATTRIBUTES: deployment.environment=production,service.namespace=formbricks taxonomy: @@ -287,9 +287,29 @@ taxonomy: OTEL_RESOURCE_ATTRIBUTES: deployment.environment=production,service.namespace=formbricks ``` -Metrics use only bounded lifecycle, phase, provider, outcome, and reason attributes. Run, request, tenant, source, -and field identifiers are emitted only in correlated JSON logs. Prompt text, feedback, model output, embeddings, -credentials, authorization tokens, provider response bodies, and collector URLs are never telemetry fields. +`OTEL_TRACES_EXPORTER` is set for Hub only, and it is what puts `trace_id` and `span_id` in Hub's logs — Hub +stamps them onto a log record only when tracing is enabled. Without it, Hub's JSON logs carry `request_id` alone +and Hub warns `tracing not enabled (OTEL_TRACES_EXPORTER empty or unset)` at startup. The taxonomy service exports +metrics but does not emit traces, so the variable is deliberately absent from `taxonomy.env`; it correlates its +logs through `request_id` and `run_id`, both of which Hub also logs, so a run can still be followed across the two +services. + +`OTEL_SERVICE_NAME` is set for the taxonomy service but deliberately not for Hub. `hub.env` is applied to both +the Hub API and the Hub worker Deployments, so setting it there would report two different processes under one +`service.name` and make them indistinguishable at the collector. Hub already names each binary itself — +`hub-api` and `hub-worker` — and only falls back to that when the variable is unset, so leaving it out is what +keeps them apart. If you do want custom names, override per component in `hub.worker.env` rather than widening +`hub.env`. The taxonomy service has no such built-in default and would report `unknown_service` without it. + +Both blocks need recent images. Hub reads `LOG_FORMAT` from 0.8.3 onward, and the taxonomy service's +OpenTelemetry and JSON-logging support is newer than `v0.1.0`. Older images ignore these variables entirely +rather than failing, so applying them ahead of the image bump is silent — expect text logs and no taxonomy +metrics until each image is new enough. + +Hub's metric attributes are restricted to a fixed, low-cardinality set — for its taxonomy metrics that is +`scope_type`, `status`, `failure_code`, and `reason`. Run, request, tenant, source, and field identifiers are +emitted only in correlated JSON logs. Prompt text, feedback, model output, embeddings, credentials, authorization +tokens, provider response bodies, and collector URLs are never telemetry fields. ## Values @@ -415,10 +435,10 @@ credentials, authorization tokens, provider response bodies, and collector URLs | hub.existingSecret | string | `""` | | | hub.extraVolumeMounts | list | `[]` | Additional volume mounts for Hub API and worker. | | hub.extraVolumes | list | `[]` | Additional pod volumes for Hub API and worker. | -| hub.image.digest | string | `"sha256:5d7e7c6138ff77984db54b7c91693d8f56017cefa74bc69390fc7191403208c5"` | When set, takes precedence over tag (immutable pin). | +| hub.image.digest | string | `"sha256:4dc0c4f26cf999b3bf4a26d7b09634fc65ae23cbb30c9ad82042da019d231458"` | When set, takes precedence over tag (immutable pin). | | hub.image.pullPolicy | string | `"IfNotPresent"` | | | hub.image.repository | string | `"ghcr.io/formbricks/hub"` | | -| hub.image.tag | string | `"0.8.1"` | Fallback when digest is empty. | +| hub.image.tag | string | `"0.8.3"` | Fallback when digest is empty. | | hub.migration.activeDeadlineSeconds | int | `900` | | | hub.migration.backoffLimit | int | `3` | | | hub.migration.ttlSecondsAfterFinished | int | `300` | | diff --git a/charts/formbricks/values.yaml b/charts/formbricks/values.yaml index 237ff1c18e9f..26f98332ae7a 100644 --- a/charts/formbricks/values.yaml +++ b/charts/formbricks/values.yaml @@ -948,10 +948,10 @@ hub: # Pinned by digest for immutable, reproducible deployments. When digest is set it takes # precedence over tag, and deployment, init container, and migration job all resolve to the # same immutable image. Update on each Hub release. - # Current digest corresponds to ghcr.io/formbricks/hub:0.8.1. - digest: "sha256:5d7e7c6138ff77984db54b7c91693d8f56017cefa74bc69390fc7191403208c5" + # Current digest corresponds to ghcr.io/formbricks/hub:0.8.3. + digest: "sha256:4dc0c4f26cf999b3bf4a26d7b09634fc65ae23cbb30c9ad82042da019d231458" # Tag is a fallback for dev/non-prod when digest is cleared; keep aligned with the digest above. - tag: "0.8.1" + tag: "0.8.3" pullPolicy: IfNotPresent # Optional override for the secret Hub reads from. diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index 52facce5cd3a..bf3ade43b620 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -97,7 +97,7 @@ services: # Keep hub, hub-migrate, and any future hub-worker on the same tag — they share one image and # drift breaks migrations or job processing. hub-migrate: - image: ghcr.io/formbricks/hub:${HUB_IMAGE_TAG:-0.8.1} + image: ghcr.io/formbricks/hub:${HUB_IMAGE_TAG:-0.8.3} restart: "no" entrypoint: ["sh", "-c"] command: @@ -112,7 +112,7 @@ services: # Formbricks Hub API (ghcr.io/formbricks/hub). Uses a dedicated local Hub database by default. hub: - image: ghcr.io/formbricks/hub:${HUB_IMAGE_TAG:-0.8.1} + image: ghcr.io/formbricks/hub:${HUB_IMAGE_TAG:-0.8.3} depends_on: hub-migrate: condition: service_completed_successfully @@ -166,7 +166,7 @@ services: # Hub worker processes async jobs enqueued by the API, including embeddings. hub-worker: - image: ghcr.io/formbricks/hub:${HUB_IMAGE_TAG:-0.8.1} + image: ghcr.io/formbricks/hub:${HUB_IMAGE_TAG:-0.8.3} depends_on: hub-migrate: condition: service_completed_successfully diff --git a/docs/api-v3-reference/openapi.yml b/docs/api-v3-reference/openapi.yml index 88dfef3b8e66..d437f6a49136 100644 --- a/docs/api-v3-reference/openapi.yml +++ b/docs/api-v3-reference/openapi.yml @@ -38,7 +38,7 @@ info: **Workflows Scope 1** Workflows are a follow-up extension of the existing v3 Survey API work. The Survey API establishes API-first survey authoring; Workflows build on that same v3 style to automate actions around survey responses through workspace-scoped JSON definitions. Scope 1 models the existing Follow-ups capability as workflows: one `response.completed` trigger with an optional `endingCardIds` filter, and `send_email` actions with Follow-up email field parity. The definition and run shapes mirror the shared Zod schemas in `packages/workflows` (ENG-1100), which are the implementation source of truth for this contract. - Workflows use `draft`, `enabled`, and `disabled` lifecycle states plus an `archived` soft-delete state. Status only changes through the lifecycle endpoints (`enable`, `disable`, `archive`, `unarchive`); it is not writable via `POST` or `PATCH`. Enabling validates that the definition is executable and snapshots an immutable workflow version; runs reference that snapshot via `workflowVersionId`. Dry-run tests create real run records with `isDryRun: true` that mock action execution and never send email. Runs execute asynchronously: `POST .../test` returns a `queued` run to poll via `GET /api/v3/workflows/runs/{runId}`. + Workflows use `draft`, `enabled`, and `disabled` lifecycle states plus an `archived` soft-delete state. Status only changes through the lifecycle endpoints (`enable`, `disable`, `archive`, `unarchive`); it is not writable via `POST` or `PATCH`. Enabling validates that the definition is executable and snapshots an immutable workflow version; runs reference that snapshot via `workflowVersionId`. `POST .../test` is a dry run: it validates the definition and resolves the trigger's references synchronously, creating no run and sending no email, and returns `{ workflowId, ok, problems }` directly. Real runs execute asynchronously and are polled via `GET /api/v3/workflows/runs/{runId}`. **Overview migration note** The v3-backed survey overview page intentionally removes actions that are not yet exposed by this contract: `Created by` filtering, `Duplicate`, `Copy...`, `Preview`, and `Copy link`. @@ -1973,7 +1973,7 @@ paths: The response is always `200` with `{ data: { workflowId, ok, problems } }`. `data.ok` is `true` when the workflow would execute; otherwise `data.problems` lists every issue found — each with a machine-readable `code` and the offending `field` — so they can be fixed in a single pass. - Only `enabled` and `disabled` workflows can be tested. Testing a `draft` or `archived` workflow returns **422** with code `invalid_workflow_state`. + Drafts are testable — validating the setup before going live is the point of a dry run. Only an `archived` workflow is rejected, with **422** and code `invalid_workflow_state`. tags: - V3 Workflows parameters: diff --git a/docs/api-v3-reference/src/openapi.yml b/docs/api-v3-reference/src/openapi.yml index 97a76539f410..0a383f0d0fbb 100644 --- a/docs/api-v3-reference/src/openapi.yml +++ b/docs/api-v3-reference/src/openapi.yml @@ -129,9 +129,10 @@ info: endpoints (`enable`, `disable`, `archive`, `unarchive`); it is not writable via `POST` or `PATCH`. Enabling validates that the definition is executable and snapshots an immutable workflow version; runs reference that snapshot - via `workflowVersionId`. Dry-run tests create real run records with - `isDryRun: true` that mock action execution and never send email. Runs - execute asynchronously: `POST .../test` returns a `queued` run to poll via + via `workflowVersionId`. `POST .../test` is a dry run: it validates the + definition and resolves the trigger's references synchronously, creating no + run and sending no email, and returns `{ workflowId, ok, problems }` + directly. Real runs execute asynchronously and are polled via `GET /api/v3/workflows/runs/{runId}`. diff --git a/docs/api-v3-reference/src/paths/api_v3_workflows_{workflowId}_test.yml b/docs/api-v3-reference/src/paths/api_v3_workflows_{workflowId}_test.yml index afabe17b2463..0901249bcf5a 100644 --- a/docs/api-v3-reference/src/paths/api_v3_workflows_{workflowId}_test.yml +++ b/docs/api-v3-reference/src/paths/api_v3_workflows_{workflowId}_test.yml @@ -14,8 +14,8 @@ post: machine-readable `code` and the offending `field` — so they can be fixed in a single pass. - Only `enabled` and `disabled` workflows can be tested. Testing a `draft` or `archived` workflow - returns **422** with code `invalid_workflow_state`. + Drafts are testable — validating the setup before going live is the point of a dry run. Only an + `archived` workflow is rejected, with **422** and code `invalid_workflow_state`. tags: - V3 Workflows parameters: diff --git a/docs/development/technical-handbook/mcp-server.mdx b/docs/development/technical-handbook/mcp-server.mdx index 7d7e80bc8108..bddf040db6a6 100644 --- a/docs/development/technical-handbook/mcp-server.mdx +++ b/docs/development/technical-handbook/mcp-server.mdx @@ -79,8 +79,8 @@ Supported scopes: | Scope | Use | | --- | --- | -| `surveys:read` | `list_surveys`, `get_survey`, and read-only validation paths | -| `surveys:write` | `create_survey`, `patch_survey`, `delete_survey`, and write validations | +| `surveys:read` | `list_surveys`, `get_survey`, `validate_survey` | +| `surveys:write` | `create_survey`, `patch_survey`, `delete_survey` | | `workflows:read` | `list_workflows`, `get_workflow`, `list_workflow_runs`, `get_workflow_run`, `test_workflow` | | `workflows:write` | `create_workflow`, `patch_workflow`, `duplicate_workflow`, `delete_workflow`, and the enable/disable/archive/unarchive mutations | | `feedbackRecords:read` | `list_feedback_datasets`, `list_feedback_records`, `count_feedback_records`, `get_feedback_record`, `search_feedback_records`, `find_similar_feedback_records` | @@ -104,7 +104,7 @@ handlers is still gated, but the guarantee is by convention there rather than by converging them onto `registerScopedTool` is a known follow-up. **There is no single mandatory baseline scope.** Authentication requires *at least one* resource scope -(`MCP_MINIMUM_SCOPES`), so a workflows-only or `feedbackRecords:read`-only grant is a legitimate MCP +(`MCP_RESOURCE_SCOPES`), so a workflows-only or `feedbackRecords:read`-only grant is a legitimate MCP client. `list_workspaces` — the workspaceId-discovery prerequisite for every resource tool — therefore gates on any resource read scope rather than one specific one; its result is derived from the caller's own memberships and key grants, so admitting any read scope exposes nothing extra. @@ -159,14 +159,14 @@ API key permissions are enforced through the same workspace access checks as the | `list_surveys` | `read` | | `get_survey` | `read` | | `create_survey` | `write` or `manage` | -| `validate_survey` | `write` or `manage` when the validation request checks write access | +| `validate_survey` | `read` — a dry run; it validates a create or patch payload without writing | | `patch_survey` | `write` or `manage` | | `delete_survey` | `write` or `manage` | | `list_workflows` | `read` | | `get_workflow` | `read` | | `list_workflow_runs` | `read` | | `get_workflow_run` | `read` | -| `test_workflow` | `write` or `manage` — the tool mutates nothing, but the handler authorizes it like a write | +| `test_workflow` | `read` — a dry run; it validates the definition and resolves its trigger references, creating no run | | `create_workflow` | `write` or `manage` | | `patch_workflow` | `write` or `manage` | | `duplicate_workflow` | `write` or `manage` | @@ -252,6 +252,14 @@ curl -sS -X POST http://localhost:3000/api/auth/oauth2/register \ set, or pass exactly what the metadata advertises. + + For the same reason, the `scope` in the MCP endpoint's 401 `WWW-Authenticate` challenge is exactly + the metadata's `scopes_supported`, not a subset — a client that hits the 401 before fetching the + metadata uses the challenge string as its registration scope. The two are derived from one constant + (`MCP_CHALLENGE_SCOPE`) and a test asserts they stay equal; when they diverged, every new client + failed its first connect with `invalid_scope` and only succeeded on retry. + + The consent screen is served at `/account/authorize`. Users can revoke approved MCP clients from `/account/settings/authorized-apps`. @@ -636,7 +644,8 @@ Output uses the same survey resource shape as `GET /api/v3/surveys/{surveyId}` a ### validate_survey Validates a create or patch payload without writing survey changes. The tool is read-only and -idempotent, but create validation still checks workspace write access when `workspaceId` is present. +idempotent; workspace access is still checked when `workspaceId` is present, at `read` level — +matching the `surveys:read` scope the tool declares. Create validation input: @@ -798,9 +807,9 @@ Gets one workflow run with its ordered step logs. Read-only and idempotent. Dry-runs a workflow: validates its live definition would execute and resolves the trigger's survey + ending cards. No run is persisted and no side effects occur; the result reports `{ ok, problems }`. -Annotated read-only (no world mutation). Only **enabled or disabled** workflows can be tested — a -draft or archived workflow is rejected with `422 invalid_workflow_state`, so after `create_workflow` -(which always creates a draft), enable the workflow before testing it. +Annotated read-only (no world mutation). **Drafts are testable** — checking the setup before going +live is the point of a dry run — so a workflow can be tested straight after `create_workflow`. Only +**archived** workflows are rejected, with `422 invalid_workflow_state`, since they are soft-deleted. ```json { "workflowId": "clwf1234567890123456789012" } diff --git a/packages/workflows/src/handlers/workflows.handlers.test.ts b/packages/workflows/src/handlers/workflows.handlers.test.ts index 5bda70f55669..ff6faadb4a87 100644 --- a/packages/workflows/src/handlers/workflows.handlers.test.ts +++ b/packages/workflows/src/handlers/workflows.handlers.test.ts @@ -668,6 +668,16 @@ describe("testWorkflow", () => { expect(body.data).toEqual({ workflowId, ok: true, problems: [] }); }); + test("authorizes at read — it is a dry run and the MCP tool is workflows:read scoped", async () => { + // ENG-2223: this authorized at "readWrite", so a read-scoped caller got a 403 from a tool that + // persists nothing. Raising it back means moving test_workflow's declared MCP scope with it. + service.getWorkflowById.mockResolvedValue(makeRow({ status: "enabled" })); + + await handlers.testWorkflow({ ctx: makeCtx(), params: { workflowId } }); + + expect(authorizeAllow).toHaveBeenCalledWith(workspaceId, "read"); + }); + test("tests a disabled workflow", async () => { service.getWorkflowById.mockResolvedValue(makeRow({ status: "disabled" })); @@ -1274,3 +1284,44 @@ describe("getRun", () => { expect(body.code).toBe("forbidden"); }); }); + +// Negative controls for the authorization level itself. This PR lowered two handlers from +// "readWrite" to "read" after verifying they write nothing; without these, making the same move on a +// handler that DOES write would pass the whole suite — the level was previously asserted in only +// three places, all of them expecting "read". +describe("write handlers authorize at readWrite", () => { + const jsonRequest = (method: string, body: unknown): Request => + new Request("http://localhost/api/v3/workflows/x", { + method, + body: JSON.stringify(body), + headers: { "Content-Type": "application/json" }, + }); + + const params = { workflowId }; + + const writeHandlers: [string, () => Promise][] = [ + [ + "create", + () => + handlers.create({ req: jsonRequest("POST", { workspaceId, name: "n", definition }), ctx: makeCtx() }), + ], + ["patch", () => handlers.patch({ req: jsonRequest("PATCH", { name: "n" }), ctx: makeCtx(), params })], + ["duplicate", () => handlers.duplicate({ req: jsonRequest("POST", {}), ctx: makeCtx(), params })], + ["delete", () => handlers.delete({ ctx: makeCtx(), params })], + ["archive", () => handlers.archive({ ctx: makeCtx(), params })], + ["unarchive", () => handlers.unarchive({ ctx: makeCtx(), params })], + ["enable", () => handlers.enable({ ctx: makeCtx(), params })], + ["disable", () => handlers.disable({ ctx: makeCtx(), params })], + ]; + + test.each(writeHandlers)("%s authorizes at readWrite", async (_name, invoke) => { + service.getWorkflowById.mockResolvedValue(makeRow({ status: "enabled" })); + service.createWorkflow.mockResolvedValue(makeRow()); + service.updateWorkflow.mockResolvedValue(makeRow()); + service.duplicateWorkflow.mockResolvedValue(makeRow()); + + await invoke(); + + expect(authorizeAllow).toHaveBeenCalledWith(workspaceId, "readWrite"); + }); +}); diff --git a/packages/workflows/src/handlers/workflows.handlers.ts b/packages/workflows/src/handlers/workflows.handlers.ts index 6a90951a3143..0865c8bd5f86 100644 --- a/packages/workflows/src/handlers/workflows.handlers.ts +++ b/packages/workflows/src/handlers/workflows.handlers.ts @@ -505,7 +505,12 @@ export const createWorkflowsHandlers = (service: WorkflowsService): WorkflowsHan */ async testWorkflow({ ctx, params }) { try { - const loaded = await loadAndAuthorize(service, ctx, params.workflowId, "readWrite"); + // "read", not "readWrite": as the doc comment above says, this validates the definition and + // resolves its trigger references — nothing is executed and no run is created, so there is no + // side effect to gate on. The MCP test_workflow tool is registered workflows:read and + // annotated readOnlyHint, so requiring write here 403'd every read-scoped caller on a tool + // that writes nothing (ENG-2223). Raising this back means moving that tool's scope with it. + const loaded = await loadAndAuthorize(service, ctx, params.workflowId, "read"); if (loaded instanceof Response) return loaded; // Drafts are testable too — the whole point of a dry run is checking the setup BEFORE going // live. Only archived workflows are rejected: they are soft-deleted.