diff --git a/packages/mcp-cloudflare/src/server/oauth/authorize.test.ts b/packages/mcp-cloudflare/src/server/oauth/authorize.test.ts index 42b72308..d5f01de8 100644 --- a/packages/mcp-cloudflare/src/server/oauth/authorize.test.ts +++ b/packages/mcp-cloudflare/src/server/oauth/authorize.test.ts @@ -196,4 +196,288 @@ 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"); + }); + + it("should reject empty resource parameter", async () => { + mockOAuthProvider.parseAuthRequest.mockResolvedValueOnce({ + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + }); + + const request = new Request( + "http://localhost/oauth/authorize?resource=&redirect_uri=https://example.com/callback", + { method: "GET" }, + ); + const response = await app.fetch(request, testEnv as Env); + + expect(response.status).toBe(302); + const location = response.headers.get("location"); + const locationUrl = new URL(location!); + expect(locationUrl.searchParams.get("error")).toBe("invalid_target"); + }); + }); + + 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"); + }); + + 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"); + }); + + it("should reject empty resource parameter", async () => { + const oauthReqInfo = { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + resource: "", + }; + 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(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..3a9e3368 100644 --- a/packages/mcp-cloudflare/src/server/oauth/callback.test.ts +++ b/packages/mcp-cloudflare/src/server/oauth/callback.test.ts @@ -216,4 +216,333 @@ 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"); + }); + + it("should reject empty resource parameter", async () => { + mockOAuthProvider.lookupClient.mockResolvedValue({ + clientId: "test-client", + clientName: "Test Client", + redirectUris: ["https://example.com/callback"], + }); + + const now = Date.now(); + const payload: OAuthState = { + req: { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + resource: "", + }, + iat: now, + exp: now + 10 * 60 * 1000, + } as unknown as OAuthState; + const signedState = await signState(payload, testEnv.COOKIE_SECRET!); + + // Get approval cookie + const validApprovalFormData = new FormData(); + validApprovalFormData.append( + "state", + btoa( + JSON.stringify({ + oauthReqInfo: { + clientId: "test-client", + redirectUri: "https://example.com/callback", + scope: ["read"], + }, + }), + ), + ); + 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); + + 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..bc494913 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,310 @@ 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 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", + "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 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", + "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 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)", + "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 reject URL with fragment (RFC 8707)", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev/mcp#fragment", + "https://mcp.sentry.dev/oauth/authorize", + ); + expect(result).toBe(false); + }); + + 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 reject URL-encoded slashes in path", () => { + const result = validateResourceParameter( + "https://mcp.sentry.dev/mcp%2Forg", + "https://mcp.sentry.dev/oauth/authorize", + ); + // 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); + } + }); + }); +}); + +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..ab37c51b 100644 --- a/packages/mcp-cloudflare/src/server/oauth/helpers.ts +++ b/packages/mcp-cloudflare/src/server/oauth/helpers.ts @@ -306,3 +306,86 @@ 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); + + // 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; + } + + // Normalize default ports for comparison + const getPort = (url: URL) => + url.port || (url.protocol === "https:" ? "443" : "80"); + + if (getPort(resourceUrl) !== getPort(requestUrlObj)) { + 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" || + resourceUrl.pathname.startsWith("/mcp/") + ); + } 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 ec764ffe..36ca9ea1 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 skills + * Extended AuthRequest that includes skills and resource parameter */ interface AuthRequestWithSkills extends AuthRequest { skills?: unknown; + resource?: string; } async function redirectToUpstream( @@ -73,6 +78,39 @@ 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 !== null && + !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); + } + + // Preserve resource in state, not exported upstream + const oauthReqInfoWithResource: AuthRequestWithSkills = { + ...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 @@ -90,12 +128,12 @@ export default new Hono<{ Bindings: Env }>() // return redirectToUpstream(c.env, c.req.raw, oauthReqInfo); // } - return await renderApprovalDialog(c.req.raw, { + return renderApprovalDialog(c.req.raw, { client: await c.env.OAUTH_PROVIDER.lookupClient(clientId), server: { name: "Sentry MCP", }, - state: { oauthReqInfo }, // arbitrary data that flows through the form submission below + state: { oauthReqInfo: oauthReqInfoWithResource }, }); }) @@ -130,7 +168,7 @@ export default new Hono<{ Bindings: Env }>() skills, }; - // 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( oauthReqWithSkills.clientId, @@ -156,6 +194,26 @@ export default new Hono<{ Bindings: Env }>() return c.text("Invalid request", 400); } + // Validate resource parameter (RFC 8707) + const resourceFromState = oauthReqWithSkills.resource; + if ( + resourceFromState !== undefined && + !validateResourceParameter(resourceFromState, c.req.url) + ) { + logWarn("Invalid resource parameter in authorization approval", { + loggerScope: ["cloudflare", "oauth", "authorize"], + extra: { + resource: resourceFromState, + clientId: oauthReqWithSkills.clientId, + }, + }); + + return createResourceValidationError( + oauthReqWithSkills.redirectUri, + oauthReqWithSkills.state, + ); + } + // Build signed state for redirect to Sentry (10 minute validity) const now = Date.now(); const payload: OAuthState = { diff --git a/packages/mcp-cloudflare/src/server/oauth/routes/callback.ts b/packages/mcp-cloudflare/src/server/oauth/routes/callback.ts index 30ee55af..f25cb7c2 100644 --- a/packages/mcp-cloudflare/src/server/oauth/routes/callback.ts +++ b/packages/mcp-cloudflare/src/server/oauth/routes/callback.ts @@ -3,16 +3,20 @@ import type { AuthRequest } from "@cloudflare/workers-oauth-provider"; import { clientIdAlreadyApproved } from "../../lib/approval-dialog"; import type { Env, WorkerProps } from "../../types"; 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"; import { parseSkills, getScopesForSkills } from "@sentry/mcp-server/skills"; /** - * Extended AuthRequest that includes skills + * Extended AuthRequest that includes skills and resource parameter */ interface AuthRequestWithSkills extends AuthRequest { skills?: unknown; // Skill-based authorization system + resource?: string; } /** @@ -66,6 +70,23 @@ 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 !== undefined && + !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 @@ -159,7 +180,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,