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 c2a26b0b..699bf32f 100644 --- a/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts +++ b/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, vi } from "vitest"; import "urlpattern-polyfill"; +import { SentryApiService } from "@sentry/mcp-core/api-client"; import { verifyConstraintsAccess, type CachedConstraints, @@ -10,13 +11,20 @@ import { * Create a mock KVNamespace for testing cache behavior. */ function createMockKV(options?: { - getResult?: CachedConstraints | null; + getResult?: unknown; + getResultByKey?: Record; getError?: Error; putError?: Error; }): KVNamespace { return { - get: vi.fn().mockImplementation(async () => { + get: vi.fn().mockImplementation(async (key: string) => { if (options?.getError) throw options.getError; + if ( + options?.getResultByKey && + Object.hasOwn(options.getResultByKey, key) + ) { + return options.getResultByKey[key]; + } return options?.getResult ?? null; }), put: vi.fn().mockImplementation(async () => { @@ -28,6 +36,38 @@ function createMockKV(options?: { } as unknown as KVNamespace; } +type TestCacheOptions = CacheOptions & { + waitUntil: ReturnType; +}; + +function createCache(kv: KVNamespace, userId: string): TestCacheOptions { + return { + kv, + userId, + waitUntil: vi.fn((promise: Promise) => { + void promise; + }), + }; +} + +async function flushCacheWrites(cache: TestCacheOptions): Promise { + await Promise.all( + cache.waitUntil.mock.calls.map(([promise]) => promise as Promise), + ); +} + +function getCachePuts(kv: KVNamespace): Array<{ + key: string; + body: CachedConstraints; + ttl: number | undefined; +}> { + return vi.mocked(kv.put).mock.calls.map(([key, value, options]) => ({ + key: key as string, + body: JSON.parse(value as string) as CachedConstraints, + ttl: (options as { expirationTtl?: number } | undefined)?.expirationTtl, + })); +} + describe("verifyConstraintsAccess", () => { const token = "test-token"; const host = "sentry.io"; @@ -137,6 +177,8 @@ describe("verifyConstraintsAccess", () => { describe("caching", () => { const cachedData: CachedConstraints = { + scope: "project", + status: "verified", regionUrl: "https://us.sentry.io", projectCapabilities: { profiles: true, @@ -149,10 +191,7 @@ describe("verifyConstraintsAccess", () => { it("returns cached data without making API calls on cache hit", async () => { const mockKV = createMockKV({ getResult: cachedData }); - const cache: CacheOptions = { - kv: mockKV, - userId: "user-123", - }; + const cache = createCache(mockKV, "user-123"); const result = await verifyConstraintsAccess( { organizationSlug: "sentry-mcp-evals", projectSlug: "cloudflare-mcp" }, @@ -177,7 +216,7 @@ describe("verifyConstraintsAccess", () => { // Verify cache was checked expect(mockKV.get).toHaveBeenCalledOnce(); expect(mockKV.get).toHaveBeenCalledWith( - "caps:v1:user-123:sentry.io:sentry-mcp-evals:project:cloudflare-mcp", + "caps:v2:user-123:sentry.io:sentry-mcp-evals:project:cloudflare-mcp", "json", ); @@ -187,10 +226,7 @@ describe("verifyConstraintsAccess", () => { it("fetches from API and populates cache on cache miss", async () => { const mockKV = createMockKV({ getResult: null }); - const cache: CacheOptions = { - kv: mockKV, - userId: "user-456", - }; + const cache = createCache(mockKV, "user-456"); const result = await verifyConstraintsAccess( { organizationSlug: "sentry-mcp-evals", projectSlug: "cloudflare-mcp" }, @@ -213,41 +249,221 @@ describe("verifyConstraintsAccess", () => { }); } - // Verify cache was checked - expect(mockKV.get).toHaveBeenCalledOnce(); + // Verify project cache miss checked the org-only fallback. + expect(mockKV.get).toHaveBeenCalledTimes(2); + expect(mockKV.get).toHaveBeenNthCalledWith( + 1, + "caps:v2:user-456:sentry.io:sentry-mcp-evals:project:cloudflare-mcp", + "json", + ); + expect(mockKV.get).toHaveBeenNthCalledWith( + 2, + "caps:v2:user-456:sentry.io:sentry-mcp-evals:org", + "json", + ); - // Wait for fire-and-forget cache write - await new Promise((resolve) => setTimeout(resolve, 10)); + await flushCacheWrites(cache); + + // Verify org and project cache entries were written with correct keys and TTL. + expect(mockKV.put).toHaveBeenCalledTimes(2); + expect(getCachePuts(mockKV)).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + key: "caps:v2:user-456:sentry.io:sentry-mcp-evals:org", + ttl: 900, + body: expect.objectContaining({ + scope: "org", + regionUrl: "https://us.sentry.io", + cachedAt: expect.any(Number), + }), + }), + expect.objectContaining({ + key: "caps:v2:user-456:sentry.io:sentry-mcp-evals:project:cloudflare-mcp", + ttl: 900, + body: expect.objectContaining({ + scope: "project", + status: "verified", + regionUrl: "https://us.sentry.io", + projectCapabilities: { + profiles: false, + replays: false, + logs: false, + traces: false, + }, + cachedAt: expect.any(Number), + }), + }), + ]), + ); + }); + + it("schedules cache writes with waitUntil", async () => { + const mockKV = createMockKV({ getResult: null }); + const cache = createCache(mockKV, "user-wait-until"); - // Verify cache was written with correct key and TTL + const result = await verifyConstraintsAccess( + { organizationSlug: "sentry-mcp-evals", projectSlug: null }, + { accessToken: token, sentryHost: host, cache }, + ); + + expect(result.ok).toBe(true); + expect(cache.waitUntil).toHaveBeenCalledOnce(); + await flushCacheWrites(cache); expect(mockKV.put).toHaveBeenCalledOnce(); expect(mockKV.put).toHaveBeenCalledWith( - "caps:v1:user-456:sentry.io:sentry-mcp-evals:project:cloudflare-mcp", + "caps:v2:user-wait-until:sentry.io:sentry-mcp-evals:org", expect.any(String), { expirationTtl: 900 }, ); + }); - // Verify cached data structure - const cachedJson = vi.mocked(mockKV.put).mock.calls[0][1] as string; - const parsed = JSON.parse(cachedJson); - expect(parsed).toMatchObject({ + it("uses the org-only cache before verifying a project cache miss", async () => { + const orgOnlyCached: CachedConstraints = { + scope: "org", regionUrl: "https://us.sentry.io", - projectCapabilities: { - profiles: false, - replays: false, - logs: false, - traces: false, + cachedAt: Date.now(), + }; + const mockKV = createMockKV({ + getResultByKey: { + "caps:v2:user-project-fallback:sentry.io:sentry-mcp-evals:project:cloudflare-mcp": + null, + "caps:v2:user-project-fallback:sentry.io:sentry-mcp-evals:org": + orgOnlyCached, }, - cachedAt: expect.any(Number), }); + const cache = createCache(mockKV, "user-project-fallback"); + + const result = await verifyConstraintsAccess( + { organizationSlug: "sentry-mcp-evals", projectSlug: "cloudflare-mcp" }, + { accessToken: token, sentryHost: host, cache }, + ); + + expect(result.ok).toBe(true); + expect(mockKV.get).toHaveBeenCalledTimes(2); + expect(mockKV.get).toHaveBeenNthCalledWith( + 1, + "caps:v2:user-project-fallback:sentry.io:sentry-mcp-evals:project:cloudflare-mcp", + "json", + ); + expect(mockKV.get).toHaveBeenNthCalledWith( + 2, + "caps:v2:user-project-fallback:sentry.io:sentry-mcp-evals:org", + "json", + ); + }); + + it("briefly caches project verification timeouts", async () => { + vi.useFakeTimers(); + const mockKV = createMockKV({ getResult: null }); + const cache = createCache(mockKV, "user-project-timeout"); + let markProjectStarted = () => {}; + const projectStarted = new Promise((resolve) => { + markProjectStarted = resolve; + }); + const getProjectSpy = vi + .spyOn(SentryApiService.prototype, "getProject") + .mockImplementation(() => { + markProjectStarted(); + return new Promise(() => {}) as ReturnType< + SentryApiService["getProject"] + >; + }); + + try { + const resultPromise = verifyConstraintsAccess( + { + organizationSlug: "sentry-mcp-evals", + projectSlug: "cloudflare-mcp", + }, + { accessToken: token, sentryHost: host, cache }, + ); + + await projectStarted; + await vi.advanceTimersByTimeAsync(5000); + + const result = await resultPromise; + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.constraints).toEqual({ + organizationSlug: "sentry-mcp-evals", + projectSlug: "cloudflare-mcp", + regionUrl: "https://us.sentry.io", + projectCapabilities: null, + }); + } + + expect(cache.waitUntil).toHaveBeenCalledTimes(2); + await flushCacheWrites(cache); + const cachePuts = getCachePuts(mockKV); + expect(cachePuts).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + key: "caps:v2:user-project-timeout:sentry.io:sentry-mcp-evals:org", + ttl: 900, + body: expect.objectContaining({ + scope: "org", + regionUrl: "https://us.sentry.io", + cachedAt: expect.any(Number), + }), + }), + expect.objectContaining({ + key: "caps:v2:user-project-timeout:sentry.io:sentry-mcp-evals:project:cloudflare-mcp", + ttl: 60, + body: expect.objectContaining({ + scope: "project", + status: "timeout", + regionUrl: "https://us.sentry.io", + cachedAt: expect.any(Number), + }), + }), + ]), + ); + const projectTimeoutPut = cachePuts.find( + (put) => + put.key === + "caps:v2:user-project-timeout:sentry.io:sentry-mcp-evals:project:cloudflare-mcp", + ); + expect(projectTimeoutPut).toBeDefined(); + if (projectTimeoutPut) { + expect("projectCapabilities" in projectTimeoutPut.body).toBe(false); + } + } finally { + getProjectSpy.mockRestore(); + vi.useRealTimers(); + } + }); + + it("uses cached project timeouts as explicit fail-open entries", async () => { + const cachedTimeout: CachedConstraints = { + scope: "project", + status: "timeout", + regionUrl: "https://us.sentry.io", + cachedAt: Date.now(), + }; + const mockKV = createMockKV({ getResult: cachedTimeout }); + const cache = createCache(mockKV, "user-cached-timeout"); + + const result = await verifyConstraintsAccess( + { organizationSlug: "sentry-mcp-evals", projectSlug: "cloudflare-mcp" }, + { accessToken: token, sentryHost: host, cache }, + ); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.constraints).toEqual({ + organizationSlug: "sentry-mcp-evals", + projectSlug: "cloudflare-mcp", + regionUrl: "https://us.sentry.io", + projectCapabilities: null, + }); + } + expect(mockKV.get).toHaveBeenCalledOnce(); + expect(mockKV.put).not.toHaveBeenCalled(); }); it("proceeds without cache when cache read fails", async () => { const mockKV = createMockKV({ getError: new Error("KV unavailable") }); - const cache: CacheOptions = { - kv: mockKV, - userId: "user-789", - }; + const cache = createCache(mockKV, "user-789"); const result = await verifyConstraintsAccess( { organizationSlug: "sentry-mcp-evals", projectSlug: "cloudflare-mcp" }, @@ -261,8 +477,8 @@ describe("verifyConstraintsAccess", () => { expect(result.constraints.projectSlug).toBe("cloudflare-mcp"); } - // Verify cache read was attempted - expect(mockKV.get).toHaveBeenCalledOnce(); + // Verify project cache and org fallback reads were attempted. + expect(mockKV.get).toHaveBeenCalledTimes(2); }); it("succeeds even when cache write fails", async () => { @@ -270,10 +486,7 @@ describe("verifyConstraintsAccess", () => { getResult: null, putError: new Error("KV write failed"), }); - const cache: CacheOptions = { - kv: mockKV, - userId: "user-write-fail", - }; + const cache = createCache(mockKV, "user-write-fail"); const result = await verifyConstraintsAccess( { organizationSlug: "sentry-mcp-evals", projectSlug: "cloudflare-mcp" }, @@ -287,24 +500,20 @@ 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 flushCacheWrites(cache); - // Verify cache write was attempted - expect(mockKV.put).toHaveBeenCalledOnce(); + // Verify org and project cache writes were attempted. + expect(mockKV.put).toHaveBeenCalledTimes(2); }); it("uses cache for org-only verification", async () => { const orgOnlyCached: CachedConstraints = { + scope: "org", regionUrl: "https://us.sentry.io", - projectCapabilities: null, cachedAt: Date.now(), }; const mockKV = createMockKV({ getResult: orgOnlyCached }); - const cache: CacheOptions = { - kv: mockKV, - userId: "user-org-only", - }; + const cache = createCache(mockKV, "user-org-only"); const result = await verifyConstraintsAccess( { organizationSlug: "sentry-mcp-evals", projectSlug: null }, @@ -323,18 +532,26 @@ describe("verifyConstraintsAccess", () => { expect(mockKV.get).toHaveBeenCalledOnce(); expect(mockKV.get).toHaveBeenCalledWith( - "caps:v1:user-org-only:sentry.io:sentry-mcp-evals:org", + "caps:v2:user-org-only:sentry.io:sentry-mcp-evals:org", "json", ); expect(mockKV.put).not.toHaveBeenCalled(); }); it("uses distinct cache keys for org-only and '__org__' project constraints", async () => { - const mockKV = createMockKV({ getResult: cachedData }); - const cache: CacheOptions = { - kv: mockKV, - userId: "user-sentinel", + const orgOnlyCached: CachedConstraints = { + scope: "org", + regionUrl: "https://us.sentry.io", + cachedAt: Date.now(), }; + const mockKV = createMockKV({ + getResultByKey: { + "caps:v2:user-sentinel:sentry.io:sentry-mcp-evals:org": orgOnlyCached, + "caps:v2:user-sentinel:sentry.io:sentry-mcp-evals:project:__org__": + cachedData, + }, + }); + const cache = createCache(mockKV, "user-sentinel"); const orgOnlyResult = await verifyConstraintsAccess( { organizationSlug: "sentry-mcp-evals", projectSlug: null }, @@ -349,22 +566,113 @@ describe("verifyConstraintsAccess", () => { expect(projectResult.ok).toBe(true); expect(mockKV.get).toHaveBeenNthCalledWith( 1, - "caps:v1:user-sentinel:sentry.io:sentry-mcp-evals:org", + "caps:v2:user-sentinel:sentry.io:sentry-mcp-evals:org", "json", ); expect(mockKV.get).toHaveBeenNthCalledWith( 2, - "caps:v1:user-sentinel:sentry.io:sentry-mcp-evals:project:__org__", + "caps:v2:user-sentinel:sentry.io:sentry-mcp-evals:project:__org__", "json", ); }); + it("does not collapse a project scope into the org cache key during key construction", async () => { + const orgOnlyCached: CachedConstraints = { + scope: "org", + regionUrl: "https://us.sentry.io", + cachedAt: Date.now(), + }; + const mockKV = createMockKV({ + getResultByKey: { + "caps:v2:user-no-collapse:sentry.io:sentry-mcp-evals:org": + orgOnlyCached, + "caps:v2:user-no-collapse:sentry.io:sentry-mcp-evals:project: ": + cachedData, + }, + }); + const cache = createCache(mockKV, "user-no-collapse"); + + const result = await verifyConstraintsAccess( + { organizationSlug: "sentry-mcp-evals", projectSlug: " " }, + { accessToken: token, sentryHost: host, cache }, + ); + + expect(result.ok).toBe(true); + expect(mockKV.get).toHaveBeenCalledOnce(); + expect(mockKV.get).toHaveBeenCalledWith( + "caps:v2:user-no-collapse:sentry.io:sentry-mcp-evals:project: ", + "json", + ); + }); + + it("does not treat an org cache entry as a project cache hit", async () => { + const orgOnlyCached: CachedConstraints = { + scope: "org", + regionUrl: "https://us.sentry.io", + cachedAt: Date.now(), + }; + const mockKV = createMockKV({ + getResultByKey: { + "caps:v2:user-wrong-scope:sentry.io:sentry-mcp-evals:project:cloudflare-mcp": + orgOnlyCached, + "caps:v2:user-wrong-scope:sentry.io:sentry-mcp-evals:org": null, + }, + }); + const cache = createCache(mockKV, "user-wrong-scope"); + + const result = await verifyConstraintsAccess( + { organizationSlug: "sentry-mcp-evals", projectSlug: "cloudflare-mcp" }, + { accessToken: token, sentryHost: host, cache }, + ); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.constraints.projectCapabilities).toEqual({ + profiles: false, + replays: false, + logs: false, + traces: false, + }); + } + }); + + it("ignores malformed cached entries", async () => { + const mockKV = createMockKV({ + getResultByKey: { + "caps:v2:user-malformed:sentry.io:sentry-mcp-evals:project:cloudflare-mcp": + { + scope: "project", + status: "verified", + regionUrl: "https://us.sentry.io", + cachedAt: Date.now(), + projectCapabilities: { + profiles: "yes", + }, + }, + "caps:v2:user-malformed:sentry.io:sentry-mcp-evals:org": null, + }, + }); + const cache = createCache(mockKV, "user-malformed"); + + const result = await verifyConstraintsAccess( + { organizationSlug: "sentry-mcp-evals", projectSlug: "cloudflare-mcp" }, + { accessToken: token, sentryHost: host, cache }, + ); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.constraints.projectCapabilities).toEqual({ + profiles: false, + replays: false, + logs: false, + traces: false, + }); + } + }); + it("writes KV cache after org-only verification on miss", async () => { const mockKV = createMockKV({ getResult: null }); - const cache: CacheOptions = { - kv: mockKV, - userId: "user-org-cache-write", - }; + const cache = createCache(mockKV, "user-org-cache-write"); const result = await verifyConstraintsAccess( { organizationSlug: "sentry-mcp-evals", projectSlug: null }, @@ -373,10 +681,10 @@ describe("verifyConstraintsAccess", () => { expect(result.ok).toBe(true); expect(mockKV.get).toHaveBeenCalledOnce(); - await new Promise((resolve) => setTimeout(resolve, 10)); + await flushCacheWrites(cache); expect(mockKV.put).toHaveBeenCalledOnce(); expect(mockKV.put).toHaveBeenCalledWith( - "caps:v1:user-org-cache-write:sentry.io:sentry-mcp-evals:org", + "caps:v2:user-org-cache-write:sentry.io:sentry-mcp-evals:org", expect.any(String), { expirationTtl: 900 }, ); @@ -384,8 +692,8 @@ describe("verifyConstraintsAccess", () => { vi.mocked(mockKV.put).mock.calls[0][1] as string, ); expect(parsed).toMatchObject({ + scope: "org", regionUrl: "https://us.sentry.io", - projectCapabilities: null, cachedAt: expect.any(Number), }); }); diff --git a/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts b/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts index f45eedf1..9869d131 100644 --- a/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts +++ b/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts @@ -1,23 +1,57 @@ import type { Constraints, ProjectCapabilities } from "@sentry/mcp-core/types"; import { SentryApiService, ApiError } from "@sentry/mcp-core/api-client"; import { logIssue, logWarn } from "@sentry/mcp-core/telem/logging"; +import { z } from "zod"; // Timeout for fetching project capabilities (5 seconds) const CAPABILITIES_TIMEOUT_MS = 5000; // Cache configuration const CACHE_TTL_SECONDS = 900; // 15 minutes -const CACHE_KEY_VERSION = "v1"; +const PROJECT_TIMEOUT_CACHE_TTL_SECONDS = 60; // Avoid repeated startup stalls. +const CACHE_KEY_VERSION = "v2"; + +const ProjectCapabilitiesSchema = z.object({ + profiles: z.boolean().optional(), + replays: z.boolean().optional(), + logs: z.boolean().optional(), + traces: z.boolean().optional(), +}); + +const CachedOrgConstraintsSchema = z.object({ + scope: z.literal("org"), + regionUrl: z.string().nullable(), + cachedAt: z.number(), +}); + +const CachedProjectConstraintsSchema = z.discriminatedUnion("status", [ + z.object({ + scope: z.literal("project"), + status: z.literal("verified"), + regionUrl: z.string().nullable(), + projectCapabilities: ProjectCapabilitiesSchema, + cachedAt: z.number(), + }), + z.object({ + scope: z.literal("project"), + status: z.literal("timeout"), + regionUrl: z.string().nullable(), + cachedAt: z.number(), + }), +]); + +const CachedConstraintsSchema = z.union([ + CachedOrgConstraintsSchema, + CachedProjectConstraintsSchema, +]); + +type CachedOrgConstraints = z.infer; +type CachedProjectConstraints = z.infer; /** * Cached constraints data stored in KV. */ -export type CachedConstraints = { - regionUrl: string | null; - /** Populated when a project constraint was verified; null for org-only cache entries. */ - projectCapabilities: ProjectCapabilities | null; - cachedAt: number; -}; +export type CachedConstraints = CachedOrgConstraints | CachedProjectConstraints; /** * Options for caching constraints verification results. @@ -25,11 +59,12 @@ export type CachedConstraints = { export type CacheOptions = { kv: KVNamespace; userId: string; + waitUntil: (promise: Promise) => void; }; /** * Build a cache key for constraints verification. - * Format: caps:v1:{userId}:{sentryHost}:{organizationSlug}:{scopeKey} + * Format: caps:v2:{userId}:{sentryHost}:{organizationSlug}:{scopeKey} * scopeKey is "org" for org-only entries or "project:{slug}" for project-scoped entries. */ function buildCacheKey( @@ -38,10 +73,7 @@ function buildCacheKey( organizationSlug: string, projectSlug: string | null | undefined, ): string { - const normalizedProjectSlug = projectSlug?.trim(); - const scopeKey = normalizedProjectSlug - ? `project:${normalizedProjectSlug}` - : "org"; + const scopeKey = projectSlug ? `project:${projectSlug}` : "org"; return `caps:${CACHE_KEY_VERSION}:${userId}:${sentryHost}:${organizationSlug}:${scopeKey}`; } @@ -55,7 +87,8 @@ async function getCachedConstraints( ): Promise { try { const cached = await kv.get(key, "json"); - return cached as CachedConstraints | null; + const parsed = CachedConstraintsSchema.safeParse(cached); + return parsed.success ? parsed.data : null; } catch (error) { logWarn("Failed to read constraints cache", { loggerScope: ["cloudflare", "constraint-utils"], @@ -65,18 +98,30 @@ async function getCachedConstraints( } } +type ConstraintCache = { + readOrg(): Promise; + readProject(): Promise; + writeOrg(regionUrl: string | null): void; + writeProjectVerified( + regionUrl: string | null, + projectCapabilities: ProjectCapabilities, + ): void; + writeProjectTimeout(regionUrl: string | null): void; +}; + /** * Store constraints in KV cache. - * Fire-and-forget - errors are logged but don't block the response. + * Scheduled with waitUntil so errors are logged without blocking the response. */ async function setCachedConstraints( kv: KVNamespace, key: string, data: CachedConstraints, + ttlSeconds = CACHE_TTL_SECONDS, ): Promise { try { await kv.put(key, JSON.stringify(data), { - expirationTtl: CACHE_TTL_SECONDS, + expirationTtl: ttlSeconds, }); } catch (error) { logWarn("Failed to write constraints cache", { @@ -86,6 +131,82 @@ async function setCachedConstraints( } } +function writeCachedConstraints( + cache: CacheOptions, + key: string, + data: CachedConstraints, + ttlSeconds = CACHE_TTL_SECONDS, +): void { + const writePromise = setCachedConstraints(cache.kv, key, data, ttlSeconds); + cache.waitUntil(writePromise); +} + +function createConstraintCache( + cache: CacheOptions, + sentryHost: string, + organizationSlug: string, + projectSlug: string | null | undefined, +): ConstraintCache { + const orgKey = buildCacheKey( + cache.userId, + sentryHost, + organizationSlug, + null, + ); + const projectKey = projectSlug + ? buildCacheKey(cache.userId, sentryHost, organizationSlug, projectSlug) + : null; + + return { + async readOrg() { + const cached = await getCachedConstraints(cache.kv, orgKey); + return cached?.scope === "org" ? cached : null; + }, + async readProject() { + if (!projectKey) { + return null; + } + const cached = await getCachedConstraints(cache.kv, projectKey); + return cached?.scope === "project" ? cached : null; + }, + writeOrg(regionUrl) { + writeCachedConstraints(cache, orgKey, { + scope: "org", + regionUrl, + cachedAt: Date.now(), + }); + }, + writeProjectVerified(regionUrl, projectCapabilities) { + if (!projectKey) { + return; + } + writeCachedConstraints(cache, projectKey, { + scope: "project", + status: "verified", + regionUrl, + projectCapabilities, + cachedAt: Date.now(), + }); + }, + writeProjectTimeout(regionUrl) { + if (!projectKey) { + return; + } + writeCachedConstraints( + cache, + projectKey, + { + scope: "project", + status: "timeout", + regionUrl, + cachedAt: Date.now(), + }, + PROJECT_TIMEOUT_CACHE_TTL_SECONDS, + ); + }, + }; +} + /** * Helper to race a promise against a timeout with proper cleanup. * Returns the result or throws a timeout error. @@ -114,8 +235,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. + * If cache options are provided, results will be cached in KV to avoid + * repeated API calls during MCP sessions. */ export async function verifyConstraintsAccess( { organizationSlug, projectSlug }: Constraints, @@ -157,51 +278,73 @@ export async function verifyConstraintsAccess( // Check KV cache when available (org-only and org+project keys). // Cache key includes userId to ensure per-user isolation. - let cacheKey: string | null = null; - if (cache) { - cacheKey = buildCacheKey( - cache.userId, - sentryHost, - organizationSlug, - projectSlug, - ); - const cached = await getCachedConstraints(cache.kv, cacheKey); - if (cached) { - return { - ok: true, - constraints: { - organizationSlug, - projectSlug: projectSlug || null, - regionUrl: cached.regionUrl, - projectCapabilities: cached.projectCapabilities, - }, - }; + const constraintCache = cache + ? createConstraintCache(cache, sentryHost, organizationSlug, projectSlug) + : null; + if (constraintCache) { + if (projectSlug) { + const cachedProject = await constraintCache.readProject(); + if (cachedProject) { + return { + ok: true, + constraints: { + organizationSlug, + projectSlug, + regionUrl: cachedProject.regionUrl, + projectCapabilities: + cachedProject.status === "verified" + ? cachedProject.projectCapabilities + : null, + }, + }; + } + } else { + const cachedOrg = await constraintCache.readOrg(); + if (cachedOrg) { + return { + ok: true, + constraints: { + organizationSlug, + projectSlug: null, + regionUrl: cachedOrg.regionUrl, + projectCapabilities: null, + }, + }; + } } } + const cachedOrgConstraints = + constraintCache && projectSlug ? await constraintCache.readOrg() : null; + // Use shared API client for consistent behavior and error handling const api = new SentryApiService({ accessToken, host: sentryHost }); // Verify organization using API client - let regionUrl: string | null | undefined = null; - try { - const org = await api.getOrganization(organizationSlug); - regionUrl = org.links?.regionUrl || null; - } catch (error) { - if (error instanceof ApiError) { - const message = - error.status === 404 - ? `Organization '${organizationSlug}' not found` - : error.message; - return { ok: false, status: error.status, message }; + let regionUrl: string | null = null; + if (cachedOrgConstraints) { + regionUrl = cachedOrgConstraints.regionUrl; + } else { + try { + const org = await api.getOrganization(organizationSlug); + regionUrl = org.links?.regionUrl || null; + constraintCache?.writeOrg(regionUrl); + } catch (error) { + if (error instanceof ApiError) { + const message = + error.status === 404 + ? `Organization '${organizationSlug}' not found` + : error.message; + return { ok: false, status: error.status, message }; + } + const eventId = logIssue(error); + return { + ok: false, + status: 502, + message: "Failed to verify organization", + eventId, + }; } - const eventId = logIssue(error); - return { - ok: false, - status: 502, - message: "Failed to verify organization", - eventId, - }; } // Verify project access if specified @@ -228,6 +371,7 @@ export async function verifyConstraintsAccess( logs: project.hasLogs === true, traces: project.firstTransactionEvent === true, }; + constraintCache?.writeProjectVerified(regionUrl, projectCapabilities); } catch (error) { // Check if this was a timeout if (error instanceof Error && error.message === "CAPABILITY_TIMEOUT") { @@ -240,6 +384,7 @@ export async function verifyConstraintsAccess( `Project verification timed out after ${CAPABILITIES_TIMEOUT_MS}ms for ${projectSlug}`, ), ); + constraintCache?.writeProjectTimeout(regionUrl); } else if (error instanceof ApiError) { // API errors (404, 401, 403, etc.) should fail verification const message = @@ -259,22 +404,12 @@ 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(), - }); - } - return { ok: true, constraints: { organizationSlug, projectSlug: projectSlug || null, - regionUrl: regionUrl || null, + regionUrl, projectCapabilities, }, }; 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 7895f13a..78abca15 100644 --- a/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts +++ b/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts @@ -75,6 +75,16 @@ async function parseSSEResponse(response: Response): Promise { return JSON.parse(dataLine.slice(6)) as T; } +function createMockKV(): KVNamespace { + return { + get: vi.fn().mockResolvedValue(null), + put: vi.fn().mockResolvedValue(undefined), + delete: vi.fn(), + list: vi.fn(), + getWithMetadata: vi.fn(), + } as unknown as KVNamespace; +} + function createTestEnv(): Env { return { COOKIE_SECRET: "test-cookie-secret-32-characters", @@ -83,6 +93,7 @@ function createTestEnv(): Env { SENTRY_HOST: "sentry.io", OPENAI_API_KEY: "test-openai-key", OAUTH_KV: {} as KVNamespace, + MCP_CACHE: createMockKV(), OAUTH_PROVIDER: { listUserGrants: vi.fn().mockResolvedValue({ items: [] }), revokeGrant: vi.fn().mockResolvedValue(undefined), @@ -280,6 +291,26 @@ describe("MCP Handler", () => { expect(response.status).toBe(404); }); + it("should reject invalid organization and project slugs", async () => { + const testCases = [ + { path: "/mcp/%20", message: "Invalid organization slug" }, + { + path: "/mcp/sentry-mcp-evals/%20", + message: "Invalid project slug", + }, + ]; + + for (const { path, message } of testCases) { + const request = createMcpRequest("initialize", {}, { path }); + const ctx = createMcpContext(); + + const response = await mcpHandler.fetch!(request, createTestEnv(), ctx); + + expect(response.status).toBe(400); + expect(await response.text()).toBe(message); + } + }); + it("should handle /mcp/:org with valid organization", async () => { const request = createMcpRequest( "initialize", @@ -301,6 +332,45 @@ describe("MCP Handler", () => { expect(body.result?.protocolVersion).toBeDefined(); }); + it("should handle /mcp/:org/:project with valid project", async () => { + const request = createMcpRequest( + "initialize", + { + protocolVersion: "2024-11-05", + capabilities: {}, + clientInfo: { name: "test-client", version: "1.0.0" }, + }, + { path: "/mcp/sentry-mcp-evals/cloudflare-mcp" }, + ); + const ctx = createMcpContext(); + + const response = await mcpHandler.fetch!(request, createTestEnv(), ctx); + + expect(response.status).toBe(200); + const body = await parseSSEResponse<{ + result?: { protocolVersion: string }; + }>(response); + expect(body.result?.protocolVersion).toBeDefined(); + }); + + it("should return 404 for non-existent project", async () => { + const request = createMcpRequest( + "initialize", + { + protocolVersion: "2024-11-05", + capabilities: {}, + clientInfo: { name: "test-client", version: "1.0.0" }, + }, + { path: "/mcp/sentry-mcp-evals/nonexistent-project" }, + ); + const ctx = createMcpContext(); + + const response = await mcpHandler.fetch!(request, createTestEnv(), ctx); + + expect(response.status).toBe(404); + expect(await response.text()).toContain("Project"); + }); + it("should return 404 for non-existent organization", 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 1827186b..a1407487 100644 --- a/packages/mcp-cloudflare/src/server/lib/mcp-handler.ts +++ b/packages/mcp-cloudflare/src/server/lib/mcp-handler.ts @@ -26,6 +26,7 @@ import { import { setSentryUserFromRequest } from "../utils/sentry-user"; import { resolveClientFamily } from "./client-family"; import { verifyConstraintsAccess } from "./constraint-utils"; +import { isValidSlug } from "./slug-validation"; /** * ExecutionContext with OAuth props injected by the OAuth provider. @@ -138,6 +139,12 @@ const mcpHandler: ExportedHandler = { const { groups } = result.pathname; const organizationSlug = groups?.org || null; const projectSlug = groups?.project || null; + if (organizationSlug && !isValidSlug(organizationSlug)) { + return new Response("Invalid organization slug", { status: 400 }); + } + if (projectSlug && !isValidSlug(projectSlug)) { + return new Response("Invalid project slug", { status: 400 }); + } // Check for agent mode query parameter const isAgentMode = url.searchParams.get("agent") === "1"; @@ -300,8 +307,7 @@ const mcpHandler: ExportedHandler = { } } - // Verify user has access to the requested org/project - // Cache verification results in KV to avoid repeated API calls + // Verify URL constraints and cache results in KV to avoid repeated API calls. const verification = await verifyConstraintsAccess( { organizationSlug, projectSlug }, { @@ -310,6 +316,7 @@ const mcpHandler: ExportedHandler = { cache: { kv: env.MCP_CACHE, userId, + waitUntil: (promise) => ctx.waitUntil(promise), }, }, );