Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions apps/web/app/api/mcp/route.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ vi.mock("@formbricks/database", () => ({
}));

vi.mock("@/modules/auth/lib/oauth-urls", () => ({
MCP_RESOURCE_SCOPES: ["surveys:read", "surveys:write"],
getAuthIssuerUrl: () => "http://localhost/api/auth",
getMcpOrigin: () => "http://localhost",
getMcpProtectedResourceMetadataUrl: () => "http://localhost/.well-known/oauth-protected-resource/api/mcp",
Expand Down Expand Up @@ -160,7 +161,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"'
'Bearer resource_metadata="http://localhost/.well-known/oauth-protected-resource/api/mcp" scope="surveys:read surveys:write"'
);
expect(applyIPRateLimit).toHaveBeenCalled();
});
Expand Down Expand Up @@ -410,7 +411,7 @@ describe("POST /api/mcp", () => {
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"'
'Bearer resource_metadata="http://localhost/.well-known/oauth-protected-resource/api/mcp" scope="surveys:read surveys:write"'
);
});

Expand Down
27 changes: 27 additions & 0 deletions apps/web/modules/auth/lib/mcp-oauth-dcr.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,33 @@ describe("MCP OAuth Dynamic Client Registration → authorize (real-client shape
expect(registration.body.scope?.split(" ")).toEqual(expect.arrayContaining(advertisedScopes));
});

test("default registration (no scope requested) grants read + write", async () => {
const auth = createAuthInstance();

// A client that registers without an explicit scope must receive write by default — otherwise the
// consent screen only offers "Read surveys" and every write tool 403s (the ENG-1055 QA regression:
// clients that key off the challenge/defaults rather than the PRM never requested write).
const response = await auth.handler(
new Request(`${BASE_URL}/api/auth/oauth2/register`, {
method: "POST",
headers: { "content-type": "application/json" },
body: JSON.stringify({
client_name: "MCP DCR default-scope client",
redirect_uris: [REDIRECT_URI],
grant_types: ["authorization_code", "refresh_token"],
response_types: ["code"],
token_endpoint_auth_method: "none",
}),
})
);
const body = (await response.json()) as { scope?: string };

expect(response.status).toBe(200);
expect(body.scope?.split(" ")).toEqual(
expect.arrayContaining(["surveys:read", "surveys:write", "offline_access"])
);
});

test("authorize accepts the PRM-advertised scopes for a DCR client (no invalid_scope)", async () => {
const auth = createAuthInstance();
const advertisedScopes = await fetchAdvertisedScopes();
Expand Down
14 changes: 12 additions & 2 deletions apps/web/modules/auth/lib/mcp-oauth-provider-options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,18 @@ export const getMcpOauthProviderOptions = (): TOauthProviderOptions => ({
validAudiences: [getMcpResourceUrl()],
allowDynamicClientRegistration: true,
allowUnauthenticatedClientRegistration: true,
clientRegistrationDefaultScopes: ["openid", "profile", "email", "offline_access", "surveys:read"],
clientRegistrationAllowedScopes: ["surveys:write"],
// Register MCP clients with read + write by default so the consent screen offers write and the
// write tools are reachable (clients derive their DCR/authorize scopes from what we advertise, and
// the plugin validates authorize against the client's registered scopes). Granting the write scope
// is safe: actual write access is still enforced downstream by the user's workspace permissions.
clientRegistrationDefaultScopes: [
"openid",
"profile",
"email",
"offline_access",
"surveys:read",
"surveys:write",
],
accessTokenExpiresIn: 15 * 60,
refreshTokenExpiresIn: 30 * 24 * 60 * 60,
scopeExpirations: {
Expand Down
3 changes: 3 additions & 0 deletions apps/web/modules/mcp/auth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,9 @@ describe("authenticateMcpRequest", () => {
expect(result.ok).toBe(false);
if (!result.ok) {
expect(result.response.status).toBe(401);
// The challenge must advertise read + write so clients request write 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"');
expect(await result.response.json()).toMatchObject({
code: "not_authenticated",
detail: "API key or OAuth access token required",
Expand Down
9 changes: 8 additions & 1 deletion apps/web/modules/mcp/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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_RESOURCE_SCOPES,
getAuthIssuerUrl,
getMcpOrigin,
getMcpProtectedResourceMetadataUrl,
Expand All @@ -36,7 +37,13 @@ const QUERY_CREDENTIAL_PARAMS = new Set([
"authorization",
]);

// Minimum scope required to authenticate against the MCP server at all.
const DEFAULT_OAUTH_SCOPE = "surveys:read";
// 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 & {
Expand Down Expand Up @@ -198,7 +205,7 @@ export function withMcpResponseHeaders(response: Response, requestId: string): R
});
}

function withOAuthChallenge(response: Response, scope = DEFAULT_OAUTH_SCOPE): Response {
function withOAuthChallenge(response: Response, scope = MCP_CHALLENGE_SCOPE): Response {
const headers = new Headers(response.headers);
headers.set(
"WWW-Authenticate",
Expand Down
15 changes: 8 additions & 7 deletions apps/web/modules/mcp/tools/surveys.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -503,8 +503,11 @@ describe("registerSurveyTools", () => {
});
});

test("validate_survey rejects write validations for read-only OAuth scopes", async () => {
test("validate_survey allows patch validations for read-only OAuth scopes", async () => {
const { tools } = createToolServer();
vi.mocked(validateV3SurveyFromRawInput).mockResolvedValue(
successResponse({ valid: true, operation: "patch", invalid_params: [] }, { requestId: "req_tool" })
);

const result = await tools.get("validate_survey")!.handler(
{
Expand All @@ -517,12 +520,10 @@ describe("registerSurveyTools", () => {
{ authInfo: readOnlyOAuthAuthInfo }
);

expect(validateV3SurveyFromRawInput).not.toHaveBeenCalled();
expect(result.isError).toBe(true);
expect(result.structuredContent.error).toMatchObject({
status: 403,
code: "forbidden",
detail: "OAuth token does not include the required MCP scope",
expect(validateV3SurveyFromRawInput).toHaveBeenCalled();
expect(result.isError).toBeFalsy();
expect(result.structuredContent).toEqual({
data: { valid: true, operation: "patch", invalid_params: [] },
requestId: "req_tool",
});
});
Expand Down
8 changes: 4 additions & 4 deletions apps/web/modules/mcp/tools/surveys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,10 +215,10 @@ export function registerSurveyTools(server: McpServer): void {
},
async (input: TMcpValidateSurveyInput, extra) => {
const requestId = getMcpRequestId(extra.authInfo);
const requiredScopes =
input.operation === "patch" || input.operation === "create" ? ["surveys:write"] : ["surveys:read"];

const scopeError = await guardMcpScopes(extra.authInfo, requiredScopes, requestId);
// 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.
const scopeError = await guardMcpScopes(extra.authInfo, ["surveys:read"], requestId);
if (scopeError) {
return scopeError;
}
Expand Down
Loading