diff --git a/docs/testing/remote.md b/docs/testing/remote.md index 11539c29d..113b5e769 100644 --- a/docs/testing/remote.md +++ b/docs/testing/remote.md @@ -195,6 +195,27 @@ pnpm -w run cli "who am I?" - Automatically refreshed when expired - To force re-auth: delete the token file +**Running in a VM or container:** + +The callback server defaults to `127.0.0.1:8765`, which a browser on the host +cannot reach. Three environment variables override it: + +| Variable | Default | Purpose | +| --- | --- | --- | +| `MCP_OAUTH_PORT` | `8765` | Port the callback server listens on | +| `MCP_OAUTH_HOST` | `127.0.0.1` | Address the callback server binds to | +| `MCP_OAUTH_REDIRECT_URI` | `http://localhost:$MCP_OAUTH_PORT/callback` | Redirect URI sent to the OAuth server | + +```bash +# Bind on all interfaces and send the browser to the forwarded address +MCP_OAUTH_HOST=0.0.0.0 \ +MCP_OAUTH_REDIRECT_URI=http://192.168.1.20:8765/callback \ + pnpm -w run cli "who am I?" +``` + +Set `MCP_OAUTH_REDIRECT_URI` whenever the address the browser uses differs from +the one the CLI binds to, such as behind port forwarding or a devcontainer. + ### Direct Sentry Token Testing Remote `/mcp` also accepts explicit upstream Sentry API tokens with a separate diff --git a/packages/mcp-test-client/src/auth/config.ts b/packages/mcp-test-client/src/auth/config.ts index 20bf455db..fcbbc8fcb 100644 --- a/packages/mcp-test-client/src/auth/config.ts +++ b/packages/mcp-test-client/src/auth/config.ts @@ -1,10 +1,12 @@ import { promises as fs } from "node:fs"; -import { join } from "node:path"; import { homedir } from "node:os"; +import { join } from "node:path"; export interface OAuthClientConfig { clientId: string; mcpHost: string; + /** Redirect URI this client was registered with. */ + redirectUri?: string; registeredAt: string; accessToken?: string; tokenExpiresAt?: string; @@ -66,15 +68,26 @@ export class ConfigManager { * Get OAuth client ID for a specific MCP host */ async getOAuthClientId(mcpHost: string): Promise { - const config = await this.loadConfig(); - const clientConfig = config.oauthClients[mcpHost]; + const clientConfig = await this.getOAuthClient(mcpHost); return clientConfig?.clientId || null; } + /** + * Get the full OAuth client config for a specific MCP host + */ + async getOAuthClient(mcpHost: string): Promise { + const config = await this.loadConfig(); + return config.oauthClients[mcpHost] || null; + } + /** * Store OAuth client ID for a specific MCP host */ - async setOAuthClientId(mcpHost: string, clientId: string): Promise { + async setOAuthClientId( + mcpHost: string, + clientId: string, + redirectUri?: string, + ): Promise { const config = await this.loadConfig(); // Preserve existing access token if present @@ -82,6 +95,7 @@ export class ConfigManager { config.oauthClients[mcpHost] = { clientId, mcpHost, + redirectUri, registeredAt: new Date().toISOString(), accessToken: existing?.accessToken, tokenExpiresAt: existing?.tokenExpiresAt, diff --git a/packages/mcp-test-client/src/auth/oauth.test.ts b/packages/mcp-test-client/src/auth/oauth.test.ts index d35d73618..4c569976f 100644 --- a/packages/mcp-test-client/src/auth/oauth.test.ts +++ b/packages/mcp-test-client/src/auth/oauth.test.ts @@ -1,5 +1,5 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; import open from "open"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { OAuthClient } from "./oauth.js"; vi.mock("open", () => ({ @@ -12,6 +12,11 @@ describe("OAuthClient", () => { vi.spyOn(console, "log").mockImplementation(() => {}); }); + afterEach(() => { + // biome-ignore lint/performance/noDelete: Required to properly unset environment variable + delete process.env.MCP_OAUTH_REDIRECT_URI; + }); + it("includes the protected resource in the authorization request", async () => { const client = new OAuthClient({ mcpHost: "https://mcp.sentry.dev/mcp/sentry/javascript", @@ -50,4 +55,108 @@ describe("OAuthClient", () => { "https://mcp.sentry.dev/mcp/sentry/javascript", ); }); + + it("sends one redirect URI to registration, authorization, and token exchange", async () => { + const redirectUri = "http://192.168.1.20:8765/callback"; + process.env.MCP_OAUTH_REDIRECT_URI = redirectUri; + + const client = new OAuthClient({ mcpHost: "https://mcp.sentry.dev" }); + + const registerClient = vi + .spyOn(client as never, "registerClient") + .mockResolvedValue("client-123"); + const exchangeCodeForToken = vi + .spyOn(client as never, "exchangeCodeForToken") + .mockResolvedValue({ + access_token: "access-token", + token_type: "Bearer", + }); + vi.spyOn(client as never, "startCallbackServer").mockResolvedValue({ + waitForCallback: async () => ({ + code: "auth-code", + state: "oauth-state", + }), + }); + vi.spyOn(client as never, "generateState").mockReturnValue("oauth-state"); + vi.spyOn(client as never, "generatePKCE").mockReturnValue({ + verifier: "verifier", + challenge: "challenge", + }); + vi.spyOn( + (client as never).configManager, + "getOAuthClient", + ).mockResolvedValue(null); + vi.spyOn( + (client as never).configManager, + "setOAuthClientId", + ).mockResolvedValue(undefined); + vi.spyOn( + (client as never).configManager, + "setAccessToken", + ).mockResolvedValue(undefined); + + await client.authenticate(); + + // Registration is what binds the URI server-side; the other two are + // validated against it and must match byte for byte. + expect(registerClient).toHaveBeenCalledTimes(1); + expect((client as never).redirect.redirectUri).toBe(redirectUri); + expect( + new URL(vi.mocked(open).mock.calls[0][0]).searchParams.get( + "redirect_uri", + ), + ).toBe(redirectUri); + expect(exchangeCodeForToken).toHaveBeenCalledTimes(1); + }); + + it("re-registers when the redirect URI no longer matches the stored client", async () => { + process.env.MCP_OAUTH_REDIRECT_URI = "http://192.168.1.20:8765/callback"; + + const client = new OAuthClient({ mcpHost: "https://mcp.sentry.dev" }); + + const registerClient = vi + .spyOn(client as never, "registerClient") + .mockResolvedValue("client-new"); + vi.spyOn( + (client as never).configManager, + "getOAuthClient", + ).mockResolvedValue({ + clientId: "client-old", + mcpHost: "https://mcp.sentry.dev/mcp", + redirectUri: "http://localhost:8765/callback", + registeredAt: new Date().toISOString(), + }); + const setOAuthClientId = vi + .spyOn((client as never).configManager, "setOAuthClientId") + .mockResolvedValue(undefined); + + const clientId = await (client as never).getOrRegisterClientId(); + + expect(registerClient).toHaveBeenCalledTimes(1); + expect(clientId).toBe("client-new"); + expect(setOAuthClientId).toHaveBeenCalledWith( + "https://mcp.sentry.dev/mcp", + "client-new", + "http://192.168.1.20:8765/callback", + ); + }); + + it("reuses a client registered before the redirect URI was recorded", async () => { + const client = new OAuthClient({ mcpHost: "https://mcp.sentry.dev" }); + + const registerClient = vi.spyOn(client as never, "registerClient"); + vi.spyOn( + (client as never).configManager, + "getOAuthClient", + ).mockResolvedValue({ + clientId: "client-legacy", + mcpHost: "https://mcp.sentry.dev/mcp", + registeredAt: new Date().toISOString(), + }); + + const clientId = await (client as never).getOrRegisterClientId(); + + expect(clientId).toBe("client-legacy"); + expect(registerClient).not.toHaveBeenCalled(); + }); }); diff --git a/packages/mcp-test-client/src/auth/oauth.ts b/packages/mcp-test-client/src/auth/oauth.ts index 2ec3d8389..7ba34245f 100644 --- a/packages/mcp-test-client/src/auth/oauth.ts +++ b/packages/mcp-test-client/src/auth/oauth.ts @@ -1,19 +1,21 @@ -import { randomBytes, createHash } from "node:crypto"; +import { createHash, randomBytes } from "node:crypto"; +import { type Server, createServer } from "node:http"; import { URL } from "node:url"; -import { createServer, type Server } from "node:http"; -import open from "open"; import chalk from "chalk"; -import { - OAUTH_REDIRECT_PORT, - OAUTH_REDIRECT_URI, - DEFAULT_OAUTH_SCOPES, -} from "../constants.js"; -import { logInfo, logSuccess, logToolResult, logError } from "../logger.js"; +import open from "open"; +import { DEFAULT_OAUTH_SCOPES } from "../constants.js"; +import { logError, logInfo, logSuccess, logToolResult } from "../logger.js"; import { resolveAuthorizationServerUrl, resolveProtectedResourceUrl, } from "../mcp-url.js"; import { ConfigManager } from "./config.js"; +import { + type OAuthRedirect, + defaultOAuthRedirectUri, + isLoopbackHost, + resolveOAuthRedirect, +} from "./redirect.js"; export interface OAuthConfig { mcpHost: string; @@ -44,6 +46,7 @@ export class OAuthClient { private config: OAuthConfig; private server: Server | null = null; private configManager: ConfigManager; + private redirect: OAuthRedirect; constructor(config: OAuthConfig) { this.config = { @@ -51,6 +54,7 @@ export class OAuthClient { scopes: config.scopes || DEFAULT_OAUTH_SCOPES, }; this.configManager = new ConfigManager(); + this.redirect = resolveOAuthRedirect(); } private getProtectedResourceUrl(): URL { @@ -91,7 +95,7 @@ export class OAuthClient { const registrationData = { client_name: "Sentry MCP CLI", client_uri: "https://github.com/getsentry/sentry-mcp", - redirect_uris: [OAUTH_REDIRECT_URI], + redirect_uris: [this.redirect.redirectUri], grant_types: ["authorization_code"], response_types: ["code"], token_endpoint_auth_method: "none", // PKCE, no client secret @@ -139,7 +143,7 @@ export class OAuthClient { return; } - const url = new URL(req.url, `http://localhost:${OAUTH_REDIRECT_PORT}`); + const url = new URL(req.url, `http://localhost:${this.redirect.port}`); if (url.pathname === "/callback") { const code = url.searchParams.get("code"); @@ -214,7 +218,15 @@ export class OAuthClient { } }); - this.server.listen(OAUTH_REDIRECT_PORT, "127.0.0.1", () => { + if (!isLoopbackHost(this.redirect.host)) { + logInfo( + chalk.yellow( + `Serving the OAuth callback on ${this.redirect.host}:${this.redirect.port}, reachable from the network`, + ), + ); + } + + this.server.listen(this.redirect.port, this.redirect.host, () => { const waitForCallback = () => new Promise<{ code: string; state: string }>((res, rej) => { resolveCallback = res; @@ -242,7 +254,7 @@ export class OAuthClient { grant_type: "authorization_code", client_id: params.clientId, code: params.code, - redirect_uri: OAUTH_REDIRECT_URI, + redirect_uri: this.redirect.redirectUri, code_verifier: params.codeVerifier, }); @@ -270,10 +282,16 @@ export class OAuthClient { private async getOrRegisterClientId(): Promise { const configKey = this.getConfigKey(); - // Check if we already have a registered client for this host - let clientId = await this.configManager.getOAuthClientId(configKey); + // Check if we already have a registered client for this host. The OAuth + // server validates the request against the redirect URIs bound to the + // client, so a changed redirect URI needs a new registration. Clients + // registered before the URI was recorded used the default. + const existing = await this.configManager.getOAuthClient(configKey); + const registeredRedirectUri = + existing?.redirectUri ?? defaultOAuthRedirectUri(); - if (clientId) { + let clientId = existing?.clientId ?? null; + if (clientId && registeredRedirectUri === this.redirect.redirectUri) { return clientId; } @@ -283,7 +301,11 @@ export class OAuthClient { clientId = await this.registerClient(); // Store the client ID for future use - await this.configManager.setOAuthClientId(configKey, clientId); + await this.configManager.setOAuthClientId( + configKey, + clientId, + this.redirect.redirectUri, + ); logSuccess("Client registered and saved"); logToolResult(clientId); @@ -331,7 +353,7 @@ export class OAuthClient { // Build authorization URL const authUrl = new URL(this.getAuthorizationServerUrl("/oauth/authorize")); authUrl.searchParams.set("client_id", clientId); - authUrl.searchParams.set("redirect_uri", OAUTH_REDIRECT_URI); + authUrl.searchParams.set("redirect_uri", this.redirect.redirectUri); authUrl.searchParams.set("response_type", "code"); authUrl.searchParams.set("scope", this.config.scopes!.join(" ")); authUrl.searchParams.set("state", state); diff --git a/packages/mcp-test-client/src/auth/redirect.test.ts b/packages/mcp-test-client/src/auth/redirect.test.ts new file mode 100644 index 000000000..df9c6b87a --- /dev/null +++ b/packages/mcp-test-client/src/auth/redirect.test.ts @@ -0,0 +1,116 @@ +import { describe, expect, it } from "vitest"; +import { + defaultOAuthRedirectUri, + isLoopbackHost, + resolveOAuthRedirect, +} from "./redirect.js"; + +describe("resolveOAuthRedirect", () => { + it("defaults to the loopback callback server", () => { + expect(resolveOAuthRedirect({})).toEqual({ + port: 8765, + host: "127.0.0.1", + redirectUri: "http://localhost:8765/callback", + }); + }); + + it("derives the redirect URI from MCP_OAUTH_PORT", () => { + expect(resolveOAuthRedirect({ MCP_OAUTH_PORT: "9000" })).toEqual({ + port: 9000, + host: "127.0.0.1", + redirectUri: "http://localhost:9000/callback", + }); + }); + + it("binds to MCP_OAUTH_HOST so the callback is reachable from a host browser", () => { + expect(resolveOAuthRedirect({ MCP_OAUTH_HOST: "0.0.0.0" }).host).toBe( + "0.0.0.0", + ); + }); + + it("uses MCP_OAUTH_REDIRECT_URI verbatim", () => { + const redirect = resolveOAuthRedirect({ + MCP_OAUTH_REDIRECT_URI: "http://192.168.1.20:8765/callback", + }); + + expect(redirect.redirectUri).toBe("http://192.168.1.20:8765/callback"); + expect(redirect.port).toBe(8765); + }); + + it("keeps an explicit redirect URI independent of the listen port", () => { + const redirect = resolveOAuthRedirect({ + MCP_OAUTH_PORT: "9000", + MCP_OAUTH_REDIRECT_URI: "http://host.docker.internal:8765/callback", + }); + + expect(redirect.port).toBe(9000); + expect(redirect.redirectUri).toBe( + "http://host.docker.internal:8765/callback", + ); + }); + + it.each(["", " "])("ignores blank overrides (%j)", (value) => { + expect( + resolveOAuthRedirect({ + MCP_OAUTH_PORT: value, + MCP_OAUTH_HOST: value, + MCP_OAUTH_REDIRECT_URI: value, + }), + ).toEqual({ + port: 8765, + host: "127.0.0.1", + redirectUri: "http://localhost:8765/callback", + }); + }); + + it.each(["not-a-port", "8765.5", "-1", "70000", "0", "0x1F90", "1e4"])( + "rejects invalid MCP_OAUTH_PORT (%s)", + (value) => { + expect(() => resolveOAuthRedirect({ MCP_OAUTH_PORT: value })).toThrow( + /MCP_OAUTH_PORT/, + ); + }, + ); + + it.each(["javascript:alert(1)", "file:///etc/passwd"])( + "rejects a non-http MCP_OAUTH_REDIRECT_URI (%s)", + (value) => { + expect(() => + resolveOAuthRedirect({ MCP_OAUTH_REDIRECT_URI: value }), + ).toThrow(/http or https/); + }, + ); + + it("rejects a relative MCP_OAUTH_REDIRECT_URI", () => { + expect(() => + resolveOAuthRedirect({ MCP_OAUTH_REDIRECT_URI: "/callback" }), + ).toThrow(/absolute URL/); + }); + + it("rejects a redirect URI containing userinfo", () => { + expect(() => + resolveOAuthRedirect({ + MCP_OAUTH_REDIRECT_URI: "http://mcp.sentry.dev@evil.example/callback", + }), + ).toThrow(/userinfo/); + }); + + it("matches the default resolution", () => { + expect(defaultOAuthRedirectUri()).toBe( + resolveOAuthRedirect({}).redirectUri, + ); + }); +}); + +describe("isLoopbackHost", () => { + it.each(["127.0.0.1", "::1", "localhost"])( + "treats %s as loopback", + (host) => { + expect(isLoopbackHost(host)).toBe(true); + }, + ); + + it.each(["0.0.0.0", "192.168.1.20"])("treats %s as reachable", (host) => { + expect(isLoopbackHost(host)).toBe(false); + }); +}); diff --git a/packages/mcp-test-client/src/auth/redirect.ts b/packages/mcp-test-client/src/auth/redirect.ts new file mode 100644 index 000000000..08b9cecf2 --- /dev/null +++ b/packages/mcp-test-client/src/auth/redirect.ts @@ -0,0 +1,101 @@ +import { + DEFAULT_OAUTH_CALLBACK_HOST, + DEFAULT_OAUTH_REDIRECT_PORT, +} from "../constants.js"; + +export interface OAuthRedirect { + /** Port the local callback server listens on. */ + port: number; + /** Address the local callback server binds to. */ + host: string; + /** Redirect URI sent to the OAuth server. */ + redirectUri: string; +} + +/** + * Resolve where the OAuth callback is served and how the OAuth server should + * reach it. + * + * Defaults keep the flow on loopback. Running inside a VM or container the + * browser usually lives on the host, so `MCP_OAUTH_HOST=0.0.0.0` makes the + * callback server reachable and `MCP_OAUTH_REDIRECT_URI` points the OAuth + * server at the forwarded address. + * + * The redirect URI must be byte-identical across client registration, the + * authorization request, and the token exchange, so it is resolved once and + * reused. + * + * - `MCP_OAUTH_PORT` -> callback port (default 8765) + * - `MCP_OAUTH_HOST` -> bind address (default 127.0.0.1) + * - `MCP_OAUTH_REDIRECT_URI` -> full redirect URI (default derived from port) + */ +export function resolveOAuthRedirect( + env: NodeJS.ProcessEnv = process.env, +): OAuthRedirect { + const port = resolvePort(env.MCP_OAUTH_PORT); + const host = env.MCP_OAUTH_HOST?.trim() || DEFAULT_OAUTH_CALLBACK_HOST; + const redirectUri = + resolveRedirectUri(env.MCP_OAUTH_REDIRECT_URI) ?? + `http://localhost:${port}/callback`; + + return { port, host, redirectUri }; +} + +/** + * The redirect URI used before it became configurable. Clients registered then + * have no recorded redirect URI, so this is what they were registered with. + */ +export function defaultOAuthRedirectUri(): string { + return `http://localhost:${DEFAULT_OAUTH_REDIRECT_PORT}/callback`; +} + +/** Whether the callback server is reachable from outside this machine. */ +export function isLoopbackHost(host: string): boolean { + return host === "127.0.0.1" || host === "::1" || host === "localhost"; +} + +function resolvePort(value: string | undefined): number { + if (value === undefined || value.trim() === "") { + return DEFAULT_OAUTH_REDIRECT_PORT; + } + + // The browser needs the port before the server binds, so an ephemeral port + // (0) cannot work here. + const trimmed = value.trim(); + const port = /^\d+$/.test(trimmed) ? Number(trimmed) : Number.NaN; + if (!Number.isInteger(port) || port < 1 || port > 65535) { + throw new Error( + `MCP_OAUTH_PORT must be an integer between 1 and 65535, got: ${value}`, + ); + } + + return port; +} + +function resolveRedirectUri(value: string | undefined): string | null { + const redirectUri = value?.trim(); + if (!redirectUri) { + return null; + } + + let parsed: URL; + try { + parsed = new URL(redirectUri); + } catch { + throw new Error( + `MCP_OAUTH_REDIRECT_URI must be an absolute URL, got: ${value}`, + ); + } + + if (parsed.protocol !== "http:" && parsed.protocol !== "https:") { + throw new Error( + `MCP_OAUTH_REDIRECT_URI must be http or https, got: ${parsed.protocol}`, + ); + } + + if (parsed.username || parsed.password) { + throw new Error("MCP_OAUTH_REDIRECT_URI must not contain userinfo"); + } + + return redirectUri; +} diff --git a/packages/mcp-test-client/src/constants.ts b/packages/mcp-test-client/src/constants.ts index 0a95bf05e..d96d28cab 100644 --- a/packages/mcp-test-client/src/constants.ts +++ b/packages/mcp-test-client/src/constants.ts @@ -5,9 +5,10 @@ export const DEFAULT_OPENAI_MODEL = "gpt-4o"; export const DEFAULT_OPENROUTER_MODEL = "openai/gpt-5"; export const OPENROUTER_BASE_URL = "https://openrouter.ai/api/v1"; -// OAuth configuration -export const OAUTH_REDIRECT_PORT = 8765; -export const OAUTH_REDIRECT_URI = `http://localhost:${OAUTH_REDIRECT_PORT}/callback`; +// OAuth configuration. Overridable via MCP_OAUTH_PORT, MCP_OAUTH_HOST, and +// MCP_OAUTH_REDIRECT_URI - see auth/redirect.ts. +export const DEFAULT_OAUTH_REDIRECT_PORT = 8765; +export const DEFAULT_OAUTH_CALLBACK_HOST = "127.0.0.1"; // Default OAuth scopes export const DEFAULT_OAUTH_SCOPES = [