From 1a83297bc675f85fbce23642032bb8f0c96ad8ac Mon Sep 17 00:00:00 2001 From: Marek Dano Date: Thu, 3 Sep 2026 14:25:53 +0100 Subject: [PATCH 1/6] fix: proxy /oauth/* through the BFF and stop guessing redirect_uri Signed-off-by: Marek Dano --- e2e/oauth-authorization.spec.ts | 168 ++++++++++++++++++ server/src/config.ts | 11 ++ server/src/index.ts | 4 + server/src/lib/oauth-upstream-forward.ts | 67 +++++++ server/src/routes/proxy/oauth-authorize.ts | 77 ++++++++ server/src/routes/proxy/oauth-callback.ts | 55 ++++++ server/test/helpers/build-app.ts | 4 + server/test/oauth-authorize.test.ts | 140 +++++++++++++++ server/test/oauth-callback.test.ts | 86 +++++++++ .../mcp-servers/AdvancedSettings.test.tsx | 1 - .../mcp-servers/AdvancedSettings.tsx | 3 - src/components/mcp-servers/MCPServerForm.tsx | 11 +- .../mcp-servers/OAuth2Auth.test.tsx | 62 +++---- src/components/mcp-servers/OAuth2Auth.tsx | 80 +++++---- src/i18n/locales/en-US/mcpServer.json | 2 + src/i18n/locales/es-ES/mcpServer.json | 2 + src/i18n/locales/pt-BR/mcpServer.json | 2 + 17 files changed, 694 insertions(+), 81 deletions(-) create mode 100644 e2e/oauth-authorization.spec.ts create mode 100644 server/src/lib/oauth-upstream-forward.ts create mode 100644 server/src/routes/proxy/oauth-authorize.ts create mode 100644 server/src/routes/proxy/oauth-callback.ts create mode 100644 server/test/oauth-authorize.test.ts create mode 100644 server/test/oauth-callback.test.ts diff --git a/e2e/oauth-authorization.spec.ts b/e2e/oauth-authorization.spec.ts new file mode 100644 index 00000000..d43bf184 --- /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 5439f671..1e3b4ee3 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 9c8d5ba5..5225ff69 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 00000000..955c85d8 --- /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 00000000..403d21df --- /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 00000000..c23cdaf1 --- /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 716d9572..05b1a213 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 00000000..daa5801f --- /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 00000000..a9a222aa --- /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 21b4bbfe..69918709 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 41b5dceb..6b8ce1d9 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 501002c2..6cc67cfa 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"; @@ -93,7 +93,6 @@ export function MCPServerForm({ isOpen, onToggle, serverId, onSuccess }: MCPServ oauthIssuerUrl, setOAuthIssuerUrl, oauthRedirectUri, - setOAuthRedirectUri, oauthAuthorizationUrl, setOAuthAuthorizationUrl, oauthScopes, @@ -112,13 +111,6 @@ export function MCPServerForm({ isOpen, onToggle, serverId, onSuccess }: MCPServ setQueryParamApiKey, } = useMCPServerForm(serverId, prefill); - const handleRedirectUriChange = useCallback( - (uri: string) => { - setOAuthRedirectUri(uri); - }, - [setOAuthRedirectUri], - ); - const handleCancel = () => { setCreatedGateway(null); onToggle(); @@ -392,7 +384,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 9674a518..c41ed996 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 914c5531..a1d74d9f 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 3316e127..f4ff7fab 100644 --- a/src/i18n/locales/en-US/mcpServer.json +++ b/src/i18n/locales/en-US/mcpServer.json @@ -194,6 +194,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 1ca2a95d..86f6ce43 100644 --- a/src/i18n/locales/es-ES/mcpServer.json +++ b/src/i18n/locales/es-ES/mcpServer.json @@ -194,6 +194,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 e0687e77..b0879336 100644 --- a/src/i18n/locales/pt-BR/mcpServer.json +++ b/src/i18n/locales/pt-BR/mcpServer.json @@ -194,6 +194,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", From 980d86add21438191fb375b6251fa0af80a54354 Mon Sep 17 00:00:00 2001 From: Marek Dano Date: Mon, 7 Sep 2026 09:10:48 +0100 Subject: [PATCH 2/6] fix: postMessage popup errors and drop APP_DOMAIN from redirect URI copy - Wire an onSend hook into the /oauth/authorize and /oauth/callback proxy routes so the three failures the BFF itself can produce (401 session, 403 cross-site, 502 upstream unreachable) post an oauth_callback error and close the popup instead of rendering raw JSON, matching mcpgateway's own callback HTML shape. Without this, triggerOAuthAuthorization never resolved or rejected until the user closed the popup by hand, then wrongly reported "cancelled". - Reword redirectUriAutoHelp/redirectUriLocalWarning (en-US/es-ES/pt-BR): drop the internal APP_DOMAIN term, and fix redirectUriLocalWarning's wording now that it only fires for an explicitly stored localhost URI, not a derived one. Addresses review feedback on #101 Signed-off-by: Marek Dano --- server/src/lib/oauth-upstream-forward.ts | 56 +++++++++++++++++++ server/src/routes/proxy/oauth-authorize.ts | 4 +- server/src/routes/proxy/oauth-callback.ts | 26 +++++---- server/test/oauth-authorize.test.ts | 37 +++++++++++- server/test/oauth-callback.test.ts | 22 ++++++++ .../mcp-servers/OAuth2Auth.test.tsx | 8 +-- src/i18n/locales/en-US/mcpServer.json | 4 +- src/i18n/locales/es-ES/mcpServer.json | 4 +- src/i18n/locales/pt-BR/mcpServer.json | 4 +- 9 files changed, 138 insertions(+), 27 deletions(-) diff --git a/server/src/lib/oauth-upstream-forward.ts b/server/src/lib/oauth-upstream-forward.ts index 955c85d8..afef4037 100644 --- a/server/src/lib/oauth-upstream-forward.ts +++ b/server/src/lib/oauth-upstream-forward.ts @@ -17,9 +17,65 @@ // 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. +// +// htmlizeOAuthPopupErrors turns the JSON errors the BFF itself can produce on +// these two routes (401 from sessionAuth, 403 from isForbiddenCrossOrigin, +// 502 from the fetch failure below) into the same postMessage-and-close HTML +// shape mcpgateway's own callback page uses on success. Without this, those +// three failures leave the popup rendering raw JSON: nothing posts a +// message, so triggerOAuthAuthorization (src/api/servers.ts) never resolves +// or rejects until the user closes the popup by hand, at which point it +// reports "cancelled" -- which isn't what happened. Registered as this +// route's `onSend` hook, which still runs (and can still rewrite the +// payload) even though sessionAuth's preHandler is what called reply.send(). +// Upstream (mcpgateway) error responses forwarded as-is are untouched here -- +// they're whatever status/body mcpgateway itself chose to send, not one of +// these three BFF-generated cases. import type { FastifyReply, FastifyRequest } from "fastify"; +const OAUTH_POPUP_ERROR_MESSAGES: Record = { + 401: { + error: "unauthenticated", + errorDescription: "Your session has expired. Please sign in again and retry.", + }, + 403: { + error: "cross_site_request_forbidden", + errorDescription: "This request could not be verified. Please retry from the original page.", + }, + 502: { + error: "upstream_unavailable", + errorDescription: "The OAuth provider could not be reached. Please try again.", + }, +}; + +function oauthPopupErrorHtml(error: string, errorDescription: string): string { + const payload = JSON.stringify({ + type: "oauth_callback", + status: "error", + error, + errorDescription, + }); + return ``; +} + +export async function htmlizeOAuthPopupErrors( + _request: FastifyRequest, + reply: FastifyReply, + payload: unknown, +): Promise { + const mapped = OAUTH_POPUP_ERROR_MESSAGES[reply.statusCode]; + if (!mapped) return payload; + + reply.header("content-type", "text/html; charset=utf-8"); + return oauthPopupErrorHtml(mapped.error, mapped.errorDescription); +} + interface ForwardOAuthGetOptions { /** Extra headers merged into the upstream request (e.g. the injected bearer token). */ headers?: Record; diff --git a/server/src/routes/proxy/oauth-authorize.ts b/server/src/routes/proxy/oauth-authorize.ts index 403d21df..1aae6765 100644 --- a/server/src/routes/proxy/oauth-authorize.ts +++ b/server/src/routes/proxy/oauth-authorize.ts @@ -42,7 +42,7 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; import { config } from "../../config.js"; -import { forwardOAuthGet } from "../../lib/oauth-upstream-forward.js"; +import { forwardOAuthGet, htmlizeOAuthPopupErrors } 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"; @@ -54,7 +54,7 @@ interface AuthorizeParams { export default async function oauthAuthorizeProxyRoute(fastify: FastifyInstance): Promise { fastify.get<{ Params: AuthorizeParams }>( "/oauth/authorize/:gatewayId", - { preHandler: fastify.sessionAuth }, + { preHandler: fastify.sessionAuth, onSend: htmlizeOAuthPopupErrors }, async (request: FastifyRequest<{ Params: AuthorizeParams }>, reply: FastifyReply) => { setNoStore(reply); diff --git a/server/src/routes/proxy/oauth-callback.ts b/server/src/routes/proxy/oauth-callback.ts index c23cdaf1..0a825e33 100644 --- a/server/src/routes/proxy/oauth-callback.ts +++ b/server/src/routes/proxy/oauth-callback.ts @@ -36,20 +36,24 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; import { config } from "../../config.js"; -import { forwardOAuthGet } from "../../lib/oauth-upstream-forward.js"; +import { forwardOAuthGet, htmlizeOAuthPopupErrors } 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); + fastify.get( + "/oauth/callback", + { onSend: htmlizeOAuthPopupErrors }, + 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}`; + 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", - }); - }); + return forwardOAuthGet(request, reply, upstreamUrl, { + timeoutMs: config.oauthProxyTimeoutMs, + logLabel: "OAuth callback", + }); + }, + ); } diff --git a/server/test/oauth-authorize.test.ts b/server/test/oauth-authorize.test.ts index daa5801f..ecfa5996 100644 --- a/server/test/oauth-authorize.test.ts +++ b/server/test/oauth-authorize.test.ts @@ -31,6 +31,14 @@ beforeAll(async () => { return; } + if (req.url?.startsWith("/oauth/authorize/network-error")) { + // No response at all -- destroying the socket is what makes fetch() + // reject inside forwardOAuthGet's catch, the actual trigger for its + // 502, as opposed to a well-formed non-2xx upstream response. + req.socket.destroy(); + 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" }); @@ -59,13 +67,18 @@ async function seedSession(app: Awaited>) { } describe("GET /oauth/authorize/:gatewayId", () => { - it("401s without a session cookie", async () => { + it("401s without a session cookie, posting an oauth_callback error the opener can read", 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); + expect(response.headers["content-type"]).toContain("text/html"); + expect(response.body).toContain("window.opener"); + expect(response.body).toContain('"type":"oauth_callback"'); + expect(response.body).toContain('"status":"error"'); + expect(response.body).toContain("window.close()"); }); it("injects the bearer token and forwards the provider redirect untouched", async () => { @@ -119,12 +132,14 @@ describe("GET /oauth/authorize/:gatewayId", () => { }); expect(response.statusCode).toBe(403); - expect(response.json()).toEqual({ error: "cross_site_request_forbidden" }); + expect(response.headers["content-type"]).toContain("text/html"); + expect(response.body).toContain('"error":"cross_site_request_forbidden"'); + expect(response.body).toContain("window.close()"); // Rejected before ever reaching upstream. expect(lastRequest).toBeUndefined(); }); - it("forwards a non-redirect upstream error response", async () => { + it("forwards a non-redirect upstream error response as-is, not the popup HTML shape", async () => { const app = await buildApp(); const { cookie } = await seedSession(app); @@ -137,4 +152,20 @@ describe("GET /oauth/authorize/:gatewayId", () => { expect(response.statusCode).toBe(400); expect(response.json()).toEqual({ detail: "Gateway is not configured for OAuth" }); }); + + it("posts an oauth_callback error instead of raw JSON when the upstream connection fails", async () => { + const app = await buildApp(); + const { cookie } = await seedSession(app); + + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/authorize/network-error", + headers: { cookie }, + }); + + expect(response.statusCode).toBe(502); + expect(response.headers["content-type"]).toContain("text/html"); + expect(response.body).toContain('"error":"upstream_unavailable"'); + expect(response.body).toContain("window.close()"); + }); }); diff --git a/server/test/oauth-callback.test.ts b/server/test/oauth-callback.test.ts index a9a222aa..bfc232da 100644 --- a/server/test/oauth-callback.test.ts +++ b/server/test/oauth-callback.test.ts @@ -20,6 +20,14 @@ beforeAll(async () => { upstream = createServer((req: IncomingMessage, res) => { lastRequest = { path: req.url ?? "" }; + if (req.url?.startsWith("/oauth/callback?network-error")) { + // No response at all -- destroying the socket is what makes fetch() + // reject inside forwardOAuthGet's catch, the actual trigger for its + // 502, as opposed to a well-formed non-2xx upstream response. + req.socket.destroy(); + return; + } + // 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, { @@ -70,6 +78,20 @@ describe("GET /oauth/callback", () => { expect(response.headers["set-cookie"]).toBeUndefined(); }); + it("posts an oauth_callback error instead of raw JSON when the upstream connection fails", async () => { + const app = await buildApp(); + + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/callback?network-error=1&state=popup.xyz", + }); + + expect(response.statusCode).toBe(502); + expect(response.headers["content-type"]).toContain("text/html"); + expect(response.body).toContain('"error":"upstream_unavailable"'); + expect(response.body).toContain("window.close()"); + }); + it("forwards an OAuth provider error callback", async () => { const app = await buildApp(); diff --git a/src/components/mcp-servers/OAuth2Auth.test.tsx b/src/components/mcp-servers/OAuth2Auth.test.tsx index c41ed996..cec55211 100644 --- a/src/components/mcp-servers/OAuth2Auth.test.tsx +++ b/src/components/mcp-servers/OAuth2Auth.test.tsx @@ -270,7 +270,7 @@ describe("OAuth2Auth", () => { render(); expect( - screen.queryByText(/Redirect URIs derived from localhost will not work/i), + screen.queryByText(/will not work for external OAuth providers/i), ).not.toBeInTheDocument(); }); @@ -284,7 +284,7 @@ describe("OAuth2Auth", () => { ); expect( - screen.queryByText(/Redirect URIs derived from localhost will not work/i), + screen.queryByText(/will not work for external OAuth providers/i), ).not.toBeInTheDocument(); }); @@ -297,9 +297,7 @@ describe("OAuth2Auth", () => { />, ); - expect( - screen.getByText(/Redirect URIs derived from localhost will not work/i), - ).toBeInTheDocument(); + expect(screen.getByText(/will not work for external OAuth providers/i)).toBeInTheDocument(); }); }); }); diff --git a/src/i18n/locales/en-US/mcpServer.json b/src/i18n/locales/en-US/mcpServer.json index f4ff7fab..a2e74be5 100644 --- a/src/i18n/locales/en-US/mcpServer.json +++ b/src/i18n/locales/en-US/mcpServer.json @@ -195,8 +195,8 @@ "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.redirectUriAutoHelp": "Set automatically from the server's public URL when authorization starts. Set one explicitly only if this gateway needs a different redirect URI registered with the provider.", + "mcpServer.auth.oauth.redirectUriLocalWarning": "This redirect URI points at localhost, which will not work for external OAuth providers.", "mcpServer.auth.oauth.usernameLabel": "Username", "mcpServer.auth.oauth.usernamePlaceholder": "e.g. service-account", "mcpServer.auth.oauth.passwordLabel": "Password", diff --git a/src/i18n/locales/es-ES/mcpServer.json b/src/i18n/locales/es-ES/mcpServer.json index 86f6ce43..a91ce727 100644 --- a/src/i18n/locales/es-ES/mcpServer.json +++ b/src/i18n/locales/es-ES/mcpServer.json @@ -195,8 +195,8 @@ "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.redirectUriAutoHelp": "Se configura automáticamente a partir de la URL pública del servidor 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": "Esta URI de redirección apunta a localhost, lo cual no funcionará con proveedores OAuth externos.", "mcpServer.auth.oauth.usernameLabel": "Nombre de usuario", "mcpServer.auth.oauth.usernamePlaceholder": "p. ej. service-account", "mcpServer.auth.oauth.passwordLabel": "Contraseña", diff --git a/src/i18n/locales/pt-BR/mcpServer.json b/src/i18n/locales/pt-BR/mcpServer.json index b0879336..4e069ae7 100644 --- a/src/i18n/locales/pt-BR/mcpServer.json +++ b/src/i18n/locales/pt-BR/mcpServer.json @@ -195,8 +195,8 @@ "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.redirectUriAutoHelp": "Definido automaticamente a partir da URL pública do servidor 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": "Esta URI de redirecionamento aponta para localhost, o que não funcionará com provedores OAuth externos.", "mcpServer.auth.oauth.usernameLabel": "Nome de usuário", "mcpServer.auth.oauth.usernamePlaceholder": "ex.: service-account", "mcpServer.auth.oauth.passwordLabel": "Senha", From 58264b7f64fe896b56eba06233a58b8d16da8547 Mon Sep 17 00:00:00 2001 From: Marek Dano Date: Mon, 7 Sep 2026 09:22:00 +0100 Subject: [PATCH 3/6] fix: lint issue Signed-off-by: Marek Dano --- src/components/mcp-servers/MCPServerForm.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/mcp-servers/MCPServerForm.tsx b/src/components/mcp-servers/MCPServerForm.tsx index 6cc67cfa..77d46765 100644 --- a/src/components/mcp-servers/MCPServerForm.tsx +++ b/src/components/mcp-servers/MCPServerForm.tsx @@ -1,4 +1,4 @@ -import { useState, type ReactNode } from "react"; +import { useCallback, useState, type ReactNode } from "react"; import { useIntl } from "react-intl"; import { ChevronDown } from "lucide-react"; import { Button } from "@/components/ui/button"; From 29f0de40554f5d11da84dd7b7a6666b376ddc71e Mon Sep 17 00:00:00 2001 From: Marek Dano Date: Mon, 7 Sep 2026 16:20:03 +0100 Subject: [PATCH 4/6] fix: nonce-gate OAuth authorize, default redirect_uri to BFF callback - Require a single-use, session-bound nonce (POST /oauth/authorize-nonce, CSRF-protected) before GET /oauth/authorize/:gatewayId will proceed. isForbiddenCrossOrigin alone let a same-site sibling subdomain through -- a top-level GET navigation carries no Origin header and reports Sec-Fetch-Site: same-site (not cross-site), so it still rode the victim's SameSite=Lax session cookie into DCR registration and DB writes. The nonce closes that: it can only be minted by a same-origin, CSRF-protected POST a hostile sibling has no way to forge. - Add GET /oauth/callback-url so the SPA can default a new gateway's redirect_uri to this deployment's own /oauth/callback proxy (derived server-side via origin-guard.ts's resolvePublicOrigin, not window.location.origin) instead of leaving it unset. Unset relied on mcpgateway's own APP_DOMAIN default, unreachable in a split deployment where only the web UI is public-facing. - Catch a body-read failure in forwardOAuthGet after upstream headers arrive (e.g. the connection drops mid-response), returning the same postMessage-and-close 502 shape instead of an uncaught 500. - Fix triggerOAuthAuthorization's nonce-mint call to send {} instead of an empty body under Content-Type: application/json, which Fastify's default JSON parser rejects before the route ever runs -- same fix /auth/logout already needed for the same reason. Addresses review feedback on #101 Signed-off-by: Marek Dano --- e2e/oauth-authorization.spec.ts | 74 +++++++++ server/src/config.ts | 15 ++ server/src/index.ts | 4 + server/src/lib/oauth-authorize-nonce.ts | 58 +++++++ server/src/lib/oauth-upstream-forward.ts | 21 ++- server/src/lib/origin-guard.ts | 15 +- .../src/routes/proxy/oauth-authorize-nonce.ts | 29 ++++ server/src/routes/proxy/oauth-authorize.ts | 43 +++++- server/src/routes/proxy/oauth-callback-url.ts | 41 +++++ server/test/helpers/build-app.ts | 4 + server/test/oauth-authorize-nonce.test.ts | 144 ++++++++++++++++++ server/test/oauth-authorize.test.ts | 127 +++++++++++++-- server/test/oauth-callback-url.test.ts | 82 ++++++++++ server/test/oauth-callback.test.ts | 25 +++ src/api/client.ts | 12 +- src/api/servers.test.ts | 74 +++++++-- src/api/servers.ts | 36 ++++- src/components/mcp-servers/OAuth2Auth.tsx | 18 ++- src/hooks/useMCPServerForm.ts | 23 +++ 19 files changed, 799 insertions(+), 46 deletions(-) create mode 100644 server/src/lib/oauth-authorize-nonce.ts create mode 100644 server/src/routes/proxy/oauth-authorize-nonce.ts create mode 100644 server/src/routes/proxy/oauth-callback-url.ts create mode 100644 server/test/oauth-authorize-nonce.test.ts create mode 100644 server/test/oauth-callback-url.test.ts diff --git a/e2e/oauth-authorization.spec.ts b/e2e/oauth-authorization.spec.ts index d43bf184..4171b34b 100644 --- a/e2e/oauth-authorization.spec.ts +++ b/e2e/oauth-authorization.spec.ts @@ -63,6 +63,17 @@ test.describe("OAuth authorization-code popup flow", () => { }); }); + // triggerOAuthAuthorization mints this before navigating the popup (see + // src/api/servers.ts) -- a same-origin, CSRF-protected POST the popup's + // own window.open() navigation can't carry itself. + await page.route("**/oauth/authorize-nonce", async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ nonce: "e2e-test-nonce" }), + }); + }); + await page.route(`**/gateways/${GATEWAY_ID}/state*`, async (route) => { await route.fulfill({ status: 200, @@ -143,6 +154,14 @@ test.describe("OAuth authorization-code popup flow", () => { }); }); + await page.route("**/oauth/authorize-nonce", async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ nonce: "e2e-test-nonce" }), + }); + }); + await page.goto(APP.SERVERS); await page.waitForLoadState("networkidle"); @@ -165,4 +184,59 @@ test.describe("OAuth authorization-code popup flow", () => { // The form must stay open on error so the user can see it and retry. await expect(page.getByRole("button", { name: "Connect server" })).toBeVisible(); }); + + test("defaults redirect_uri to the BFF's own callback URL and submits it -- the split-deployment case", async ({ + page, + }) => { + // Stands in for server/src/routes/proxy/oauth-callback-url.ts's real + // response: a public origin distinct from this page's own origin, the + // way it would differ when mcpgateway itself isn't independently + // browser-reachable (see that route's doc comment for why the field + // can't just be left unset in that topology). + const BFF_CALLBACK_URL = "https://web.example.com/oauth/callback"; + await page.route("**/oauth/callback-url", async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ redirectUri: BFF_CALLBACK_URL }), + }); + }); + + const createRequest = page.waitForRequest( + (request) => request.url().includes("/gateways") && request.method() === "POST", + ); + 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 expect(page.getByLabel(/Redirect URI/i)).toHaveValue(BFF_CALLBACK_URL); + + 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(); + + const request = await createRequest; + const body = request.postDataJSON() as { oauth_config?: { redirect_uri?: string } }; + expect(body.oauth_config?.redirect_uri).toBe(BFF_CALLBACK_URL); + }); }); diff --git a/server/src/config.ts b/server/src/config.ts index 1e3b4ee3..ecd747f0 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -44,6 +44,14 @@ export const config = { // headroom than a plain API call needs. oauthProxyTimeoutMs: Number(optional("OAUTH_PROXY_TIMEOUT_MS", "30000")), + // TTL for the one-time nonce minted by POST /oauth/authorize-nonce and + // required by GET /oauth/authorize/:gatewayId (see + // lib/oauth-authorize-nonce.ts). Short-lived on purpose: the SPA consumes + // it within milliseconds of minting it, so this only needs to cover + // however long a client can plausibly sit on a minted-but-unused nonce + // (e.g. a popup blocked before it navigates), not the OAuth flow itself. + oauthAuthorizeNonceTtlSeconds: Number(optional("OAUTH_AUTHORIZE_NONCE_TTL_SECONDS", "120")), + // 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 @@ -115,6 +123,13 @@ if (!Number.isSafeInteger(config.oauthProxyTimeoutMs) || config.oauthProxyTimeou throw new Error("OAUTH_PROXY_TIMEOUT_MS must be a positive integer"); } +if ( + !Number.isSafeInteger(config.oauthAuthorizeNonceTtlSeconds) || + config.oauthAuthorizeNonceTtlSeconds <= 0 +) { + throw new Error("OAUTH_AUTHORIZE_NONCE_TTL_SECONDS 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 5225ff69..69270114 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -24,7 +24,9 @@ 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 oauthAuthorizeNonceRoute from "./routes/proxy/oauth-authorize-nonce.js"; import oauthCallbackProxyRoute from "./routes/proxy/oauth-callback.js"; +import oauthCallbackUrlRoute from "./routes/proxy/oauth-callback-url.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"; @@ -51,8 +53,10 @@ await fastify.register(sessionRoute); await fastify.register(changePasswordRequiredRoute); await fastify.register(sseRoutes); await fastify.register(publicPasswordResetRoute); +await fastify.register(oauthAuthorizeNonceRoute); await fastify.register(oauthAuthorizeProxyRoute); await fastify.register(oauthCallbackProxyRoute); +await fastify.register(oauthCallbackUrlRoute); await fastify.register(catchAllProxyRoute); await fastify.register(appRoute); diff --git a/server/src/lib/oauth-authorize-nonce.ts b/server/src/lib/oauth-authorize-nonce.ts new file mode 100644 index 00000000..a1f96d68 --- /dev/null +++ b/server/src/lib/oauth-authorize-nonce.ts @@ -0,0 +1,58 @@ +// Location: ./client/server/src/lib/oauth-authorize-nonce.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// One-time, session-bound nonce gating GET /oauth/authorize/:gatewayId (see +// routes/proxy/oauth-authorize-nonce.ts, routes/proxy/oauth-authorize.ts). +// +// isForbiddenCrossOrigin alone isn't enough on that route: window.open()'s +// top-level navigation carries no Origin header (GET navigations don't send +// one -- see origin-guard.ts), so the guard falls back to Sec-Fetch-Site, +// which reports "same-site" -- not "cross-site" -- for a request from a +// hostile *sibling* subdomain under the same registrable domain (e.g. +// evil.example.com against app.example.com). That sibling still rides the +// victim's SameSite=Lax session cookie on a top-level GET, so without this +// nonce it could otherwise trigger DCR registration and DB writes against a +// gateway of its choosing using the victim's session. +// +// The fix: mint the nonce only from a same-origin, CSRF-protected POST +// (fastify.csrfProtection -- the plugin already used for every other +// mutating browser->BFF call). A hostile sibling subdomain can't forge that +// POST: it doesn't have the CSRF token, which is handed to the SPA only in +// the JSON body of /auth/login and /auth/session, readable by same-origin +// script alone. The authorize route then requires this nonce and consumes +// it, so a captured or guessed authorize URL is usable at most once, and +// only by the session that minted it. + +import { randomUUID } from "node:crypto"; + +import { config } from "../config.js"; +import type { RedisLike } from "./session-store.js"; + +function nonceRedisKey(nonce: string): string { + return `${config.redisKeyPrefix}:oauth-authorize-nonce:${nonce}`; +} + +export async function mintOAuthAuthorizeNonce( + redis: RedisLike, + sessionId: string, +): Promise { + const nonce = randomUUID(); + await redis.setex(nonceRedisKey(nonce), config.oauthAuthorizeNonceTtlSeconds, sessionId); + return nonce; +} + +// Deletes the nonce whether or not it matches -- a captured query string +// (browser history, a proxy access log, a copy-pasted URL) must not be +// replayable even by the same session that minted it. +export async function consumeOAuthAuthorizeNonce( + redis: RedisLike, + sessionId: string, + nonce: string | undefined, +): Promise { + if (!nonce) return false; + const key = nonceRedisKey(nonce); + const mintedForSession = await redis.get(key); + await redis.del(key); + return mintedForSession === sessionId; +} diff --git a/server/src/lib/oauth-upstream-forward.ts b/server/src/lib/oauth-upstream-forward.ts index afef4037..484059a5 100644 --- a/server/src/lib/oauth-upstream-forward.ts +++ b/server/src/lib/oauth-upstream-forward.ts @@ -118,6 +118,25 @@ export async function forwardOAuthGet( const contentType = upstreamResponse.headers.get("content-type"); if (contentType) reply.header("content-type", contentType); - const body = await upstreamResponse.text(); + let body: string; + try { + body = await upstreamResponse.text(); + } catch (err) { + // Headers already arrived (2xx/3xx/4xx status committed above), but the + // connection dropped mid-body -- e.g. the IdP redirect's response closing + // early. Without this, the thrown error would escape this function + // entirely and Fastify would emit a bare 500 with no body: a worse dead + // end for the popup than the 502 the pre-fetch failure above already + // produces, and one htmlizeOAuthPopupErrors can't help with since it only + // runs on a normal reply.send() completion. Clear the Location header set + // above so a stale redirect target doesn't ride along on a 502. + reply.removeHeader("location"); + request.log.error( + { errorType: err instanceof Error ? err.name : typeof err }, + `upstream ${logLabel} response body read failed`, + ); + return reply.code(502).send({ error: "upstream_unavailable" }); + } + return reply.code(upstreamResponse.status).send(body || undefined); } diff --git a/server/src/lib/origin-guard.ts b/server/src/lib/origin-guard.ts index 794e6368..00252ed8 100644 --- a/server/src/lib/origin-guard.ts +++ b/server/src/lib/origin-guard.ts @@ -22,17 +22,26 @@ export function isCrossSiteRequest(request: FastifyRequest): boolean { return request.headers["sec-fetch-site"] === "cross-site"; } -// null = no Origin header to check (caller falls back to isCrossSiteRequest). // config.publicOrigin, when set, is the source of truth (needed behind a // reverse proxy that isn't reflected in request.protocol/host — e.g. // TLS-terminated without TRUST_PROXY=true). Otherwise fall back to this // request's own scheme://host, which is only as trustworthy as // trustProxy's X-Forwarded-* handling (see config.ts). +// +// Also the source of truth for where *this* deployment's own /oauth/callback +// proxy is reachable (see routes/proxy/oauth-callback-url.ts) — the same +// value, for the same reason: it must not be guessed client-side from +// window.location.origin, which this exact derivation replaced for +// redirect_uri in the first place (mcp-context-forge#6458). +export function resolvePublicOrigin(request: FastifyRequest): string { + return config.publicOrigin ?? `${request.protocol}://${request.host}`; +} + +// null = no Origin header to check (caller falls back to isCrossSiteRequest). function originMismatch(request: FastifyRequest): boolean | null { const origin = request.headers.origin; if (typeof origin !== "string" || !origin) return null; - const expected = config.publicOrigin ?? `${request.protocol}://${request.host}`; - return origin !== expected; + return origin !== resolvePublicOrigin(request); } export function isForbiddenCrossOrigin(request: FastifyRequest): boolean { diff --git a/server/src/routes/proxy/oauth-authorize-nonce.ts b/server/src/routes/proxy/oauth-authorize-nonce.ts new file mode 100644 index 00000000..19b844dc --- /dev/null +++ b/server/src/routes/proxy/oauth-authorize-nonce.ts @@ -0,0 +1,29 @@ +// Location: ./client/server/src/routes/proxy/oauth-authorize-nonce.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// POST /oauth/authorize-nonce: mints the one-time nonce GET +// /oauth/authorize/:gatewayId now requires (see +// lib/oauth-authorize-nonce.ts for the threat this closes). Unlike that +// route, this one isn't a window.open() navigation -- triggerOAuthAuthorization +// (src/api/servers.ts) calls it as an ordinary same-origin fetch through the +// API client first, so it carries both the session cookie and the +// X-CSRF-Token header and fastify.csrfProtection applies exactly as it does +// on every other mutating browser->BFF call. + +import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; + +import { mintOAuthAuthorizeNonce } from "../../lib/oauth-authorize-nonce.js"; +import { setNoStore } from "../../lib/no-store.js"; + +export default async function oauthAuthorizeNonceRoute(fastify: FastifyInstance): Promise { + fastify.post( + "/oauth/authorize-nonce", + { preHandler: [fastify.sessionAuth, fastify.csrfProtection] }, + async (request: FastifyRequest, reply: FastifyReply) => { + setNoStore(reply); + const nonce = await mintOAuthAuthorizeNonce(fastify.redis, request.session!.sessionId); + return reply.send({ nonce }); + }, + ); +} diff --git a/server/src/routes/proxy/oauth-authorize.ts b/server/src/routes/proxy/oauth-authorize.ts index 1aae6765..9454c778 100644 --- a/server/src/routes/proxy/oauth-authorize.ts +++ b/server/src/routes/proxy/oauth-authorize.ts @@ -33,6 +33,18 @@ // category (cookie-authenticated, can't carry a CSRF token) -- see // lib/origin-guard.ts. // +// isForbiddenCrossOrigin alone still leaves a gap here that it doesn't for +// login.ts/proxy-sse.ts: a top-level GET navigation carries no Origin header +// at all (browsers only send Origin on non-GET or non-navigate requests), so +// the guard falls back to Sec-Fetch-Site -- which reports "same-site" rather +// than "cross-site" for a hostile *sibling* subdomain under the same +// registrable domain, and the SameSite=Lax session cookie rides along +// regardless. The `nonce` query param closes that: it must have been minted +// moments earlier by a same-origin, CSRF-protected POST (see +// routes/proxy/oauth-authorize-nonce.ts, lib/oauth-authorize-nonce.ts) that a +// sibling subdomain has no way to forge, and it is single-use, so a captured +// or replayed authorize URL can't be reused even by the session that minted it. +// // 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 @@ -42,6 +54,7 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; import { config } from "../../config.js"; +import { consumeOAuthAuthorizeNonce } from "../../lib/oauth-authorize-nonce.js"; import { forwardOAuthGet, htmlizeOAuthPopupErrors } from "../../lib/oauth-upstream-forward.js"; import { isForbiddenCrossOrigin } from "../../lib/origin-guard.js"; import { setNoStore } from "../../lib/no-store.js"; @@ -51,21 +64,43 @@ interface AuthorizeParams { gatewayId: string; } +interface AuthorizeQuerystring { + nonce?: string; +} + export default async function oauthAuthorizeProxyRoute(fastify: FastifyInstance): Promise { - fastify.get<{ Params: AuthorizeParams }>( + fastify.get<{ Params: AuthorizeParams; Querystring: AuthorizeQuerystring }>( "/oauth/authorize/:gatewayId", { preHandler: fastify.sessionAuth, onSend: htmlizeOAuthPopupErrors }, - async (request: FastifyRequest<{ Params: AuthorizeParams }>, reply: FastifyReply) => { + async ( + request: FastifyRequest<{ Params: AuthorizeParams; Querystring: AuthorizeQuerystring }>, + reply: FastifyReply, + ) => { setNoStore(reply); if (isForbiddenCrossOrigin(request)) { return reply.code(403).send({ error: "cross_site_request_forbidden" }); } + const sessionId = request.session!.sessionId; + const nonceIsValid = await consumeOAuthAuthorizeNonce( + fastify.redis, + sessionId, + request.query.nonce, + ); + if (!nonceIsValid) { + 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}`; + const rawQuery = queryIndex === -1 ? "" : request.url.slice(queryIndex + 1); + // Strip the now-consumed nonce before forwarding -- mcpgateway's own + // /oauth/authorize/{id} has no use for it and shouldn't see it. + const forwardedParams = new URLSearchParams(rawQuery); + forwardedParams.delete("nonce"); + const query = forwardedParams.toString(); + const upstreamUrl = `${config.contextforgeUrl}/oauth/authorize/${encodeURIComponent(request.params.gatewayId)}${query ? `?${query}` : ""}`; return forwardOAuthGet(request, reply, upstreamUrl, { headers: upstreamAuthHeader(bearerToken), diff --git a/server/src/routes/proxy/oauth-callback-url.ts b/server/src/routes/proxy/oauth-callback-url.ts new file mode 100644 index 00000000..40e1ec0f --- /dev/null +++ b/server/src/routes/proxy/oauth-callback-url.ts @@ -0,0 +1,41 @@ +// Location: ./client/server/src/routes/proxy/oauth-callback-url.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// GET /oauth/callback-url: tells the SPA where *this* deployment's own +// /oauth/callback proxy is reachable, so OAuth2Auth.tsx can default a new +// gateway's redirect_uri to it instead of leaving the field unset. +// +// Why unset isn't enough on its own: when nothing is stored, mcpgateway's +// own GET /oauth/authorize/{id} defaults redirect_uri to its *own* +// APP_DOMAIN-derived callback (see initiate_oauth_flow / +// _default_redirect_uri in mcp-context-forge's oauth_router.py). That's only +// browser-reachable when the gateway is independently exposed. In the common +// split deployment (only the web UI is public-facing, mcpgateway is not), +// the OAuth provider's redirect then lands on an address nothing answers. +// Defaulting the field to this BFF's own callback proxy instead makes the +// flow work regardless of topology: the provider redirects here, and +// oauth-callback.ts forwards the final hop to mcpgateway server-to-server +// over CONTEXTFORGE_URL, which is reachable by definition. +// +// Uses the same resolvePublicOrigin as origin-guard.ts's CSRF check — not +// window.location.origin — for the identical reason redirect_uri stopped +// being guessed client-side in the first place (mcp-context-forge#6458): +// behind a reverse proxy, the browser's own address isn't reliably this +// deployment's public one. + +import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; + +import { resolvePublicOrigin } from "../../lib/origin-guard.js"; +import { setNoStore } from "../../lib/no-store.js"; + +export default async function oauthCallbackUrlRoute(fastify: FastifyInstance): Promise { + fastify.get( + "/oauth/callback-url", + { preHandler: fastify.sessionAuth }, + async (request: FastifyRequest, reply: FastifyReply) => { + setNoStore(reply); + return reply.send({ redirectUri: `${resolvePublicOrigin(request)}/oauth/callback` }); + }, + ); +} diff --git a/server/test/helpers/build-app.ts b/server/test/helpers/build-app.ts index 05b1a213..b1cc662b 100644 --- a/server/test/helpers/build-app.ts +++ b/server/test/helpers/build-app.ts @@ -19,7 +19,9 @@ 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 oauthAuthorizeNonceRoute from "../../src/routes/proxy/oauth-authorize-nonce.js"; import oauthCallbackProxyRoute from "../../src/routes/proxy/oauth-callback.js"; +import oauthCallbackUrlRoute from "../../src/routes/proxy/oauth-callback-url.js"; import publicPasswordResetRoute from "../../src/routes/proxy/public-password-reset.js"; export class FakeRedis { @@ -66,8 +68,10 @@ export async function buildTestApp(opts: { withProxy?: boolean } = {}): Promise< await fastify.register(publicPasswordResetRoute); if (opts.withProxy) { + await fastify.register(oauthAuthorizeNonceRoute); await fastify.register(oauthAuthorizeProxyRoute); await fastify.register(oauthCallbackProxyRoute); + await fastify.register(oauthCallbackUrlRoute); await fastify.register(catchAllProxyRoute); } diff --git a/server/test/oauth-authorize-nonce.test.ts b/server/test/oauth-authorize-nonce.test.ts new file mode 100644 index 00000000..dd8be95f --- /dev/null +++ b/server/test/oauth-authorize-nonce.test.ts @@ -0,0 +1,144 @@ +// Location: ./client/server/test/oauth-authorize-nonce.test.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it } from "vitest"; + +import { buildTestApp, cookieHeaderFrom } from "./helpers/build-app.js"; + +async function seedSession() { + const { createSession } = await import("../src/lib/session-store.js"); + const app = await buildTestApp({ withProxy: true }); + const sessionId = await createSession(app.redis as never, { + bearerToken: "test-bearer-token", // pragma: allowlist secret + user: { email: "user@example.com", isAdmin: false }, + }); + return { app, cookie: `bff_sid=${sessionId}`, sessionId }; +} + +describe("POST /oauth/authorize-nonce", () => { + it("401s without a session cookie", async () => { + const { app } = await seedSession(); + + const response = await app.fastify.inject({ + method: "POST", + url: "/oauth/authorize-nonce", + }); + + expect(response.statusCode).toBe(401); + }); + + it("403s without a CSRF token even with a valid session", async () => { + const { app, cookie } = await seedSession(); + + const response = await app.fastify.inject({ + method: "POST", + url: "/oauth/authorize-nonce", + headers: { cookie }, + }); + + expect(response.statusCode).toBe(403); + }); + + it("mints a nonce for a same-origin, CSRF-token-carrying request", async () => { + const { app, cookie, sessionId } = await seedSession(); + + // Establish the CSRF secret cookie the same way every other mutating + // route's caller does: read /auth/session (or /auth/login) first. Doing + // that here rather than hand-rolling a token keeps this test bound to + // the real plugin contract instead of @fastify/csrf-protection internals. + const { getSession } = await import("../src/lib/session-store.js"); + const record = await getSession(app.redis as never, sessionId); + expect(record).not.toBeNull(); + + const sessionResponse = await app.fastify.inject({ + method: "GET", + url: "/auth/session", + headers: { cookie }, + }); + expect(sessionResponse.statusCode).toBe(200); + const { csrfToken } = sessionResponse.json() as { csrfToken: string }; + const setCookie = sessionResponse.headers["set-cookie"]; + const setCookieHeaders = Array.isArray(setCookie) ? setCookie : [setCookie as string]; + const fullCookie = `${cookie}; ${cookieHeaderFrom(setCookieHeaders)}`; + + // {} + Content-Type: application/json — exactly what src/api/client.ts's + // requestWithMeta actually sends for a bodyless POST (it always sets + // Content-Type: application/json; the caller passes {} rather than + // omitting the body). Asserted explicitly, not left to inject()'s + // defaults, because omitting it here would silently stop covering the + // FST_ERR_CTP_EMPTY_JSON_BODY regression below. + const response = await app.fastify.inject({ + method: "POST", + url: "/oauth/authorize-nonce", + headers: { + cookie: fullCookie, + "x-csrf-token": csrfToken, + "content-type": "application/json", + }, + payload: "{}", + }); + + expect(response.statusCode).toBe(200); + const { nonce } = response.json() as { nonce: string }; + expect(typeof nonce).toBe("string"); + expect(nonce.length).toBeGreaterThan(10); + }); + + // Regression: src/api/servers.ts's triggerOAuthAuthorization originally + // called api.post("/oauth/authorize-nonce") with no body argument. + // client.ts's requestWithMeta always sets Content-Type: application/json + // on a POST regardless of whether there's a body, so that produced a + // truly empty body under a json Content-Type -- which Fastify's default + // JSON parser rejects with FST_ERR_CTP_EMPTY_JSON_BODY before this route's + // preHandler (sessionAuth, csrfProtection) ever runs, a 400 with no useful + // error shape for the caller. The fix was client-side (pass {} explicitly, + // matching /auth/logout's existing call), not a server-side parser + // override like catch-all.ts's -- this route has exactly one caller. This + // test pins that empty-body-plus-json-Content-Type shape as the failure + // mode traceable back to the correct fix. + it("400s FST_ERR_CTP_EMPTY_JSON_BODY on a truly empty body under Content-Type: application/json", async () => { + const { app, cookie, sessionId } = await seedSession(); + const { getSession } = await import("../src/lib/session-store.js"); + expect(await getSession(app.redis as never, sessionId)).not.toBeNull(); + + const sessionResponse = await app.fastify.inject({ + method: "GET", + url: "/auth/session", + headers: { cookie }, + }); + const { csrfToken } = sessionResponse.json() as { csrfToken: string }; + const setCookie = sessionResponse.headers["set-cookie"]; + const setCookieHeaders = Array.isArray(setCookie) ? setCookie : [setCookie as string]; + const fullCookie = `${cookie}; ${cookieHeaderFrom(setCookieHeaders)}`; + + const response = await app.fastify.inject({ + method: "POST", + url: "/oauth/authorize-nonce", + headers: { + cookie: fullCookie, + "x-csrf-token": csrfToken, + "content-type": "application/json", + }, + // No payload at all -- the shape that used to reach the real server + // before triggerOAuthAuthorization was fixed to send {}. + }); + + expect(response.statusCode).toBe(400); + }); + + it("is unreachable via a cross-site Sec-Fetch-Site with no CSRF token, closing the same gap as oauth-authorize.ts", async () => { + const { app, cookie } = await seedSession(); + + const response = await app.fastify.inject({ + method: "POST", + url: "/oauth/authorize-nonce", + headers: { cookie, "sec-fetch-site": "same-site" }, + }); + + // No X-CSRF-Token header -> csrfProtection rejects regardless of + // Sec-Fetch-Site, which is the whole point: a hostile sibling subdomain + // has no way to have obtained the token in the first place. + expect(response.statusCode).toBe(403); + }); +}); diff --git a/server/test/oauth-authorize.test.ts b/server/test/oauth-authorize.test.ts index ecfa5996..a8daa5a9 100644 --- a/server/test/oauth-authorize.test.ts +++ b/server/test/oauth-authorize.test.ts @@ -63,7 +63,16 @@ async function seedSession(app: Awaited>) { bearerToken: "test-bearer-token", // pragma: allowlist secret user: { email: "user@example.com", isAdmin: false }, }); - return { cookie: `bff_sid=${sessionId}` }; + return { cookie: `bff_sid=${sessionId}`, sessionId }; +} + +// Bypasses the HTTP-level POST /oauth/authorize-nonce route (covered in its +// own test file) the same way seedSession bypasses POST /auth/login -- +// exercises the authorize route's *consumption* of a nonce, not the minting +// route. +async function mintNonce(app: Awaited>, sessionId: string) { + const { mintOAuthAuthorizeNonce } = await import("../src/lib/oauth-authorize-nonce.js"); + return mintOAuthAuthorizeNonce(app.redis as never, sessionId); } describe("GET /oauth/authorize/:gatewayId", () => { @@ -83,11 +92,12 @@ describe("GET /oauth/authorize/:gatewayId", () => { it("injects the bearer token and forwards the provider redirect untouched", async () => { const app = await buildApp(); - const { cookie } = await seedSession(app); + const { cookie, sessionId } = await seedSession(app); + const nonce = await mintNonce(app, sessionId); const response = await app.fastify.inject({ method: "GET", - url: "/oauth/authorize/gw-1?popup=true", + url: `/oauth/authorize/gw-1?popup=true&nonce=${nonce}`, headers: { cookie }, }); @@ -96,17 +106,20 @@ describe("GET /oauth/authorize/:gatewayId", () => { // 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"); + // nonce is BFF-internal and consumed before forwarding -- mcpgateway + // never sees it. 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); + const { cookie, sessionId } = await seedSession(app); + const nonce = await mintNonce(app, sessionId); await app.fastify.inject({ method: "GET", - url: "/oauth/authorize/gw-1", + url: `/oauth/authorize/gw-1?nonce=${nonce}`, headers: { cookie, authorization: "Bearer attacker-supplied-token" }, // pragma: allowlist secret }); @@ -141,11 +154,12 @@ describe("GET /oauth/authorize/:gatewayId", () => { it("forwards a non-redirect upstream error response as-is, not the popup HTML shape", async () => { const app = await buildApp(); - const { cookie } = await seedSession(app); + const { cookie, sessionId } = await seedSession(app); + const nonce = await mintNonce(app, sessionId); const response = await app.fastify.inject({ method: "GET", - url: "/oauth/authorize/missing-config", + url: `/oauth/authorize/missing-config?nonce=${nonce}`, headers: { cookie }, }); @@ -155,11 +169,12 @@ describe("GET /oauth/authorize/:gatewayId", () => { it("posts an oauth_callback error instead of raw JSON when the upstream connection fails", async () => { const app = await buildApp(); - const { cookie } = await seedSession(app); + const { cookie, sessionId } = await seedSession(app); + const nonce = await mintNonce(app, sessionId); const response = await app.fastify.inject({ method: "GET", - url: "/oauth/authorize/network-error", + url: `/oauth/authorize/network-error?nonce=${nonce}`, headers: { cookie }, }); @@ -168,4 +183,98 @@ describe("GET /oauth/authorize/:gatewayId", () => { expect(response.body).toContain('"error":"upstream_unavailable"'); expect(response.body).toContain("window.close()"); }); + + describe("nonce requirement", () => { + it("rejects a missing nonce before calling upstream", async () => { + const app = await buildApp(); + const { cookie } = await seedSession(app); + lastRequest = undefined; + + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/authorize/gw-1?popup=true", + headers: { cookie }, + }); + + expect(response.statusCode).toBe(403); + expect(response.body).toContain('"error":"cross_site_request_forbidden"'); + expect(lastRequest).toBeUndefined(); + }); + + it("rejects a nonce that was never minted", async () => { + const app = await buildApp(); + const { cookie } = await seedSession(app); + lastRequest = undefined; + + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/authorize/gw-1?popup=true&nonce=00000000-0000-0000-0000-000000000000", + headers: { cookie }, + }); + + expect(response.statusCode).toBe(403); + expect(lastRequest).toBeUndefined(); + }); + + it("rejects a nonce reused for a second request", async () => { + const app = await buildApp(); + const { cookie, sessionId } = await seedSession(app); + const nonce = await mintNonce(app, sessionId); + + const first = await app.fastify.inject({ + method: "GET", + url: `/oauth/authorize/gw-1?popup=true&nonce=${nonce}`, + headers: { cookie }, + }); + expect(first.statusCode).toBe(302); + + lastRequest = undefined; + const replay = await app.fastify.inject({ + method: "GET", + url: `/oauth/authorize/gw-1?popup=true&nonce=${nonce}`, + headers: { cookie }, + }); + + expect(replay.statusCode).toBe(403); + expect(lastRequest).toBeUndefined(); + }); + + it("rejects a nonce minted for a different session", async () => { + const app = await buildApp(); + const { cookie } = await seedSession(app); + const { sessionId: otherSessionId } = await seedSession(app); + const nonceForOtherSession = await mintNonce(app, otherSessionId); + lastRequest = undefined; + + const response = await app.fastify.inject({ + method: "GET", + url: `/oauth/authorize/gw-1?popup=true&nonce=${nonceForOtherSession}`, + headers: { cookie }, + }); + + expect(response.statusCode).toBe(403); + expect(lastRequest).toBeUndefined(); + }); + + // Regression for the gap isForbiddenCrossOrigin leaves on its own: a + // top-level GET navigation from a hostile *sibling* subdomain sends no + // Origin header and a Sec-Fetch-Site of "same-site" (not "cross-site"), + // so the origin guard alone would let it through carrying the victim's + // SameSite=Lax session cookie. The nonce requirement is what actually + // stops it, since the sibling has no way to have minted one. + it("rejects a same-site request with no Origin header and no nonce", async () => { + const app = await buildApp(); + const { cookie } = await seedSession(app); + lastRequest = undefined; + + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/authorize/gw-1?popup=true", + headers: { cookie, host: "app.example.test", "sec-fetch-site": "same-site" }, + }); + + expect(response.statusCode).toBe(403); + expect(lastRequest).toBeUndefined(); + }); + }); }); diff --git a/server/test/oauth-callback-url.test.ts b/server/test/oauth-callback-url.test.ts new file mode 100644 index 00000000..a89642c6 --- /dev/null +++ b/server/test/oauth-callback-url.test.ts @@ -0,0 +1,82 @@ +// Location: ./client/server/test/oauth-callback-url.test.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// config.ts reads process.env.PUBLIC_ORIGIN once at import time (see +// config.ts's own module-load comment), so exercising both the "unset" and +// "set" derivations in one file needs vi.resetModules() + a fresh dynamic +// import between them -- a plain re-import would just return the +// already-evaluated, cached config. + +import { afterEach, describe, expect, it, vi } from "vitest"; + +const originalPublicOrigin = process.env.PUBLIC_ORIGIN; + +afterEach(() => { + if (originalPublicOrigin === undefined) delete process.env.PUBLIC_ORIGIN; + else process.env.PUBLIC_ORIGIN = originalPublicOrigin; +}); + +async function buildAppWithPublicOrigin(publicOrigin: string | undefined) { + vi.resetModules(); + if (publicOrigin === undefined) delete process.env.PUBLIC_ORIGIN; + else process.env.PUBLIC_ORIGIN = publicOrigin; + + 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/callback-url", () => { + it("401s without a session cookie", async () => { + const app = await buildAppWithPublicOrigin(undefined); + + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/callback-url", + }); + + expect(response.statusCode).toBe(401); + }); + + it("derives the callback URL from the request's own scheme/host when PUBLIC_ORIGIN is unset", async () => { + const app = await buildAppWithPublicOrigin(undefined); + const { cookie } = await seedSession(app); + + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/callback-url", + headers: { cookie, host: "app.example.test" }, + }); + + expect(response.statusCode).toBe(200); + expect(response.json()).toEqual({ redirectUri: "http://app.example.test/oauth/callback" }); + }); + + it("prefers PUBLIC_ORIGIN over the request's own host -- the split-deployment case", async () => { + const app = await buildAppWithPublicOrigin("https://web.example.com"); + const { cookie } = await seedSession(app); + + // Even though this request arrived with a different Host (e.g. an + // internal LB hostname), PUBLIC_ORIGIN is the operator-declared source of + // truth for where the browser actually reaches this deployment -- and + // this is the same value the gateway's own APP_DOMAIN default would NOT + // resolve to in a split deployment, which is the whole point. + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/callback-url", + headers: { cookie, host: "internal-lb.local:8080" }, + }); + + expect(response.statusCode).toBe(200); + expect(response.json()).toEqual({ redirectUri: "https://web.example.com/oauth/callback" }); + }); +}); diff --git a/server/test/oauth-callback.test.ts b/server/test/oauth-callback.test.ts index bfc232da..ca75adc6 100644 --- a/server/test/oauth-callback.test.ts +++ b/server/test/oauth-callback.test.ts @@ -28,6 +28,17 @@ beforeAll(async () => { return; } + if (req.url?.startsWith("/oauth/callback?truncated-body")) { + // Headers commit (fetch() resolves, status/Location/Content-Type are + // already readable), then the connection dies before the declared + // Content-Length is satisfied -- unlike network-error above, this + // fails inside upstreamResponse.text(), not the fetch() call itself. + res.writeHead(200, { "content-type": "text/html", "content-length": "1000" }); + res.write(""); + res.socket?.destroy(); + return; + } + // 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, { @@ -92,6 +103,20 @@ describe("GET /oauth/callback", () => { expect(response.body).toContain("window.close()"); }); + it("posts an oauth_callback error instead of a raw 500 when the body read fails after headers arrive", async () => { + const app = await buildApp(); + + const response = await app.fastify.inject({ + method: "GET", + url: "/oauth/callback?truncated-body=1&state=popup.xyz", + }); + + expect(response.statusCode).toBe(502); + expect(response.headers["content-type"]).toContain("text/html"); + expect(response.body).toContain('"error":"upstream_unavailable"'); + expect(response.body).toContain("window.close()"); + }); + it("forwards an OAuth provider error callback", async () => { const app = await buildApp(); diff --git a/src/api/client.ts b/src/api/client.ts index 6e0de44e..b1bd9206 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -23,11 +23,19 @@ const LOGIN_PATH = "/app/login"; const API_PREFIX = "/api"; const SESSION_CHECK_PATH = "/auth/session"; -const BFF_OWNED_AUTH_PATHS = new Set([ +// Bare paths the BFF serves itself (not proxied to the upstream API) and +// that callers reach through this client rather than a raw window.open() +// navigation. /oauth/authorize-nonce joins the /auth/* routes here for the +// same reason: mintOAuthAuthorizeNonce (src/api/servers.ts) needs the +// session cookie + CSRF header this client already attaches to POSTs, and +// /api/* would route it to the upstream gateway, which has no such endpoint. +const BFF_OWNED_PATHS = new Set([ "/auth/login", "/auth/logout", "/auth/change-password-required", SESSION_CHECK_PATH, + "/oauth/authorize-nonce", + "/oauth/callback-url", ]); export class ApiError extends Error { @@ -69,7 +77,7 @@ function isAbsoluteUrl(path: string): boolean { /** Bare paths get /api/* (BFF proxy to the API); the BFF's own auth routes and absolute URLs pass through untouched. */ function resolveApiPath(path: string): string { if (isAbsoluteUrl(path)) return path; - if (BFF_OWNED_AUTH_PATHS.has(path)) return path; + if (BFF_OWNED_PATHS.has(path)) return path; if (path === API_PREFIX || path.startsWith(`${API_PREFIX}/`)) return path; return path.startsWith("/") ? `${API_PREFIX}${path}` : `${API_PREFIX}/${path}`; } diff --git a/src/api/servers.test.ts b/src/api/servers.test.ts index 3d99b937..aeb217c1 100644 --- a/src/api/servers.test.ts +++ b/src/api/servers.test.ts @@ -1,8 +1,23 @@ +import { waitFor } from "@testing-library/react"; import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { serversApi } from "./servers"; import { setCsrfToken } from "./client"; import type { GatewayTestRequest, GatewayHandshakeRequest } from "@/generated/types"; +function nonceResponse(nonce = "test-nonce"): Response { + return new Response(JSON.stringify({ nonce }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); +} + +function mockAuthPopup(): Window & { location: { href: string }; close: () => void } { + return { closed: false, location: { href: "" }, close: vi.fn() } as unknown as Window & { + location: { href: string }; + close: () => void; + }; +} + describe("serversApi", () => { const mockFetch = vi.fn(); @@ -146,25 +161,50 @@ describe("serversApi", () => { await expect(serversApi.triggerOAuthAuthorization("server-123")).rejects.toThrow( "Failed to open OAuth authorization window", ); + // Blocked before ever needing a nonce. + expect(mockFetch).not.toHaveBeenCalled(); }); - it("opens the popup with the correct URL and popup=true flag", () => { - const mockAuthWindow = { closed: false } as unknown as Window; + it("opens a blank popup synchronously, then navigates it to the authorize URL once the nonce arrives", async () => { + const mockAuthWindow = mockAuthPopup(); vi.spyOn(window, "open").mockReturnValue(mockAuthWindow); + mockFetch.mockResolvedValueOnce(nonceResponse("minted-nonce")); - // Don't await — just trigger the call + // Don't await — just trigger the call. window.open() must happen + // synchronously within this call (a user-gesture requirement popup + // blockers enforce), before the async nonce fetch below resolves. serversApi.triggerOAuthAuthorization("server-abc"); - expect(window.open).toHaveBeenCalledWith( - expect.stringContaining("/oauth/authorize/server-abc?popup=true"), - "oauth_authorization", - expect.any(String), + expect(window.open).toHaveBeenCalledWith("", "oauth_authorization", expect.any(String)); + expect(mockFetch).toHaveBeenCalledWith( + expect.stringContaining("/oauth/authorize-nonce"), + expect.objectContaining({ method: "POST" }), ); + + await waitFor(() => { + expect(mockAuthWindow.location.href).toContain( + "/oauth/authorize/server-abc?popup=true&nonce=minted-nonce", + ); + }); + }); + + it("closes the popup and rejects when minting the nonce fails", async () => { + const mockAuthWindow = mockAuthPopup(); + vi.spyOn(window, "open").mockReturnValue(mockAuthWindow); + mockFetch.mockRejectedValueOnce(new TypeError("Failed to fetch")); + + const promise = serversApi.triggerOAuthAuthorization("server-123"); + + await expect(promise).rejects.toThrow(); + expect(mockAuthWindow.close).toHaveBeenCalled(); + // Never navigated -- the popup must not sit on a blank page silently. + expect(mockAuthWindow.location.href).toBe(""); }); it("resolves with success data when popup sends a success postMessage", async () => { - const mockAuthWindow = { closed: false } as unknown as Window; + const mockAuthWindow = mockAuthPopup(); vi.spyOn(window, "open").mockReturnValue(mockAuthWindow); + mockFetch.mockResolvedValueOnce(nonceResponse()); const promise = serversApi.triggerOAuthAuthorization("server-123"); @@ -182,8 +222,9 @@ describe("serversApi", () => { }); it("rejects with errorDescription when popup sends an error postMessage", async () => { - const mockAuthWindow = { closed: false } as unknown as Window; + const mockAuthWindow = mockAuthPopup(); vi.spyOn(window, "open").mockReturnValue(mockAuthWindow); + mockFetch.mockResolvedValueOnce(nonceResponse()); const promise = serversApi.triggerOAuthAuthorization("server-123"); @@ -202,8 +243,9 @@ describe("serversApi", () => { }); it("falls back to the error code when errorDescription is absent", async () => { - const mockAuthWindow = { closed: false } as unknown as Window; + const mockAuthWindow = mockAuthPopup(); vi.spyOn(window, "open").mockReturnValue(mockAuthWindow); + mockFetch.mockResolvedValueOnce(nonceResponse()); const promise = serversApi.triggerOAuthAuthorization("server-123"); @@ -217,8 +259,9 @@ describe("serversApi", () => { }); it("falls back to generic message when neither errorDescription nor error is present", async () => { - const mockAuthWindow = { closed: false } as unknown as Window; + const mockAuthWindow = mockAuthPopup(); vi.spyOn(window, "open").mockReturnValue(mockAuthWindow); + mockFetch.mockResolvedValueOnce(nonceResponse()); const promise = serversApi.triggerOAuthAuthorization("server-123"); @@ -233,8 +276,9 @@ describe("serversApi", () => { it("ignores postMessages from other sources", async () => { vi.useFakeTimers(); - const mockAuthWindow = { closed: false } as unknown as Window; + const mockAuthWindow = mockAuthPopup(); vi.spyOn(window, "open").mockReturnValue(mockAuthWindow); + mockFetch.mockResolvedValueOnce(nonceResponse()); const promise = serversApi.triggerOAuthAuthorization("server-123"); @@ -256,8 +300,9 @@ describe("serversApi", () => { it("ignores non-oauth_callback messages from the popup", async () => { vi.useFakeTimers(); - const mockAuthWindow = { closed: false } as unknown as Window; + const mockAuthWindow = mockAuthPopup(); vi.spyOn(window, "open").mockReturnValue(mockAuthWindow); + mockFetch.mockResolvedValueOnce(nonceResponse()); const promise = serversApi.triggerOAuthAuthorization("server-123"); @@ -281,8 +326,9 @@ describe("serversApi", () => { it("rejects with cancellation message when user closes the popup", async () => { vi.useFakeTimers(); - const mockAuthWindow = { closed: false } as unknown as Window; + const mockAuthWindow = mockAuthPopup(); vi.spyOn(window, "open").mockReturnValue(mockAuthWindow); + mockFetch.mockResolvedValueOnce(nonceResponse()); const promise = serversApi.triggerOAuthAuthorization("server-123"); diff --git a/src/api/servers.ts b/src/api/servers.ts index 4f7b6b91..416532c1 100644 --- a/src/api/servers.ts +++ b/src/api/servers.ts @@ -173,15 +173,23 @@ export const serversApi = { /** * Trigger OAuth authorization flow for a gateway via a popup window. * - * Opens /oauth/authorize/{id}?popup=true in a centered popup. The backend - * encodes a "popup." prefix in the OAuth state so the callback page responds - * with window.opener.postMessage instead of rendering a full HTML page. + * Opens /oauth/authorize/{id}?popup=true&nonce=... in a centered popup. The + * backend encodes a "popup." prefix in the OAuth state so the callback page + * responds with window.opener.postMessage instead of rendering a full HTML + * page. + * + * The popup itself is a raw window.open() navigation, so it can't carry a + * CSRF header the way this client's other POSTs do — the nonce is the + * substitute (see server/src/lib/oauth-authorize-nonce.ts). Minting it is + * an async POST, so the popup window is opened blank *first*, synchronously + * within this click handler, and only navigated once the nonce comes back; + * doing the fetch before window.open() would lose the "opened from a user + * gesture" status popup blockers require. * * Returns a Promise that resolves on success or rejects on error / cancellation. */ triggerOAuthAuthorization: (id: string): Promise => { const validId = validateServerId(id); - const authUrl = `/oauth/authorize/${validId}?popup=true`; return new Promise((resolve, reject) => { const width = 600; @@ -190,7 +198,7 @@ export const serversApi = { const top = window.screenY + (window.outerHeight - height) / 2; const authWindow = window.open( - authUrl, + "", "oauth_authorization", `width=${width},height=${height},left=${left},top=${top},toolbar=no,location=no,status=no,menubar=no,scrollbars=yes,resizable=yes`, ); @@ -242,6 +250,24 @@ export const serversApi = { } } }, 1000); + + api + // {} — not omitted — so this isn't a Content-Type: application/json + // POST with a truly empty body, which Fastify's default JSON parser + // rejects with FST_ERR_CTP_EMPTY_JSON_BODY before the route handler + // (or even sessionAuth) ever runs. Same reasoning as /auth/logout. + .post<{ nonce: string }>("/oauth/authorize-nonce", {}) + .then(({ nonce }) => { + if (settled) return; + authWindow.location.href = `/oauth/authorize/${validId}?popup=true&nonce=${encodeURIComponent(nonce)}`; + }) + .catch((err: unknown) => { + if (settled) return; + settled = true; + cleanup(); + authWindow.close(); + reject(err instanceof Error ? err : new Error("Failed to start OAuth authorization")); + }); }); }, }; diff --git a/src/components/mcp-servers/OAuth2Auth.tsx b/src/components/mcp-servers/OAuth2Auth.tsx index a1d74d9f..e522751d 100644 --- a/src/components/mcp-servers/OAuth2Auth.tsx +++ b/src/components/mcp-servers/OAuth2Auth.tsx @@ -67,14 +67,16 @@ export function OAuth2Auth({ errors, }: OAuth2AuthProps) { const intl = useIntl(); - // 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. + // Deliberately NOT derived from window.location.origin here: useMCPServerForm.ts + // fetches this deployment's own /oauth/callback proxy URL from the BFF + // (GET /oauth/callback-url, server-side-derived the same trustworthy way + // origin-guard.ts validates Origin) and defaults redirectUri to it as soon + // as the grant type is authorization_code, rather than leaving the field + // unset for mcpgateway's own APP_DOMAIN-based default to apply -- that + // default only works when mcpgateway is independently browser-reachable, + // not the common split deployment where only this web UI is (see + // mcp-context-forge#6458). This still briefly renders the placeholder + // branch below while that fetch is in flight. const hasStoredRedirectUri = Boolean(redirectUri); const isLocalRedirect = hasStoredRedirectUri && diff --git a/src/hooks/useMCPServerForm.ts b/src/hooks/useMCPServerForm.ts index 06492b19..392d4e36 100644 --- a/src/hooks/useMCPServerForm.ts +++ b/src/hooks/useMCPServerForm.ts @@ -361,6 +361,29 @@ export function useMCPServerForm( const isEditMode = Boolean(gatewayId); + // Defaults oauthRedirectUri to this deployment's own /oauth/callback proxy + // (server/src/routes/proxy/oauth-callback-url.ts) rather than leaving the + // field unset. Unset relies on mcpgateway's own APP_DOMAIN-based default, + // which only works when the gateway is independently browser-reachable — + // not the case in a split deployment where only this web UI is + // public-facing. Fetched (not derived from window.location.origin) for the + // same reason redirect_uri stopped being guessed client-side in the first + // place: behind a reverse proxy the browser's own address isn't reliably + // this deployment's public one (see oauth-callback-url.ts). + const { data: defaultOAuthRedirectUri } = useQuery<{ redirectUri: string }>( + "/oauth/callback-url", + { enabled: authType === "oauth" && oauthGrantType === "authorization_code" }, + ); + + useEffect(() => { + // Only fills a genuinely empty field — a value already loaded from a + // stored gateway (the effect below) or restored some other way always + // wins, and this never overwrites it. + if (defaultOAuthRedirectUri?.redirectUri && !oauthRedirectUri) { + setOAuthRedirectUri(defaultOAuthRedirectUri.redirectUri); + } + }, [defaultOAuthRedirectUri, oauthRedirectUri]); + // Fetch server data when in edit mode // API response uses camelCase outer keys (via alias_generator), but oauth_config dict stays snake_case const { data: serverData, error: serverFetchError } = useQuery<{ From 10e1b62f715c56808889ffcb0414bc6901697704 Mon Sep 17 00:00:00 2001 From: Marek Dano Date: Tue, 8 Sep 2026 12:27:09 +0100 Subject: [PATCH 5/6] fix: atomic nonce consumption and gate submit on OAuth redirect URI fetch - consumeOAuthAuthorizeNonce did GET then DEL as two separate awaits, letting two concurrent requests for the same nonce both read its session binding before either delete ran, so both could proceed. Switched to atomic GETDEL (added to RedisLike, FakeRedis, and MemoryRedis) and added a concurrent-request regression test. - useMCPServerForm let the form submit while GET /oauth/callback-url was still pending or had failed, leaving oauthRedirectUri empty and falling back to mcpgateway's own APP_DOMAIN default -- the exact split-deployment failure the redirect_uri default was added to fix. Submission is now blocked until it resolves (isValid gate plus a handleSubmit backstop), and OAuth2Auth shows a retryable error with a Retry button while it's down. Addresses review feedback on #101: https://github.com/contextforge-org/contextforge-web-ui/pull/101#pullrequestreview-5140469421 Signed-off-by: Marek Dano --- server/src/lib/memory-redis.ts | 7 ++ server/src/lib/oauth-authorize-nonce.ts | 13 ++-- server/src/lib/session-store.ts | 5 ++ server/test/helpers/build-app.ts | 8 ++- server/test/oauth-authorize.test.ts | 28 ++++++++ .../mcp-servers/AdvancedSettings.tsx | 9 +++ .../mcp-servers/MCPServerForm.test.tsx | 65 +++++++++++++++++++ src/components/mcp-servers/MCPServerForm.tsx | 13 ++++ src/components/mcp-servers/OAuth2Auth.tsx | 34 +++++++++- src/hooks/useMCPServerForm.ts | 56 ++++++++++++++-- src/i18n/locales/en-US/mcpServer.json | 3 + src/i18n/locales/es-ES/mcpServer.json | 3 + src/i18n/locales/pt-BR/mcpServer.json | 3 + 13 files changed, 233 insertions(+), 14 deletions(-) diff --git a/server/src/lib/memory-redis.ts b/server/src/lib/memory-redis.ts index 1ac29147..a8e5c284 100644 --- a/server/src/lib/memory-redis.ts +++ b/server/src/lib/memory-redis.ts @@ -54,6 +54,13 @@ export class MemoryRedis extends EventEmitter { return "OK"; } + async getdel(key: string): Promise { + const entry = store.get(key); + store.delete(key); + if (!entry || isExpired(entry)) return null; + return entry.value; + } + async del(key: string): Promise { return store.delete(key) ? 1 : 0; } diff --git a/server/src/lib/oauth-authorize-nonce.ts b/server/src/lib/oauth-authorize-nonce.ts index a1f96d68..adac4914 100644 --- a/server/src/lib/oauth-authorize-nonce.ts +++ b/server/src/lib/oauth-authorize-nonce.ts @@ -42,17 +42,18 @@ export async function mintOAuthAuthorizeNonce( return nonce; } -// Deletes the nonce whether or not it matches -- a captured query string -// (browser history, a proxy access log, a copy-pasted URL) must not be -// replayable even by the same session that minted it. +// GETDEL, not GET-then-DEL: reading and deleting must be one atomic op, or +// two concurrent requests for the same nonce can both read its session +// binding before either delete runs, letting both proceed. That deletes the +// nonce whether or not it matches -- a captured query string (browser +// history, a proxy access log, a copy-pasted URL) must not be replayable +// even by the same session that minted it. export async function consumeOAuthAuthorizeNonce( redis: RedisLike, sessionId: string, nonce: string | undefined, ): Promise { if (!nonce) return false; - const key = nonceRedisKey(nonce); - const mintedForSession = await redis.get(key); - await redis.del(key); + const mintedForSession = await redis.getdel(nonceRedisKey(nonce)); return mintedForSession === sessionId; } diff --git a/server/src/lib/session-store.ts b/server/src/lib/session-store.ts index dbdfb28b..c4adb028 100644 --- a/server/src/lib/session-store.ts +++ b/server/src/lib/session-store.ts @@ -18,6 +18,11 @@ export const SESSION_COOKIE_NAME = "bff_sid"; // ioredis with its own generics) and lets tests pass an in-memory fake. export interface RedisLike { get(key: string): Promise; + // Atomic read-then-delete (Redis GETDEL, >=6.2). Needed anywhere a value + // is consumed exactly once -- see oauth-authorize-nonce.ts -- since a + // separate get() + del() leaves a window between the two awaits where a + // second concurrent caller's get() can still observe the value. + getdel(key: string): Promise; setex(key: string, ttlSeconds: number, value: string): Promise; del(key: string): Promise; publish(channel: string, message: string): Promise; diff --git a/server/test/helpers/build-app.ts b/server/test/helpers/build-app.ts index b1cc662b..3610caaa 100644 --- a/server/test/helpers/build-app.ts +++ b/server/test/helpers/build-app.ts @@ -5,7 +5,7 @@ // Test fixture: a Fastify instance wired the same way as src/index.ts, but // with an in-memory fake in place of plugins/redis.ts so tests don't need a // real Redis instance. Only the ioredis surface the app actually touches -// (get/setex/del/publish) is implemented. +// (get/getdel/setex/del/publish) is implemented. import Fastify, { type FastifyInstance } from "fastify"; import { type Redis } from "ioredis"; @@ -32,6 +32,12 @@ export class FakeRedis { return this.store.has(key) ? this.store.get(key)! : null; } + async getdel(key: string): Promise { + const value = this.store.has(key) ? this.store.get(key)! : null; + this.store.delete(key); + return value; + } + async setex(key: string, _ttlSeconds: number, value: string): Promise<"OK"> { this.store.set(key, value); return "OK"; diff --git a/server/test/oauth-authorize.test.ts b/server/test/oauth-authorize.test.ts index a8daa5a9..4933e415 100644 --- a/server/test/oauth-authorize.test.ts +++ b/server/test/oauth-authorize.test.ts @@ -239,6 +239,34 @@ describe("GET /oauth/authorize/:gatewayId", () => { expect(lastRequest).toBeUndefined(); }); + // Regression: consumeOAuthAuthorizeNonce used to be a plain + // GET-then-DEL, leaving a window between the two awaits where a second + // concurrent request for the same nonce could still read its session + // binding before either request's DEL ran, letting both proceed. Fired + // together (not awaited sequentially, unlike the reuse test above) to + // actually exercise that interleaving. + it("lets exactly one of two concurrent requests for the same nonce through", async () => { + const app = await buildApp(); + const { cookie, sessionId } = await seedSession(app); + const nonce = await mintNonce(app, sessionId); + + const [first, second] = await Promise.all([ + app.fastify.inject({ + method: "GET", + url: `/oauth/authorize/gw-1?popup=true&nonce=${nonce}`, + headers: { cookie }, + }), + app.fastify.inject({ + method: "GET", + url: `/oauth/authorize/gw-1?popup=true&nonce=${nonce}`, + headers: { cookie }, + }), + ]); + + const statusCodes = [first.statusCode, second.statusCode].sort(); + expect(statusCodes).toEqual([302, 403]); + }); + it("rejects a nonce minted for a different session", async () => { const app = await buildApp(); const { cookie } = await seedSession(app); diff --git a/src/components/mcp-servers/AdvancedSettings.tsx b/src/components/mcp-servers/AdvancedSettings.tsx index 6b8ce1d9..fa42a406 100644 --- a/src/components/mcp-servers/AdvancedSettings.tsx +++ b/src/components/mcp-servers/AdvancedSettings.tsx @@ -51,6 +51,9 @@ interface AdvancedSettingsProps { oauthGrantType: string; oauthIssuerUrl: string; oauthRedirectUri: string; + isOAuthRedirectUriLoading?: boolean; + oauthRedirectUriError?: string; + onRetryOAuthRedirectUri?: () => void; oauthAuthorizationUrl: string; oauthScopes: string; oauthStoreTokens: boolean; @@ -103,6 +106,9 @@ export function AdvancedSettings({ oauthGrantType, oauthIssuerUrl, oauthRedirectUri, + isOAuthRedirectUriLoading, + oauthRedirectUriError, + onRetryOAuthRedirectUri, oauthAuthorizationUrl, oauthScopes, oauthStoreTokens, @@ -167,6 +173,9 @@ export function AdvancedSettings({ grantType={oauthGrantType} issuerUrl={oauthIssuerUrl} redirectUri={oauthRedirectUri} + isRedirectUriLoading={isOAuthRedirectUriLoading} + redirectUriError={oauthRedirectUriError} + onRetryRedirectUri={onRetryOAuthRedirectUri} authorizationUrl={oauthAuthorizationUrl} scopes={oauthScopes} storeTokens={oauthStoreTokens} diff --git a/src/components/mcp-servers/MCPServerForm.test.tsx b/src/components/mcp-servers/MCPServerForm.test.tsx index c9cd57ff..f3dfd89a 100644 --- a/src/components/mcp-servers/MCPServerForm.test.tsx +++ b/src/components/mcp-servers/MCPServerForm.test.tsx @@ -100,6 +100,12 @@ const server = setupServer( providers: [], }); }), + // Default happy path for the OAuth authorization_code redirect-uri default + // (see the "OAuth redirect URI default" describe block for slow/failing + // overrides of this). + http.get("/oauth/callback-url", () => { + return HttpResponse.json({ redirectUri: "https://app.example.com/oauth/callback" }); + }), ); beforeAll(() => server.listen({ onUnhandledRequest: "warn" })); @@ -929,6 +935,65 @@ describe("MCPServerForm", () => { }); }); + // Regression for the split-deployment bug the /oauth/callback-url default + // exists to fix: submitting while that fetch is still pending, or after it + // has failed, must not be possible -- otherwise oauthRedirectUri stays + // empty and the request falls back to mcpgateway's own APP_DOMAIN default. + describe("OAuth redirect URI default", () => { + async function selectOAuthAuthorizationCode() { + const user = userEvent.setup(); + renderWithRouter(); + await user.click(screen.getByRole("button", { name: /Advanced settings/i })); + await user.click(screen.getByRole("radio", { name: /OAuth 2\.0/i })); + await user.click(screen.getByRole("combobox", { name: /Grant type/i })); + await user.click(screen.getByRole("option", { name: /Authorization code/i })); + fireEvent.change(screen.getByLabelText(/^Name/i), { target: { value: "Test Server" } }); + fireEvent.change(screen.getByLabelText(/^URL/i), { + target: { value: "http://localhost:3000" }, + }); + return user; + } + + it("disables submit while the default redirect URI is still loading", async () => { + server.use( + http.get("/oauth/callback-url", async () => { + await new Promise(() => {}); // never resolves within the test + return HttpResponse.json({ redirectUri: "https://app.example.com/oauth/callback" }); + }), + ); + + await selectOAuthAuthorizationCode(); + + const submitButton = screen.getByRole("button", { name: /Connect server/i }); + expect(submitButton).toBeDisabled(); + }); + + it("shows a retryable error and keeps submit disabled when the fetch fails, then enables it once retried successfully", async () => { + let callCount = 0; + server.use( + http.get("/oauth/callback-url", () => { + callCount += 1; + if (callCount === 1) { + return HttpResponse.json({ detail: "unavailable" }, { status: 502 }); + } + return HttpResponse.json({ redirectUri: "https://app.example.com/oauth/callback" }); + }), + ); + + const user = await selectOAuthAuthorizationCode(); + + const submitButton = await screen.findByRole("button", { name: /Connect server/i }); + await waitFor(() => expect(submitButton).toBeDisabled()); + expect(screen.getByText(/Couldn't load the default redirect URI/i)).toBeInTheDocument(); + + const retryButton = screen.getByRole("button", { name: /Retry/i }); + await user.click(retryButton); + + await waitFor(() => expect(submitButton).toBeEnabled()); + expect(screen.queryByText(/Couldn't load the default redirect URI/i)).not.toBeInTheDocument(); + }); + }); + describe("CA Certificate Upload", () => { it("should render CA certificate upload section in advanced settings", async () => { const user = userEvent.setup(); diff --git a/src/components/mcp-servers/MCPServerForm.tsx b/src/components/mcp-servers/MCPServerForm.tsx index 77d46765..f9573b4f 100644 --- a/src/components/mcp-servers/MCPServerForm.tsx +++ b/src/components/mcp-servers/MCPServerForm.tsx @@ -93,6 +93,9 @@ export function MCPServerForm({ isOpen, onToggle, serverId, onSuccess }: MCPServ oauthIssuerUrl, setOAuthIssuerUrl, oauthRedirectUri, + isOAuthRedirectUriLoading, + oauthRedirectUriError, + retryOAuthRedirectUri, oauthAuthorizationUrl, setOAuthAuthorizationUrl, oauthScopes, @@ -373,6 +376,16 @@ export function MCPServerForm({ isOpen, onToggle, serverId, onSuccess }: MCPServ oauthGrantType={oauthGrantType} oauthIssuerUrl={oauthIssuerUrl} oauthRedirectUri={oauthRedirectUri} + isOAuthRedirectUriLoading={isOAuthRedirectUriLoading} + oauthRedirectUriError={oauthRedirectUriError} + onRetryOAuthRedirectUri={() => { + // useQuery's execute() rejects on failure in addition to + // setting its own error state (which this component + // reads back as oauthRedirectUriError) -- swallow the + // rejection here so a repeat failure doesn't surface as + // an unhandled promise rejection. + retryOAuthRedirectUri().catch(() => {}); + }} oauthAuthorizationUrl={oauthAuthorizationUrl} oauthScopes={oauthScopes} oauthStoreTokens={oauthStoreTokens} diff --git a/src/components/mcp-servers/OAuth2Auth.tsx b/src/components/mcp-servers/OAuth2Auth.tsx index e522751d..cf777419 100644 --- a/src/components/mcp-servers/OAuth2Auth.tsx +++ b/src/components/mcp-servers/OAuth2Auth.tsx @@ -17,6 +17,9 @@ interface OAuth2AuthProps { grantType: string; issuerUrl: string; redirectUri: string; + isRedirectUriLoading?: boolean; + redirectUriError?: string; + onRetryRedirectUri?: () => void; clientId: string; clientSecret: string; tokenUrl: string; @@ -44,6 +47,9 @@ export function OAuth2Auth({ grantType, issuerUrl, redirectUri, + isRedirectUriLoading, + redirectUriError, + onRetryRedirectUri, clientId, clientSecret, tokenUrl, @@ -189,11 +195,35 @@ export function OAuth2Auth({ id="oauth-redirect-uri" type="text" readOnly - value={intl.formatMessage({ id: "mcpServer.auth.oauth.redirectUriAutoPlaceholder" })} + value={intl.formatMessage({ + id: redirectUriError + ? "mcpServer.auth.oauth.redirectUriLoadError" + : isRedirectUriLoading + ? "mcpServer.auth.oauth.redirectUriLoading" + : "mcpServer.auth.oauth.redirectUriAutoPlaceholder", + })} className="rounded-md border-neutral-300 px-4 text-sm text-neutral-500 shadow-none focus-visible:ring-1 focus-visible:ring-ring focus-visible:ring-offset-0 dark:border-neutral-700 dark:text-neutral-500" /> )} - {!hasStoredRedirectUri && ( + {!hasStoredRedirectUri && redirectUriError && ( +

+

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

+ {onRetryRedirectUri && ( + + )} +
+ )} + {!hasStoredRedirectUri && !redirectUriError && (

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

diff --git a/src/hooks/useMCPServerForm.ts b/src/hooks/useMCPServerForm.ts index 392d4e36..a07d3547 100644 --- a/src/hooks/useMCPServerForm.ts +++ b/src/hooks/useMCPServerForm.ts @@ -212,6 +212,9 @@ export interface UseMCPServerFormReturn { oauthGrantType: string; oauthIssuerUrl: string; oauthRedirectUri: string; + isOAuthRedirectUriLoading: boolean; + oauthRedirectUriError: string | undefined; + retryOAuthRedirectUri: () => Promise; oauthAuthorizationUrl: string; oauthScopes: string; oauthStoreTokens: boolean; @@ -370,10 +373,16 @@ export function useMCPServerForm( // same reason redirect_uri stopped being guessed client-side in the first // place: behind a reverse proxy the browser's own address isn't reliably // this deployment's public one (see oauth-callback-url.ts). - const { data: defaultOAuthRedirectUri } = useQuery<{ redirectUri: string }>( - "/oauth/callback-url", - { enabled: authType === "oauth" && oauthGrantType === "authorization_code" }, - ); + const oauthRedirectUriNeeded = authType === "oauth" && oauthGrantType === "authorization_code"; + + const { + data: defaultOAuthRedirectUri, + isLoading: isOAuthRedirectUriLoading, + error: oauthRedirectUriFetchError, + refetch: retryOAuthRedirectUri, + } = useQuery<{ redirectUri: string }>("/oauth/callback-url", { + enabled: oauthRedirectUriNeeded, + }); useEffect(() => { // Only fills a genuinely empty field — a value already loaded from a @@ -384,6 +393,16 @@ export function useMCPServerForm( } }, [defaultOAuthRedirectUri, oauthRedirectUri]); + // True while the field this deployment's own redirect_uri belongs in is + // still empty for a reason the user hasn't chosen -- the fetch above is + // in flight or failed. Submitting in that window would send redirect_uri: + // undefined and silently fall back to mcpgateway's APP_DOMAIN default, + // reintroducing the split-deployment failure this fetch exists to fix. + const oauthRedirectUriUnresolved = + oauthRedirectUriNeeded && + !oauthRedirectUri && + (isOAuthRedirectUriLoading || Boolean(oauthRedirectUriFetchError)); + // Fetch server data when in edit mode // API response uses camelCase outer keys (via alias_generator), but oauth_config dict stays snake_case const { data: serverData, error: serverFetchError } = useQuery<{ @@ -778,6 +797,18 @@ export function useMCPServerForm( async (event: FormEvent, onSuccess?: (response?: unknown) => void) => { event.preventDefault(); + // Backstop for the isValid gate the submit button already applies -- + // an implicit form submit (Enter in a text field) isn't blocked by a + // disabled button, and this field going empty here would silently + // fall back to mcpgateway's APP_DOMAIN default (see + // oauthRedirectUriUnresolved above). + if (oauthRedirectUriUnresolved) { + setErrors({ + submit: "Still determining this deployment's OAuth redirect URI. Please try again.", + }); + return; + } + const formValid = validateForm(); if (formValid) { @@ -835,6 +866,7 @@ export function useMCPServerForm( authType, pendingOAuthGatewayId, handleOAuthFlow, + oauthRedirectUriUnresolved, ], ); @@ -854,8 +886,19 @@ export function useMCPServerForm( if (visibility === "team" && (!teamId || !teamId.trim())) { return false; } + if (oauthRedirectUriUnresolved) return false; return true; - }, [name, url, authType, oauthGrantType, oauthUsername, oauthPassword, visibility, teamId]); + }, [ + name, + url, + authType, + oauthGrantType, + oauthUsername, + oauthPassword, + visibility, + teamId, + oauthRedirectUriUnresolved, + ]); return { // State @@ -899,6 +942,9 @@ export function useMCPServerForm( clearOAuthNotification, fetchToolsNotification, clearFetchToolsNotification, + isOAuthRedirectUriLoading, + oauthRedirectUriError: oauthRedirectUriFetchError?.message, + retryOAuthRedirectUri, // Setters setName, diff --git a/src/i18n/locales/en-US/mcpServer.json b/src/i18n/locales/en-US/mcpServer.json index a2e74be5..43b7ca10 100644 --- a/src/i18n/locales/en-US/mcpServer.json +++ b/src/i18n/locales/en-US/mcpServer.json @@ -197,6 +197,9 @@ "mcpServer.auth.oauth.redirectUriAutoPlaceholder": "Determined automatically by the server", "mcpServer.auth.oauth.redirectUriAutoHelp": "Set automatically from the server's public URL when authorization starts. Set one explicitly only if this gateway needs a different redirect URI registered with the provider.", "mcpServer.auth.oauth.redirectUriLocalWarning": "This redirect URI points at localhost, which will not work for external OAuth providers.", + "mcpServer.auth.oauth.redirectUriLoading": "Loading redirect URI…", + "mcpServer.auth.oauth.redirectUriLoadError": "Couldn't load the default redirect URI. Save is disabled until this resolves.", + "mcpServer.auth.oauth.redirectUriRetry": "Retry", "mcpServer.auth.oauth.usernameLabel": "Username", "mcpServer.auth.oauth.usernamePlaceholder": "e.g. service-account", "mcpServer.auth.oauth.passwordLabel": "Password", diff --git a/src/i18n/locales/es-ES/mcpServer.json b/src/i18n/locales/es-ES/mcpServer.json index a91ce727..031eae99 100644 --- a/src/i18n/locales/es-ES/mcpServer.json +++ b/src/i18n/locales/es-ES/mcpServer.json @@ -197,6 +197,9 @@ "mcpServer.auth.oauth.redirectUriAutoPlaceholder": "Determinado automáticamente por el servidor", "mcpServer.auth.oauth.redirectUriAutoHelp": "Se configura automáticamente a partir de la URL pública del servidor 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": "Esta URI de redirección apunta a localhost, lo cual no funcionará con proveedores OAuth externos.", + "mcpServer.auth.oauth.redirectUriLoading": "Cargando URI de redirección…", + "mcpServer.auth.oauth.redirectUriLoadError": "No se pudo cargar la URI de redirección predeterminada. Guardar está deshabilitado hasta que esto se resuelva.", + "mcpServer.auth.oauth.redirectUriRetry": "Reintentar", "mcpServer.auth.oauth.usernameLabel": "Nombre de usuario", "mcpServer.auth.oauth.usernamePlaceholder": "p. ej. service-account", "mcpServer.auth.oauth.passwordLabel": "Contraseña", diff --git a/src/i18n/locales/pt-BR/mcpServer.json b/src/i18n/locales/pt-BR/mcpServer.json index 4e069ae7..6a96be4c 100644 --- a/src/i18n/locales/pt-BR/mcpServer.json +++ b/src/i18n/locales/pt-BR/mcpServer.json @@ -197,6 +197,9 @@ "mcpServer.auth.oauth.redirectUriAutoPlaceholder": "Determinado automaticamente pelo servidor", "mcpServer.auth.oauth.redirectUriAutoHelp": "Definido automaticamente a partir da URL pública do servidor 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": "Esta URI de redirecionamento aponta para localhost, o que não funcionará com provedores OAuth externos.", + "mcpServer.auth.oauth.redirectUriLoading": "Carregando URI de redirecionamento…", + "mcpServer.auth.oauth.redirectUriLoadError": "Não foi possível carregar a URI de redirecionamento padrão. Salvar está desabilitado até que isso seja resolvido.", + "mcpServer.auth.oauth.redirectUriRetry": "Tentar novamente", "mcpServer.auth.oauth.usernameLabel": "Nome de usuário", "mcpServer.auth.oauth.usernamePlaceholder": "ex.: service-account", "mcpServer.auth.oauth.passwordLabel": "Senha", From 234e6fd3aab0bb6d4406bd37d49c9bd0eb581f87 Mon Sep 17 00:00:00 2001 From: Marek Dano Date: Tue, 8 Sep 2026 12:32:21 +0100 Subject: [PATCH 6/6] fix: failing oauth playwright tests Signed-off-by: Marek Dano --- e2e/oauth-authorization.spec.ts | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/e2e/oauth-authorization.spec.ts b/e2e/oauth-authorization.spec.ts index 4171b34b..15d646a3 100644 --- a/e2e/oauth-authorization.spec.ts +++ b/e2e/oauth-authorization.spec.ts @@ -32,6 +32,19 @@ test.describe("OAuth authorization-code popup flow", () => { body: JSON.stringify({ gateways: [], nextCursor: null }), }); }); + // Default happy-path stub for the redirect_uri default fetch (see the + // dedicated test below for the split-deployment value it actually + // returns). Submission is gated on this resolving (useMCPServerForm.ts's + // oauthRedirectUriUnresolved), so leaving it unmocked would leave + // "Connect server" permanently disabled here the way it correctly does + // for a real deployment where this fetch fails. + await page.route("**/oauth/callback-url", async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ redirectUri: "https://app.example.com/oauth/callback" }), + }); + }); }); test("create -> popup -> postMessage -> activate -> fetch tools", async ({ page, context }) => { @@ -110,10 +123,11 @@ test.describe("OAuth authorization-code popup flow", () => { 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. + // Defaulted from the beforeEach's /oauth/callback-url stub (the + // redirect_uri fix, mcp-context-forge#6458) -- never guessed from + // window.location.origin. await expect(page.getByLabel(/Redirect URI/i)).toHaveValue( - "Determined automatically by the server", + "https://app.example.com/oauth/callback", ); await page.getByRole("button", { name: "Connect server" }).click();