diff --git a/server/src/__tests__/generic-mcp-connection.test.ts b/server/src/__tests__/generic-mcp-connection.test.ts index bcd99b7a52a..6a8f011a33e 100644 --- a/server/src/__tests__/generic-mcp-connection.test.ts +++ b/server/src/__tests__/generic-mcp-connection.test.ts @@ -274,7 +274,10 @@ async function createCompany(db: ReturnType) { function createRouteApp( db: ReturnType, - deployment?: { deploymentMode: "authenticated"; deploymentExposure: "public" }, + deployment?: { + deploymentMode: "authenticated" | "local_trusted"; + deploymentExposure: "public" | "private"; + }, requestLogger?: express.RequestHandler, ) { const app = express(); @@ -360,6 +363,73 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { expect(connection!.config).not.toHaveProperty("connectionMethodKey"); }); + it("emits DNS guidance for a real NXDOMAIN failure", async () => { + const company = await createCompany(db); + const app = createRouteApp(db, { + deploymentMode: "local_trusted", + deploymentExposure: "private", + }); + + const response = await request(app) + .post(`/api/companies/${company.id}/tools/apps/connect`) + .send({ link: "https://qa-nonexistent.invalid/mcp", name: "Missing DNS fixture" }); + + // Public-mode preflight returns 400; local-mode platform fetch reports the + // same failure from the health check as 502. The machine code is the stable + // UI contract across both paths. + expect([400, 502]).toContain(response.status); + expect(response.body).toMatchObject({ + details: { code: "remote_http_dns_failed" }, + }); + expect(response.body.error).not.toBe("fetch failed"); + await expect(db.select().from(toolApplications)).resolves.toHaveLength(0); + }); + + it("emits a stable code when an application name is already used", async () => { + const company = await createCompany(db); + await db.insert(toolApplications).values({ + companyId: company.id, + applicationKey: `existing:${randomUUID()}`, + name: "Taken fixture name", + type: "mcp_http", + status: "active", + metadata: {}, + }); + const app = createRouteApp(db, { + deploymentMode: "local_trusted", + deploymentExposure: "private", + }); + + const response = await request(app) + .post(`/api/companies/${company.id}/tools/apps/connect`) + .send({ link: "http://127.0.0.1:8848/mcp", name: "Taken fixture name" }) + .expect(409); + + expect(response.body).toMatchObject({ + details: { code: "tool_access_name_conflict" }, + }); + }); + + it("emits deployment guidance without exposing server env-var names", async () => { + vi.stubEnv("PAPERCLIP_PUBLIC_URL", ""); + vi.stubEnv("PAPERCLIP_AUTH_PUBLIC_BASE_URL", ""); + vi.stubEnv("BETTER_AUTH_URL", ""); + vi.stubEnv("BETTER_AUTH_BASE_URL", ""); + const app = createRouteApp(db, { + deploymentMode: "local_trusted", + deploymentExposure: "private", + }); + + const response = await request(app) + .get("/api/tools/oauth/client-metadata") + .expect(422); + + expect(response.body).toMatchObject({ + details: { code: "oauth_redirect_origin_unsupported" }, + }); + expect(JSON.stringify(response.body)).not.toContain("PAPERCLIP_PUBLIC_URL"); + }); + it("stores a bearer key as a secret and never reads it back", async () => { installMcpOAuthFixture({ auth: "header", diff --git a/server/src/__tests__/remote-http-rebinding.test.ts b/server/src/__tests__/remote-http-rebinding.test.ts index fc67679f621..14766737583 100644 --- a/server/src/__tests__/remote-http-rebinding.test.ts +++ b/server/src/__tests__/remote-http-rebinding.test.ts @@ -116,6 +116,24 @@ function rebindingLookup(): { lookup: () => Promise { + it("turns a platform-fetch DNS cause into the stable DNS code", async () => { + const cause = Object.assign(new Error("getaddrinfo ENOTFOUND missing.invalid"), { + code: "ENOTFOUND", + }); + const unpinnedFetch = (async () => { + throw Object.assign(new TypeError("fetch failed"), { cause }); + }) as typeof fetch; + + await expect(guardedRemoteHttpFetch("https://8.8.8.8/mcp", {}, { + allowPrivateNetwork: true, + unpinnedFetch, + error: guardError, + })).rejects.toMatchObject({ + code: "remote_http_dns_failed", + message: "Remote MCP connection hostname could not be resolved", + }); + }); + it("pins the connection to the approved address so a rebind never reaches loopback", async () => { const upstream = await startServer(); const internal = await startServer(); diff --git a/server/src/routes/tool-access.ts b/server/src/routes/tool-access.ts index b9ebcbae253..36d0fef13e3 100644 --- a/server/src/routes/tool-access.ts +++ b/server/src/routes/tool-access.ts @@ -113,7 +113,10 @@ export function toolAccessRoutes( function oauthRedirectUri() { const configured = configuredPublicBaseUrl(); if (!configured) { - throw unprocessable("OAuth connections require PAPERCLIP_PUBLIC_URL or an auth public base URL"); + throw unprocessable( + "This Paperclip needs a browser-reachable HTTPS address (or loopback HTTP) before browser sign-in can start.", + { code: "oauth_redirect_origin_unsupported" }, + ); } return new URL("/api/tools/oauth/callback", configured).toString(); } diff --git a/server/src/services/remote-http-fetch.ts b/server/src/services/remote-http-fetch.ts index 87bfae484c4..5190df97df1 100644 --- a/server/src/services/remote-http-fetch.ts +++ b/server/src/services/remote-http-fetch.ts @@ -32,6 +32,7 @@ const DEFAULT_CONNECT_TIMEOUT_MS = 10_000; * pass `responseTimeoutMs` so this default never truncates it. */ const DEFAULT_RESPONSE_TIMEOUT_MS = 30_000; +const DNS_RESOLUTION_ERROR_CODES = new Set(["ENODATA", "ENOTFOUND", "EAI_AGAIN"]); /** How a verified socket is opened. Overridable so tests can simulate a rebind. */ export type RemoteHttpSocketFactory = (target: { @@ -103,11 +104,34 @@ export async function guardedRemoteHttpFetch( const literalHost = isIP(endpoint.hostname.replace(/^\[|\]$/g, "")) !== 0; const platformFetch = options.unpinnedFetch ?? fetch; if (approved.length === 0 || literalHost) { - return platformFetch(endpoint.toString(), { ...init, redirect: "manual" }); + try { + return await platformFetch(endpoint.toString(), { ...init, redirect: "manual" }); + } catch (error) { + if (isDnsResolutionError(error)) { + throw options.error( + "Remote MCP connection hostname could not be resolved", + "remote_http_dns_failed", + ); + } + throw error; + } } return pinnedRequest(endpoint, approved, init, options); } +/** Node's platform fetch wraps DNS failures in TypeError.cause. */ +function isDnsResolutionError(error: unknown): boolean { + const seen = new Set(); + let current = error; + while (typeof current === "object" && current !== null && !seen.has(current)) { + seen.add(current); + const record = current as { code?: unknown; cause?: unknown }; + if (typeof record.code === "string" && DNS_RESOLUTION_ERROR_CODES.has(record.code)) return true; + current = record.cause; + } + return false; +} + async function pinnedRequest( endpoint: URL, approved: string[], diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index 03df823afbd..d8abf8374a0 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -1655,6 +1655,9 @@ function healthFailureHttpStatus(failure: { status: ToolConnectionHealthStatus; function sanitizeHttpFailure(error: unknown): { status: ToolConnectionHealthStatus; message: string; code: string } { if (error instanceof HttpError) { const code = asRecord(error.details).code; + if (typeof code === "string" && code.startsWith("remote_http_")) { + return { status: "error", message: error.message, code }; + } if (isOAuthEndpointRejection(error)) { return { status: "error", message: error.message, code: String(code) }; } @@ -4120,7 +4123,9 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} function defaultLinkName(link: string): string { try { const url = new URL(link); - return url.hostname.replace(/^www\./, "") || "MCP app"; + const host = url.host.replace(/^www\./, ""); + const path = url.pathname === "/" ? "" : url.pathname.replace(/\/+$/, ""); + return `${host}${path}`.slice(0, 160) || "MCP app"; } catch { return "MCP app"; } @@ -7285,7 +7290,11 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} ), ) .limit(1); - if (duplicate) throw conflict("A tool access record with that name already exists"); + if (duplicate) { + throw conflict("A tool access record with that name already exists", { + code: "tool_access_name_conflict", + }); + } } const [row] = await db .update(toolApplications) @@ -8863,7 +8872,9 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} constraint?.includes("tool_applications") || /duplicate key value|unique constraint|tool_applications_company_id_name_unique/i.test(message) ) { - throw conflict("A tool access record with that name already exists"); + throw conflict("A tool access record with that name already exists", { + code: "tool_access_name_conflict", + }); } throw error; }, diff --git a/ui/src/pages/apps/AppsConnect.test.tsx b/ui/src/pages/apps/AppsConnect.test.tsx index fd6d8bce805..b50e779460a 100644 --- a/ui/src/pages/apps/AppsConnect.test.tsx +++ b/ui/src/pages/apps/AppsConnect.test.tsx @@ -202,7 +202,7 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { const nameInput = Array.from(container.querySelectorAll("input")).find( (i) => i.getAttribute("placeholder") === "My app", ); - expect(nameInput?.value).toBe("example.com"); + expect(nameInput?.value).toBe("example.com/actions"); }); it("opens the selected app directly on its setup route", async () => { @@ -628,7 +628,7 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { expect(connectAppMock).toHaveBeenCalledTimes(1); const [, input] = connectAppMock.mock.calls[0]; - expect(input).toMatchObject({ link: "https://www.example.com/actions", name: "example.com" }); + expect(input).toMatchObject({ link: "https://www.example.com/actions", name: "example.com/actions" }); expect(input.credentialValues).toBeUndefined(); }); @@ -1148,6 +1148,14 @@ describe("AppsConnect — guided generic MCP flow (PAP-17087)", () => { return new ApiError(message, status, { error: message, details: { code } }); } + it("defaults the connection name from host, port, and path", async () => { + await render(); + await gotoLinkFrame(container, "http://127.0.0.1:47399/mcp"); + + expect(container.querySelector("#generic-mcp-name")?.value) + .toBe("127.0.0.1:47399/mcp"); + }); + it("keeps the endpoint host and Unverified server visible from setup through review", async () => { await render(); await gotoLinkFrame(container, "https://mcp.example.test/mcp"); @@ -1249,6 +1257,43 @@ describe("AppsConnect — guided generic MCP flow (PAP-17087)", () => { expect(container.textContent).toContain("We couldn't find that host"); }); + it("uses deployment guidance without rendering the server env-var message", async () => { + connectAppMock.mockRejectedValue(apiError( + 422, + "oauth_redirect_origin_unsupported", + "OAuth connections require PAPERCLIP_PUBLIC_URL or an auth public base URL", + )); + await render(); + await gotoLinkFrame(container, "https://mcp.example.test/mcp"); + await act(async () => { + buttonByText("Check link")?.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + await flushReact(); + await flushReact(); + + expect(container.textContent).toContain("This Paperclip needs a public HTTPS address first"); + expect(container.textContent).not.toContain("PAPERCLIP_PUBLIC_URL"); + }); + + it("renders a name conflict as name guidance and focuses the Name field", async () => { + connectAppMock.mockRejectedValue(apiError( + 409, + "tool_access_name_conflict", + "A tool access record with that name already exists", + )); + await render(); + await gotoLinkFrame(container, "https://mcp.example.test/mcp"); + await act(async () => { + buttonByText("Check link")?.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + await flushReact(); + await flushReact(); + + const nameInput = container.querySelector("#generic-mcp-name"); + expect(container.textContent).toContain("That name is taken"); + expect(document.activeElement).toBe(nameInput); + }); + it("opens advanced authentication when the server wants a credential we can't discover", async () => { connectAppMock.mockRejectedValue(apiError(502, "oauth_challenge", "This app needs you to sign in.")); await render(); diff --git a/ui/src/pages/apps/generic-mcp-connect.test.ts b/ui/src/pages/apps/generic-mcp-connect.test.ts index d8cae041b72..ca6952b0a6d 100644 --- a/ui/src/pages/apps/generic-mcp-connect.test.ts +++ b/ui/src/pages/apps/generic-mcp-connect.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { canSubmitGenericConnect, customHeaderError, + defaultGenericMcpName, endpointHost, genericConnectGuidance, genericConnectPayload, @@ -35,6 +36,20 @@ describe("endpointHost", () => { }); }); +describe("defaultGenericMcpName", () => { + it("keeps the port and path so endpoints on one host get distinct names", () => { + expect(defaultGenericMcpName("http://127.0.0.1:47399/mcp")) + .toBe("127.0.0.1:47399/mcp"); + expect(defaultGenericMcpName("http://127.0.0.1:47400/analytics/mcp")) + .toBe("127.0.0.1:47400/analytics/mcp"); + }); + + it("does not copy a potentially secret query string into the name", () => { + expect(defaultGenericMcpName("https://www.example.test/mcp?token=secret")) + .toBe("example.test/mcp"); + }); +}); + describe("genericConnectGuidance", () => { it("names the URL as the thing to fix for invalid, unsafe and unreachable endpoints", () => { for (const code of [ @@ -57,6 +72,11 @@ describe("genericConnectGuidance", () => { expect(genericConnectGuidance("oauth_redirect_origin_unsupported", null).focus).toBe("deployment"); }); + it("points at the name field when the connection name is already used", () => { + const guidance = genericConnectGuidance("tool_access_name_conflict", null); + expect(guidance).toMatchObject({ title: "That name is taken", focus: "name" }); + }); + it("passes a rejected header's own message through", () => { const guidance = genericConnectGuidance( "mcp_header_rejected", diff --git a/ui/src/pages/apps/generic-mcp-connect.ts b/ui/src/pages/apps/generic-mcp-connect.ts index e68c560e52e..075c21fc52a 100644 --- a/ui/src/pages/apps/generic-mcp-connect.ts +++ b/ui/src/pages/apps/generic-mcp-connect.ts @@ -35,12 +35,31 @@ export function endpointHost(url: string): string | null { } } +/** + * A useful, non-secret default label for an arbitrary endpoint. + * + * Keep the port and path: one host commonly serves several MCP endpoints, and + * collapsing all of them to the hostname makes the second connection fail the + * company-wide application-name constraint. Query strings are deliberately + * excluded because remote MCP URLs can carry credentials there. + */ +export function defaultGenericMcpName(url: string): string | null { + try { + const parsed = new URL(url); + const host = parsed.host.replace(/^www\./, ""); + const path = parsed.pathname === "/" ? "" : parsed.pathname.replace(/\/+$/, ""); + return `${host}${path}`.slice(0, 160) || null; + } catch { + return null; + } +} + /** * Where the operator has to look to fix a failed connect attempt. The wizard uses * this to decide whether to reopen the URL field or the credential section, rather * than leaving them to guess which of the two was wrong. */ -export type GenericConnectFocus = "url" | "credentials" | "deployment" | "none"; +export type GenericConnectFocus = "url" | "name" | "credentials" | "deployment" | "none"; export interface GenericConnectGuidance { title: string; @@ -86,6 +105,12 @@ export function genericConnectGuidance( body: fallback, focus: "credentials", }; + case "tool_access_name_conflict": + return { + title: "That name is taken", + body: "Choose a different name for this connection, then try again.", + focus: "name", + }; case "oauth_challenge": return { title: "This server wants a credential",