- Notifications
You must be signed in to change notification settings - Fork 0
fix(security): constrain remote MCP/OpenAPI egress targets#53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
f1dc25ed89e04139869cc57efbc54edfd20cbf8c51b09e340c75017bFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -8,6 +8,7 @@ | ||
| import { timingSafeEqual } from "crypto"; | ||
| import { NextRequest, NextResponse } from "next/server"; | ||
| import { toolRegistry } from "@/lib/covenant/tool-registry"; | ||
| import { OutboundTargetError, validateOutboundTarget } from "@/lib/security/outbound-target"; | ||
| export const dynamic = "force-dynamic"; | ||
| @@ -56,20 +57,38 @@ async function handleProxy( | ||
| } | ||
| const path = `/${pathParts.join("/")}`; | ||
| const targetUrl = `${server.base_url}${path}${req.nextUrl.search}`; | ||
| let targetUrl: URL; | ||
| try { | ||
| const baseUrl = new URL(server.base_url); | ||
| const basePath = baseUrl.pathname.replace(/\/+$/, ""); | ||
| const requestPath = path.replace(/^\/+/, ""); | ||
| baseUrl.pathname = `${basePath}/${requestPath}`.replace(/\/{2,}/g, "/"); | ||
| baseUrl.search = req.nextUrl.search; | ||
| targetUrl = await validateOutboundTarget(baseUrl.toString()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When DNS for a registered upstream stalls, this awaited validation can block on Useful? React with 👍 / 👎. | ||
| } catch (error) { | ||
| if (error instanceof OutboundTargetError) { | ||
| return NextResponse.json( | ||
| { error: "Registered upstream target is not permitted", code: error.code }, | ||
| { status: 403 }, | ||
| ); | ||
| } | ||
| console.error("Proxy target validation failed", error); | ||
| return NextResponse.json({ error: "Registered upstream target is unavailable" }, { status: 502 }); | ||
| } | ||
| // Forward caller-supplied upstream headers, but never leak the cAPI internal | ||
| // credential or reverse-proxy internals to the registered destination. | ||
| // Forward only headers that are explicitly safe for registered upstreams. | ||
| // Caller credentials, cookies, proxy credentials, internal keys, response-only | ||
| // headers, and hop-by-hop headers must never leave cAPI through this route. | ||
| const forwardHeaders = new Headers(); | ||
| const blockedHeaders = new Set([ | ||
| "host", | ||
| "x-forwarded-host", | ||
| "x-forwarded-proto", | ||
| "x-api-key", | ||
| "x-covenant-admin-token", | ||
| const allowedRequestHeaders = new Set([ | ||
| "accept", | ||
| "content-type", | ||
| "if-match", | ||
| "if-none-match", | ||
| "range", | ||
| ]); | ||
| for (const [key, value] of req.headers.entries()) { | ||
| if (blockedHeaders.has(key.toLowerCase())) continue; | ||
| if (!allowedRequestHeaders.has(key.toLowerCase())) continue; | ||
| forwardHeaders.set(key, value); | ||
| } | ||
| @@ -91,6 +110,9 @@ async function handleProxy( | ||
| headers: forwardHeaders, | ||
| body: body ?? null, | ||
| signal: controller.signal, | ||
| // A previously validated public target must not be allowed to redirect | ||
| // the proxy to a private or metadata destination. | ||
| redirect: "error", | ||
reprewindai-dev marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| }); | ||
| clearTimeout(timer); | ||
| @@ -111,8 +133,9 @@ async function handleProxy( | ||
| clearTimeout(timer); | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| const isTimeout = message.includes("abort") || message.includes("timeout"); | ||
| console.error("MCP proxy upstream request failed", err); | ||
| return NextResponse.json( | ||
| { error: isTimeout ? "Upstream request timed out" : `Proxy error: ${message}` }, | ||
| { error: isTimeout ? "Upstream request timed out" : "Upstream request failed" }, | ||
| { status: isTimeout ? 504 : 502 }, | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -2,6 +2,7 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js"; | ||
| import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; | ||
| import { SSEClientTransport } from "@modelcontextprotocol/sdk/client/sse.js"; | ||
| import type { McpServerDescriptor } from "../schema"; | ||
| import { validateOutboundTarget } from "@/lib/security/outbound-target"; | ||
| const LOCAL_PROCESS_ENV_ALLOWLIST = [ | ||
| "PATH", | ||
| @@ -56,7 +57,8 @@ export class McpDriver { | ||
| env: buildLocalProcessEnv(descriptor), | ||
| }); | ||
| } else if (descriptor.type === "remote-sse" && descriptor.serverUrl) { | ||
| transport = new SSEClientTransport(new URL(descriptor.serverUrl)); | ||
| const validatedUrl = await validateOutboundTarget(descriptor.serverUrl); | ||
| transport = new SSEClientTransport(validatedUrl); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an allowlisted SSE endpoint responds with an HTTP redirect to a private or metadata address, constructing the SDK transport this way delegates the connection to its EventSource implementation, which follows redirects without calling Useful? React with 👍 / 👎. | ||
| } else { | ||
| throw new Error(`Unsupported or misconfigured MCP descriptor type: ${descriptor.type}`); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { validateOutboundTarget } from "./outbound-target"; | ||
| const publicResolver = async () => ["93.184.216.34"]; | ||
| describe("validateOutboundTarget", () => { | ||
| it("accepts an explicitly allowlisted public HTTPS host", async () => { | ||
| const url = await validateOutboundTarget("https://api.example.com/v1", { | ||
| production: true, | ||
| allowedHosts: ["api.example.com"], | ||
| resolver: publicResolver, | ||
| }); | ||
| expect(url.hostname).toBe("api.example.com"); | ||
| }); | ||
| it("fails closed in production when no host allowlist is configured", async () => { | ||
| await expect(validateOutboundTarget("https://api.example.com", { | ||
| production: true, | ||
| allowedHosts: [], | ||
| resolver: publicResolver, | ||
| })).rejects.toMatchObject({ code: "OUTBOUND_ALLOWLIST_UNCONFIGURED" }); | ||
| }); | ||
| it.each([ | ||
| "http://127.0.0.1:8080", | ||
| "http://169.254.169.254/latest/meta-data", | ||
| "http://10.1.2.3", | ||
| "http://192.168.1.2", | ||
| "http://[::1]/", | ||
| ])("rejects local or private literal address %s", async (target) => { | ||
| await expect(validateOutboundTarget(target, { | ||
| production: false, | ||
| allowedHosts: [], | ||
| })).rejects.toMatchObject({ code: "OUTBOUND_ADDRESS_FORBIDDEN" }); | ||
| }); | ||
| it("rejects a public-looking hostname that resolves to a private address", async () => { | ||
| await expect(validateOutboundTarget("https://api.example.com", { | ||
| production: true, | ||
| allowedHosts: ["api.example.com"], | ||
| resolver: async () => ["10.0.0.8"], | ||
| })).rejects.toMatchObject({ code: "OUTBOUND_ADDRESS_FORBIDDEN" }); | ||
| }); | ||
| it("rejects non-allowlisted hosts", async () => { | ||
| await expect(validateOutboundTarget("https://other.example.com", { | ||
| production: true, | ||
| allowedHosts: ["api.example.com"], | ||
| resolver: publicResolver, | ||
| })).rejects.toMatchObject({ code: "OUTBOUND_HOST_NOT_ALLOWLISTED" }); | ||
| }); | ||
| it.each([ | ||
| "file:///etc/passwd", | ||
| "https://user:pass@api.example.com/", | ||
| "https://api.example.com/#fragment", | ||
| "http://localhost:3000/", | ||
| ])("rejects unsafe URL form %s", async (target) => { | ||
| await expect(validateOutboundTarget(target, { | ||
| production: false, | ||
| allowedHosts: [], | ||
| resolver: publicResolver, | ||
| })).rejects.toBeTruthy(); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: reprewindai-dev/cAPI
Length of output: 9939
🏁 Script executed:
Repository: reprewindai-dev/cAPI
Length of output: 7604
🏁 Script executed:
Repository: reprewindai-dev/cAPI
Length of output: 40015
🏁 Script executed:
Repository: reprewindai-dev/cAPI
Length of output: 27641
🌐 Web query:
@modelcontextprotocol/sdk 1.30.0 SSEClientTransport source redirect reconnect POST💡 Result:
In the Model Context Protocol (MCP) TypeScript SDK, the
SSEClientTransportis used for Server-Sent Events (SSE) connections, where SSE is employed for receiving messages andPOSTrequests are used for sending messages [1][2]. Regarding authentication, redirects, and reconnection: 1. Authentication and Reconnection: When the server responds with a 401 Unauthorized status, theSSEClientTransportcalls theonUnauthorizedmethod (if provided in theauthProviderconfiguration) to refresh credentials [1][3]. After refreshing, the request is retried exactly once [1][3]. If the subsequent attempt also fails with a 401, or if noonUnauthorizedhandler is provided, anUnauthorizedErroris thrown [4][1][3]. 2. Interactive OAuth Redirects: For interactive OAuth flows, when anUnauthorizedErroroccurs, the client is expected to redirect the user to an authorization URL [1][3]. After the user completes the authorization and is redirected back to the client application, thefinishAuth(authorizationCode)method must be called with the provided authorization code before attempting to reconnect [4][1]. 3. SSE Lifecycle and Redirects: Note thatSSEClientTransportdoes not inherently manage complex HTTP redirect logic for the SSE stream connection itself beyond standardEventSourcebehavior [4][5]. BecauseSSEClientTransportis now deprecated in favor ofStreamableHTTPClientTransport, it is recommended to migrate to the newer transport for better connection and lifecycle management [1][6]. If an SSE connection is lost, the transport does not automatically re-establish state; developers often implement logic to recreate the transport or handle reconnects on a per-operation basis if long-lived sessions are required [7]. 4. POST Requests: The SDK usesPOSTrequests for sending JSON-RPC messages [8]. TheSSEClientTransportOptionsallow for customization of these requests through therequestInitproperty, which can be used to set custom headers or other request configurations [6][3].Citations:
🏁 Script executed:
Repository: reprewindai-dev/cAPI
Length of output: 13078
🏁 Script executed:
Repository: reprewindai-dev/cAPI
Length of output: 13290
🏁 Script executed:
Repository: reprewindai-dev/cAPI
Length of output: 4053
Add explicit remote-SSE redirect verification.
The SDK follows redirects for SSE connections and reconnection attempts.
SSEClientTransportsends messagePOSTrequests without redirect-hop validation. Add deployment tests for all three paths, or constrain remote SSE until equivalent enforcement exists.🤖 Prompt for AI Agents