From f82943e6b3cd2b4239a41a037ab4929038e2a302 Mon Sep 17 00:00:00 2001 From: betegon Date: Tue, 20 Oct 2026 14:54:20 +0200 Subject: [PATCH 1/4] fix(mcp): reduce project constraint verification delays --- docs/specs/subpath-constraints.md | 9 + .../src/server/lib/constraint-utils.test.ts | 203 +++++++++++++++++- .../src/server/lib/constraint-utils.ts | 42 ++-- .../src/server/lib/mcp-handler.test.ts | 58 +++++ .../src/server/lib/mcp-handler.ts | 1 + .../src/test-utils/fetch-mock-setup.ts | 20 ++ .../mcp-core/src/api-client/client.test.ts | 77 +++++++ packages/mcp-core/src/api-client/client.ts | 11 +- 8 files changed, 395 insertions(+), 26 deletions(-) diff --git a/docs/specs/subpath-constraints.md b/docs/specs/subpath-constraints.md index f5cdd9eab..43756311e 100644 --- a/docs/specs/subpath-constraints.md +++ b/docs/specs/subpath-constraints.md @@ -44,6 +44,15 @@ If there’s a problem, you’ll receive a clear HTTP error when connecting: - 403: You don’t have access to the specified org/project - 404: Organization or project not found +For OAuth connections, project verification requests `collapse=organization` +to avoid fetching organization metadata that is not needed for project +capabilities. Successful verification is cached for 15 minutes. If project +verification takes longer than five seconds, the request continues with unknown +project capabilities. This result is cached for 60 seconds to reduce repeated +delays; after it expires, the next request retries verification. Cache propagation +and concurrent requests can still cause repeated verification. Each tool still +requires permission to access the requested data. + ## Region awareness For Sentry Cloud, your organization may be hosted in a regional cluster. When you scope by organization, we automatically determine the region (if available) and use it for API calls. You don’t need to take any action—this happens behind the scenes. For self-hosted Sentry, the region concept doesn’t apply. diff --git a/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts b/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts index c2a26b0bd..94399f6d4 100644 --- a/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts +++ b/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from "vitest"; +import { afterEach, describe, it, expect, vi } from "vitest"; import "urlpattern-polyfill"; import { verifyConstraintsAccess, @@ -14,24 +14,60 @@ function createMockKV(options?: { getError?: Error; putError?: Error; }): KVNamespace { + let stored = options?.getResult ?? null; + let expiresAt = Number.POSITIVE_INFINITY; return { get: vi.fn().mockImplementation(async () => { if (options?.getError) throw options.getError; - return options?.getResult ?? null; - }), - put: vi.fn().mockImplementation(async () => { - if (options?.putError) throw options.putError; + return Date.now() < expiresAt ? stored : null; }), + put: vi + .fn() + .mockImplementation( + async ( + _key: string, + value: string, + { expirationTtl }: { expirationTtl: number }, + ) => { + if (options?.putError) throw options.putError; + stored = JSON.parse(value); + expiresAt = Date.now() + expirationTtl * 1000; + }, + ), delete: vi.fn(), list: vi.fn(), getWithMetadata: vi.fn(), } as unknown as KVNamespace; } +/** + * Override the project HTTP response while keeping the shared org interceptor. + * The Node-side fetchMock cannot follow workerd's fake timeout clock. + */ +function mockProjectResponse( + respond: (url: URL) => Response | Promise, +) { + const originalFetch = globalThis.fetch; + return vi.spyOn(globalThis, "fetch").mockImplementation((input, init) => { + const url = input instanceof Request ? input.url : String(input); + const projectUrl = + "https://us.sentry.io/api/0/projects/sentry-mcp-evals/cloudflare-mcp/"; + if (url === projectUrl || url === `${projectUrl}?collapse=organization`) { + return Promise.resolve(respond(new URL(url))); + } + return originalFetch(input, init); + }); +} + describe("verifyConstraintsAccess", () => { const token = "test-token"; const host = "sentry.io"; + afterEach(() => { + vi.restoreAllMocks(); + vi.useRealTimers(); + }); + it("returns ok with empty constraints when no org constraint provided", async () => { const result = await verifyConstraintsAccess( { organizationSlug: null, projectSlug: null }, @@ -106,6 +142,44 @@ describe("verifyConstraintsAccess", () => { } }); + it("collapses organization metadata while preserving project capabilities", async () => { + let projectUrl: URL | undefined; + mockProjectResponse((url) => { + projectUrl = url; + return Response.json({ + id: "123", + slug: "cloudflare-mcp", + name: "Cloudflare MCP", + organization: { id: "456", slug: "sentry-mcp-evals" }, + hasProfiles: true, + hasReplays: false, + hasLogs: true, + firstTransactionEvent: false, + }); + }); + + const result = await verifyConstraintsAccess( + { organizationSlug: "sentry-mcp-evals", projectSlug: "cloudflare-mcp" }, + { accessToken: token, sentryHost: host }, + ); + + expect(projectUrl?.searchParams.get("collapse")).toBe("organization"); + expect(result).toEqual({ + ok: true, + constraints: { + organizationSlug: "sentry-mcp-evals", + projectSlug: "cloudflare-mcp", + regionUrl: "https://us.sentry.io", + projectCapabilities: { + profiles: true, + replays: false, + logs: true, + traces: false, + }, + }, + }); + }); + it("fails when org does not exist", async () => { const result = await verifyConstraintsAccess( { organizationSlug: "nonexistent-org", projectSlug: null }, @@ -136,6 +210,108 @@ describe("verifyConstraintsAccess", () => { }); describe("caching", () => { + it("reuses a project timeout for 60 seconds, then refreshes capabilities", async () => { + vi.useFakeTimers(); + let notifyProjectStarted!: () => void; + const projectStarted = new Promise((resolve) => { + notifyProjectStarted = resolve; + }); + const projectResponse = mockProjectResponse(() => { + notifyProjectStarted(); + return new Promise(() => {}); + }); + const kv = createMockKV(); + const cache: CacheOptions = { + kv, + userId: "timeout-user", + waitUntil: vi.fn(), + }; + const constraints = { + organizationSlug: "sentry-mcp-evals", + projectSlug: "cloudflare-mcp", + }; + const options = { accessToken: token, sentryHost: host, cache }; + const firstRequest = verifyConstraintsAccess(constraints, options); + await projectStarted; + await vi.advanceTimersByTimeAsync(5000); + + const timedOut = await firstRequest; + projectResponse.mockRestore(); + expect(timedOut).toEqual({ + ok: true, + constraints: { + ...constraints, + regionUrl: "https://us.sentry.io", + projectCapabilities: null, + }, + }); + expect(cache.waitUntil).toHaveBeenCalledOnce(); + await vi.mocked(cache.waitUntil).mock.calls[0][0]; + expect(kv.put).toHaveBeenCalledWith( + "caps:v1:timeout-user:sentry.io:sentry-mcp-evals:project:cloudflare-mcp", + expect.any(String), + { expirationTtl: 60 }, + ); + + await vi.advanceTimersByTimeAsync(59_999); + expect(await verifyConstraintsAccess(constraints, options)).toEqual( + timedOut, + ); + expect(kv.put).toHaveBeenCalledOnce(); + + await vi.advanceTimersByTimeAsync(1); + expect(await verifyConstraintsAccess(constraints, options)).toMatchObject( + { + ok: true, + constraints: { + projectCapabilities: { + profiles: false, + replays: false, + logs: false, + traces: false, + }, + }, + }, + ); + await vi.mocked(cache.waitUntil).mock.calls[1][0]; + expect(kv.put).toHaveBeenLastCalledWith( + "caps:v1:timeout-user:sentry.io:sentry-mcp-evals:project:cloudflare-mcp", + expect.any(String), + { expirationTtl: 900 }, + ); + }); + + it.each([401, 403, 404])( + "does not cache a project verification failure with HTTP %i", + async (status) => { + mockProjectResponse( + () => + new Response(JSON.stringify({ detail: "Project access denied" }), { + status, + headers: { "Content-Type": "application/json" }, + }), + ); + const kv = createMockKV(); + const cache: CacheOptions = { + kv, + userId: "denied-user", + waitUntil: vi.fn(), + }; + + const result = await verifyConstraintsAccess( + { + organizationSlug: "sentry-mcp-evals", + projectSlug: "cloudflare-mcp", + }, + { accessToken: token, sentryHost: host, cache }, + ); + + expect(result).toMatchObject({ ok: false, status }); + expect(kv.put).not.toHaveBeenCalled(); + expect(cache.waitUntil).not.toHaveBeenCalled(); + }, + ); + const cachedData: CachedConstraints = { regionUrl: "https://us.sentry.io", projectCapabilities: { @@ -152,6 +328,7 @@ describe("verifyConstraintsAccess", () => { const cache: CacheOptions = { kv: mockKV, userId: "user-123", + waitUntil: vi.fn(), }; const result = await verifyConstraintsAccess( @@ -190,6 +367,7 @@ describe("verifyConstraintsAccess", () => { const cache: CacheOptions = { kv: mockKV, userId: "user-456", + waitUntil: vi.fn(), }; const result = await verifyConstraintsAccess( @@ -216,8 +394,7 @@ describe("verifyConstraintsAccess", () => { // Verify cache was checked expect(mockKV.get).toHaveBeenCalledOnce(); - // Wait for fire-and-forget cache write - await new Promise((resolve) => setTimeout(resolve, 10)); + await vi.mocked(cache.waitUntil).mock.calls[0][0]; // Verify cache was written with correct key and TTL expect(mockKV.put).toHaveBeenCalledOnce(); @@ -247,6 +424,7 @@ describe("verifyConstraintsAccess", () => { const cache: CacheOptions = { kv: mockKV, userId: "user-789", + waitUntil: vi.fn(), }; const result = await verifyConstraintsAccess( @@ -273,6 +451,7 @@ describe("verifyConstraintsAccess", () => { const cache: CacheOptions = { kv: mockKV, userId: "user-write-fail", + waitUntil: vi.fn(), }; const result = await verifyConstraintsAccess( @@ -287,8 +466,9 @@ describe("verifyConstraintsAccess", () => { expect(result.constraints.projectSlug).toBe("cloudflare-mcp"); } - // Wait for fire-and-forget cache write attempt - await new Promise((resolve) => setTimeout(resolve, 10)); + await expect( + vi.mocked(cache.waitUntil).mock.calls[0][0], + ).resolves.toBeUndefined(); // Verify cache write was attempted expect(mockKV.put).toHaveBeenCalledOnce(); @@ -304,6 +484,7 @@ describe("verifyConstraintsAccess", () => { const cache: CacheOptions = { kv: mockKV, userId: "user-org-only", + waitUntil: vi.fn(), }; const result = await verifyConstraintsAccess( @@ -334,6 +515,7 @@ describe("verifyConstraintsAccess", () => { const cache: CacheOptions = { kv: mockKV, userId: "user-sentinel", + waitUntil: vi.fn(), }; const orgOnlyResult = await verifyConstraintsAccess( @@ -364,6 +546,7 @@ describe("verifyConstraintsAccess", () => { const cache: CacheOptions = { kv: mockKV, userId: "user-org-cache-write", + waitUntil: vi.fn(), }; const result = await verifyConstraintsAccess( @@ -373,7 +556,7 @@ describe("verifyConstraintsAccess", () => { expect(result.ok).toBe(true); expect(mockKV.get).toHaveBeenCalledOnce(); - await new Promise((resolve) => setTimeout(resolve, 10)); + await vi.mocked(cache.waitUntil).mock.calls[0][0]; expect(mockKV.put).toHaveBeenCalledOnce(); expect(mockKV.put).toHaveBeenCalledWith( "caps:v1:user-org-cache-write:sentry.io:sentry-mcp-evals:org", diff --git a/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts b/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts index f45eedf17..60e6d3e78 100644 --- a/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts +++ b/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts @@ -7,6 +7,7 @@ const CAPABILITIES_TIMEOUT_MS = 5000; // Cache configuration const CACHE_TTL_SECONDS = 900; // 15 minutes +const PROJECT_TIMEOUT_CACHE_TTL_SECONDS = 60; const CACHE_KEY_VERSION = "v1"; /** @@ -14,7 +15,7 @@ const CACHE_KEY_VERSION = "v1"; */ export type CachedConstraints = { regionUrl: string | null; - /** Populated when a project constraint was verified; null for org-only cache entries. */ + /** Null for org-only entries or a project verification timeout. */ projectCapabilities: ProjectCapabilities | null; cachedAt: number; }; @@ -25,6 +26,7 @@ export type CachedConstraints = { export type CacheOptions = { kv: KVNamespace; userId: string; + waitUntil: (promise: Promise) => void; }; /** @@ -54,8 +56,7 @@ async function getCachedConstraints( key: string, ): Promise { try { - const cached = await kv.get(key, "json"); - return cached as CachedConstraints | null; + return await kv.get(key, "json"); } catch (error) { logWarn("Failed to read constraints cache", { loggerScope: ["cloudflare", "constraint-utils"], @@ -67,16 +68,17 @@ async function getCachedConstraints( /** * Store constraints in KV cache. - * Fire-and-forget - errors are logged but don't block the response. + * Registered with waitUntil by the caller; write errors do not fail requests. */ async function setCachedConstraints( kv: KVNamespace, key: string, data: CachedConstraints, + ttlSeconds: number, ): Promise { try { await kv.put(key, JSON.stringify(data), { - expirationTtl: CACHE_TTL_SECONDS, + expirationTtl: ttlSeconds, }); } catch (error) { logWarn("Failed to write constraints cache", { @@ -114,8 +116,8 @@ async function withTimeout( * Verify that provided org/project constraints exist and the user has access * by querying Sentry's API using the provided OAuth access token. * - * If cache options are provided with a projectSlug, results will be cached - * in KV to avoid repeated API calls during MCP sessions. + * Cache verified constraints for 15 minutes and project timeouts for 60 seconds + * to reduce repeated request delays while periodically retrying verification. */ export async function verifyConstraintsAccess( { organizationSlug, projectSlug }: Constraints, @@ -214,6 +216,7 @@ export async function verifyConstraintsAccess( { organizationSlug, projectSlugOrId: projectSlug, + collapse: "organization", }, regionUrl ? { host: new URL(regionUrl).host } : undefined, ), @@ -259,14 +262,23 @@ export async function verifyConstraintsAccess( } } - // Cache: org-only after org fetch; org+project only when project verification - // succeeded (skip caching on project timeout so the next request can retry). - if (cacheKey && (!projectSlug || projectCapabilities !== null)) { - void setCachedConstraints(cache!.kv, cacheKey, { - regionUrl: regionUrl || null, - projectCapabilities: projectSlug ? projectCapabilities : null, - cachedAt: Date.now(), - }); + if (cache && cacheKey) { + const ttlSeconds = + projectSlug && projectCapabilities === null + ? PROJECT_TIMEOUT_CACHE_TTL_SECONDS + : CACHE_TTL_SECONDS; + cache.waitUntil( + setCachedConstraints( + cache.kv, + cacheKey, + { + regionUrl: regionUrl || null, + projectCapabilities, + cachedAt: Date.now(), + }, + ttlSeconds, + ), + ); } return { diff --git a/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts b/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts index f0535a2d5..3d5fcca14 100644 --- a/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts +++ b/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts @@ -560,6 +560,64 @@ describe("MCP Handler", () => { }); describe("URL constraints", () => { + it("keeps the constraint cache write alive after returning the response", async () => { + let finishWrite!: () => void; + const writeReady = new Promise((resolve) => { + finishWrite = resolve; + }); + let stored: unknown = null; + const env = createTestEnv(); + env.MCP_CACHE = { + get: vi.fn().mockResolvedValue(null), + put: vi.fn().mockImplementation(async (_key: string, value: string) => { + await writeReady; + stored = JSON.parse(value); + }), + } as unknown as KVNamespace; + const ctx = createMcpContext(); + const request = createMcpRequest( + "initialize", + { + protocolVersion: "2024-11-05", + capabilities: {}, + clientInfo: { name: "test-client", version: "1.0.0" }, + }, + { path: "/mcp/sentry-mcp-evals/cloudflare-mcp" }, + ); + + try { + const response = await mcpHandler.fetch!(request, env, ctx); + expect(response.status).toBe(200); + expect(stored).toBeNull(); + expect(ctx.waitUntil).toHaveBeenCalled(); + + let backgroundCompleted = false; + const background = Promise.all( + vi.mocked(ctx.waitUntil).mock.calls.map(([task]) => task), + ).then(() => { + backgroundCompleted = true; + }); + // Let an unrelated, already-resolved task settle before checking ownership. + await Promise.resolve(); + await Promise.resolve(); + expect(backgroundCompleted).toBe(false); + + finishWrite(); + await background; + expect(stored).toMatchObject({ + regionUrl: "https://us.sentry.io", + projectCapabilities: { + profiles: false, + replays: false, + logs: false, + traces: false, + }, + }); + } finally { + finishWrite(); + } + }); + it("should handle /mcp without constraints", async () => { const request = createMcpRequest( "initialize", diff --git a/packages/mcp-cloudflare/src/server/lib/mcp-handler.ts b/packages/mcp-cloudflare/src/server/lib/mcp-handler.ts index c884567f0..9959652e4 100644 --- a/packages/mcp-cloudflare/src/server/lib/mcp-handler.ts +++ b/packages/mcp-cloudflare/src/server/lib/mcp-handler.ts @@ -412,6 +412,7 @@ async function handleAuthenticatedMcpRequest( cache: { kv: env.MCP_CACHE, userId: auth.userId, + waitUntil: (promise) => ctx.waitUntil(promise), }, }, ); diff --git a/packages/mcp-cloudflare/src/test-utils/fetch-mock-setup.ts b/packages/mcp-cloudflare/src/test-utils/fetch-mock-setup.ts index 7b1d5c7dc..1ce2769e9 100644 --- a/packages/mcp-cloudflare/src/test-utils/fetch-mock-setup.ts +++ b/packages/mcp-cloudflare/src/test-utils/fetch-mock-setup.ts @@ -239,6 +239,14 @@ export function registerFetchMockInterceptors(fetchMock: FetchMockLike) { .reply(200, projectFixture, { headers: JSON_HEADERS }) .persist(); + pool + .intercept({ + path: "/api/0/projects/sentry-mcp-evals/cloudflare-mcp/?collapse=organization", + method: "GET", + }) + .reply(200, projectFixture, { headers: JSON_HEADERS }) + .persist(); + pool .intercept({ path: "/api/0/projects/sentry-mcp-evals/cloudflare-mcp/", @@ -258,6 +266,18 @@ export function registerFetchMockInterceptors(fetchMock: FetchMockLike) { ) .persist(); + pool + .intercept({ + path: "/api/0/projects/sentry-mcp-evals/nonexistent-project/?collapse=organization", + method: "GET", + }) + .reply( + 404, + { detail: "The requested resource does not exist" }, + { headers: JSON_HEADERS }, + ) + .persist(); + // ===== Client Keys ===== pool .intercept({ diff --git a/packages/mcp-core/src/api-client/client.test.ts b/packages/mcp-core/src/api-client/client.test.ts index bbc599a0b..6c88bec7b 100644 --- a/packages/mcp-core/src/api-client/client.test.ts +++ b/packages/mcp-core/src/api-client/client.test.ts @@ -997,6 +997,83 @@ describe("monitor time parameters", () => { }); }); +describe("getProject", () => { + const project = { + ...projectFixture, + id: "123", + slug: "backend", + name: "Backend", + organization: { + id: "456", + slug: "test-org", + name: "Test Organization", + features: ["organizations:issue-basic"], + }, + hasProfiles: true, + hasReplays: false, + hasLogs: true, + firstTransactionEvent: false, + }; + const api = new SentryApiService({ + host: "sentry.io", + accessToken: "test-token", + }); + + it("keeps the default request and full organization response", async () => { + const requests: string[] = []; + mswServer.use( + http.get( + "https://sentry.io/api/0/projects/test-org/backend/", + ({ request }) => { + requests.push(request.url); + return HttpResponse.json(project); + }, + ), + ); + + const result = await api.getProject({ + organizationSlug: "test-org", + projectSlugOrId: "backend", + }); + + expect(requests).toEqual([ + "https://sentry.io/api/0/projects/test-org/backend/", + ]); + expect(result).toEqual(project); + }); + + it("collapses organization on the regional host and preserves project capabilities", async () => { + const collapsedProject = { + ...project, + organization: { id: "456", slug: "test-org" }, + }; + const requests: string[] = []; + mswServer.use( + http.get( + "https://eu.sentry.io/api/0/projects/test-org/backend/", + ({ request }) => { + requests.push(request.url); + return HttpResponse.json(collapsedProject); + }, + ), + ); + + const result = await api.getProject( + { + organizationSlug: "test-org", + projectSlugOrId: "backend", + collapse: "organization", + }, + { host: "eu.sentry.io" }, + ); + + expect(requests).toEqual([ + "https://eu.sentry.io/api/0/projects/test-org/backend/?collapse=organization", + ]); + expect(result).toEqual(collapsedProject); + }); +}); + describe("getIssue", () => { it("uses the numeric issue ID when Sentry returns a null shortId", async () => { mswServer.use( diff --git a/packages/mcp-core/src/api-client/client.ts b/packages/mcp-core/src/api-client/client.ts index 24352c1da..6ef0a6955 100644 --- a/packages/mcp-core/src/api-client/client.ts +++ b/packages/mcp-core/src/api-client/client.ts @@ -1981,6 +1981,7 @@ export class SentryApiService { * @param params Project fetch parameters * @param params.organizationSlug Organization identifier * @param params.projectSlugOrId Project slug or numeric ID + * @param params.collapse Return only organization ID and slug instead of full organization data * @param opts Request options * @returns Project data */ @@ -1988,14 +1989,22 @@ export class SentryApiService { { organizationSlug, projectSlugOrId, + collapse, }: { organizationSlug: string; projectSlugOrId: string; + collapse?: "organization"; }, opts?: RequestOptions, ): Promise { + const queryParams = new URLSearchParams(); + if (collapse) { + queryParams.set("collapse", collapse); + } + const queryString = queryParams.toString(); + const projectPath = apiPath`/projects/${organizationSlug}/${projectSlugOrId}/`; const body = await this.requestJSON( - apiPath`/projects/${organizationSlug}/${projectSlugOrId}/`, + `${projectPath}${queryString ? `?${queryString}` : ""}`, undefined, opts, ); From 3e6da34ca8fc4f0a0023c7b94f66616bf0089ed5 Mon Sep 17 00:00:00 2001 From: betegon Date: Tue, 20 Oct 2026 16:35:59 +0200 Subject: [PATCH 2/4] test(mcp): cover authorization with unknown capabilities --- .../src/server/lib/mcp-handler.test.ts | 70 +++++++++++++++++++ packages/mcp-core/src/server.test.ts | 45 +++++++++++- 2 files changed, 114 insertions(+), 1 deletion(-) diff --git a/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts b/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts index 3d5fcca14..a473767ac 100644 --- a/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts +++ b/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts @@ -560,6 +560,76 @@ describe("MCP Handler", () => { }); describe("URL constraints", () => { + it("rejects upstream permission failures with cached unknown capabilities", async () => { + const env = createTestEnv(); + env.MCP_CACHE = { + get: vi.fn().mockResolvedValue({ + regionUrl: "https://us.sentry.io", + projectCapabilities: null, + cachedAt: Date.now(), + }), + put: vi.fn(), + } as unknown as KVNamespace; + const ctx = createMcpContext({ + accessToken: "test-current-user-token", + grantedSkills: ["inspect"], + constraintOrganizationSlug: "test-org", + constraintProjectSlug: "test-project", + }); + const profileId = "00000000000040008000000000000001"; + const request = createMcpRequest( + "tools/call", + { + name: "execute_sentry_tool", + arguments: { + name: "get_profile_details", + arguments: { profileId }, + }, + }, + { path: "/mcp/test-org/test-project" }, + ); + const apiFetch = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue( + Response.json( + { detail: "You do not have permission to perform this action." }, + { status: 403 }, + ), + ); + + try { + const response = await mcpHandler.fetch!(request, env, ctx); + + expect(response.status).toBe(200); + const body = await parseSSEResponse<{ + result: { + isError?: boolean; + content: Array<{ type: string; text: string }>; + }; + }>(response); + expect(body.result.isError).toBe(true); + expect(body.result.content).toEqual([ + { type: "text", text: expect.stringContaining("HTTP 403") }, + ]); + expect(env.MCP_CACHE.get).toHaveBeenCalledWith( + "caps:v1:test-user-123:sentry.io:test-org:project:test-project", + "json", + ); + expect(env.MCP_CACHE.put).not.toHaveBeenCalled(); + expect(apiFetch).toHaveBeenCalledOnce(); + expect(apiFetch).toHaveBeenCalledWith( + `https://us.sentry.io/api/0/projects/test-org/test-project/profiling/profiles/${profileId}/`, + expect.objectContaining({ + headers: expect.objectContaining({ + Authorization: `Bearer ${ctx.props.accessToken}`, + }), + }), + ); + } finally { + apiFetch.mockRestore(); + } + }); + it("keeps the constraint cache write alive after returning the response", async () => { let finishWrite!: () => void; const writeReady = new Promise((resolve) => { diff --git a/packages/mcp-core/src/server.test.ts b/packages/mcp-core/src/server.test.ts index 061ac44b9..c37f79f9a 100644 --- a/packages/mcp-core/src/server.test.ts +++ b/packages/mcp-core/src/server.test.ts @@ -94,7 +94,7 @@ async function listRegisteredTools(server: ReturnType) { } async function callRegisteredTool( - server: ReturnType, + server: ReturnType | ModernMcpServer, name: string, args: Record, ) { @@ -1358,6 +1358,49 @@ describe("buildServer", () => { ); }); + it.each(["v1", "v2"] as const)( + "execute_sentry_tool rejects missing skills with unknown capabilities on SDK %s", + async (sdkVersion) => { + const upstreamRequest = vi.fn(() => + HttpResponse.json( + { detail: "You do not have permission to perform this action." }, + { status: 403 }, + ), + ); + mswServer.use( + http.get( + "https://us.sentry.io/api/0/projects/test-org/test-project/profiling/profiles/:profileId/", + upstreamRequest, + ), + ); + const context: ServerContext = { + ...baseContext, + grantedSkills: new Set(["docs"]), + constraints: { + organizationSlug: "test-org", + projectSlug: "test-project", + regionUrl: "https://us.sentry.io", + projectCapabilities: null, + }, + }; + const server = + sdkVersion === "v2" + ? buildServer({ context, sdkVersion }) + : buildServer({ context }); + + const result = await callRegisteredTool(server, "execute_sentry_tool", { + name: "get_profile_details", + arguments: { profileId: "00000000000040008000000000000001" }, + }); + + expect(result).toMatchObject({ isError: true }); + expect(getTextContent(result)).toContain( + 'Tool "get_profile_details" is not available in this session', + ); + expect(upstreamRequest).not.toHaveBeenCalled(); + }, + ); + it("search_sentry_tools and execute_sentry_tool enforce project capabilities by default", async () => { const server = buildServer({ context: { From d02b400689369fdda8105698d35ad4e7389e8678 Mon Sep 17 00:00:00 2001 From: betegon Date: Tue, 20 Oct 2026 17:59:28 +0200 Subject: [PATCH 3/4] fix(mcp): preserve verified capabilities across timeout races --- docs/specs/subpath-constraints.md | 9 +- .../src/server/lib/constraint-utils.test.ts | 91 ++++++++++++++++--- .../src/server/lib/constraint-utils.ts | 19 ++-- .../src/server/lib/mcp-handler.test.ts | 6 +- 4 files changed, 102 insertions(+), 23 deletions(-) diff --git a/docs/specs/subpath-constraints.md b/docs/specs/subpath-constraints.md index 43756311e..983209876 100644 --- a/docs/specs/subpath-constraints.md +++ b/docs/specs/subpath-constraints.md @@ -49,9 +49,12 @@ to avoid fetching organization metadata that is not needed for project capabilities. Successful verification is cached for 15 minutes. If project verification takes longer than five seconds, the request continues with unknown project capabilities. This result is cached for 60 seconds to reduce repeated -delays; after it expires, the next request retries verification. Cache propagation -and concurrent requests can still cause repeated verification. Each tool still -requires permission to access the requested data. +delays; after it expires, the next request retries verification. Timeout results +use separate cache keys, and verified results take precedence, so a concurrent +timeout cannot overwrite verified capabilities or shorten their cache lifetime. +Cache propagation and concurrent requests can still cause repeated verification +or temporarily return unknown capabilities. Each tool still requires permission +to access the requested data. ## Region awareness diff --git a/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts b/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts index 94399f6d4..230ad742b 100644 --- a/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts +++ b/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts @@ -14,24 +14,32 @@ function createMockKV(options?: { getError?: Error; putError?: Error; }): KVNamespace { - let stored = options?.getResult ?? null; - let expiresAt = Number.POSITIVE_INFINITY; + const entries = new Map< + string, + { value: CachedConstraints; expiresAt: number } + >(); return { - get: vi.fn().mockImplementation(async () => { + get: vi.fn().mockImplementation(async (key: string) => { if (options?.getError) throw options.getError; - return Date.now() < expiresAt ? stored : null; + const entry = entries.get(key); + if (entry) { + return Date.now() < entry.expiresAt ? entry.value : null; + } + return options?.getResult ?? null; }), put: vi .fn() .mockImplementation( async ( - _key: string, + key: string, value: string, { expirationTtl }: { expirationTtl: number }, ) => { if (options?.putError) throw options.putError; - stored = JSON.parse(value); - expiresAt = Date.now() + expirationTtl * 1000; + entries.set(key, { + value: JSON.parse(value), + expiresAt: Date.now() + expirationTtl * 1000, + }); }, ), delete: vi.fn(), @@ -210,6 +218,65 @@ describe("verifyConstraintsAccess", () => { }); describe("caching", () => { + it("preserves verified capabilities when a concurrent request times out later", async () => { + vi.useFakeTimers(); + let notifyProjectStarted!: () => void; + const projectStarted = new Promise((resolve) => { + notifyProjectStarted = resolve; + }); + const slowProjectResponse = mockProjectResponse(() => { + notifyProjectStarted(); + return new Promise(() => {}); + }); + const kv = createMockKV(); + const cache: CacheOptions = { + kv, + userId: "concurrent-user", + waitUntil: vi.fn(), + }; + const constraints = { + organizationSlug: "sentry-mcp-evals", + projectSlug: "cloudflare-mcp", + }; + const options = { accessToken: token, sentryHost: host, cache }; + + const slowRequest = verifyConstraintsAccess(constraints, options); + await projectStarted; + slowProjectResponse.mockRestore(); + + const verified = await verifyConstraintsAccess(constraints, options); + expect(verified).toMatchObject({ + ok: true, + constraints: { + projectCapabilities: { + profiles: false, + replays: false, + logs: false, + traces: false, + }, + }, + }); + await vi.mocked(cache.waitUntil).mock.calls[0][0]; + + await vi.advanceTimersByTimeAsync(5000); + expect(await slowRequest).toMatchObject({ + ok: true, + constraints: { projectCapabilities: null }, + }); + await vi.mocked(cache.waitUntil).mock.calls[1][0]; + + const apiFetch = vi.spyOn(globalThis, "fetch"); + expect(await verifyConstraintsAccess(constraints, options)).toEqual( + verified, + ); + // The late timeout must not shorten the verified entry's lifetime either. + await vi.advanceTimersByTimeAsync(60_000); + expect(await verifyConstraintsAccess(constraints, options)).toEqual( + verified, + ); + expect(apiFetch).not.toHaveBeenCalled(); + }); + it("reuses a project timeout for 60 seconds, then refreshes capabilities", async () => { vi.useFakeTimers(); let notifyProjectStarted!: () => void; @@ -248,7 +315,7 @@ describe("verifyConstraintsAccess", () => { expect(cache.waitUntil).toHaveBeenCalledOnce(); await vi.mocked(cache.waitUntil).mock.calls[0][0]; expect(kv.put).toHaveBeenCalledWith( - "caps:v1:timeout-user:sentry.io:sentry-mcp-evals:project:cloudflare-mcp", + "timeout:caps:v1:timeout-user:sentry.io:sentry-mcp-evals:project:cloudflare-mcp", expect.any(String), { expirationTtl: 60 }, ); @@ -391,8 +458,8 @@ describe("verifyConstraintsAccess", () => { }); } - // Verify cache was checked - expect(mockKV.get).toHaveBeenCalledOnce(); + // Check verified constraints before the timeout fallback. + expect(mockKV.get).toHaveBeenCalledTimes(2); await vi.mocked(cache.waitUntil).mock.calls[0][0]; @@ -439,8 +506,8 @@ describe("verifyConstraintsAccess", () => { expect(result.constraints.projectSlug).toBe("cloudflare-mcp"); } - // Verify cache read was attempted - expect(mockKV.get).toHaveBeenCalledOnce(); + // Both cache reads can fail without blocking API verification. + expect(mockKV.get).toHaveBeenCalledTimes(2); }); it("succeeds even when cache write fails", async () => { diff --git a/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts b/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts index 60e6d3e78..c3a8c6200 100644 --- a/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts +++ b/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts @@ -33,6 +33,7 @@ export type CacheOptions = { * Build a cache key for constraints verification. * Format: caps:v1:{userId}:{sentryHost}:{organizationSlug}:{scopeKey} * scopeKey is "org" for org-only entries or "project:{slug}" for project-scoped entries. + * Timeouts use a separate timeout:{cacheKey} so late writes cannot overwrite verified data. */ function buildCacheKey( userId: string, @@ -167,7 +168,11 @@ export async function verifyConstraintsAccess( organizationSlug, projectSlug, ); - const cached = await getCachedConstraints(cache.kv, cacheKey); + const cached = + (await getCachedConstraints(cache.kv, cacheKey)) ?? + (projectSlug + ? await getCachedConstraints(cache.kv, `timeout:${cacheKey}`) + : null); if (cached) { return { ok: true, @@ -263,20 +268,20 @@ export async function verifyConstraintsAccess( } if (cache && cacheKey) { - const ttlSeconds = - projectSlug && projectCapabilities === null - ? PROJECT_TIMEOUT_CACHE_TTL_SECONDS - : CACHE_TTL_SECONDS; + const isProjectTimeout = + Boolean(projectSlug) && projectCapabilities === null; cache.waitUntil( setCachedConstraints( cache.kv, - cacheKey, + isProjectTimeout ? `timeout:${cacheKey}` : cacheKey, { regionUrl: regionUrl || null, projectCapabilities, cachedAt: Date.now(), }, - ttlSeconds, + isProjectTimeout + ? PROJECT_TIMEOUT_CACHE_TTL_SECONDS + : CACHE_TTL_SECONDS, ), ); } diff --git a/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts b/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts index a473767ac..352e1fbcc 100644 --- a/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts +++ b/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts @@ -563,7 +563,7 @@ describe("MCP Handler", () => { it("rejects upstream permission failures with cached unknown capabilities", async () => { const env = createTestEnv(); env.MCP_CACHE = { - get: vi.fn().mockResolvedValue({ + get: vi.fn().mockResolvedValueOnce(null).mockResolvedValueOnce({ regionUrl: "https://us.sentry.io", projectCapabilities: null, cachedAt: Date.now(), @@ -615,6 +615,10 @@ describe("MCP Handler", () => { "caps:v1:test-user-123:sentry.io:test-org:project:test-project", "json", ); + expect(env.MCP_CACHE.get).toHaveBeenCalledWith( + "timeout:caps:v1:test-user-123:sentry.io:test-org:project:test-project", + "json", + ); expect(env.MCP_CACHE.put).not.toHaveBeenCalled(); expect(apiFetch).toHaveBeenCalledOnce(); expect(apiFetch).toHaveBeenCalledWith( From 56edddacd4f4dc738995a608a228e1672dd1b077 Mon Sep 17 00:00:00 2001 From: betegon Date: Tue, 20 Oct 2026 18:21:14 +0200 Subject: [PATCH 4/4] test(mcp): strengthen project verification regression coverage --- .../src/server/lib/constraint-utils.test.ts | 14 ++-- .../src/server/lib/mcp-handler.test.ts | 84 ++++++++++--------- .../src/test-utils/fetch-mock-setup.ts | 12 ++- .../mcp-core/src/api-client/client.test.ts | 2 + 4 files changed, 64 insertions(+), 48 deletions(-) diff --git a/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts b/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts index 230ad742b..b560227fe 100644 --- a/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts +++ b/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts @@ -315,16 +315,17 @@ describe("verifyConstraintsAccess", () => { expect(cache.waitUntil).toHaveBeenCalledOnce(); await vi.mocked(cache.waitUntil).mock.calls[0][0]; expect(kv.put).toHaveBeenCalledWith( - "timeout:caps:v1:timeout-user:sentry.io:sentry-mcp-evals:project:cloudflare-mcp", + expect.any(String), expect.any(String), { expirationTtl: 60 }, ); + const apiFetch = vi.spyOn(globalThis, "fetch"); await vi.advanceTimersByTimeAsync(59_999); expect(await verifyConstraintsAccess(constraints, options)).toEqual( timedOut, ); - expect(kv.put).toHaveBeenCalledOnce(); + expect(apiFetch).not.toHaveBeenCalled(); await vi.advanceTimersByTimeAsync(1); expect(await verifyConstraintsAccess(constraints, options)).toMatchObject( @@ -340,9 +341,10 @@ describe("verifyConstraintsAccess", () => { }, }, ); + expect(apiFetch).toHaveBeenCalled(); await vi.mocked(cache.waitUntil).mock.calls[1][0]; expect(kv.put).toHaveBeenLastCalledWith( - "caps:v1:timeout-user:sentry.io:sentry-mcp-evals:project:cloudflare-mcp", + expect.any(String), expect.any(String), { expirationTtl: 900 }, ); @@ -458,9 +460,6 @@ describe("verifyConstraintsAccess", () => { }); } - // Check verified constraints before the timeout fallback. - expect(mockKV.get).toHaveBeenCalledTimes(2); - await vi.mocked(cache.waitUntil).mock.calls[0][0]; // Verify cache was written with correct key and TTL @@ -505,9 +504,6 @@ describe("verifyConstraintsAccess", () => { expect(result.constraints.organizationSlug).toBe("sentry-mcp-evals"); expect(result.constraints.projectSlug).toBe("cloudflare-mcp"); } - - // Both cache reads can fail without blocking API verification. - expect(mockKV.get).toHaveBeenCalledTimes(2); }); it("succeeds even when cache write fails", async () => { diff --git a/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts b/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts index 352e1fbcc..237e12bd4 100644 --- a/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts +++ b/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts @@ -1,4 +1,8 @@ import type { ExecutionContext, RateLimit } from "@cloudflare/workers-types"; +import { + createExecutionContext, + waitOnExecutionContext, +} from "cloudflare:test"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { DIRECT_AUTH_ASSERTION_TOKEN } from "../../test-utils/fetch-mock-setup"; import type { Env } from "../types"; @@ -8,7 +12,8 @@ const { sentryMetricsCount, sentrySetUser } = vi.hoisted(() => ({ sentrySetUser: vi.fn(), })); -vi.mock("@sentry/cloudflare", () => ({ +vi.mock("@sentry/cloudflare", async (importOriginal) => ({ + ...(await importOriginal()), getActiveSpan: vi.fn(() => undefined), metrics: { count: sentryMetricsCount, @@ -562,12 +567,18 @@ describe("MCP Handler", () => { describe("URL constraints", () => { it("rejects upstream permission failures with cached unknown capabilities", async () => { const env = createTestEnv(); + const timeoutKey = + "timeout:caps:v1:test-user-123:sentry.io:test-org:project:test-project"; env.MCP_CACHE = { - get: vi.fn().mockResolvedValueOnce(null).mockResolvedValueOnce({ - regionUrl: "https://us.sentry.io", - projectCapabilities: null, - cachedAt: Date.now(), - }), + get: vi.fn().mockImplementation(async (key: string) => + key === timeoutKey + ? { + regionUrl: "https://us.sentry.io", + projectCapabilities: null, + cachedAt: Date.now(), + } + : null, + ), put: vi.fn(), } as unknown as KVNamespace; const ctx = createMcpContext({ @@ -577,6 +588,7 @@ describe("MCP Handler", () => { constraintProjectSlug: "test-project", }); const profileId = "00000000000040008000000000000001"; + const profileUrl = `https://us.sentry.io/api/0/projects/test-org/test-project/profiling/profiles/${profileId}/`; const request = createMcpRequest( "tools/call", { @@ -588,14 +600,23 @@ describe("MCP Handler", () => { }, { path: "/mcp/test-org/test-project" }, ); + const originalFetch = globalThis.fetch; const apiFetch = vi .spyOn(globalThis, "fetch") - .mockResolvedValue( - Response.json( - { detail: "You do not have permission to perform this action." }, - { status: 403 }, - ), - ); + .mockImplementation((input, init) => { + const url = input instanceof Request ? input.url : String(input); + if (url === profileUrl) { + return Promise.resolve( + Response.json( + { + detail: "You do not have permission to perform this action.", + }, + { status: 403 }, + ), + ); + } + return originalFetch(input, init); + }); try { const response = await mcpHandler.fetch!(request, env, ctx); @@ -611,18 +632,10 @@ describe("MCP Handler", () => { expect(body.result.content).toEqual([ { type: "text", text: expect.stringContaining("HTTP 403") }, ]); - expect(env.MCP_CACHE.get).toHaveBeenCalledWith( - "caps:v1:test-user-123:sentry.io:test-org:project:test-project", - "json", - ); - expect(env.MCP_CACHE.get).toHaveBeenCalledWith( - "timeout:caps:v1:test-user-123:sentry.io:test-org:project:test-project", - "json", - ); expect(env.MCP_CACHE.put).not.toHaveBeenCalled(); expect(apiFetch).toHaveBeenCalledOnce(); expect(apiFetch).toHaveBeenCalledWith( - `https://us.sentry.io/api/0/projects/test-org/test-project/profiling/profiles/${profileId}/`, + profileUrl, expect.objectContaining({ headers: expect.objectContaining({ Authorization: `Bearer ${ctx.props.accessToken}`, @@ -648,7 +661,9 @@ describe("MCP Handler", () => { stored = JSON.parse(value); }), } as unknown as KVNamespace; - const ctx = createMcpContext(); + const ctx = Object.assign(createExecutionContext(), { + props: { ...DEFAULT_OAUTH_PROPS }, + }); const request = createMcpRequest( "initialize", { @@ -662,33 +677,26 @@ describe("MCP Handler", () => { try { const response = await mcpHandler.fetch!(request, env, ctx); expect(response.status).toBe(200); + const body = await parseSSEResponse<{ + result: { protocolVersion: string }; + }>(response); + expect(body.result.protocolVersion).toBeDefined(); expect(stored).toBeNull(); - expect(ctx.waitUntil).toHaveBeenCalled(); let backgroundCompleted = false; - const background = Promise.all( - vi.mocked(ctx.waitUntil).mock.calls.map(([task]) => task), - ).then(() => { + const background = waitOnExecutionContext(ctx).then(() => { backgroundCompleted = true; }); - // Let an unrelated, already-resolved task settle before checking ownership. - await Promise.resolve(); - await Promise.resolve(); + // Let completed tasks drain; only the blocked KV write should remain. + await new Promise((resolve) => setTimeout(resolve, 0)); expect(backgroundCompleted).toBe(false); finishWrite(); await background; - expect(stored).toMatchObject({ - regionUrl: "https://us.sentry.io", - projectCapabilities: { - profiles: false, - replays: false, - logs: false, - traces: false, - }, - }); + expect(stored).not.toBeNull(); } finally { finishWrite(); + await waitOnExecutionContext(ctx); } }); diff --git a/packages/mcp-cloudflare/src/test-utils/fetch-mock-setup.ts b/packages/mcp-cloudflare/src/test-utils/fetch-mock-setup.ts index 1ce2769e9..7081f0835 100644 --- a/packages/mcp-cloudflare/src/test-utils/fetch-mock-setup.ts +++ b/packages/mcp-cloudflare/src/test-utils/fetch-mock-setup.ts @@ -244,7 +244,17 @@ export function registerFetchMockInterceptors(fetchMock: FetchMockLike) { path: "/api/0/projects/sentry-mcp-evals/cloudflare-mcp/?collapse=organization", method: "GET", }) - .reply(200, projectFixture, { headers: JSON_HEADERS }) + .reply( + 200, + { + ...projectFixture, + organization: { + id: organizationFixture.id, + slug: organizationFixture.slug, + }, + }, + { headers: JSON_HEADERS }, + ) .persist(); pool diff --git a/packages/mcp-core/src/api-client/client.test.ts b/packages/mcp-core/src/api-client/client.test.ts index 6c88bec7b..981acad19 100644 --- a/packages/mcp-core/src/api-client/client.test.ts +++ b/packages/mcp-core/src/api-client/client.test.ts @@ -998,6 +998,8 @@ describe("monitor time parameters", () => { }); describe("getProject", () => { + afterEach(() => mswServer.resetHandlers()); + const project = { ...projectFixture, id: "123",