diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 26d9231bc6c..05a9c696ce8 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -251,6 +251,17 @@ export { connectionDisplaySecondaryHint, type HumanizableConnection, } from "./humanize-connection.js"; +export { + MCP_REMOTE_HEADER_LIMITS, + checkMcpRemoteHeaderName, + checkMcpRemoteHeaderValue, + isSafeMcpRemoteHeaderName, + isSafeMcpRemoteHeaderValue, + mcpRemoteHeaderNameFromConfigPath, + mcpRemoteHeaderRejectionMessage, + type McpRemoteHeaderCheck, + type McpRemoteHeaderRejection, +} from "./mcp-remote-headers.js"; export { COMPANY_STATUSES, DEFAULT_COMPANY_ATTACHMENT_MAX_BYTES, @@ -1245,7 +1256,9 @@ export type { SecretProposalView, ApproveSecretProposalInput, RejectSecretProposalInput, + ConnectToolAppAuthChallenge, ConnectToolAppResult, + ToolOAuthClientRegistrationSource, ToolOAuthStartResult, ToolActionRequest, ToolAccessDecision, @@ -2011,6 +2024,9 @@ export { createToolTrustRuleFromActionRequestSchema, revokeToolTrustRuleSchema, connectToolAppSchema, + GENERIC_MCP_AUTH_MODES, + genericMcpAuthModeSchema, + genericMcpOAuthClientSchema, reconnectToolAppSchema, createToolApplicationSchema, finishToolAppSchema, diff --git a/packages/shared/src/mcp-remote-headers.test.ts b/packages/shared/src/mcp-remote-headers.test.ts new file mode 100644 index 00000000000..b59cd4fc197 --- /dev/null +++ b/packages/shared/src/mcp-remote-headers.test.ts @@ -0,0 +1,127 @@ +import { describe, expect, it } from "vitest"; +import { + checkMcpRemoteHeaderName, + checkMcpRemoteHeaderValue, + mcpRemoteHeaderNameFromConfigPath, + mcpRemoteHeaderRejectionMessage, +} from "./mcp-remote-headers.js"; + +describe("checkMcpRemoteHeaderName", () => { + it("accepts the header names real MCP servers ask for", () => { + for (const name of [ + "Authorization", + "authorization", + "X-PostHog-Project-Id", + "x-api-key", + "Mcp-Session-Id", + "X_Custom.Token", + ]) { + expect(checkMcpRemoteHeaderName(name), name).toEqual({ ok: true }); + } + }); + + it("rejects hop-by-hop, framing, routing and ambient-credential headers", () => { + for (const name of [ + "Host", + "host", + "Connection", + "Content-Length", + "Transfer-Encoding", + "TE", + "Trailer", + "Upgrade", + "Keep-Alive", + "Cookie", + "Set-Cookie", + "Proxy-Authorization", + "Via", + "Expect", + ]) { + expect(checkMcpRemoteHeaderName(name), name).toEqual({ ok: false, reason: "forbidden" }); + } + }); + + it("rejects reserved prefixes browsers and proxies own", () => { + for (const name of ["Sec-Fetch-Mode", "sec-websocket-key", "Proxy-Foo", "HTTP2-Settings"]) { + expect(checkMcpRemoteHeaderName(name), name).toEqual({ ok: false, reason: "forbidden" }); + } + }); + + it("rejects names that could smuggle a separator or a new header line", () => { + for (const name of [ + "X-Bad\r\nX-Injected", + "X-Bad\nX-Injected", + "X Bad", + "X-Bad:", + "X-Bad;Other", + "X-Bad,Other", + 'X-"Bad"', + "X-Bad/Other", + "X-Bad(Other)", + "X-Bad@Other", + "X-Bad\u0000", + ]) { + expect(checkMcpRemoteHeaderName(name), JSON.stringify(name)).toEqual({ + ok: false, + reason: "invalid_characters", + }); + } + }); + + it("rejects blank and over-long names", () => { + expect(checkMcpRemoteHeaderName("")).toEqual({ ok: false, reason: "empty" }); + expect(checkMcpRemoteHeaderName(" ")).toEqual({ ok: false, reason: "empty" }); + expect(checkMcpRemoteHeaderName("x".repeat(129))).toEqual({ ok: false, reason: "too_long" }); + expect(checkMcpRemoteHeaderName("x".repeat(128))).toEqual({ ok: true }); + }); +}); + +describe("checkMcpRemoteHeaderValue", () => { + it("accepts ordinary credential values", () => { + for (const value of ["Bearer phx_abc123", "phx_abc123", "12345", "a b c", ""]) { + expect(checkMcpRemoteHeaderValue(value), JSON.stringify(value)).toEqual({ ok: true }); + } + }); + + it("rejects values that would split the request", () => { + for (const value of [ + "abc\r\nX-Injected: 1", + "abc\nX-Injected: 1", + "abc\rdef", + "abc\u0000def", + "abc\tdef", + "abc\u007fdef", + ]) { + expect(checkMcpRemoteHeaderValue(value), JSON.stringify(value)).toEqual({ + ok: false, + reason: "value_control_characters", + }); + } + }); + + it("rejects over-long values", () => { + expect(checkMcpRemoteHeaderValue("x".repeat(8_193))).toEqual({ ok: false, reason: "value_too_long" }); + expect(checkMcpRemoteHeaderValue("x".repeat(8_192))).toEqual({ ok: true }); + }); +}); + +describe("mcpRemoteHeaderNameFromConfigPath", () => { + it("extracts a header name from a headers.* config path", () => { + expect(mcpRemoteHeaderNameFromConfigPath("headers.X-Api-Key")).toBe("X-Api-Key"); + expect(mcpRemoteHeaderNameFromConfigPath("headers. X-Api-Key ")).toBe("X-Api-Key"); + }); + + it("returns null for non-header and empty paths", () => { + expect(mcpRemoteHeaderNameFromConfigPath("credentials.authorization")).toBeNull(); + expect(mcpRemoteHeaderNameFromConfigPath("headers.")).toBeNull(); + expect(mcpRemoteHeaderNameFromConfigPath("headers. ")).toBeNull(); + }); +}); + +describe("mcpRemoteHeaderRejectionMessage", () => { + it("explains the problem without echoing the value", () => { + expect(mcpRemoteHeaderRejectionMessage("Host", "forbidden")).toContain("Host"); + expect(mcpRemoteHeaderRejectionMessage("X-Api-Key", "value_control_characters")) + .toBe('The value for "X-Api-Key" contains line breaks or control characters.'); + }); +}); diff --git a/packages/shared/src/mcp-remote-headers.ts b/packages/shared/src/mcp-remote-headers.ts new file mode 100644 index 00000000000..d4522440255 --- /dev/null +++ b/packages/shared/src/mcp-remote-headers.ts @@ -0,0 +1,148 @@ +/** + * Header-name/value safety for user-supplied remote MCP credentials (PAP-17087). + * + * Both the guided "Connect your own MCP server" flow and the paste-config escape + * hatch let an operator name arbitrary request headers for an arbitrary endpoint. + * Those names reach a real outbound `fetch`, so they are validated here — once, + * in shared code — instead of at each call site: + * + * - only RFC 9110 `token` characters, so a name can never smuggle a separator, + * whitespace, or CR/LF into the request line; + * - never a hop-by-hop, framing, routing, or ambient-credential header, because + * those either belong to the transport or would let a pasted config redirect + * the request or attach a browser cookie; + * - values must stay printable single-line, which blocks header/response + * splitting through a value that carries `\r\n`. + * + * `Authorization` is deliberately allowed: it is the header the bearer-key path + * uses, and its value is stored as a Paperclip secret like every other one. + */ + +/** RFC 9110 field-name = token. */ +const HTTP_TOKEN_PATTERN = /^[!#$%&'*+\-.^_`|~0-9A-Za-z]+$/; + +const MAX_HEADER_NAME_LENGTH = 128; +const MAX_HEADER_VALUE_LENGTH = 8_192; + +/** + * Headers Paperclip refuses to project from a user-supplied config. + * + * `connection`/`keep-alive`/`te`/`trailer`/`transfer-encoding`/`upgrade` are + * hop-by-hop (RFC 9110 §7.6.1) and belong to the fetch implementation. + * `content-length`/`host` frame and route the request. `cookie` would attach + * ambient browser-style credentials that Paperclip cannot scope or rotate. + * `proxy-*` targets an intermediary rather than the MCP server. + */ +const FORBIDDEN_HEADER_NAMES = new Set([ + "connection", + "content-length", + "cookie", + "cookie2", + "expect", + "host", + "keep-alive", + "proxy-authenticate", + "proxy-authorization", + "proxy-connection", + "set-cookie", + "set-cookie2", + "te", + "trailer", + "transfer-encoding", + "upgrade", + "via", +]); + +/** Prefixes reserved for the transport or for intermediaries. */ +const FORBIDDEN_HEADER_PREFIXES = ["proxy-", "sec-", "http2-"]; + +export type McpRemoteHeaderRejection = + | "empty" + | "too_long" + | "invalid_characters" + | "forbidden" + | "value_too_long" + | "value_control_characters"; + +export interface McpRemoteHeaderCheck { + ok: boolean; + reason?: McpRemoteHeaderRejection; +} + +const OK: McpRemoteHeaderCheck = { ok: true }; + +/** + * Is `name` a header Paperclip is willing to send on a user-configured remote + * MCP request? Returns the specific rejection reason so callers can produce an + * actionable, UI-safe message. + */ +export function checkMcpRemoteHeaderName(name: string): McpRemoteHeaderCheck { + const trimmed = name.trim(); + if (!trimmed) return { ok: false, reason: "empty" }; + if (trimmed.length > MAX_HEADER_NAME_LENGTH) return { ok: false, reason: "too_long" }; + if (!HTTP_TOKEN_PATTERN.test(trimmed)) return { ok: false, reason: "invalid_characters" }; + const lower = trimmed.toLowerCase(); + if (FORBIDDEN_HEADER_NAMES.has(lower)) return { ok: false, reason: "forbidden" }; + if (FORBIDDEN_HEADER_PREFIXES.some((prefix) => lower.startsWith(prefix))) { + return { ok: false, reason: "forbidden" }; + } + return OK; +} + +/** + * Is `value` safe to send as a header value? Rejects CR/LF and other control + * characters (header splitting) and absurdly long values. + */ +export function checkMcpRemoteHeaderValue(value: string): McpRemoteHeaderCheck { + if (value.length > MAX_HEADER_VALUE_LENGTH) return { ok: false, reason: "value_too_long" }; + // Reject C0/C1 controls and DEL. A tab is legal in a field value per RFC 9110 + // but has no legitimate use in a credential, so it is rejected too. + if (/[\u0000-\u001f\u007f-\u009f]/.test(value)) { + return { ok: false, reason: "value_control_characters" }; + } + return OK; +} + +export function isSafeMcpRemoteHeaderName(name: string): boolean { + return checkMcpRemoteHeaderName(name).ok; +} + +export function isSafeMcpRemoteHeaderValue(value: string): boolean { + return checkMcpRemoteHeaderValue(value).ok; +} + +/** A UI-safe explanation for a rejected header. Never echoes the value. */ +export function mcpRemoteHeaderRejectionMessage( + headerName: string, + reason: McpRemoteHeaderRejection, +): string { + switch (reason) { + case "empty": + return "Header names cannot be blank."; + case "too_long": + return `Header name "${headerName.slice(0, MAX_HEADER_NAME_LENGTH)}" is too long.`; + case "invalid_characters": + return `"${headerName.slice(0, MAX_HEADER_NAME_LENGTH)}" is not a valid header name. Use letters, digits, and dashes.`; + case "forbidden": + return `Paperclip manages the "${headerName}" header and cannot send a custom value for it.`; + case "value_too_long": + return `The value for "${headerName}" is too long.`; + case "value_control_characters": + return `The value for "${headerName}" contains line breaks or control characters.`; + } +} + +/** + * `credentialValues` keys use a `headers.` config path. Extract the header + * name, or `null` when the path is not a header path. + */ +export function mcpRemoteHeaderNameFromConfigPath(configPath: string): string | null { + if (!configPath.startsWith("headers.")) return null; + const name = configPath.slice("headers.".length).trim(); + return name.length > 0 ? name : null; +} + +export const MCP_REMOTE_HEADER_LIMITS = { + maxNameLength: MAX_HEADER_NAME_LENGTH, + maxValueLength: MAX_HEADER_VALUE_LENGTH, +} as const; diff --git a/packages/shared/src/types/index.ts b/packages/shared/src/types/index.ts index 2f63fd280d8..38f9fb4c328 100644 --- a/packages/shared/src/types/index.ts +++ b/packages/shared/src/types/index.ts @@ -441,8 +441,10 @@ export type { ToolAccessDecisionInput, ToolAccessReasonCode, ToolAccessSelector, + ConnectToolAppAuthChallenge, ConnectToolAppResult, FinishToolAppResult, + ToolOAuthClientRegistrationSource, ToolOAuthStartResult, ToolTrustRuleArgumentFilters, ToolTrustRuleBatchApprovalConfig, diff --git a/packages/shared/src/types/tool-access.ts b/packages/shared/src/types/tool-access.ts index bd851442686..7dcedb17996 100644 --- a/packages/shared/src/types/tool-access.ts +++ b/packages/shared/src/types/tool-access.ts @@ -911,6 +911,29 @@ export interface ToolAppConnectionActionSummary { status: ToolCatalogEntryStatus; } +/** + * How Paperclip obtained the OAuth client it will use for a connection + * (PAP-17087). Ordered by preference: a client the deployment preconfigured for + * that issuer, then a Client ID Metadata Document, then dynamic registration, + * then client credentials the operator preregistered and pasted in. + */ +export type ToolOAuthClientRegistrationSource = "preconfigured" | "cimd" | "dcr" | "manual"; + +/** + * What an unknown remote MCP endpoint told Paperclip it needs, so the wizard can + * branch without re-probing. `manualClientRequired` means discovery succeeded but + * the authorization server supports neither CIMD nor DCR, so the operator has to + * supply a preregistered client under Advanced authentication. + */ +export interface ConnectToolAppAuthChallenge { + kind: "oauth"; + startUrl: string | null; + issuer?: string | null; + resource?: string | null; + registrationSource?: ToolOAuthClientRegistrationSource | null; + manualClientRequired?: boolean; +} + export interface ConnectToolAppResult { connectionId: string; application: ToolApplication; @@ -921,10 +944,7 @@ export interface ConnectToolAppResult { canMakeChanges: ToolAppConnectionActionSummary[]; }; suggestedDefaults: Record; - auth?: { - kind: "oauth"; - startUrl: string | null; - } | null; + auth?: ConnectToolAppAuthChallenge | null; } export interface ToolOAuthStartResult { diff --git a/packages/shared/src/validators/index.ts b/packages/shared/src/validators/index.ts index 2fc001d5a40..5eae4166fd2 100644 --- a/packages/shared/src/validators/index.ts +++ b/packages/shared/src/validators/index.ts @@ -865,6 +865,9 @@ export { mcpConnectionCredentialRefSchema, createToolApplicationSchema, connectToolAppSchema, + GENERIC_MCP_AUTH_MODES, + genericMcpAuthModeSchema, + genericMcpOAuthClientSchema, reconnectToolAppSchema, finishToolAppSchema, updateToolApplicationSchema, diff --git a/packages/shared/src/validators/tool-access.test.ts b/packages/shared/src/validators/tool-access.test.ts index d2d05b0332a..f8e1c5257a8 100644 --- a/packages/shared/src/validators/tool-access.test.ts +++ b/packages/shared/src/validators/tool-access.test.ts @@ -61,6 +61,68 @@ describe("tool access validators", () => { }).success).toBe(false); }); + // PAP-17087: the guided generic flow and paste-config both reach the connect + // endpoint, so unsafe header names/values are rejected once at this boundary. + it("accepts generic advanced-authentication input for a pasted URL", () => { + const parsed = connectToolAppSchema.safeParse({ + link: "https://mcp.example.test/mcp", + authMode: "custom_headers", + credentialValues: { + "headers.X-Api-Key": "phx_abc123", + "headers.X-PostHog-Project-Id": "12345", + }, + }); + expect(parsed.success).toBe(true); + + const manualClient = connectToolAppSchema.safeParse({ + link: "https://mcp.example.test/mcp", + authMode: "oauth", + oauthClient: { clientId: "client-abc", clientSecret: "shhh" }, + }); + expect(manualClient.success).toBe(true); + }); + + it("rejects header credentials Paperclip refuses to send", () => { + for (const configPath of ["headers.Host", "headers.Cookie", "headers.Transfer-Encoding", "headers.Sec-Fetch-Mode"]) { + const parsed = connectToolAppSchema.safeParse({ + link: "https://mcp.example.test/mcp", + credentialValues: { [configPath]: "value" }, + }); + expect(parsed.success, configPath).toBe(false); + } + }); + + it("rejects header names and values that could split the outbound request", () => { + const badName = connectToolAppSchema.safeParse({ + link: "https://mcp.example.test/mcp", + credentialValues: { "headers.X-Bad\r\nX-Injected": "value" }, + }); + expect(badName.success).toBe(false); + + const badValue = connectToolAppSchema.safeParse({ + link: "https://mcp.example.test/mcp", + credentialValues: { "headers.X-Api-Key": "abc\r\nX-Injected: 1" }, + }); + expect(badValue.success).toBe(false); + if (!badValue.success) { + // The message names the header but must never echo the rejected value. + const message = badValue.error.issues[0]?.message ?? ""; + expect(message).toContain("X-Api-Key"); + expect(message).not.toContain("X-Injected"); + } + }); + + it("keeps generic advanced authentication off the curated gallery path", () => { + expect(connectToolAppSchema.safeParse({ + galleryKey: "posthog", + authMode: "bearer", + }).success).toBe(false); + expect(connectToolAppSchema.safeParse({ + galleryKey: "posthog", + oauthClient: { clientId: "client-abc" }, + }).success).toBe(false); + }); + it("accepts secret references for connection credentials", () => { const parsed = createToolConnectionSchema.safeParse({ applicationId: "11111111-1111-4111-8111-111111111111", diff --git a/packages/shared/src/validators/tool-access.ts b/packages/shared/src/validators/tool-access.ts index bc2f945af52..e86312e1232 100644 --- a/packages/shared/src/validators/tool-access.ts +++ b/packages/shared/src/validators/tool-access.ts @@ -30,6 +30,12 @@ import { TOOL_RUNTIME_KINDS, TOOL_RUNTIME_SLOT_STATUSES, } from "../constants.js"; +import { + checkMcpRemoteHeaderName, + checkMcpRemoteHeaderValue, + mcpRemoteHeaderNameFromConfigPath, + mcpRemoteHeaderRejectionMessage, +} from "../mcp-remote-headers.js"; import { jsonSchemaSchema } from "./plugin.js"; export const toolApplicationTypeSchema = z.enum(TOOL_APPLICATION_TYPES); @@ -258,6 +264,73 @@ export const disableToolStdioCommandTemplateSchema = z.object({ export type DisableToolStdioCommandTemplate = z.infer; +/** + * How an operator says a generic remote MCP endpoint authenticates (PAP-17087). + * + * `auto` is the default and the only value the simple path sends: Paperclip + * probes the endpoint and branches on what it finds. The rest are the explicit + * choices behind "Advanced authentication", where the operator already knows. + */ +export const GENERIC_MCP_AUTH_MODES = ["auto", "none", "bearer", "custom_headers", "oauth"] as const; + +export const genericMcpAuthModeSchema = z.enum(GENERIC_MCP_AUTH_MODES); + +export type GenericMcpAuthMode = z.infer; + +/** + * A preregistered OAuth client an operator pasted in because the authorization + * server supports neither CIMD nor dynamic registration. The secret is write-only: + * it becomes a Paperclip secret ref and is never read back. + */ +export const genericMcpOAuthClientSchema = z.object({ + clientId: z.string().trim().min(1).max(4096), + clientSecret: z.string().min(1).max(16384).optional(), +}).strict(); + +export type GenericMcpOAuthClient = z.infer; + +/** + * Reject `headers.*` credential paths whose header name Paperclip refuses to + * send, and any value that could split the outbound request. This runs at the + * API boundary so both the guided wizard and normalized paste-config go through + * exactly one gate; the service re-checks when it projects the headers. + */ +function rejectUnsafeHeaderCredentials( + credentialValues: Record, + ctx: z.RefinementCtx, + path: (string | number)[], +) { + for (const [configPath, value] of Object.entries(credentialValues)) { + const headerName = mcpRemoteHeaderNameFromConfigPath(configPath); + if (configPath.startsWith("headers.") && !headerName) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: [...path, configPath], + message: "Header names cannot be blank.", + }); + continue; + } + if (!headerName) continue; + const nameCheck = checkMcpRemoteHeaderName(headerName); + if (!nameCheck.ok) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: [...path, configPath], + message: mcpRemoteHeaderRejectionMessage(headerName, nameCheck.reason!), + }); + continue; + } + const valueCheck = checkMcpRemoteHeaderValue(value); + if (!valueCheck.ok) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: [...path, configPath], + message: mcpRemoteHeaderRejectionMessage(headerName, valueCheck.reason!), + }); + } + } +} + export const connectToolAppSchema = z.object({ galleryKey: z.string().trim().min(1).max(120).optional(), connectionMethodKey: z.string().trim().min(1).max(120).optional(), @@ -266,8 +339,25 @@ export const connectToolAppSchema = z.object({ credentialValues: z.record(z.string().trim().min(1).max(200), z.string().min(1)).optional(), configValues: z.record(z.string().trim().min(1).max(200), z.unknown()).optional(), applicationId: z.string().uuid().optional(), + authMode: genericMcpAuthModeSchema.optional(), + oauthClient: genericMcpOAuthClientSchema.optional(), }).superRefine((value, ctx) => { if (value.configValues) rejectSensitiveConfigKeys(value.configValues, ctx, ["configValues"]); + if (value.credentialValues) rejectUnsafeHeaderCredentials(value.credentialValues, ctx, ["credentialValues"]); + if (value.authMode && value.galleryKey) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["authMode"], + message: "Authentication mode selection applies to a pasted URL, not a gallery app", + }); + } + if (value.oauthClient && value.galleryKey) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["oauthClient"], + message: "Preregistered OAuth client credentials apply to a pasted URL, not a gallery app", + }); + } }).refine( (value) => Boolean(value.galleryKey) !== Boolean(value.link), { message: "Provide exactly one of galleryKey or link" },