From b0515d65a2f9d1cb2efa37da2490aa5e558959ea Mon Sep 17 00:00:00 2001 From: Dotta Date: Wed, 12 Aug 2026 12:01:54 +0000 Subject: [PATCH] fix(security): guard connection token brokers Co-Authored-By: Paperclip --- doc/DEVELOPING.md | 16 ++ doc/connections/SECURITY-THREAT-MODEL.md | 4 + .../src/__tests__/tool-access-service.test.ts | 156 +++++++++++++++++- server/src/services/tool-access.ts | 90 +++++++++- 4 files changed, 255 insertions(+), 11 deletions(-) diff --git a/doc/DEVELOPING.md b/doc/DEVELOPING.md index 3a5e153175c..03bcfa7c23e 100644 --- a/doc/DEVELOPING.md +++ b/doc/DEVELOPING.md @@ -924,6 +924,22 @@ pnpm secrets:migrate-inline-env # dry run pnpm secrets:migrate-inline-env --apply # apply migration ``` +## Internal Connection Token Brokers + +Connection token exchanges carry a resolved parent credential, so their URLs +are public-only by default even in local/private deployments. To intentionally +use an internal broker, configure an exact hostname allowlist: + +```sh +PAPERCLIP_TOKEN_BROKER_ALLOWED_HOSTS=broker.internal.example,10.0.0.42 +``` + +Entries are comma- or whitespace-separated exact hostnames (no wildcards). The +host configured by `PAPERCLIP_PAGES_API_URL` is included automatically. Every +broker hostname is resolved once and the request is pinned to the approved +address; IPv4 and IPv6 link-local destinations remain denied even when their +host is allowlisted. + ## Company Deletion Toggle Company deletion is intended as a dev/debug capability and can be disabled at runtime: diff --git a/doc/connections/SECURITY-THREAT-MODEL.md b/doc/connections/SECURITY-THREAT-MODEL.md index c8cce179fc1..7fb880ccdc5 100644 --- a/doc/connections/SECURITY-THREAT-MODEL.md +++ b/doc/connections/SECURITY-THREAT-MODEL.md @@ -111,6 +111,10 @@ Required controls: first, then persist only refs and redacted account metadata on the connection. - Create/update routes validate same-company ownership for every referenced secret, app, connection, agent, user, project, routine, and issue. +- Create/update routes validate every configured token-broker exchange URL + against the private-host policy before persisting the connection. Minting + repeats the check and pins the approved address before transmitting the + parent credential, covering legacy rows and DNS rebinding. - Health and auth failures transition failure-closed: `missing_secret`, `degraded`, `failed`, `auth_required`, or disabled equivalents. - Error payloads and logs redact provider responses that may contain credentials. diff --git a/server/src/__tests__/tool-access-service.test.ts b/server/src/__tests__/tool-access-service.test.ts index 4402d1debbb..86fd66cfcbf 100644 --- a/server/src/__tests__/tool-access-service.test.ts +++ b/server/src/__tests__/tool-access-service.test.ts @@ -254,6 +254,7 @@ async function createBrokerConnection( rateLimitPerHour?: number; healthStatus?: "unknown" | "healthy" | "degraded" | "failed" | "unchecked" | "ok" | "error" | "missing_secret"; tokenUrl?: string; + protocol?: "pages" | "generic" | "rfc8693"; } = {}, ) { const secret = await secretService(db).create(companyId, { @@ -284,7 +285,8 @@ async function createBrokerConnection( tokenBroker: { enabled: true, path: input.path ?? "exchange", - tokenUrl: input.tokenUrl ?? "https://pages.example.test/v1/tokens/exchange", + tokenUrl: input.tokenUrl ?? "https://93.184.216.34/v1/tokens/exchange", + ...(input.protocol ? { protocol: input.protocol } : {}), parentCredentialConfigPath: "credentials.deploy_token", parentScopes: input.parentScopes ?? ["pages:publish:ns/dotta"], defaultScopes: input.defaultScopes ?? [], @@ -475,7 +477,7 @@ describeEmbeddedPostgres("tool access service", () => { const app = createRouteApp(db, agentJwtActor(company.id, agent.id, run.id)); const fetchMock = vi.spyOn(globalThis, "fetch").mockImplementation(async (url, init) => { - expect(String(url)).toBe("https://pages.example.test/v1/tokens/exchange"); + expect(String(url)).toBe("https://93.184.216.34/v1/tokens/exchange"); expect(init?.headers).toEqual(expect.objectContaining({ authorization: "Bearer parent-deploy-token" })); const body = JSON.parse(String(init?.body)); expect(body).toMatchObject({ @@ -543,6 +545,76 @@ describeEmbeddedPostgres("tool access service", () => { ])); }); + it.each([ + ["generic", undefined], + ["RFC 8693", "rfc8693" as const], + ])("blocks a link-local %s token broker before credentials reach fetch", async (_label, protocol) => { + const company = await createCompany(db); + const agent = await createAgent(db, company.id); + const { run } = await createIssueAndRun(db, company.id, agent.id); + const { connection } = await createBrokerConnection(db, company.id, { + tokenUrl: "http://169.254.169.254/latest/meta-data", + ...(protocol ? { protocol } : {}), + }); + await allowConnectionForAgent(db, company.id, agent.id, connection.id); + const app = createRouteApp(db, agentJwtActor(company.id, agent.id, run.id)); + const fetchMock = vi.spyOn(globalThis, "fetch").mockRejectedValue( + new Error("the parent credential must never reach the broker"), + ); + + const res = await request(app) + .post(`/api/agents/me/connections/${connection.id}/token`) + .send({ scope: "pages:publish:ns/dotta" }); + + expect(res.status).toBe(400); + expect(res.body).toMatchObject({ code: "remote_http_private_endpoint" }); + expect(fetchMock).not.toHaveBeenCalled(); + const [issuance] = await db.select().from(connectionTokenIssuances); + expect(issuance).toMatchObject({ + connectionId: connection.id, + outcome: "failure", + errorCode: "remote_http_private_endpoint", + tokenHash: null, + }); + }); + + it("allows an explicitly allowlisted internal token broker through the guarded fetch", async () => { + vi.stubEnv("PAPERCLIP_TOKEN_BROKER_ALLOWED_HOSTS", "broker.example, 127.0.0.1"); + const company = await createCompany(db); + const agent = await createAgent(db, company.id); + const { run } = await createIssueAndRun(db, company.id, agent.id); + const { connection } = await createBrokerConnection(db, company.id, { + tokenUrl: "http://127.0.0.1:8787/v1/tokens/exchange", + }); + await allowConnectionForAgent(db, company.id, agent.id, connection.id); + const app = createRouteApp(db, agentJwtActor(company.id, agent.id, run.id)); + const fetchMock = vi.spyOn(globalThis, "fetch").mockImplementation(async (url, init) => { + expect(String(url)).toBe("http://127.0.0.1:8787/v1/tokens/exchange"); + expect(init).toMatchObject({ + method: "POST", + redirect: "manual", + headers: expect.objectContaining({ authorization: "Bearer parent-deploy-token" }), + }); + return { + ok: true, + status: 201, + json: async () => ({ + token: "allowlisted-child-token", + expires_in: 600, + scope: "pages:publish:ns/dotta", + }), + } as Response; + }); + + const res = await request(app) + .post(`/api/agents/me/connections/${connection.id}/token`) + .send({ scope: "pages:publish:ns/dotta" }); + + expect(res.status).toBe(200); + expect(res.body).toMatchObject({ token: "allowlisted-child-token" }); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + it("selects scoped credentials for array scopes and fails closed for unknown selectors", async () => { const company = await createCompany(db); const agent = await createAgent(db, company.id); @@ -1271,6 +1343,86 @@ describeEmbeddedPostgres("tool access service", () => { }); }); + it.each([ + ["tokenBroker.tokenUrl", { tokenBroker: { enabled: true, tokenUrl: "http://169.254.169.254/token" } }], + ["tokenBroker.exchangeTokenUrl", { tokenBroker: { enabled: true, exchangeTokenUrl: "http://169.254.169.254/token" } }], + ["tokenExchangeUrl", { tokenExchangeUrl: "http://169.254.169.254/token" }], + ["pagesTokenExchangeUrl", { pagesTokenExchangeUrl: "http://169.254.169.254/token" }], + ])("rejects a link-local %s when a remote connection is created", async (_field, brokerConfig) => { + const company = await createCompany(db); + const service = toolAccessService(db, { + deploymentMode: "authenticated", + deploymentExposure: "public", + }); + + await expect(service.createConnection(company.id, { + name: `Rejected broker ${randomUUID()}`, + transport: "mcp_remote", + config: { url: "https://93.184.216.34/mcp", ...brokerConfig }, + enabled: true, + status: "active", + })).rejects.toMatchObject({ + status: 400, + details: { code: "remote_http_private_endpoint" }, + }); + await expect(db.select().from(toolConnections)).resolves.toHaveLength(0); + }); + + it("rejects a link-local token broker when a remote connection is updated", async () => { + const company = await createCompany(db); + const service = toolAccessService(db, { + deploymentMode: "authenticated", + deploymentExposure: "public", + }); + const connection = await service.createConnection(company.id, { + name: "Initially safe broker", + transport: "mcp_remote", + config: { url: "https://93.184.216.34/mcp" }, + enabled: true, + status: "active", + }); + + await expect(service.updateConnection(connection.id, { + config: { + ...connection.config, + tokenBroker: { enabled: true, tokenUrl: "http://169.254.169.254/token" }, + }, + })).rejects.toMatchObject({ + status: 400, + details: { code: "remote_http_private_endpoint" }, + }); + await expect(service.getConnection(connection.id)).resolves.toMatchObject({ + config: { url: "https://93.184.216.34/mcp" }, + }); + }); + + it("implicitly allowlists the configured Pages API host for internal token brokers", async () => { + vi.stubEnv("PAPERCLIP_PAGES_API_URL", "http://127.0.0.1:8787"); + const company = await createCompany(db); + const service = toolAccessService(db, { + deploymentMode: "authenticated", + deploymentExposure: "public", + }); + + await expect(service.createConnection(company.id, { + name: "Internal Pages broker", + transport: "mcp_remote", + config: { + url: "https://93.184.216.34/mcp", + tokenBroker: { + enabled: true, + tokenUrl: "http://127.0.0.1:9999/v1/tokens/exchange", + }, + }, + enabled: true, + status: "active", + })).resolves.toMatchObject({ + config: { + tokenBroker: { tokenUrl: "http://127.0.0.1:9999/v1/tokens/exchange" }, + }, + }); + }); + it("lists testable agents with per-connection effective access summaries", async () => { const company = await createCompany(db); const userId = `tool-tester-${randomUUID()}`; diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index c32eed92574..c84a5809f78 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -1841,6 +1841,63 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} return assertRemoteHttpUrlAllowed(remoteEndpoint(config)); } + function normalizeTokenBrokerAllowedHost(value: string): string | null { + const trimmed = value.trim(); + if (!trimmed) return null; + try { + const parsed = new URL(trimmed.includes("://") ? trimmed : `http://${trimmed}`); + return parsed.hostname.replace(/^\[|\]$/g, "").replace(/\.$/, "").toLowerCase() || null; + } catch { + // Invalid allowlist entries grant no access. The configured broker URL is + // still evaluated under the public-only policy below. + return null; + } + } + + function tokenBrokerAllowedPrivateHosts(): Set { + const configured = (process.env.PAPERCLIP_TOKEN_BROKER_ALLOWED_HOSTS ?? "") + .split(/[,\s]+/) + .map(normalizeTokenBrokerAllowedHost) + .filter((host): host is string => host !== null); + const pagesApiHost = normalizeTokenBrokerAllowedHost(process.env.PAPERCLIP_PAGES_API_URL ?? ""); + if (pagesApiHost) configured.push(pagesApiHost); + return new Set(configured); + } + + function tokenBrokerAllowsPrivateNetwork(endpoint: URL): boolean { + const hostname = endpoint.hostname.replace(/^\[|\]$/g, "").replace(/\.$/, "").toLowerCase(); + return tokenBrokerAllowedPrivateHosts().has(hostname); + } + + function tokenBrokerHttpFetchOptions(endpoint: URL): GuardedRemoteHttpFetchOptions { + return { + allowPrivateNetwork: tokenBrokerAllowsPrivateNetwork(endpoint), + error: (message, code) => badRequest(message, { code }), + }; + } + + async function assertTokenBrokerHttpUrlAllowed(value: string): Promise { + const endpoint = parseRemoteHttpEndpoint(value, (message, code) => badRequest(message, { code })); + await assertPublicRemoteHttpEndpoint( + endpoint, + { allowPrivateNetwork: tokenBrokerAllowsPrivateNetwork(endpoint) }, + (message, code) => badRequest(message, { code }), + ); + return endpoint.toString(); + } + + async function assertConfiguredTokenBrokerEndpointsAllowed(config: Record): Promise { + for (const url of configuredTokenBrokerExchangeUrls(config)) { + await assertTokenBrokerHttpUrlAllowed(url); + } + } + + async function assertRemoteConnectionEndpointsAllowed(config: Record): Promise { + const endpoint = await assertRemoteEndpointAllowed(config); + await assertConfiguredTokenBrokerEndpointsAllowed(config); + return endpoint; + } + /** * OAuth endpoint scheme/transport gate (PAP-17099). * @@ -2031,13 +2088,26 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} return []; } - function tokenBrokerConfig(connection: typeof toolConnections.$inferSelect): Record { - const config = asRecord(connection.config); + function tokenBrokerConfigFromConnectionConfig(config: Record): Record { const broker = asRecord(config.tokenBroker); if (Object.keys(broker).length > 0) return broker; return asRecord(config.broker); } + function tokenBrokerConfig(connection: typeof toolConnections.$inferSelect): Record { + return tokenBrokerConfigFromConnectionConfig(asRecord(connection.config)); + } + + function configuredTokenBrokerExchangeUrls(config: Record): string[] { + const broker = tokenBrokerConfigFromConnectionConfig(config); + return [...new Set([ + readConfigString(broker, "tokenUrl"), + readConfigString(broker, "exchangeTokenUrl"), + readConfigString(config, "tokenExchangeUrl"), + readConfigString(config, "pagesTokenExchangeUrl"), + ].filter((url): url is string => url !== null))]; + } + function connectionTokenBrokerEnabled(connection: typeof toolConnections.$inferSelect): boolean { const config = asRecord(connection.config); const tokenBroker = asRecord(config.tokenBroker); @@ -2421,21 +2491,23 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} body.set("requested_token_type", readConfigString(broker, "requestedTokenType") ?? "urn:ietf:params:oauth:token-type:access_token"); body.set("actor_token", Buffer.from(JSON.stringify(actor)).toString("base64url")); body.set("actor_token_type", readConfigString(broker, "actorTokenType") ?? "urn:ietf:params:oauth:token-type:jwt"); - response = await fetch(url, { + const endpoint = parseRemoteHttpEndpoint(url, (message, code) => badRequest(message, { code })); + response = await guardedRemoteHttpFetch(endpoint, { method: "POST", headers: { "content-type": "application/x-www-form-urlencoded" }, body, - }); + }, tokenBrokerHttpFetchOptions(endpoint)); } else { const namespace = isPages ? pagesNamespaceFromScope(input.scope) : null; const body = isPages && namespace ? { namespace, ttlSeconds: input.ttlSeconds, actions: ["publish"], actor } : { scope: input.scope, ttlSeconds: input.ttlSeconds, actor, audience: readConfigString(broker, "audience") }; - response = await fetch(url, { + const endpoint = parseRemoteHttpEndpoint(url, (message, code) => badRequest(message, { code })); + response = await guardedRemoteHttpFetch(endpoint, { method: "POST", headers: { authorization: `Bearer ${parentToken}`, "content-type": "application/json" }, body: JSON.stringify(body), - }); + }, tokenBrokerHttpFetchOptions(endpoint)); } const payload = await response.json().catch(() => ({})) as unknown; const record = asRecord(payload); @@ -6224,7 +6296,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} config = normalizeGoogleSheetsConnectionConfig(config); await assertGoogleSheetsSpreadsheetOwnership(companyId, config); } - if (transport === "mcp_remote") await assertRemoteEndpointAllowed(config); + if (transport === "mcp_remote") await assertRemoteConnectionEndpointsAllowed(config); if (transport === "local_stdio") await stdioTemplateId(companyId, config); assertLocalStdioCanBeEnabled(transport, false); @@ -7859,7 +7931,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} const transport = input.transport; if (!transport) throw badRequest("Tool connection transport is required"); const config = normalizeGoogleSheetsConnectionConfig(input.config ?? input.transportConfig ?? {}); - if (transport === "mcp_remote") await assertRemoteEndpointAllowed(config); + if (transport === "mcp_remote") await assertRemoteConnectionEndpointsAllowed(config); if (transport === "local_stdio") await stdioTemplateId(companyId, config); assertLocalStdioCanBeEnabled(transport, input.enabled ?? false); await assertGoogleSheetsSpreadsheetOwnership(companyId, config); @@ -8095,7 +8167,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} updateConnection: async (connectionId: string, input: UpdateToolConnection): Promise => { const existing = await getConnectionRow(connectionId); const config = normalizeGoogleSheetsConnectionConfig(input.config ?? input.transportConfig ?? existing.config); - if (existing.transport === "mcp_remote") await assertRemoteEndpointAllowed(config); + if (existing.transport === "mcp_remote") await assertRemoteConnectionEndpointsAllowed(config); if (existing.transport === "local_stdio") await stdioTemplateId(existing.companyId, config); assertLocalStdioCanBeEnabled(existing.transport, input.enabled ?? existing.enabled); await assertGoogleSheetsSpreadsheetOwnership(existing.companyId, config, { excludeConnectionId: existing.id });