From 57616262e971df13e5a70add693de787f159c1a4 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Sat, 30 May 2026 13:49:49 -0700 Subject: [PATCH 1/2] fix(cloudflare): Make constraint cache states explicit Cache project verification timeouts with explicit timeout entries. Keep fail-open behavior while giving org, verified project, and timeout entries separate shapes. Co-Authored-By: GPT-5 Codex --- .../src/server/lib/constraint-utils.test.ts | 363 +++++++++++++++--- .../src/server/lib/constraint-utils.ts | 286 +++++++++++--- .../src/server/lib/mcp-handler.test.ts | 50 +++ .../src/server/lib/mcp-handler.ts | 4 +- 4 files changed, 583 insertions(+), 120 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 c2a26b0bd..bb9773dc0 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, @@ -11,12 +12,19 @@ import { */ function createMockKV(options?: { getResult?: CachedConstraints | null; + 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", + ); + + 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), + }), + }), + ]), + ); + }); - // Wait for fire-and-forget cache write - await new Promise((resolve) => setTimeout(resolve, 10)); + it("schedules cache writes with waitUntil", async () => { + const mockKV = createMockKV({ getResult: null }); + const cache = createCache(mockKV, "user-wait-until"); + + const result = await verifyConstraintsAccess( + { organizationSlug: "sentry-mcp-evals", projectSlug: null }, + { accessToken: token, sentryHost: host, cache }, + ); - // Verify cache was written with correct key and TTL + 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,50 @@ 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 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("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 +618,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 +629,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 f45eedf17..247899b6b 100644 --- a/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts +++ b/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts @@ -7,29 +7,50 @@ 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"; /** - * Cached constraints data stored in KV. + * Cached org constraints data stored in KV. */ -export type CachedConstraints = { +type CachedOrgConstraints = { + scope: "org"; regionUrl: string | null; - /** Populated when a project constraint was verified; null for org-only cache entries. */ - projectCapabilities: ProjectCapabilities | null; cachedAt: number; }; +type CachedProjectConstraints = + | { + scope: "project"; + status: "verified"; + regionUrl: string | null; + projectCapabilities: ProjectCapabilities; + cachedAt: number; + } + | { + scope: "project"; + status: "timeout"; + regionUrl: string | null; + cachedAt: number; + }; + +/** + * Cached constraints data stored in KV. + */ +export type CachedConstraints = CachedOrgConstraints | CachedProjectConstraints; + /** * Options for caching constraints verification results. */ 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( @@ -45,6 +66,51 @@ function buildCacheKey( return `caps:${CACHE_KEY_VERSION}:${userId}:${sentryHost}:${organizationSlug}:${scopeKey}`; } +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null; +} + +function isNullableString(value: unknown): value is string | null { + return typeof value === "string" || value === null; +} + +function isProjectCapabilities(value: unknown): value is ProjectCapabilities { + return ( + isRecord(value) && + (value.profiles === undefined || typeof value.profiles === "boolean") && + (value.replays === undefined || typeof value.replays === "boolean") && + (value.logs === undefined || typeof value.logs === "boolean") && + (value.traces === undefined || typeof value.traces === "boolean") + ); +} + +function isCachedConstraints(value: unknown): value is CachedConstraints { + if ( + !isRecord(value) || + !isNullableString(value.regionUrl) || + typeof value.cachedAt !== "number" + ) { + return false; + } + + if (value.scope === "org") { + return true; + } + + if (value.scope !== "project") { + return false; + } + + if (value.status === "timeout") { + return true; + } + + return ( + value.status === "verified" && + isProjectCapabilities(value.projectCapabilities) + ); +} + /** * Attempt to retrieve cached constraints from KV. * Returns null on cache miss or any error (fail-open). @@ -55,7 +121,7 @@ async function getCachedConstraints( ): Promise { try { const cached = await kv.get(key, "json"); - return cached as CachedConstraints | null; + return isCachedConstraints(cached) ? cached : null; } catch (error) { logWarn("Failed to read constraints cache", { loggerScope: ["cloudflare", "constraint-utils"], @@ -65,18 +131,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 +164,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 +268,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 +311,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 +404,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 +417,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 +437,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 7895f13a8..85157be4a 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), @@ -301,6 +312,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 1827186ba..a405c7c27 100644 --- a/packages/mcp-cloudflare/src/server/lib/mcp-handler.ts +++ b/packages/mcp-cloudflare/src/server/lib/mcp-handler.ts @@ -300,8 +300,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 +309,7 @@ const mcpHandler: ExportedHandler = { cache: { kv: env.MCP_CACHE, userId, + waitUntil: (promise) => ctx.waitUntil(promise), }, }, ); From ecefad84311b53ec952517cbdccbbab1e25420b4 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Sat, 30 May 2026 16:44:09 -0700 Subject: [PATCH 2/2] fix(cloudflare): Harden constraint cache boundaries Validate URL constraint slugs before they reach verification or cache key construction. Replace the hand-rolled KV shape guards with a Zod schema so cached org, verified project, and timeout entries share one runtime contract. Keep project cache keys literal instead of trimming inside the key builder, which avoids collapsing malformed project scopes into the org cache namespace. Co-Authored-By: GPT-5 Codex --- .../src/server/lib/constraint-utils.test.ts | 67 ++++++++++- .../src/server/lib/constraint-utils.ts | 113 +++++++----------- .../src/server/lib/mcp-handler.test.ts | 20 ++++ .../src/server/lib/mcp-handler.ts | 7 ++ 4 files changed, 132 insertions(+), 75 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 bb9773dc0..699bf32f7 100644 --- a/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts +++ b/packages/mcp-cloudflare/src/server/lib/constraint-utils.test.ts @@ -11,8 +11,8 @@ import { * Create a mock KVNamespace for testing cache behavior. */ function createMockKV(options?: { - getResult?: CachedConstraints | null; - getResultByKey?: Record; + getResult?: unknown; + getResultByKey?: Record; getError?: Error; putError?: Error; }): KVNamespace { @@ -576,6 +576,35 @@ describe("verifyConstraintsAccess", () => { ); }); + 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", @@ -607,6 +636,40 @@ describe("verifyConstraintsAccess", () => { } }); + 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 = createCache(mockKV, "user-org-cache-write"); diff --git a/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts b/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts index 247899b6b..9869d131e 100644 --- a/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts +++ b/packages/mcp-cloudflare/src/server/lib/constraint-utils.ts @@ -1,6 +1,7 @@ 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; @@ -10,29 +11,42 @@ const CACHE_TTL_SECONDS = 900; // 15 minutes const PROJECT_TIMEOUT_CACHE_TTL_SECONDS = 60; // Avoid repeated startup stalls. const CACHE_KEY_VERSION = "v2"; -/** - * Cached org constraints data stored in KV. - */ -type CachedOrgConstraints = { - scope: "org"; - regionUrl: string | null; - cachedAt: number; -}; - -type CachedProjectConstraints = - | { - scope: "project"; - status: "verified"; - regionUrl: string | null; - projectCapabilities: ProjectCapabilities; - cachedAt: number; - } - | { - scope: "project"; - status: "timeout"; - regionUrl: string | null; - cachedAt: number; - }; +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. @@ -59,58 +73,10 @@ 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}`; } -function isRecord(value: unknown): value is Record { - return typeof value === "object" && value !== null; -} - -function isNullableString(value: unknown): value is string | null { - return typeof value === "string" || value === null; -} - -function isProjectCapabilities(value: unknown): value is ProjectCapabilities { - return ( - isRecord(value) && - (value.profiles === undefined || typeof value.profiles === "boolean") && - (value.replays === undefined || typeof value.replays === "boolean") && - (value.logs === undefined || typeof value.logs === "boolean") && - (value.traces === undefined || typeof value.traces === "boolean") - ); -} - -function isCachedConstraints(value: unknown): value is CachedConstraints { - if ( - !isRecord(value) || - !isNullableString(value.regionUrl) || - typeof value.cachedAt !== "number" - ) { - return false; - } - - if (value.scope === "org") { - return true; - } - - if (value.scope !== "project") { - return false; - } - - if (value.status === "timeout") { - return true; - } - - return ( - value.status === "verified" && - isProjectCapabilities(value.projectCapabilities) - ); -} - /** * Attempt to retrieve cached constraints from KV. * Returns null on cache miss or any error (fail-open). @@ -121,7 +87,8 @@ async function getCachedConstraints( ): Promise { try { const cached = await kv.get(key, "json"); - return isCachedConstraints(cached) ? cached : null; + const parsed = CachedConstraintsSchema.safeParse(cached); + return parsed.success ? parsed.data : null; } catch (error) { logWarn("Failed to read constraints cache", { loggerScope: ["cloudflare", "constraint-utils"], 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 85157be4a..78abca154 100644 --- a/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts +++ b/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts @@ -291,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", diff --git a/packages/mcp-cloudflare/src/server/lib/mcp-handler.ts b/packages/mcp-cloudflare/src/server/lib/mcp-handler.ts index a405c7c27..a1407487d 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";