diff --git a/e2e/oauth-authorization.spec.ts b/e2e/oauth-authorization.spec.ts new file mode 100644 index 0000000..d43bf18 --- /dev/null +++ b/e2e/oauth-authorization.spec.ts @@ -0,0 +1,168 @@ +/** + * OAuth authorization-code popup flow (mcp-context-forge#6458). + * + * The real round trip -- BFF proxies GET /oauth/authorize/{id} to mcpgateway, + * which 302s to the OAuth provider; the provider redirects back to + * /oauth/callback, which the BFF also proxies; that page posts the result to + * window.opener and closes -- can't be driven through a real IdP in CI. What + * *is* testable end to end through a real browser, without any backend, is + * the client-side contract those two hops feed into: triggerOAuthAuthorization + * (client/src/api/servers.ts) opens the popup, listens for a same-window + * postMessage, and resolves/rejects the promise that drives the form's + * pending/success/error states. This stubs the popup's very first navigation + * (the oauth/authorize route) with the exact HTML shape mcpgateway's own + * _popup_notification_script produces, so the assertion is: does the whole + * chain from clicking "Connect server" to the success notification actually + * work, not just each piece in isolation (already covered by + * src/api/servers.test.ts and server/test/oauth-*.test.ts). + */ +import { test, expect } from "./fixtures/auth"; +import { APP } from "./utils/paths"; + +const GATEWAY_ID = "gw-oauth-1"; +const GATEWAY_NAME = "GitHub OAuth Test"; + +test.describe("OAuth authorization-code popup flow", () => { + test.beforeEach(async ({ page, apiMock }) => { + await apiMock.mockPermissions(); + await page.route("**/gateways?*", async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ gateways: [], nextCursor: null }), + }); + }); + }); + + test("create -> popup -> postMessage -> activate -> fetch tools", async ({ page, context }) => { + // Registered at the browser-context level (not just this page) so it also + // covers the popup window's own navigation, exactly like mcpgateway's + // popup-branch callback HTML: postMessage(payload, '*') then window.close(). + await context.route("**/oauth/authorize/**", async (route) => { + await route.fulfill({ + status: 200, + contentType: "text/html", + body: ``, + }); + }); + + await page.route("**/gateways", async (route) => { + if (route.request().method() !== "POST") return route.fallback(); + await route.fulfill({ + status: 201, + contentType: "application/json", + body: JSON.stringify({ id: GATEWAY_ID, name: GATEWAY_NAME }), + }); + }); + + await page.route(`**/gateways/${GATEWAY_ID}/state*`, async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ status: "success", message: "activated" }), + }); + }); + + await page.route(`**/oauth/fetch-tools/${GATEWAY_ID}`, async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ success: true, message: "Fetched 3 tools." }), + }); + }); + + await page.goto(APP.SERVERS); + await page.waitForLoadState("networkidle"); + + await page.getByRole("button", { name: /Connect/i }).click(); + + await page.getByLabel("Name").fill(GATEWAY_NAME); + await page.getByLabel("URL").fill("https://api.githubcopilot.com/mcp"); + + await page.getByRole("button", { name: "Advanced settings" }).click(); + await page.getByText("OAuth 2.0", { exact: true }).click(); + + await page.getByLabel(/Grant type/).click(); + await page.getByRole("option", { name: /Authorization code/i }).click(); + + await page.getByLabel("Issuer URL").fill("https://github.com"); + await page.getByLabel("Client ID").fill("test-client-id"); + await page.getByLabel("Client Secret").fill("test-client-secret"); // pragma: allowlist secret + await page.getByLabel("Authorization URL").fill("https://github.com/login/oauth/authorize"); + await page.getByLabel("Token URL").fill("https://github.com/login/oauth/access_token"); + + // The auto-placeholder from the redirect_uri fix (mcp-context-forge#6458): + // never guessed from window.location.origin, never submitted as a value. + await expect(page.getByLabel(/Redirect URI/i)).toHaveValue( + "Determined automatically by the server", + ); + + await page.getByRole("button", { name: "Connect server" }).click(); + + await expect( + page.getByText(/Waiting for OAuth authorization in the popup window/i), + ).toBeVisible(); + await expect(page.getByText(/OAuth authorization successful/i)).toBeVisible(); + await expect(page.getByText(/Fetched 3 tools\./i)).toBeVisible(); + }); + + test("shows an error notification when the popup posts an error result", async ({ + page, + context, + }) => { + await context.route("**/oauth/authorize/**", async (route) => { + await route.fulfill({ + status: 200, + contentType: "text/html", + body: ``, + }); + }); + + await page.route("**/gateways", async (route) => { + if (route.request().method() !== "POST") return route.fallback(); + await route.fulfill({ + status: 201, + contentType: "application/json", + body: JSON.stringify({ id: GATEWAY_ID, name: GATEWAY_NAME }), + }); + }); + + await page.goto(APP.SERVERS); + await page.waitForLoadState("networkidle"); + + await page.getByRole("button", { name: /Connect/i }).click(); + await page.getByLabel("Name").fill(GATEWAY_NAME); + await page.getByLabel("URL").fill("https://api.githubcopilot.com/mcp"); + await page.getByRole("button", { name: "Advanced settings" }).click(); + await page.getByText("OAuth 2.0", { exact: true }).click(); + await page.getByLabel(/Grant type/).click(); + await page.getByRole("option", { name: /Authorization code/i }).click(); + await page.getByLabel("Issuer URL").fill("https://github.com"); + await page.getByLabel("Client ID").fill("test-client-id"); + await page.getByLabel("Client Secret").fill("test-client-secret"); // pragma: allowlist secret + await page.getByLabel("Authorization URL").fill("https://github.com/login/oauth/authorize"); + await page.getByLabel("Token URL").fill("https://github.com/login/oauth/access_token"); + + await page.getByRole("button", { name: "Connect server" }).click(); + + await expect(page.getByText(/User cancelled/i)).toBeVisible(); + // The form must stay open on error so the user can see it and retry. + await expect(page.getByRole("button", { name: "Connect server" })).toBeVisible(); + }); +}); diff --git a/server/src/config.ts b/server/src/config.ts index 5439f67..1e3b4ee 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -37,6 +37,13 @@ export const config = { // default), so this must stay above the upstream email-delivery timeout. passwordResetRequestTimeoutMs: Number(optional("PASSWORD_RESET_REQUEST_TIMEOUT_MS", "30000")), + // Shared by both OAuth popup proxy routes (routes/proxy/oauth-authorize.ts, + // oauth-callback.ts). GET /oauth/authorize/{id} can synchronously run DCR + // registration (an outbound call to the IdP's own registration/discovery + // endpoints) before it redirects, so this is sized for that -- more + // headroom than a plain API call needs. + oauthProxyTimeoutMs: Number(optional("OAUTH_PROXY_TIMEOUT_MS", "30000")), + // memory:// (default) = in-process store, no Redis needed — dev only. // See lib/memory-redis.ts. Use a real redis:// URL beyond a single // local dev process. optionalUnset so REDIS_URL="" also falls through @@ -104,6 +111,10 @@ if ( throw new Error("PASSWORD_RESET_REQUEST_TIMEOUT_MS must be a positive integer"); } +if (!Number.isSafeInteger(config.oauthProxyTimeoutMs) || config.oauthProxyTimeoutMs <= 0) { + throw new Error("OAUTH_PROXY_TIMEOUT_MS must be a positive integer"); +} + // COOKIE_SECURE=true (prod default) with neither PUBLIC_ORIGIN nor TRUST_PROXY // set means origin-guard.ts derives its expected origin from request.protocol, // which is wrong behind a TLS-terminating proxy (it reads "http" while the diff --git a/server/src/index.ts b/server/src/index.ts index 9c8d5ba..5225ff6 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -23,6 +23,8 @@ import loginRoute from "./routes/auth/login.js"; import logoutRoute from "./routes/auth/logout.js"; import sessionRoute from "./routes/auth/session.js"; import catchAllProxyRoute from "./routes/proxy/catch-all.js"; +import oauthAuthorizeProxyRoute from "./routes/proxy/oauth-authorize.js"; +import oauthCallbackProxyRoute from "./routes/proxy/oauth-callback.js"; import publicPasswordResetRoute from "./routes/proxy/public-password-reset.js"; import { startRevocationSubscriber } from "./routes/sse/revocation-subscriber.js"; import sseRoutes from "./routes/sse/routes.js"; @@ -49,6 +51,8 @@ await fastify.register(sessionRoute); await fastify.register(changePasswordRequiredRoute); await fastify.register(sseRoutes); await fastify.register(publicPasswordResetRoute); +await fastify.register(oauthAuthorizeProxyRoute); +await fastify.register(oauthCallbackProxyRoute); await fastify.register(catchAllProxyRoute); await fastify.register(appRoute); diff --git a/server/src/lib/oauth-upstream-forward.ts b/server/src/lib/oauth-upstream-forward.ts new file mode 100644 index 0000000..955c85d --- /dev/null +++ b/server/src/lib/oauth-upstream-forward.ts @@ -0,0 +1,67 @@ +// Location: ./client/server/src/lib/oauth-upstream-forward.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// Shared GET-and-forward for the two OAuth popup proxy routes +// (routes/proxy/oauth-authorize.ts, oauth-callback.ts): fetch upstream with +// a timeout, forward status/Location/Content-Type/body, 502 on network +// failure. Kept in one place so a fix to one hop's forwarding behavior +// (e.g. a missing header, the empty-body edge case) can't silently drift +// out of sync with the other's -- same rationale as catch-all.ts centralizing +// rewriteUpstreamLocation/stripInboundHeaders for the /api/* proxy. +// +// Location is forwarded whenever present regardless of caller: harmless for +// oauth-callback.ts (mcpgateway's GET /oauth/callback never redirects), and +// it's the whole point for oauth-authorize.ts (the 302 to the OAuth +// provider). Never rewritten -- unlike catch-all's rewriteUpstreamLocation, +// which only rewrites Location values pointing back at config.contextforgeUrl +// -- because both hops here only ever redirect to an external OAuth +// provider's own absolute URL. + +import type { FastifyReply, FastifyRequest } from "fastify"; + +interface ForwardOAuthGetOptions { + /** Extra headers merged into the upstream request (e.g. the injected bearer token). */ + headers?: Record; + timeoutMs: number; + /** Included in the network-failure log line, e.g. "OAuth authorize". */ + logLabel: string; +} + +export async function forwardOAuthGet( + request: FastifyRequest, + reply: FastifyReply, + upstreamUrl: string, + { headers = {}, timeoutMs, logLabel }: ForwardOAuthGetOptions, +): Promise { + let upstreamResponse: Response; + try { + upstreamResponse = await fetch(upstreamUrl, { + method: "GET", + headers: { + accept: "text/html", + // Preserve real client IP for upstream audit logging, same as catch-all.ts. + "x-forwarded-for": request.ip, + "x-real-ip": request.ip, + ...headers, + }, + redirect: "manual", + signal: AbortSignal.timeout(timeoutMs), + }); + } catch (err) { + request.log.error( + { errorType: err instanceof Error ? err.name : typeof err }, + `upstream ${logLabel} request failed`, + ); + return reply.code(502).send({ error: "upstream_unavailable" }); + } + + const location = upstreamResponse.headers.get("location"); + if (location) reply.header("location", location); + + const contentType = upstreamResponse.headers.get("content-type"); + if (contentType) reply.header("content-type", contentType); + + const body = await upstreamResponse.text(); + return reply.code(upstreamResponse.status).send(body || undefined); +} diff --git a/server/src/routes/proxy/oauth-authorize.ts b/server/src/routes/proxy/oauth-authorize.ts new file mode 100644 index 0000000..403d21d --- /dev/null +++ b/server/src/routes/proxy/oauth-authorize.ts @@ -0,0 +1,77 @@ +// Location: ./client/server/src/routes/proxy/oauth-authorize.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// Authenticated proxy for the OAuth authorization-code popup's first hop. +// +// src/api/servers.ts's triggerOAuthAuthorization opens this path with a raw +// `window.open` navigation, not through the API client, so it carries no +// Authorization header the way /api/* calls do (find-my-way would otherwise +// never have matched this route at all -- with none registered, it fell +// through to static.ts's SPA-fallback 404 handler, which unconditionally +// serves index.html; see mcp-context-forge#6458). mcpgateway's +// GET /oauth/authorize/{id} requires an authenticated user -- it may run DCR +// registration and DB writes before it 302s to the OAuth provider -- so, +// unlike /oauth/callback, this hop has to be proxied through the BFF, which +// injects the bearer token from the session the same way catch-all.ts does +// for /api/*. +// +// See oauth-callback.ts for the second leg: mcpgateway's own callback +// endpoint, which -- unlike this one -- needs no session and is proxied for +// a different reason (making the browser-facing redirect_uri work when the +// gateway itself isn't independently internet-reachable). +// +// GET is a safe method, so catch-all.ts's csrfIfUnsafe wouldn't cover this +// route even if applied -- and window.open() can't set an X-CSRF-Token +// header anyway. The session cookie is SameSite=Lax (session-store.ts), +// which still rides along on a top-level cross-site navigation, and this +// route is not side-effect-free (DCR registration, DB writes) -- so a +// hostile page could force a logged-in victim's browser into +// window.open(`${victimOrigin}/oauth/authorize/`) and +// have it execute with the victim's bearer token. isForbiddenCrossOrigin is +// the same guard login.ts and proxy-sse.ts already use for this exact +// category (cookie-authenticated, can't carry a CSRF token) -- see +// lib/origin-guard.ts. +// +// redirect: "manual" (in forwardOAuthGet) so mcpgateway's 302 Location (the +// OAuth provider's own absolute URL) is forwarded to the browser as-is +// rather than followed server-side -- undici's fetch would otherwise try to +// navigate through it, leaking nothing sensitive but pointlessly making a +// request meant for the browser. + +import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; + +import { config } from "../../config.js"; +import { forwardOAuthGet } from "../../lib/oauth-upstream-forward.js"; +import { isForbiddenCrossOrigin } from "../../lib/origin-guard.js"; +import { setNoStore } from "../../lib/no-store.js"; +import { upstreamAuthHeader } from "../../lib/upstream-auth.js"; + +interface AuthorizeParams { + gatewayId: string; +} + +export default async function oauthAuthorizeProxyRoute(fastify: FastifyInstance): Promise { + fastify.get<{ Params: AuthorizeParams }>( + "/oauth/authorize/:gatewayId", + { preHandler: fastify.sessionAuth }, + async (request: FastifyRequest<{ Params: AuthorizeParams }>, reply: FastifyReply) => { + setNoStore(reply); + + if (isForbiddenCrossOrigin(request)) { + return reply.code(403).send({ error: "cross_site_request_forbidden" }); + } + + const bearerToken = request.session!.bearerToken; + const queryIndex = request.url.indexOf("?"); + const query = queryIndex === -1 ? "" : request.url.slice(queryIndex); + const upstreamUrl = `${config.contextforgeUrl}/oauth/authorize/${encodeURIComponent(request.params.gatewayId)}${query}`; + + return forwardOAuthGet(request, reply, upstreamUrl, { + headers: upstreamAuthHeader(bearerToken), + timeoutMs: config.oauthProxyTimeoutMs, + logLabel: "OAuth authorize", + }); + }, + ); +} diff --git a/server/src/routes/proxy/oauth-callback.ts b/server/src/routes/proxy/oauth-callback.ts new file mode 100644 index 0000000..c23cdaf --- /dev/null +++ b/server/src/routes/proxy/oauth-callback.ts @@ -0,0 +1,55 @@ +// Location: ./client/server/src/routes/proxy/oauth-callback.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// Unauthenticated proxy for the OAuth authorization-code popup's second hop. +// +// The OAuth provider redirects the browser here as a top-level navigation +// using whatever `redirect_uri` was registered for the flow. Two cases both +// need this route: +// +// - A gateway saved before this fix (or one an operator has explicitly +// pointed at the web UI's own origin) carries `oauth_config.redirect_uri +// = /oauth/callback`. Without this route that request fell +// through to static.ts's SPA-fallback 404 handler -- which unconditionally +// serves index.html -- landing the popup on a client route the React +// router doesn't recognize: blank page, no postMessage, stuck forever +// (observed live against mcp-context-forge#6458's fix). +// - Even once OAuth2Auth.tsx stops guessing a redirect_uri and the gateway +// defaults to its own APP_DOMAIN (see oauth-authorize.ts), that default +// is only browser-reachable if the gateway is independently exposed. In +// the common split deployment where only the web UI is public-facing, +// the redirect_uri needs to resolve to *this* origin regardless, with the +// BFF forwarding the final hop to the gateway server-to-server. +// +// mcpgateway's GET /oauth/callback requires no session -- security comes +// from the HMAC-verified `state` query param, not a cookie -- so this proxy, +// unlike oauth-authorize.ts, injects no bearer token and needs no +// `sessionAuth` preHandler. Its non-popup response path sets a short-lived +// jwt_token/CSRF cookie pair for a legacy "fetch tools" admin button; the +// React SPA always passes popup=true through to /oauth/authorize (see +// src/api/servers.ts), so mcpgateway always takes the popup branch here and +// never emits those cookies through this proxy. Set-Cookie is stripped +// regardless, matching catch-all.ts's rule that mcpgateway's own cookies +// must never reach the browser under the BFF's session-cookie boundary. + +import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; + +import { config } from "../../config.js"; +import { forwardOAuthGet } from "../../lib/oauth-upstream-forward.js"; +import { setNoStore } from "../../lib/no-store.js"; + +export default async function oauthCallbackProxyRoute(fastify: FastifyInstance): Promise { + fastify.get("/oauth/callback", async (request: FastifyRequest, reply: FastifyReply) => { + setNoStore(reply); + + const queryIndex = request.url.indexOf("?"); + const query = queryIndex === -1 ? "" : request.url.slice(queryIndex); + const upstreamUrl = `${config.contextforgeUrl}/oauth/callback${query}`; + + return forwardOAuthGet(request, reply, upstreamUrl, { + timeoutMs: config.oauthProxyTimeoutMs, + logLabel: "OAuth callback", + }); + }); +} diff --git a/server/test/helpers/build-app.ts b/server/test/helpers/build-app.ts index 716d957..05b1a21 100644 --- a/server/test/helpers/build-app.ts +++ b/server/test/helpers/build-app.ts @@ -18,6 +18,8 @@ import loginRoute from "../../src/routes/auth/login.js"; import logoutRoute from "../../src/routes/auth/logout.js"; import sessionRoute from "../../src/routes/auth/session.js"; import catchAllProxyRoute from "../../src/routes/proxy/catch-all.js"; +import oauthAuthorizeProxyRoute from "../../src/routes/proxy/oauth-authorize.js"; +import oauthCallbackProxyRoute from "../../src/routes/proxy/oauth-callback.js"; import publicPasswordResetRoute from "../../src/routes/proxy/public-password-reset.js"; export class FakeRedis { @@ -64,6 +66,8 @@ export async function buildTestApp(opts: { withProxy?: boolean } = {}): Promise< await fastify.register(publicPasswordResetRoute); if (opts.withProxy) { + await fastify.register(oauthAuthorizeProxyRoute); + await fastify.register(oauthCallbackProxyRoute); await fastify.register(catchAllProxyRoute); } diff --git a/server/test/oauth-authorize.test.ts b/server/test/oauth-authorize.test.ts new file mode 100644 index 0000000..daa5801 --- /dev/null +++ b/server/test/oauth-authorize.test.ts @@ -0,0 +1,140 @@ +// Location: ./client/server/test/oauth-authorize.test.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// CONTEXTFORGE_URL must be set before src/config.ts (and anything importing it) +// is first evaluated, so the fake upstream server is spun up and +// process.env.CONTEXTFORGE_URL set in beforeAll, with every module under test +// dynamic-imported afterwards rather than statically at the top of the file. + +import { createServer, type IncomingMessage, type Server } from "node:http"; +import type { AddressInfo } from "node:net"; + +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +let upstream: Server; +let upstreamOrigin: string; +let lastRequest: + { path: string; authorization: string | undefined; accept: string | undefined } | undefined; + +beforeAll(async () => { + upstream = createServer((req: IncomingMessage, res) => { + lastRequest = { + path: req.url ?? "", + authorization: req.headers.authorization, + accept: req.headers.accept, + }; + + if (req.url?.startsWith("/oauth/authorize/missing-config")) { + res.writeHead(400, { "content-type": "application/json" }); + res.end(JSON.stringify({ detail: "Gateway is not configured for OAuth" })); + return; + } + + // Mirrors mcpgateway's initiate_oauth_flow: redirect to the IdP's own + // absolute authorization URL. + res.writeHead(302, { location: "https://idp.example.com/authorize?client_id=abc" }); + res.end(); + }); + await new Promise((resolve) => upstream.listen(0, "127.0.0.1", () => resolve())); + const { port } = upstream.address() as AddressInfo; + upstreamOrigin = `http://127.0.0.1:${port}`; + process.env.CONTEXTFORGE_URL = upstreamOrigin; +}); + +afterAll(() => new Promise((resolve) => upstream.close(() => resolve()))); + +async function buildApp() { + const { buildTestApp } = await import("./helpers/build-app.js"); + return buildTestApp({ withProxy: true }); +} + +async function seedSession(app: Awaited>) { + const { createSession } = await import("../src/lib/session-store.js"); + const sessionId = await createSession(app.redis as never, { + bearerToken: "test-bearer-token", // pragma: allowlist secret + user: { email: "user@example.com", isAdmin: false }, + }); + return { cookie: `bff_sid=${sessionId}` }; +} + +describe("GET /oauth/authorize/:gatewayId", () => { + it("401s without a session cookie", async () => { + const app = await buildApp(); + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/authorize/gw-1?popup=true", + }); + expect(response.statusCode).toBe(401); + }); + + it("injects the bearer token and forwards the provider redirect untouched", async () => { + const app = await buildApp(); + const { cookie } = await seedSession(app); + + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/authorize/gw-1?popup=true", + headers: { cookie }, + }); + + expect(response.statusCode).toBe(302); + // Must reach the OAuth provider directly -- rewriting this the way + // catch-all.ts rewrites upstream /api/* redirects would send the popup + // back into the BFF instead of out to the IdP. + expect(response.headers.location).toBe("https://idp.example.com/authorize?client_id=abc"); + expect(lastRequest?.path).toBe("/oauth/authorize/gw-1?popup=true"); + expect(lastRequest?.authorization).toBe("Bearer test-bearer-token"); + }); + + it("never lets the browser override the injected Authorization header", async () => { + const app = await buildApp(); + const { cookie } = await seedSession(app); + + await app.fastify.inject({ + method: "GET", + url: "/oauth/authorize/gw-1", + headers: { cookie, authorization: "Bearer attacker-supplied-token" }, // pragma: allowlist secret + }); + + expect(lastRequest?.authorization).toBe("Bearer test-bearer-token"); + }); + + it("rejects a cross-site request before calling upstream", async () => { + const app = await buildApp(); + const { cookie } = await seedSession(app); + lastRequest = undefined; + + // Same shape as password-reset.test.ts's cross-origin case: a mismatched + // Origin header is what a hostile page forcing + // window.open(`${victimOrigin}/oauth/authorize/`) would send. This + // route runs DCR registration and DB writes with the victim's injected + // bearer token, and can't rely on a CSRF token (window.open sets no + // headers), so it needs the same isForbiddenCrossOrigin guard as + // login.ts/proxy-sse.ts. + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/authorize/gw-1?popup=true", + headers: { cookie, host: "app.example.test", origin: "https://evil.example.test" }, + }); + + expect(response.statusCode).toBe(403); + expect(response.json()).toEqual({ error: "cross_site_request_forbidden" }); + // Rejected before ever reaching upstream. + expect(lastRequest).toBeUndefined(); + }); + + it("forwards a non-redirect upstream error response", async () => { + const app = await buildApp(); + const { cookie } = await seedSession(app); + + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/authorize/missing-config", + headers: { cookie }, + }); + + expect(response.statusCode).toBe(400); + expect(response.json()).toEqual({ detail: "Gateway is not configured for OAuth" }); + }); +}); diff --git a/server/test/oauth-callback.test.ts b/server/test/oauth-callback.test.ts new file mode 100644 index 0000000..a9a222a --- /dev/null +++ b/server/test/oauth-callback.test.ts @@ -0,0 +1,86 @@ +// Location: ./client/server/test/oauth-callback.test.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// CONTEXTFORGE_URL must be set before src/config.ts (and anything importing it) +// is first evaluated, so the fake upstream server is spun up and +// process.env.CONTEXTFORGE_URL set in beforeAll, with every module under test +// dynamic-imported afterwards rather than statically at the top of the file. + +import { createServer, type IncomingMessage, type Server } from "node:http"; +import type { AddressInfo } from "node:net"; + +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +let upstream: Server; +let upstreamOrigin: string; +let lastRequest: { path: string } | undefined; + +beforeAll(async () => { + upstream = createServer((req: IncomingMessage, res) => { + lastRequest = { path: req.url ?? "" }; + + // Mirrors mcpgateway's oauth_callback popup branch: an HTML page whose + // inline script posts the result to window.opener and closes itself. + res.writeHead(200, { + "content-type": "text/html", + // mcpgateway sets its own jwt_token cookie on the non-popup branch; + // must never reach the browser through this proxy. + "set-cookie": "jwt_token=upstream-secret; HttpOnly", // pragma: allowlist secret + }); + res.end( + "", + ); + }); + await new Promise((resolve) => upstream.listen(0, "127.0.0.1", () => resolve())); + const { port } = upstream.address() as AddressInfo; + upstreamOrigin = `http://127.0.0.1:${port}`; + process.env.CONTEXTFORGE_URL = upstreamOrigin; +}); + +afterAll(() => new Promise((resolve) => upstream.close(() => resolve()))); + +async function buildApp() { + const { buildTestApp } = await import("./helpers/build-app.js"); + return buildTestApp({ withProxy: true }); +} + +describe("GET /oauth/callback", () => { + it("proxies with no session cookie required", async () => { + const app = await buildApp(); + + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/callback?code=abc123&state=popup.xyz", + }); + + expect(response.statusCode).toBe(200); + expect(response.body).toContain("window.opener"); + expect(lastRequest?.path).toBe("/oauth/callback?code=abc123&state=popup.xyz"); + }); + + it("strips upstream Set-Cookie so mcpgateway's own cookie never reaches the browser", async () => { + const app = await buildApp(); + + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/callback?code=abc123&state=popup.xyz", + }); + + expect(response.headers["set-cookie"]).toBeUndefined(); + }); + + it("forwards an OAuth provider error callback", async () => { + const app = await buildApp(); + + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/callback?error=access_denied&error_description=User+cancelled&state=popup.xyz", + }); + + expect(response.statusCode).toBe(200); + expect(lastRequest?.path).toBe( + "/oauth/callback?error=access_denied&error_description=User+cancelled&state=popup.xyz", + ); + }); +}); diff --git a/src/components/mcp-servers/AdvancedSettings.test.tsx b/src/components/mcp-servers/AdvancedSettings.test.tsx index 21b4bbf..6991870 100644 --- a/src/components/mcp-servers/AdvancedSettings.test.tsx +++ b/src/components/mcp-servers/AdvancedSettings.test.tsx @@ -76,7 +76,6 @@ const makeProps = (overrides: Partial = {}): AdvancedSett onOAuthTokenUrlChange: vi.fn(), onOAuthGrantTypeChange: vi.fn(), onOAuthIssuerUrlChange: vi.fn(), - onOAuthRedirectUriChange: vi.fn(), onOAuthAuthorizationUrlChange: vi.fn(), onOAuthScopesChange: vi.fn(), onOAuthStoreTokensChange: vi.fn(), diff --git a/src/components/mcp-servers/AdvancedSettings.tsx b/src/components/mcp-servers/AdvancedSettings.tsx index 41b5dce..6b8ce1d 100644 --- a/src/components/mcp-servers/AdvancedSettings.tsx +++ b/src/components/mcp-servers/AdvancedSettings.tsx @@ -62,7 +62,6 @@ interface AdvancedSettingsProps { onOAuthTokenUrlChange: (value: string) => void; onOAuthGrantTypeChange: (value: string) => void; onOAuthIssuerUrlChange: (value: string) => void; - onOAuthRedirectUriChange: (value: string) => void; onOAuthAuthorizationUrlChange: (value: string) => void; onOAuthScopesChange: (value: string) => void; onOAuthStoreTokensChange: (checked: boolean) => void; @@ -115,7 +114,6 @@ export function AdvancedSettings({ onOAuthTokenUrlChange, onOAuthGrantTypeChange, onOAuthIssuerUrlChange, - onOAuthRedirectUriChange, onOAuthAuthorizationUrlChange, onOAuthScopesChange, onOAuthStoreTokensChange, @@ -180,7 +178,6 @@ export function AdvancedSettings({ onTokenUrlChange={onOAuthTokenUrlChange} onGrantTypeChange={onOAuthGrantTypeChange} onIssuerUrlChange={onOAuthIssuerUrlChange} - onRedirectUriChange={onOAuthRedirectUriChange} onAuthorizationUrlChange={onOAuthAuthorizationUrlChange} onScopesChange={onOAuthScopesChange} onStoreTokensChange={onOAuthStoreTokensChange} diff --git a/src/components/mcp-servers/MCPServerForm.tsx b/src/components/mcp-servers/MCPServerForm.tsx index cb329c3..0327487 100644 --- a/src/components/mcp-servers/MCPServerForm.tsx +++ b/src/components/mcp-servers/MCPServerForm.tsx @@ -1,4 +1,4 @@ -import { useCallback, useState, type ReactNode } from "react"; +import { useState, type ReactNode } from "react"; import { useIntl } from "react-intl"; import { ChevronDown } from "lucide-react"; import { Button } from "@/components/ui/button"; @@ -79,7 +79,6 @@ export function MCPServerForm({ isOpen, onToggle, serverId, onSuccess }: MCPServ oauthIssuerUrl, setOAuthIssuerUrl, oauthRedirectUri, - setOAuthRedirectUri, oauthAuthorizationUrl, setOAuthAuthorizationUrl, oauthScopes, @@ -98,13 +97,6 @@ export function MCPServerForm({ isOpen, onToggle, serverId, onSuccess }: MCPServ setQueryParamApiKey, } = useMCPServerForm(serverId); - const handleRedirectUriChange = useCallback( - (uri: string) => { - setOAuthRedirectUri(uri); - }, - [setOAuthRedirectUri], - ); - const handleCancel = () => { setCreatedGateway(null); onToggle(); @@ -358,7 +350,6 @@ export function MCPServerForm({ isOpen, onToggle, serverId, onSuccess }: MCPServ onOAuthTokenUrlChange={setOAuthTokenUrl} onOAuthGrantTypeChange={setOAuthGrantType} onOAuthIssuerUrlChange={setOAuthIssuerUrl} - onOAuthRedirectUriChange={handleRedirectUriChange} onOAuthAuthorizationUrlChange={setOAuthAuthorizationUrl} onOAuthScopesChange={setOAuthScopes} onOAuthStoreTokensChange={setOAuthStoreTokens} diff --git a/src/components/mcp-servers/OAuth2Auth.test.tsx b/src/components/mcp-servers/OAuth2Auth.test.tsx index 9674a51..c41ed99 100644 --- a/src/components/mcp-servers/OAuth2Auth.test.tsx +++ b/src/components/mcp-servers/OAuth2Auth.test.tsx @@ -19,7 +19,6 @@ describe("OAuth2Auth", () => { password: "", // pragma: allowlist secret onGrantTypeChange: vi.fn(), onIssuerUrlChange: vi.fn(), - onRedirectUriChange: vi.fn(), onClientIdChange: vi.fn(), onClientSecretChange: vi.fn(), onTokenUrlChange: vi.fn(), @@ -145,24 +144,24 @@ describe("OAuth2Auth", () => { expect(onPasswordChange).toHaveBeenCalledWith("test-pass"); }); - it("shows a read-only derived redirect URI, lifts it into form state, and triggers the authorization URL callback", () => { + it("shows an auto-placeholder (not window.location.origin) with no stored redirect URI", () => { const onAuthorizationUrlChange = vi.fn(); - const onRedirectUriChange = vi.fn(); render( , ); const redirect = screen.getByLabelText(/Redirect URI/i); expect(redirect).toHaveAttribute("readonly"); - expect(redirect).toHaveValue(`${window.location.origin}/oauth/callback`); - expect(screen.getByRole("button", { name: "Copy to clipboard" })).toBeInTheDocument(); - expect(onRedirectUriChange).toHaveBeenCalledWith(`${window.location.origin}/oauth/callback`); + // The web UI's own origin is not where the gateway serves /oauth/callback + // in a split deployment (mcp-context-forge#6458) -- must never display or + // submit it as a guess. + expect(redirect).not.toHaveValue(`${window.location.origin}/oauth/callback`); + expect(screen.queryByRole("button", { name: "Copy to clipboard" })).not.toBeInTheDocument(); fireEvent.change(screen.getByLabelText(/Authorization URL/i), { target: { value: "https://auth.com/authorize" }, @@ -170,36 +169,18 @@ describe("OAuth2Auth", () => { expect(onAuthorizationUrlChange).toHaveBeenCalledWith("https://auth.com/authorize"); }); - it("displays a stored redirect URI verbatim without overwriting it", () => { - const onRedirectUriChange = vi.fn(); - + it("displays a stored redirect URI verbatim", () => { render( , ); expect(screen.getByLabelText(/Redirect URI/i)).toHaveValue( "https://public.example.com/oauth/callback", ); - expect(onRedirectUriChange).not.toHaveBeenCalled(); - }); - - it("does not set a redirect URI for non-authorization_code grants", () => { - const onRedirectUriChange = vi.fn(); - - render( - , - ); - - expect(onRedirectUriChange).not.toHaveBeenCalled(); }); it("only offers the password grant option when already selected (legacy)", () => { @@ -236,8 +217,14 @@ describe("OAuth2Auth", () => { }); }); - it("copies the redirect URI to clipboard when the copy button is clicked", async () => { - render(); + it("copies a stored redirect URI to clipboard when the copy button is clicked", async () => { + render( + , + ); const copyButton = screen.getByRole("button", { name: /Copy to clipboard/i }); await act(async () => { @@ -245,14 +232,20 @@ describe("OAuth2Auth", () => { }); expect(navigator.clipboard.writeText).toHaveBeenCalledWith( - `${window.location.origin}/oauth/callback`, + "https://public.example.com/oauth/callback", ); }); it("shows a check icon immediately after clicking copy and reverts after 2 s", async () => { vi.useFakeTimers(); - render(); + render( + , + ); const copyButton = screen.getByRole("button", { name: /Copy to clipboard/i }); await act(async () => { @@ -271,13 +264,14 @@ describe("OAuth2Auth", () => { }); describe("localhost warning", () => { - it("shows a localhost warning when the derived redirect URI points to localhost", () => { - // jsdom sets window.location.origin to 'http://localhost' + it("does not show the localhost warning with no stored redirect URI, even though jsdom's own origin is localhost", () => { + // jsdom sets window.location.origin to 'http://localhost' -- must not + // leak into the warning now that nothing is derived from it. render(); expect( - screen.getByText(/Redirect URIs derived from localhost will not work/i), - ).toBeInTheDocument(); + screen.queryByText(/Redirect URIs derived from localhost will not work/i), + ).not.toBeInTheDocument(); }); it("does not show the localhost warning when a non-localhost stored redirect URI is used", () => { diff --git a/src/components/mcp-servers/OAuth2Auth.tsx b/src/components/mcp-servers/OAuth2Auth.tsx index 6e2f591..6fd81b4 100644 --- a/src/components/mcp-servers/OAuth2Auth.tsx +++ b/src/components/mcp-servers/OAuth2Auth.tsx @@ -2,7 +2,7 @@ import { useIntl } from "react-intl"; import { Input } from "@/components/ui/input"; import { Textarea } from "@/components/ui/textarea"; import { Checkbox } from "@/components/ui/checkbox"; -import { useEffect, useState } from "react"; +import { useState } from "react"; import { Check, Copy } from "lucide-react"; import { Button } from "@/components/ui/button"; import { @@ -28,7 +28,6 @@ interface OAuth2AuthProps { password: string; // pragma: allowlist secret onGrantTypeChange: (value: string) => void; onIssuerUrlChange: (value: string) => void; - onRedirectUriChange: (value: string) => void; onClientIdChange: (value: string) => void; onClientSecretChange: (value: string) => void; onTokenUrlChange: (value: string) => void; @@ -56,7 +55,6 @@ export function OAuth2Auth({ password, onGrantTypeChange, onIssuerUrlChange, - onRedirectUriChange, onClientIdChange, onClientSecretChange, onTokenUrlChange, @@ -69,23 +67,22 @@ export function OAuth2Auth({ errors, }: OAuth2AuthProps) { const intl = useIntl(); - const derivedRedirectUri = `${window.location.origin}/oauth/callback`; - const displayRedirectUri = redirectUri || derivedRedirectUri; - const isLocalRedirect = /^https?:\/\/(localhost|127\.0\.0\.1|\[::1\])(:|\/|$)/i.test( - displayRedirectUri, - ); + // Deliberately NOT derived from window.location.origin: the browser's own + // address is the web UI's origin, but the OAuth callback is served by the + // gateway (mcpgateway) at its own configured APP_DOMAIN, which can differ + // in any split deployment. Guessing wrong here means registering the wrong + // redirect URI with the OAuth provider with no warning (see + // mcp-context-forge#6458). When the operator hasn't set one, leave + // redirect_uri unsubmitted (see useMCPServerForm.ts) so the gateway's own + // default (based on its APP_DOMAIN) applies server-side instead. + const hasStoredRedirectUri = Boolean(redirectUri); + const isLocalRedirect = + hasStoredRedirectUri && + /^https?:\/\/(localhost|127\.0\.0\.1|\[::1\])(:|\/|$)/i.test(redirectUri); const [copied, setCopied] = useState(false); - // The displayed URI is what the OAuth app is registered with, so it has to be the value we - // store and send to the IdP — a display-only derivation submits no redirect_uri at all. - useEffect(() => { - if (grantType === "authorization_code" && !redirectUri) { - onRedirectUriChange(derivedRedirectUri); - } - }, [grantType, redirectUri, derivedRedirectUri, onRedirectUriChange]); - const handleCopyRedirect = () => { - void navigator.clipboard?.writeText(displayRedirectUri); + void navigator.clipboard?.writeText(redirectUri); setCopied(true); window.setTimeout(() => setCopied(false), 2000); }; @@ -161,27 +158,44 @@ export function OAuth2Auth({ > {intl.formatMessage({ id: "mcpServer.auth.oauth.redirectUriLabel" })} -
+ {hasStoredRedirectUri ? ( + <> +
+ + +
+

+ {intl.formatMessage({ id: "mcpServer.auth.oauth.redirectUriHelp" })} +

+ + ) : ( - -
-

- {intl.formatMessage({ id: "mcpServer.auth.oauth.redirectUriHelp" })} -

+ )} + {!hasStoredRedirectUri && ( +

+ {intl.formatMessage({ id: "mcpServer.auth.oauth.redirectUriAutoHelp" })} +

+ )} {isLocalRedirect && (

{intl.formatMessage({ id: "mcpServer.auth.oauth.redirectUriLocalWarning" })} diff --git a/src/i18n/locales/en-US/mcpServer.json b/src/i18n/locales/en-US/mcpServer.json index 0066e83..e6d0d6b 100644 --- a/src/i18n/locales/en-US/mcpServer.json +++ b/src/i18n/locales/en-US/mcpServer.json @@ -186,6 +186,8 @@ "mcpServer.auth.oauth.redirectUriLabel": "Redirect URI", "mcpServer.auth.oauth.redirectUriCopy": "Copy to clipboard", "mcpServer.auth.oauth.redirectUriHelp": "Configure your OAuth app to use this redirect URI.", + "mcpServer.auth.oauth.redirectUriAutoPlaceholder": "Determined automatically by the server", + "mcpServer.auth.oauth.redirectUriAutoHelp": "The gateway fills this in from its own configured public URL (APP_DOMAIN) when authorization starts. Set one explicitly only if this gateway needs a different redirect URI registered with the provider.", "mcpServer.auth.oauth.redirectUriLocalWarning": "The server's public URL is not configured. Redirect URIs derived from localhost will not work for external OAuth providers.", "mcpServer.auth.oauth.usernameLabel": "Username", "mcpServer.auth.oauth.usernamePlaceholder": "e.g. service-account", diff --git a/src/i18n/locales/es-ES/mcpServer.json b/src/i18n/locales/es-ES/mcpServer.json index baaf14f..bb95bd0 100644 --- a/src/i18n/locales/es-ES/mcpServer.json +++ b/src/i18n/locales/es-ES/mcpServer.json @@ -186,6 +186,8 @@ "mcpServer.auth.oauth.redirectUriLabel": "URI de redirección", "mcpServer.auth.oauth.redirectUriCopy": "Copiar al portapapeles", "mcpServer.auth.oauth.redirectUriHelp": "Configure su aplicación OAuth para usar esta URI de redirección.", + "mcpServer.auth.oauth.redirectUriAutoPlaceholder": "Determinado automáticamente por el servidor", + "mcpServer.auth.oauth.redirectUriAutoHelp": "El gateway completa este valor a partir de su propia URL pública configurada (APP_DOMAIN) cuando se inicia la autorización. Configure uno explícitamente solo si este gateway necesita una URI de redirección diferente registrada con el proveedor.", "mcpServer.auth.oauth.redirectUriLocalWarning": "La URL pública del servidor no está configurada. Las URI de redirección derivadas de localhost no funcionarán con proveedores OAuth externos.", "mcpServer.auth.oauth.usernameLabel": "Nombre de usuario", "mcpServer.auth.oauth.usernamePlaceholder": "p. ej. service-account", diff --git a/src/i18n/locales/pt-BR/mcpServer.json b/src/i18n/locales/pt-BR/mcpServer.json index a70cee4..fef90ae 100644 --- a/src/i18n/locales/pt-BR/mcpServer.json +++ b/src/i18n/locales/pt-BR/mcpServer.json @@ -186,6 +186,8 @@ "mcpServer.auth.oauth.redirectUriLabel": "URI de redirecionamento", "mcpServer.auth.oauth.redirectUriCopy": "Copiar para a área de transferência", "mcpServer.auth.oauth.redirectUriHelp": "Configure seu aplicativo OAuth para usar esta URI de redirecionamento.", + "mcpServer.auth.oauth.redirectUriAutoPlaceholder": "Determinado automaticamente pelo servidor", + "mcpServer.auth.oauth.redirectUriAutoHelp": "O gateway preenche este valor a partir de sua própria URL pública configurada (APP_DOMAIN) quando a autorização é iniciada. Defina um valor explicitamente apenas se este gateway precisar de uma URI de redirecionamento diferente registrada no provedor.", "mcpServer.auth.oauth.redirectUriLocalWarning": "A URL pública do servidor não está configurada. URIs de redirecionamento derivadas de localhost não funcionarão com provedores OAuth externos.", "mcpServer.auth.oauth.usernameLabel": "Nome de usuário", "mcpServer.auth.oauth.usernamePlaceholder": "ex.: service-account",