From 18d160d66a50c1d42271a4f135237b56d9fa3a13 Mon Sep 17 00:00:00 2001 From: Dotta Date: Sun, 16 Aug 2026 23:38:08 +0000 Subject: [PATCH] fix(oauth): fall back from private CIMD origins Co-Authored-By: Paperclip --- .../__tests__/generic-mcp-connection.test.ts | 46 +++++++- .../__tests__/oauth-client-metadata.test.ts | 31 +++++ server/src/services/tool-access.ts | 111 ++++++++++++------ 3 files changed, 150 insertions(+), 38 deletions(-) create mode 100644 server/src/__tests__/oauth-client-metadata.test.ts diff --git a/server/src/__tests__/generic-mcp-connection.test.ts b/server/src/__tests__/generic-mcp-connection.test.ts index 082740d633d..d278796d5db 100644 --- a/server/src/__tests__/generic-mcp-connection.test.ts +++ b/server/src/__tests__/generic-mcp-connection.test.ts @@ -95,6 +95,8 @@ type FixtureOptions = { tokenFailure?: { status: number; body: Record }; /** Fail the dynamic client registration endpoint with this exact body. */ registrationFailure?: { status: number; body: Record }; + /** Extra provider-owned callbacks returned alongside the requested callback. */ + registrationExtraRedirectUris?: string[]; }; type FixtureRequest = { @@ -210,8 +212,11 @@ function installMcpOAuthFixture(options: FixtureOptions = {}) { return jsonResponse({ client_id: "fixture-dcr-client", // A conforming server echoes back what it registered, and Paperclip - // rejects the response when it doesn't match what was requested. - redirect_uris: requested.redirect_uris, + // requires its own callback even when the provider adds a routing URI. + redirect_uris: [ + ...(requested.redirect_uris as string[]), + ...(options.registrationExtraRedirectUris ?? []), + ], grant_types: requested.grant_types, response_types: requested.response_types, token_endpoint_auth_method: requested.token_endpoint_auth_method, @@ -579,7 +584,9 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { it("prefers a Client ID Metadata Document over dynamic registration", async () => { const fixture = installMcpOAuthFixture({ auth: "oauth", cimd: true }); const company = await createCompany(db); - const service = toolAccessService(db); + const service = toolAccessService(db, { + oauthClientMetadataLookup: async () => [{ address: "93.184.216.34", family: 4 }], + }); const connected = await service.connectGalleryApp(company.id, { link: MCP_URL, name: "Fixture CIMD" }); const start = await service.startOAuth(company.id, connected.connectionId, { @@ -593,6 +600,39 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { expect(fixture.requestsTo("/register")).toHaveLength(0); }); + it("replaces a private-only Client ID Metadata Document with dynamic registration", async () => { + const fixture = installMcpOAuthFixture({ + auth: "oauth", + cimd: true, + registrationExtraRedirectUris: [`${ISSUER}/oauth/callback/`], + }); + const company = await createCompany(db); + let metadataAddress = "93.184.216.34"; + const service = toolAccessService(db, { + oauthClientMetadataLookup: async () => [{ address: metadataAddress, family: 4 }], + }); + + const connected = await service.connectGalleryApp(company.id, { link: MCP_URL, name: "Fixture private CIMD" }); + const firstStart = await service.startOAuth(company.id, connected.connectionId, { + redirectUri: "https://paperclip.tailnet.test:42001/api/tools/oauth/callback", + actor: { actorType: "user", actorId: "board-user" }, + }); + expect(firstStart.registrationSource).toBe("cimd"); + + // A private DNS answer models a Tailscale/MagicDNS callback. The first start + // also proves retry migration: a connection that persisted the now-unusable + // CIMD client id must not keep presenting it forever. + metadataAddress = "100.100.100.100"; + const retry = await service.startOAuth(company.id, connected.connectionId, { + redirectUri: "https://paperclip.tailnet.test:42001/api/tools/oauth/callback", + actor: { actorType: "user", actorId: "board-user" }, + }); + + expect(retry.registrationSource).toBe("dcr"); + expect(new URL(retry.authorizationUrl).searchParams.get("client_id")).toBe("fixture-dcr-client"); + expect(fixture.requestsTo("/register")).toHaveLength(1); + }); + it("falls back to dynamic registration when the callback is not public HTTPS", async () => { const fixture = installMcpOAuthFixture({ auth: "oauth", cimd: true }); const company = await createCompany(db); diff --git a/server/src/__tests__/oauth-client-metadata.test.ts b/server/src/__tests__/oauth-client-metadata.test.ts new file mode 100644 index 00000000000..94770d033a7 --- /dev/null +++ b/server/src/__tests__/oauth-client-metadata.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from "vitest"; + +import { + OAUTH_CLIENT_ID_METADATA_DOCUMENT_PATH, + resolveOAuthClientIdMetadataDocumentUrl, +} from "../services/tool-access.js"; + +describe("OAuth Client ID Metadata Document selection", () => { + it("uses a publicly resolved HTTPS callback origin", async () => { + const redirectUri = "https://paperclip.example:42001/api/tools/oauth/callback"; + + await expect(resolveOAuthClientIdMetadataDocumentUrl( + redirectUri, + async () => [{ address: "93.184.216.34", family: 4 }], + )).resolves.toBe(`https://paperclip.example:42001${OAUTH_CLIENT_ID_METADATA_DOCUMENT_PATH}`); + }); + + it("rejects a Tailscale-range hostname so OAuth can fall back to DCR", async () => { + await expect(resolveOAuthClientIdMetadataDocumentUrl( + "https://paperclip.tailnet.example:42001/api/tools/oauth/callback", + async () => [{ address: "100.100.100.100", family: 4 }], + )).resolves.toBeNull(); + }); + + it.each([ + "http://localhost:3100/api/tools/oauth/callback", + "https://127.0.0.1:3100/api/tools/oauth/callback", + ])("rejects a non-public callback origin %s", async (redirectUri) => { + await expect(resolveOAuthClientIdMetadataDocumentUrl(redirectUri)).resolves.toBeNull(); + }); +}); diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index c84a5809f78..dd5df47d9a1 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -130,7 +130,11 @@ import { badRequest, conflict, forbidden, HttpError, notFound, unprocessable } f import { logger } from "../middleware/logger.js"; import { logActivity } from "./activity-log.js"; import { mcpHttpRequestHeaders, parseMcpHttpResponseBody } from "./mcp-http.js"; -import { assertPublicRemoteHttpEndpoint, parseRemoteHttpEndpoint } from "./remote-http-endpoint-guard.js"; +import { + assertPublicRemoteHttpEndpoint, + parseRemoteHttpEndpoint, + type RemoteHttpEndpointLookup, +} from "./remote-http-endpoint-guard.js"; import { guardedRemoteHttpFetch, type GuardedRemoteHttpFetchOptions } from "./remote-http-fetch.js"; import { secretService } from "./secrets.js"; import { toolAccessPolicyService } from "./tool-access-policy.js"; @@ -267,6 +271,51 @@ function oauthProviderErrorMessage(providerError: string | null, fallback: strin */ export const OAUTH_CLIENT_ID_METADATA_DOCUMENT_PATH = "/api/tools/oauth/client-metadata"; +/** + * Resolve the URL Paperclip would use as a CIMD client id, but only when its + * hostname is not known to resolve into a private network. + * + * An authorization server fetches this URL from outside Paperclip's network and + * will normally apply an SSRF guard. Tailscale/MagicDNS names are HTTPS but + * resolve into 100.64.0.0/10, so presenting one as a client id can only produce + * an `invalid_client` response. A local DNS failure remains inconclusive because + * split-horizon public DNS may still let the authorization server resolve it. + */ +export async function resolveOAuthClientIdMetadataDocumentUrl( + redirectUri: string, + lookup?: RemoteHttpEndpointLookup, +): Promise { + try { + const parsed = new URL(redirectUri); + if (parsed.protocol !== "https:") return null; + const hostname = parsed.hostname.replace(/^\[|\]$/g, "").toLowerCase(); + const isLoopback = hostname === "localhost" + || hostname.endsWith(".localhost") + || hostname === "::1" + || /^127(?:\.\d{1,3}){3}$/.test(hostname); + if (isLoopback) return null; + const metadataUrl = new URL(OAUTH_CLIENT_ID_METADATA_DOCUMENT_PATH, parsed.origin).toString(); + try { + await assertPublicRemoteHttpEndpoint( + new URL(metadataUrl), + { allowPrivateNetwork: false, lookup }, + (message, code) => Object.assign(new Error(message), { code }), + ); + } catch (error) { + if ( + error instanceof Error + && "code" in error + && error.code === "remote_http_private_endpoint" + ) { + return null; + } + } + return metadataUrl; + } catch { + return null; + } +} + /** * Paperclip's client metadata for CIMD (RFC 7591 metadata, served rather than * registered). Only the callback for this deployment appears in it, so an @@ -421,6 +470,8 @@ type ToolAccessServiceOptions = { deploymentExposure?: DeploymentExposure; trustedLocalStdioRuntimeHost?: string | null; now?: () => Date; + /** Test seam for deciding whether an OAuth client metadata URL is publicly resolvable. */ + oauthClientMetadataLookup?: RemoteHttpEndpointLookup; }; type DbTransaction = Parameters[0]>[0]; @@ -4775,30 +4826,6 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} return oauthClientConfig(provider); } - /** - * The URL of Paperclip's published Client ID Metadata Document, derived from the - * OAuth callback so the two are guaranteed same-origin. - * - * CIMD requires a `client_id` the authorization server can fetch, so it is only - * available when this deployment has a public HTTPS base URL. Loopback and - * plain-http development fall through to DCR or manual client credentials. - */ - function clientIdMetadataDocumentUrl(redirectUri: string): string | null { - try { - const parsed = new URL(redirectUri); - if (parsed.protocol !== "https:") return null; - const hostname = parsed.hostname.replace(/^\[|\]$/g, "").toLowerCase(); - const isLoopback = hostname === "localhost" - || hostname.endsWith(".localhost") - || hostname === "::1" - || /^127(?:\.\d{1,3}){3}$/.test(hostname); - if (isLoopback) return null; - return new URL(OAUTH_CLIENT_ID_METADATA_DOCUMENT_PATH, parsed.origin).toString(); - } catch { - return null; - } - } - async function oauthClientForConnection( connection: typeof toolConnections.$inferSelect, provider: string, @@ -5347,19 +5374,23 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} record: Record, field: "redirect_uris" | "grant_types" | "response_types", expected: string[], + options: { allowAdditional?: boolean } = {}, ) { if (record[field] === undefined) throw invalidOAuthDcrResponse(field, "missing"); const value = record[field]; if ( !Array.isArray(value) - || value.length !== expected.length + || value.length < expected.length + || value.length > 32 || value.some((entry) => typeof entry !== "string" || entry.length === 0 || entry.length > 2_048) ) { throw invalidOAuthDcrResponse(field, "invalid_array"); } - const actual = [...value].sort(); - const required = [...expected].sort(); - if (actual.some((entry, index) => entry !== required[index])) { + const actual = new Set(value); + if ( + expected.some((entry) => !actual.has(entry)) + || (!options.allowAdditional && actual.size !== expected.length) + ) { throw invalidOAuthDcrResponse(field, "registered_value_mismatch"); } } @@ -5436,7 +5467,12 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} required: false, maxLength: MAX_OAUTH_DCR_CLIENT_SECRET_LENGTH, }); - assertOAuthDcrArray(record, "redirect_uris", requestedMetadata.redirect_uris); + // Some authorization servers add a first-party routing callback to the + // registered redirect set. Paperclip never navigates to that URI, and PKCE + // still binds codes to this client, so accept a bounded superset while + // requiring our exact callback to remain registered. Grants and response + // types stay exact because Paperclip must not adopt unsupported flows. + assertOAuthDcrArray(record, "redirect_uris", requestedMetadata.redirect_uris, { allowAdditional: true }); assertOAuthDcrArray(record, "grant_types", requestedMetadata.grant_types); assertOAuthDcrArray(record, "response_types", requestedMetadata.response_types); if ( @@ -5602,10 +5638,15 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} connection: typeof toolConnections.$inferSelect, endpoints: OAuthProviderEndpoints, redirectUri: string, + clientIdMetadataDocumentUrl: string | null, ): boolean { const oauth = oauthConfig(connection); if (typeof oauth.clientId !== "string" || !oauth.clientId.trim()) return false; const source = typeof oauth.clientRegistrationSource === "string" ? oauth.clientRegistrationSource : null; + // A URL client id that now resolves only to a private network is unusable by + // an external authorization server. Treat the stored binding as stale so a + // retry can replace it with a dynamically registered client. + if (source === "cimd" && oauth.clientId !== clientIdMetadataDocumentUrl) return false; // A manually preregistered client was registered by the operator against // Paperclip's callback, so it has no recorded callback until first use. const redirectMatches = source === "manual" @@ -5708,8 +5749,11 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} if (configured.clientId) { return { connection: input.connection, client: configured, source: "preconfigured" as const }; } + const metadataDocumentUrl = input.endpoints.clientIdMetadataDocumentSupported + ? await resolveOAuthClientIdMetadataDocumentUrl(input.redirectUri, options.oauthClientMetadataLookup) + : null; // 2. Client material already bound to this issuer/resource/callback/company. - if (oauthClientBindingMatches(input.connection, input.endpoints, input.redirectUri)) { + if (oauthClientBindingMatches(input.connection, input.endpoints, input.redirectUri, metadataDocumentUrl)) { const bound = await stampOAuthClientBinding(input.connection, input.endpoints, input.redirectUri); return { connection: bound, @@ -5743,7 +5787,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} if (latestConfigured.clientId) { return { connection: latest, client: latestConfigured, source: "preconfigured" as const }; } - if (oauthClientBindingMatches(latest, input.endpoints, input.redirectUri)) { + if (oauthClientBindingMatches(latest, input.endpoints, input.redirectUri, metadataDocumentUrl)) { const bound = await stampOAuthClientBinding(latest, input.endpoints, input.redirectUri); return { connection: bound, @@ -5753,9 +5797,6 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} } // 3. Client ID Metadata Documents: no registration call at all, so prefer // them over DCR when the authorization server advertises support. - const metadataDocumentUrl = input.endpoints.clientIdMetadataDocumentSupported - ? clientIdMetadataDocumentUrl(input.redirectUri) - : null; if (metadataDocumentUrl) { const adopted = await adoptClientIdMetadataDocument({ connection: latest,