From c6862f6c2926d5018f8429d2f2ebee46f878695c Mon Sep 17 00:00:00 2001 From: geoffg-sentry <165922362+geoffg-sentry@users.noreply.github.com> Date: Thu, 30 Oct 2025 13:20:19 -0400 Subject: [PATCH 1/6] fix(oauth): RFC8707 resource param validation --- .../src/server/oauth/authorize.test.ts | 220 +++++++++++++++ .../src/server/oauth/callback.test.ts | 265 ++++++++++++++++++ .../src/server/oauth/helpers.test.ts | 257 ++++++++++++++++- .../src/server/oauth/helpers.ts | 61 ++++ .../src/server/oauth/routes/authorize.ts | 54 +++- .../src/server/oauth/routes/callback.ts | 28 +- 6 files changed, 879 insertions(+), 6 deletions(-) diff --git a/packages/mcp-cloudflare/src/server/oauth/authorize.test.ts b/packages/mcp-cloudflare/src/server/oauth/authorize.test.ts index e9dfe788..19dc7e07 100644 --- a/packages/mcp-cloudflare/src/server/oauth/authorize.test.ts +++ b/packages/mcp-cloudflare/src/server/oauth/authorize.test.ts @@ -196,4 +196,224 @@ describe("oauth authorize routes", () => { expect(text).toBe("Invalid request"); }); }); + + describe("Resource parameter validation (RFC 8707)", () => { + describe("GET /oauth/authorize", () => { + it("should allow request without resource parameter (backward compatibility)", async () => { + mockOAuthProvider.parseAuthRequest.mockResolvedValueOnce({ + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + }); + mockOAuthProvider.lookupClient.mockResolvedValueOnce({ + clientId: "test-client", + clientName: "Test Client", + redirectUris: ["https://example.com/callback"], + }); + + const request = new Request("http://localhost/oauth/authorize", { + method: "GET", + }); + const response = await app.fetch(request, testEnv as Env); + + // Should proceed normally (render approval dialog) + expect(response.status).toBe(200); + const html = await response.text(); + expect(html).toContain(" { + mockOAuthProvider.parseAuthRequest.mockResolvedValueOnce({ + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + resource: "http://localhost/mcp", + }); + mockOAuthProvider.lookupClient.mockResolvedValueOnce({ + clientId: "test-client", + clientName: "Test Client", + redirectUris: ["https://example.com/callback"], + }); + + const request = new Request( + "http://localhost/oauth/authorize?resource=http://localhost/mcp", + { method: "GET" }, + ); + const response = await app.fetch(request, testEnv as Env); + + // Should proceed normally + expect(response.status).toBe(200); + }); + + it("should reject request with invalid resource hostname", async () => { + mockOAuthProvider.parseAuthRequest.mockResolvedValueOnce({ + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + state: "test-state", + }); + + const request = new Request( + "http://localhost/oauth/authorize?resource=https://attacker.com/mcp&redirect_uri=https://example.com/callback&state=test-state", + { method: "GET" }, + ); + const response = await app.fetch(request, testEnv as Env); + + // Should redirect with error + expect(response.status).toBe(302); + const location = response.headers.get("location"); + expect(location).toBeTruthy(); + + const locationUrl = new URL(location!); + expect(locationUrl.origin).toBe("https://example.com"); + expect(locationUrl.searchParams.get("error")).toBe("invalid_target"); + expect(locationUrl.searchParams.get("error_description")).toContain( + "resource parameter", + ); + expect(locationUrl.searchParams.get("state")).toBe("test-state"); + }); + + it("should reject request with invalid resource path", async () => { + mockOAuthProvider.parseAuthRequest.mockResolvedValueOnce({ + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + }); + + const request = new Request( + "http://localhost/oauth/authorize?resource=http://localhost/api&redirect_uri=https://example.com/callback", + { method: "GET" }, + ); + const response = await app.fetch(request, testEnv as Env); + + // Should redirect with error + expect(response.status).toBe(302); + const location = response.headers.get("location"); + const locationUrl = new URL(location!); + expect(locationUrl.searchParams.get("error")).toBe("invalid_target"); + }); + + it("should return 400 if invalid resource but no redirect_uri", async () => { + mockOAuthProvider.parseAuthRequest.mockResolvedValueOnce({ + clientId: "test-client", + scope: ["read"], + }); + + const request = new Request( + "http://localhost/oauth/authorize?resource=https://attacker.com/mcp", + { method: "GET" }, + ); + const response = await app.fetch(request, testEnv as Env); + + // Should return direct error + expect(response.status).toBe(400); + const text = await response.text(); + expect(text).toContain("Invalid resource parameter"); + }); + }); + + describe("POST /oauth/authorize", () => { + beforeEach(() => { + mockOAuthProvider.lookupClient.mockResolvedValue({ + clientId: "test-client", + clientName: "Test Client", + redirectUris: ["https://example.com/callback"], + }); + }); + + it("should allow request without resource parameter", async () => { + const oauthReqInfo = { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + }; + const formData = new FormData(); + formData.append("state", btoa(JSON.stringify({ oauthReqInfo }))); + + const request = new Request("http://localhost/oauth/authorize", { + method: "POST", + body: formData, + }); + const response = await app.fetch(request, testEnv as Env); + + // Should proceed normally (redirect to Sentry) + expect(response.status).toBe(302); + const location = response.headers.get("location"); + expect(location).toContain("sentry.io"); + }); + + it("should allow request with valid resource parameter", async () => { + const oauthReqInfo = { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + resource: "http://localhost/mcp", + }; + const formData = new FormData(); + formData.append("state", btoa(JSON.stringify({ oauthReqInfo }))); + + const request = new Request("http://localhost/oauth/authorize", { + method: "POST", + body: formData, + }); + const response = await app.fetch(request, testEnv as Env); + + // Should proceed normally + expect(response.status).toBe(302); + const location = response.headers.get("location"); + expect(location).toContain("sentry.io"); + }); + + it("should reject request with invalid resource hostname", async () => { + const oauthReqInfo = { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + resource: "https://attacker.com/mcp", + state: "test-state", + }; + const formData = new FormData(); + formData.append("state", btoa(JSON.stringify({ oauthReqInfo }))); + + const request = new Request("http://localhost/oauth/authorize", { + method: "POST", + body: formData, + }); + const response = await app.fetch(request, testEnv as Env); + + // Should redirect with error + expect(response.status).toBe(302); + const location = response.headers.get("location"); + expect(location).toBeTruthy(); + + const locationUrl = new URL(location!); + expect(locationUrl.origin).toBe("https://example.com"); + expect(locationUrl.searchParams.get("error")).toBe("invalid_target"); + expect(locationUrl.searchParams.get("state")).toBe("test-state"); + }); + + it("should reject request with invalid resource path", async () => { + const oauthReqInfo = { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + resource: "http://localhost/oauth", + }; + const formData = new FormData(); + formData.append("state", btoa(JSON.stringify({ oauthReqInfo }))); + + const request = new Request("http://localhost/oauth/authorize", { + method: "POST", + body: formData, + }); + const response = await app.fetch(request, testEnv as Env); + + // Should redirect with error + expect(response.status).toBe(302); + const location = response.headers.get("location"); + const locationUrl = new URL(location!); + expect(locationUrl.searchParams.get("error")).toBe("invalid_target"); + }); + }); + }); }); diff --git a/packages/mcp-cloudflare/src/server/oauth/callback.test.ts b/packages/mcp-cloudflare/src/server/oauth/callback.test.ts index ab38a513..370cc49a 100644 --- a/packages/mcp-cloudflare/src/server/oauth/callback.test.ts +++ b/packages/mcp-cloudflare/src/server/oauth/callback.test.ts @@ -216,4 +216,269 @@ describe("oauth callback routes", () => { expect(text).toBe("Invalid state"); }); }); + + describe("Resource parameter validation (RFC 8707)", () => { + it("should allow callback without resource parameter", async () => { + mockOAuthProvider.lookupClient.mockResolvedValue({ + clientId: "test-client", + clientName: "Test Client", + redirectUris: ["https://example.com/callback"], + }); + + // Submit approval to get approval cookie + const approvalFormData = new FormData(); + approvalFormData.append( + "state", + btoa( + JSON.stringify({ + oauthReqInfo: { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + }, + }), + ), + ); + const approvalRequest = new Request("http://localhost/oauth/authorize", { + method: "POST", + body: approvalFormData, + }); + const approvalResponse = await app.fetch(approvalRequest, testEnv as Env); + const setCookie = approvalResponse.headers.get("Set-Cookie"); + + // Build signed state without resource + const now = Date.now(); + const payload: OAuthState = { + req: { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + }, + iat: now, + exp: now + 10 * 60 * 1000, + } as unknown as OAuthState; + const signedState = await signState(payload, testEnv.COOKIE_SECRET!); + + // Should NOT call exchangeCodeForAccessToken in this test (we're just checking validation passes) + // but if it were called, mock it to avoid actual network requests + const request = new Request( + `http://localhost/oauth/callback?code=test-code&state=${signedState}`, + { + method: "GET", + headers: { + Cookie: setCookie!.split(";")[0], + }, + }, + ); + + // This will fail at token exchange since we didn't mock it, but that's after validation + // We just need to ensure it doesn't fail with "Invalid resource parameter" + const response = await app.fetch(request, testEnv as Env); + + // Should not be 400 with "Invalid resource parameter" + // It might be 500 or another error from token exchange, but that's OK + if (response.status === 400) { + const text = await response.text(); + expect(text).not.toContain("Invalid resource parameter"); + } + }); + + it("should allow callback with valid resource parameter", async () => { + mockOAuthProvider.lookupClient.mockResolvedValue({ + clientId: "test-client", + clientName: "Test Client", + redirectUris: ["https://example.com/callback"], + }); + + // Submit approval to get approval cookie + const approvalFormData = new FormData(); + approvalFormData.append( + "state", + btoa( + JSON.stringify({ + oauthReqInfo: { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + resource: "http://localhost/mcp", + }, + }), + ), + ); + const approvalRequest = new Request("http://localhost/oauth/authorize", { + method: "POST", + body: approvalFormData, + }); + const approvalResponse = await app.fetch(approvalRequest, testEnv as Env); + const setCookie = approvalResponse.headers.get("Set-Cookie"); + + // Build signed state with valid resource + const now = Date.now(); + const payload: OAuthState = { + req: { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + resource: "http://localhost/mcp", + }, + iat: now, + exp: now + 10 * 60 * 1000, + } as unknown as OAuthState; + const signedState = await signState(payload, testEnv.COOKIE_SECRET!); + + const request = new Request( + `http://localhost/oauth/callback?code=test-code&state=${signedState}`, + { + method: "GET", + headers: { + Cookie: setCookie!.split(";")[0], + }, + }, + ); + + const response = await app.fetch(request, testEnv as Env); + + // Should not be rejected for resource validation + if (response.status === 400) { + const text = await response.text(); + expect(text).not.toContain("Invalid resource parameter"); + } + }); + + it("should reject callback with invalid resource hostname", async () => { + mockOAuthProvider.lookupClient.mockResolvedValue({ + clientId: "test-client", + clientName: "Test Client", + redirectUris: ["https://example.com/callback"], + }); + + // Build signed state with invalid resource + // Note: We don't go through approval because it would reject the invalid resource + const now = Date.now(); + const payload: OAuthState = { + req: { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + resource: "https://attacker.com/mcp", + }, + iat: now, + exp: now + 10 * 60 * 1000, + } as unknown as OAuthState; + const signedState = await signState(payload, testEnv.COOKIE_SECRET!); + + // First approve a valid client to get the cookie + const validApprovalFormData = new FormData(); + validApprovalFormData.append( + "state", + btoa( + JSON.stringify({ + oauthReqInfo: { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + // No resource parameter for the approval + }, + }), + ), + ); + const validApprovalRequest = new Request( + "http://localhost/oauth/authorize", + { + method: "POST", + body: validApprovalFormData, + }, + ); + const validApprovalResponse = await app.fetch( + validApprovalRequest, + testEnv as Env, + ); + const setCookie = validApprovalResponse.headers.get("Set-Cookie"); + + const request = new Request( + `http://localhost/oauth/callback?code=test-code&state=${signedState}`, + { + method: "GET", + headers: { + Cookie: setCookie!.split(";")[0], + }, + }, + ); + + const response = await app.fetch(request, testEnv as Env); + + // Should be rejected with 400 error + expect(response.status).toBe(400); + const text = await response.text(); + expect(text).toContain("Invalid resource parameter"); + }); + + it("should reject callback with invalid resource path", async () => { + mockOAuthProvider.lookupClient.mockResolvedValue({ + clientId: "test-client", + clientName: "Test Client", + redirectUris: ["https://example.com/callback"], + }); + + // Build signed state with invalid resource path + // Note: We don't go through approval because it would reject the invalid resource + const now = Date.now(); + const payload: OAuthState = { + req: { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + resource: "http://localhost/api", + }, + iat: now, + exp: now + 10 * 60 * 1000, + } as unknown as OAuthState; + const signedState = await signState(payload, testEnv.COOKIE_SECRET!); + + // First approve a valid client to get the cookie + const validApprovalFormData = new FormData(); + validApprovalFormData.append( + "state", + btoa( + JSON.stringify({ + oauthReqInfo: { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + // No resource parameter for the approval + }, + }), + ), + ); + const validApprovalRequest = new Request( + "http://localhost/oauth/authorize", + { + method: "POST", + body: validApprovalFormData, + }, + ); + const validApprovalResponse = await app.fetch( + validApprovalRequest, + testEnv as Env, + ); + const setCookie = validApprovalResponse.headers.get("Set-Cookie"); + + const request = new Request( + `http://localhost/oauth/callback?code=test-code&state=${signedState}`, + { + method: "GET", + headers: { + Cookie: setCookie!.split(";")[0], + }, + }, + ); + + const response = await app.fetch(request, testEnv as Env); + + // Should be rejected with 400 error + expect(response.status).toBe(400); + const text = await response.text(); + expect(text).toContain("Invalid resource parameter"); + }); + }); }); diff --git a/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts b/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts index 605ff7a9..327f8170 100644 --- a/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts +++ b/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts @@ -1,6 +1,11 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import type { TokenExchangeCallbackOptions } from "@cloudflare/workers-oauth-provider"; -import { tokenExchangeCallback, refreshAccessToken } from "./helpers"; +import { + tokenExchangeCallback, + refreshAccessToken, + validateResourceParameter, + createResourceValidationError, +} from "./helpers"; import type { WorkerProps } from "../types"; // Mock fetch globally @@ -288,3 +293,253 @@ describe("refreshAccessToken", () => { expect(text).toContain("issue refreshing your access token"); }); }); + +describe("validateResourceParameter", () => { + describe("valid resources", () => { + it("should allow undefined resource (optional parameter)", () => { + const result = validateResourceParameter( + undefined, + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(true); + }); + + it("should allow same hostname with /mcp path", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev/mcp", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(true); + }); + + it("should allow same hostname with nested /mcp path", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev/mcp/org/project", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(true); + }); + + it("should allow localhost with /mcp path", () => { + const result = validateResourceParameter( + "http://localhost:8787/mcp", + "http://localhost:8787/oauth/authorize", + ); + expect(result).toBe(true); + }); + + it("should allow resource with query parameters", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev/mcp?foo=bar", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(true); + }); + + it("should allow resource with different port when both match", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev:8443/mcp", + "https://mcp.sentry.dev:8443/oauth/authorize", + ); + expect(result).toBe(true); + }); + + it("should allow 127.0.0.1 with /mcp path", () => { + const result = validateResourceParameter( + "http://127.0.0.1:3000/mcp", + "http://127.0.0.1:3000/oauth/authorize", + ); + expect(result).toBe(true); + }); + }); + + describe("invalid resources", () => { + it("should reject different hostname", () => { + const result = validateResourceParameter( + "https://attacker.com/mcp", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(false); + }); + + it("should reject different subdomain", () => { + const result = validateResourceParameter( + "https://evil.sentry.dev/mcp", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(false); + }); + + it("should reject invalid path (not /mcp)", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev/api", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(false); + }); + + it("should reject path without /mcp prefix", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev/oauth", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(false); + }); + + it("should reject malformed URL", () => { + const result = validateResourceParameter( + "not-a-url", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(false); + }); + + it("should reject relative path", () => { + const result = validateResourceParameter( + "/mcp", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(false); + }); + + it("should reject empty string", () => { + const result = validateResourceParameter( + "", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(false); + }); + + it("should reject different port", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev:8080/mcp", + "https://mcp.sentry.dev:443/oauth/authorize", + ); + expect(result).toBe(false); + }); + + it("should reject javascript: scheme", () => { + const result = validateResourceParameter( + "javascript:alert(1)", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(false); + }); + + it("should reject data: scheme", () => { + const result = validateResourceParameter( + "data:text/html,", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(false); + }); + }); + + describe("edge cases", () => { + it("should handle URL with fragment", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev/mcp#fragment", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(true); + }); + + it("should handle URL with trailing slash", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev/mcp/", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(true); + }); + + it("should handle case sensitivity in hostname", () => { + // URLs are case-insensitive for hostname + const result = validateResourceParameter( + "https://MCP.SENTRY.DEV/mcp", + "https://mcp.sentry.dev/oauth/authorize", + ); + // This should work because URL constructor normalizes hostnames + expect(result).toBe(true); + }); + + it("should be case-sensitive for path", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev/MCP", + "https://mcp.sentry.dev/oauth/authorize", + ); + // Paths are case-sensitive + expect(result).toBe(false); + }); + + it("should handle URL encoding in path", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev/mcp%2Forg", + "https://mcp.sentry.dev/oauth/authorize", + ); + // URL encoding should be preserved + expect(result).toBe(true); + }); + }); +}); + +describe("createResourceValidationError", () => { + it("should create redirect response with invalid_target error", () => { + const response = createResourceValidationError( + "https://client.example.com/callback", + "state123", + ); + + expect(response.status).toBe(302); + + const location = response.headers.get("Location"); + expect(location).toBeDefined(); + + const locationUrl = new URL(location!); + expect(locationUrl.origin).toBe("https://client.example.com"); + expect(locationUrl.pathname).toBe("/callback"); + expect(locationUrl.searchParams.get("error")).toBe("invalid_target"); + expect(locationUrl.searchParams.get("error_description")).toContain( + "resource parameter", + ); + expect(locationUrl.searchParams.get("state")).toBe("state123"); + }); + + it("should create redirect without state parameter if not provided", () => { + const response = createResourceValidationError( + "https://client.example.com/callback", + ); + + const location = response.headers.get("Location"); + expect(location).toBeDefined(); + + const locationUrl = new URL(location!); + expect(locationUrl.searchParams.get("error")).toBe("invalid_target"); + expect(locationUrl.searchParams.get("state")).toBeNull(); + }); + + it("should preserve existing query parameters in redirect URI", () => { + const response = createResourceValidationError( + "https://client.example.com/callback?existing=param", + "state456", + ); + + const location = response.headers.get("Location"); + const locationUrl = new URL(location!); + + expect(locationUrl.searchParams.get("existing")).toBe("param"); + expect(locationUrl.searchParams.get("error")).toBe("invalid_target"); + expect(locationUrl.searchParams.get("state")).toBe("state456"); + }); + + it("should have proper error description per RFC 8707", () => { + const response = createResourceValidationError( + "https://client.example.com/callback", + ); + + const location = response.headers.get("Location"); + const locationUrl = new URL(location!); + + const errorDescription = locationUrl.searchParams.get("error_description"); + expect(errorDescription).toContain("authorization server"); + }); +}); diff --git a/packages/mcp-cloudflare/src/server/oauth/helpers.ts b/packages/mcp-cloudflare/src/server/oauth/helpers.ts index 732cd6ad..d3c2fe5e 100644 --- a/packages/mcp-cloudflare/src/server/oauth/helpers.ts +++ b/packages/mcp-cloudflare/src/server/oauth/helpers.ts @@ -306,3 +306,64 @@ export async function tokenExchangeCallback( }); } } + +/** + * Validates resource parameter per RFC 8707. + */ +export function validateResourceParameter( + resource: string | undefined, + requestUrl: string, +): boolean { + if (resource === "") { + return false; + } + + if (!resource) { + return true; + } + + try { + const resourceUrl = new URL(resource); + const requestUrlObj = new URL(requestUrl); + + if (resourceUrl.hostname !== requestUrlObj.hostname) { + return false; + } + + if (resourceUrl.port !== requestUrlObj.port) { + return false; + } + + const validPath = "/mcp"; + return resourceUrl.pathname.startsWith(validPath); + } catch { + return false; + } +} + +/** + * Creates RFC 8707 error response for invalid resource parameter. + */ +export function createResourceValidationError( + redirectUri: string, + state?: string, +): Response { + const redirectUrl = new URL(redirectUri); + + redirectUrl.searchParams.set("error", "invalid_target"); + redirectUrl.searchParams.set( + "error_description", + "The resource parameter does not match this authorization server", + ); + + if (state) { + redirectUrl.searchParams.set("state", state); + } + + return new Response(null, { + status: 302, + headers: { + Location: redirectUrl.href, + }, + }); +} diff --git a/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts b/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts index 370df892..0fbbb8a2 100644 --- a/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts +++ b/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts @@ -6,16 +6,21 @@ import { } from "../../lib/approval-dialog"; import type { Env } from "../../types"; import { SENTRY_AUTH_URL } from "../constants"; -import { getUpstreamAuthorizeUrl } from "../helpers"; +import { + getUpstreamAuthorizeUrl, + validateResourceParameter, + createResourceValidationError, +} from "../helpers"; import { SCOPES } from "../../../constants"; import { signState, type OAuthState } from "../state"; import { logWarn } from "@sentry/mcp-server/telem/logging"; /** - * Extended AuthRequest that includes permissions + * Extended AuthRequest that includes permissions and resource parameter */ interface AuthRequestWithPermissions extends AuthRequest { permissions?: unknown; + resource?: string; } async function redirectToUpstream( @@ -73,6 +78,30 @@ export default new Hono<{ Bindings: Env }>() return c.text("Invalid request", 400); } + // Validate resource parameter per RFC 8707 + const requestUrl = new URL(c.req.url); + const resourceParam = requestUrl.searchParams.get("resource"); + + if (resourceParam && !validateResourceParameter(resourceParam, c.req.url)) { + logWarn("Invalid resource parameter in authorization request", { + loggerScope: ["cloudflare", "oauth", "authorize"], + extra: { + resource: resourceParam, + requestUrl: c.req.url, + clientId, + }, + }); + + const redirectUri = requestUrl.searchParams.get("redirect_uri"); + const state = requestUrl.searchParams.get("state"); + + if (redirectUri) { + return createResourceValidationError(redirectUri, state ?? undefined); + } + + return c.text("Invalid resource parameter", 400); + } + // XXX(dcramer): we want to confirm permissions on each time // so you can always choose new ones // This shouldn't be highly visible to users, as clients should use refresh tokens @@ -130,6 +159,27 @@ export default new Hono<{ Bindings: Env }>() permissions, }; + // Validate resource parameter per RFC 8707 + const resourceFromState = oauthReqWithPermissions.resource; + + if ( + resourceFromState && + !validateResourceParameter(resourceFromState, c.req.url) + ) { + logWarn("Invalid resource parameter in authorization approval", { + loggerScope: ["cloudflare", "oauth", "authorize"], + extra: { + resource: resourceFromState, + clientId: oauthReqWithPermissions.clientId, + }, + }); + + return createResourceValidationError( + oauthReqWithPermissions.redirectUri, + oauthReqWithPermissions.state, + ); + } + // Validate redirectUri is registered for this client before proceeding try { const client = await c.env.OAUTH_PROVIDER.lookupClient( diff --git a/packages/mcp-cloudflare/src/server/oauth/routes/callback.ts b/packages/mcp-cloudflare/src/server/oauth/routes/callback.ts index cb012de1..5f332ee6 100644 --- a/packages/mcp-cloudflare/src/server/oauth/routes/callback.ts +++ b/packages/mcp-cloudflare/src/server/oauth/routes/callback.ts @@ -5,15 +5,19 @@ import type { Env, WorkerProps } from "../../types"; import type { Scope } from "@sentry/mcp-server/permissions"; import { DEFAULT_SCOPES } from "@sentry/mcp-server/constants"; import { SENTRY_TOKEN_URL } from "../constants"; -import { exchangeCodeForAccessToken } from "../helpers"; +import { + exchangeCodeForAccessToken, + validateResourceParameter, +} from "../helpers"; import { verifyAndParseState, type OAuthState } from "../state"; import { logWarn } from "@sentry/mcp-server/telem/logging"; /** - * Extended AuthRequest that includes permissions + * Extended AuthRequest that includes permissions and resource parameter */ interface AuthRequestWithPermissions extends AuthRequest { permissions?: unknown; + resource?: string; } /** @@ -110,6 +114,24 @@ export default new Hono<{ Bindings: Env }>().get("/", async (c) => { return c.text("Authorization failed: Invalid redirect URL", 400); } + // Validate resource parameter per RFC 8707 + const resourceFromState = oauthReqInfo.resource; + + if ( + resourceFromState && + !validateResourceParameter(resourceFromState, c.req.url) + ) { + logWarn("Invalid resource parameter in callback", { + loggerScope: ["cloudflare", "oauth", "callback"], + extra: { + resource: resourceFromState, + clientId: oauthReqInfo.clientId, + }, + }); + + return c.text("Authorization failed: Invalid resource parameter", 400); + } + // because we share a clientId with the upstream provider, we need to ensure that the // downstream client has been approved by the end-user (e.g. for a new client) // https://github.com/modelcontextprotocol/modelcontextprotocol/discussions/265 @@ -169,7 +191,7 @@ export default new Hono<{ Bindings: Env }>().get("/", async (c) => { // Return back to the MCP client a new token const { redirectTo } = await c.env.OAUTH_PROVIDER.completeAuthorization({ - request: oauthReqInfo, + request: oauthReqInfo as AuthRequest, userId: payload.user.id, metadata: { label: payload.user.name, From baa5fc8558a5f60672708776bd41c0e91c521a0c Mon Sep 17 00:00:00 2001 From: geoffg-sentry <165922362+geoffg-sentry@users.noreply.github.com> Date: Thu, 30 Oct 2025 15:35:03 -0400 Subject: [PATCH 2/6] fix fragment, port, protocol, add tests --- .../src/server/oauth/helpers.test.ts | 28 +++++++++++++++++-- .../src/server/oauth/helpers.ts | 16 ++++++++++- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts b/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts index 327f8170..c63c1d67 100644 --- a/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts +++ b/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts @@ -344,6 +344,22 @@ describe("validateResourceParameter", () => { expect(result).toBe(true); }); + it("should allow explicit default port 443 for https", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev:443/mcp", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(true); + }); + + it("should allow explicit default port 80 for http", () => { + const result = validateResourceParameter( + "http://localhost:80/mcp", + "http://localhost/oauth/authorize", + ); + expect(result).toBe(true); + }); + it("should allow 127.0.0.1 with /mcp path", () => { const result = validateResourceParameter( "http://127.0.0.1:3000/mcp", @@ -418,6 +434,14 @@ describe("validateResourceParameter", () => { expect(result).toBe(false); }); + it("should reject different protocol (http vs https)", () => { + const result = validateResourceParameter( + "http://mcp.sentry.dev/mcp", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(false); + }); + it("should reject javascript: scheme", () => { const result = validateResourceParameter( "javascript:alert(1)", @@ -436,12 +460,12 @@ describe("validateResourceParameter", () => { }); describe("edge cases", () => { - it("should handle URL with fragment", () => { + it("should reject URL with fragment (RFC 8707)", () => { const result = validateResourceParameter( "https://mcp.sentry.dev/mcp#fragment", "https://mcp.sentry.dev/oauth/authorize", ); - expect(result).toBe(true); + expect(result).toBe(false); }); it("should handle URL with trailing slash", () => { diff --git a/packages/mcp-cloudflare/src/server/oauth/helpers.ts b/packages/mcp-cloudflare/src/server/oauth/helpers.ts index d3c2fe5e..6569860c 100644 --- a/packages/mcp-cloudflare/src/server/oauth/helpers.ts +++ b/packages/mcp-cloudflare/src/server/oauth/helpers.ts @@ -326,11 +326,25 @@ export function validateResourceParameter( const resourceUrl = new URL(resource); const requestUrlObj = new URL(requestUrl); + // RFC 8707: resource URI must not include fragment + if (resourceUrl.hash) { + return false; + } + + // Must use same protocol + if (resourceUrl.protocol !== requestUrlObj.protocol) { + return false; + } + if (resourceUrl.hostname !== requestUrlObj.hostname) { return false; } - if (resourceUrl.port !== requestUrlObj.port) { + // Normalize default ports for comparison + const getPort = (url: URL) => + url.port || (url.protocol === "https:" ? "443" : "80"); + + if (getPort(resourceUrl) !== getPort(requestUrlObj)) { return false; } From afb233afdb4cead47c5eb848c7f780c268fc1e6f Mon Sep 17 00:00:00 2001 From: geoffg-sentry <165922362+geoffg-sentry@users.noreply.github.com> Date: Thu, 30 Oct 2025 16:15:51 -0400 Subject: [PATCH 3/6] preserve resource in state --- .../mcp-cloudflare/src/server/oauth/routes/authorize.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts b/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts index 0fbbb8a2..fef93789 100644 --- a/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts +++ b/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts @@ -102,6 +102,12 @@ export default new Hono<{ Bindings: Env }>() return c.text("Invalid resource parameter", 400); } + // Preserve resource in state (library's AuthRequest doesn't include it) + const oauthReqInfoWithResource: AuthRequestWithPermissions = { + ...oauthReqInfo, + ...(resourceParam ? { resource: resourceParam } : {}), + }; + // XXX(dcramer): we want to confirm permissions on each time // so you can always choose new ones // This shouldn't be highly visible to users, as clients should use refresh tokens @@ -124,7 +130,7 @@ export default new Hono<{ Bindings: Env }>() server: { name: "Sentry MCP", }, - state: { oauthReqInfo }, // arbitrary data that flows through the form submission below + state: { oauthReqInfo: oauthReqInfoWithResource }, }); }) From 49491646dfd0aba1cc04717119cbbe9056289458 Mon Sep 17 00:00:00 2001 From: geoffg-sentry <165922362+geoffg-sentry@users.noreply.github.com> Date: Thu, 30 Oct 2025 16:19:41 -0400 Subject: [PATCH 4/6] fix mcp path validation --- .../src/server/oauth/helpers.test.ts | 22 ++++++++++++++++--- .../src/server/oauth/helpers.ts | 7 ++++-- .../src/server/oauth/routes/authorize.ts | 2 +- 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts b/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts index c63c1d67..b78dd4c4 100644 --- a/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts +++ b/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts @@ -402,6 +402,22 @@ describe("validateResourceParameter", () => { expect(result).toBe(false); }); + it("should reject path with /mcp prefix but no separator", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev/mcpadmin", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(false); + }); + + it("should reject path with /mcp- prefix", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev/mcp-evil", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(false); + }); + it("should reject malformed URL", () => { const result = validateResourceParameter( "not-a-url", @@ -495,13 +511,13 @@ describe("validateResourceParameter", () => { expect(result).toBe(false); }); - it("should handle URL encoding in path", () => { + it("should reject URL-encoded slashes in path", () => { const result = validateResourceParameter( "https://mcp.sentry.dev/mcp%2Forg", "https://mcp.sentry.dev/oauth/authorize", ); - // URL encoding should be preserved - expect(result).toBe(true); + // Encoded slashes could bypass path validation + expect(result).toBe(false); }); }); }); diff --git a/packages/mcp-cloudflare/src/server/oauth/helpers.ts b/packages/mcp-cloudflare/src/server/oauth/helpers.ts index 6569860c..09653ea0 100644 --- a/packages/mcp-cloudflare/src/server/oauth/helpers.ts +++ b/packages/mcp-cloudflare/src/server/oauth/helpers.ts @@ -348,8 +348,11 @@ export function validateResourceParameter( return false; } - const validPath = "/mcp"; - return resourceUrl.pathname.startsWith(validPath); + // Validate path is exactly /mcp or starts with /mcp/ + return ( + resourceUrl.pathname === "/mcp" || + resourceUrl.pathname.startsWith("/mcp/") + ); } catch { return false; } diff --git a/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts b/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts index fef93789..a969c5ca 100644 --- a/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts +++ b/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts @@ -102,7 +102,7 @@ export default new Hono<{ Bindings: Env }>() return c.text("Invalid resource parameter", 400); } - // Preserve resource in state (library's AuthRequest doesn't include it) + // Preserve resource in state, not exported upstream const oauthReqInfoWithResource: AuthRequestWithPermissions = { ...oauthReqInfo, ...(resourceParam ? { resource: resourceParam } : {}), From e96ea0907c6edcb86ae119fb56d090b8fa21e41e Mon Sep 17 00:00:00 2001 From: geoffg-sentry <165922362+geoffg-sentry@users.noreply.github.com> Date: Sat, 1 Nov 2025 14:46:56 -0400 Subject: [PATCH 5/6] disallow url-encoding in path --- .../src/server/oauth/helpers.test.ts | 17 +++++++++++++++++ .../mcp-cloudflare/src/server/oauth/helpers.ts | 5 +++++ 2 files changed, 22 insertions(+) diff --git a/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts b/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts index b78dd4c4..bc494913 100644 --- a/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts +++ b/packages/mcp-cloudflare/src/server/oauth/helpers.test.ts @@ -519,6 +519,23 @@ describe("validateResourceParameter", () => { // Encoded slashes could bypass path validation expect(result).toBe(false); }); + + it("should reject any percent-encoded characters in path", () => { + const testCases = [ + "https://mcp.sentry.dev/mcp%2Forg", // encoded slash + "https://mcp.sentry.dev/mcp/%2e%2e", // encoded dots + "https://mcp.sentry.dev/mcp%20", // encoded space + "https://mcp.sentry.dev/mcp/test%00", // encoded null byte + ]; + + for (const testCase of testCases) { + const result = validateResourceParameter( + testCase, + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(false); + } + }); }); }); diff --git a/packages/mcp-cloudflare/src/server/oauth/helpers.ts b/packages/mcp-cloudflare/src/server/oauth/helpers.ts index 09653ea0..ab37c51b 100644 --- a/packages/mcp-cloudflare/src/server/oauth/helpers.ts +++ b/packages/mcp-cloudflare/src/server/oauth/helpers.ts @@ -348,6 +348,11 @@ export function validateResourceParameter( return false; } + // Reject url-encoded characters in pathname + if (resourceUrl.pathname.includes("%")) { + return false; + } + // Validate path is exactly /mcp or starts with /mcp/ return ( resourceUrl.pathname === "/mcp" || From 7b8f90103e8726355dcb80782357ff7ed4d61c38 Mon Sep 17 00:00:00 2001 From: geoffg-sentry <165922362+geoffg-sentry@users.noreply.github.com> Date: Sat, 1 Nov 2025 14:56:36 -0400 Subject: [PATCH 6/6] fix uri and resource validation ordering --- .../src/server/oauth/authorize.test.ts | 23 ++++++++++ .../src/server/oauth/routes/authorize.ts | 43 +++++++++---------- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/packages/mcp-cloudflare/src/server/oauth/authorize.test.ts b/packages/mcp-cloudflare/src/server/oauth/authorize.test.ts index 19dc7e07..530c4719 100644 --- a/packages/mcp-cloudflare/src/server/oauth/authorize.test.ts +++ b/packages/mcp-cloudflare/src/server/oauth/authorize.test.ts @@ -414,6 +414,29 @@ describe("oauth authorize routes", () => { const locationUrl = new URL(location!); expect(locationUrl.searchParams.get("error")).toBe("invalid_target"); }); + + it("should prevent open redirect with unregistered redirectUri and invalid resource", async () => { + // Validates redirectUri before resource to prevent open redirects + const oauthReqInfo = { + clientId: "test-client", + redirectUri: "https://attacker.com/malicious", + scope: ["read"], + resource: "https://attacker.com/mcp", + state: "test-state", + }; + const formData = new FormData(); + formData.append("state", btoa(JSON.stringify({ oauthReqInfo }))); + + const request = new Request("http://localhost/oauth/authorize", { + method: "POST", + body: formData, + }); + const response = await app.fetch(request, testEnv as Env); + + expect(response.status).toBe(400); + const text = await response.text(); + expect(text).toContain("Invalid redirect URI"); + }); }); }); }); diff --git a/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts b/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts index a969c5ca..4920c87c 100644 --- a/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts +++ b/packages/mcp-cloudflare/src/server/oauth/routes/authorize.ts @@ -165,28 +165,7 @@ export default new Hono<{ Bindings: Env }>() permissions, }; - // Validate resource parameter per RFC 8707 - const resourceFromState = oauthReqWithPermissions.resource; - - if ( - resourceFromState && - !validateResourceParameter(resourceFromState, c.req.url) - ) { - logWarn("Invalid resource parameter in authorization approval", { - loggerScope: ["cloudflare", "oauth", "authorize"], - extra: { - resource: resourceFromState, - clientId: oauthReqWithPermissions.clientId, - }, - }); - - return createResourceValidationError( - oauthReqWithPermissions.redirectUri, - oauthReqWithPermissions.state, - ); - } - - // Validate redirectUri is registered for this client before proceeding + // Validate redirectUri first to prevent open redirects from error responses try { const client = await c.env.OAUTH_PROVIDER.lookupClient( oauthReqWithPermissions.clientId, @@ -212,6 +191,26 @@ export default new Hono<{ Bindings: Env }>() return c.text("Invalid request", 400); } + // Validate resource parameter (RFC 8707) + const resourceFromState = oauthReqWithPermissions.resource; + if ( + resourceFromState && + !validateResourceParameter(resourceFromState, c.req.url) + ) { + logWarn("Invalid resource parameter in authorization approval", { + loggerScope: ["cloudflare", "oauth", "authorize"], + extra: { + resource: resourceFromState, + clientId: oauthReqWithPermissions.clientId, + }, + }); + + return createResourceValidationError( + oauthReqWithPermissions.redirectUri, + oauthReqWithPermissions.state, + ); + } + // Build signed state for redirect to Sentry (10 minute validity) const now = Date.now(); const payload: OAuthState = {