mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-26 18:31:34 +01:00
fix(apps): guide generic MCP connection failures
Map OAuth origin, duplicate-name, and DNS failures to stable operator guidance, and derive collision-resistant defaults from host, port, and path. Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
048c3578e3
commit
ba603abace
8 files changed
+225
-9
No files matched your search
@@ -274,7 +274,10 @@ async function createCompany(db: ReturnType<typeof createDb>) {
|
||||
|
||||
function createRouteApp(
|
||||
db: ReturnType<typeof createDb>,
|
||||
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",
|
||||
|
||||
@@ -116,6 +116,24 @@ function rebindingLookup(): { lookup: () => Promise<Array<{ address: string; fam
|
||||
}
|
||||
|
||||
describe("guarded remote HTTP fetch (PAP-17098 DNS rebinding)", () => {
|
||||
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();
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
|
||||
@@ -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<unknown>();
|
||||
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[],
|
||||
|
||||
@@ -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;
|
||||
},
|
||||
|
||||
@@ -202,7 +202,7 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => {
|
||||
const nameInput = Array.from(container.querySelectorAll<HTMLInputElement>("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<HTMLInputElement>("#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<HTMLInputElement>("#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();
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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",
|
||||
|
||||
Reference in new issue
Block a user