From f8fb39a40e98ce1c38ec7e6b7c11e89d595c773f Mon Sep 17 00:00:00 2001 From: Matteo Date: Sun, 4 Oct 2026 10:08:34 +0200 Subject: [PATCH] Daily review fixes: MCP Host refusals explained, security events kept, setup links measurable - A remote MCP server whose DNS-rebinding protection refuses our Host ("host not allowed" / "Invalid Host header") now gets an error that says to add the hostname to its allowed hosts. Seen on a customer's MCP connector behind a trycloudflare tunnel, stuck at 0 tools. - A security event that references a deleted user or organization (TOKEN_REJECTED for a deleted user) was lost on the foreign key; it is now stored without the links, with the ids in its metadata. - Expired one-time setup links stay in the table for a week instead of being deleted at the next link, so the share of links that get opened can be measured. They stop working at expiry as before. --- .../adapters/connector-setup.service.spec.ts | 10 ++++- .../src/adapters/connector-setup.service.ts | 8 +++- .../src/audit/security-event.service.spec.ts | 23 ++++++++++ .../src/audit/security-event.service.ts | 45 ++++++++++++++----- .../engines/mcp-client.engine.spec.ts | 20 ++++++++- .../connectors/engines/mcp-client.engine.ts | 28 +++++++++++- 6 files changed, 118 insertions(+), 16 deletions(-) diff --git a/packages/backend/src/adapters/connector-setup.service.spec.ts b/packages/backend/src/adapters/connector-setup.service.spec.ts index 5b5fd892..e5ad2818 100644 --- a/packages/backend/src/adapters/connector-setup.service.spec.ts +++ b/packages/backend/src/adapters/connector-setup.service.spec.ts @@ -1,4 +1,4 @@ -import { ConnectorSetupService } from './connector-setup.service'; +import { ConnectorSetupService, SETUP_LINK_RETENTION_MS } from './connector-setup.service'; import { AdaptersService } from './adapters.service'; import { encrypt } from '../common/crypto/encryption.util'; @@ -181,6 +181,14 @@ describe('ConnectorSetupService — links', () => { return out.body.finishSetupUrl.split('/s/')[1] as string; } + it('keeps expired links a week (for measurement), not just until they expire', async () => { + const b = build(); + const before = Date.now(); + await linkFor(b); + const cutoff: Date = b.prisma.connectorSetupLink.deleteMany.mock.calls[0][0].where.expiresAt.lt; + expect(before - cutoff.getTime()).toBeGreaterThanOrEqual(SETUP_LINK_RETENTION_MS - 1000); + }); + it('opens once, for the user it was made for, on the guided setup of that connector', async () => { const b = build(); const token = await linkFor(b); diff --git a/packages/backend/src/adapters/connector-setup.service.ts b/packages/backend/src/adapters/connector-setup.service.ts index 79e985a6..d2c68bdb 100644 --- a/packages/backend/src/adapters/connector-setup.service.ts +++ b/packages/backend/src/adapters/connector-setup.service.ts @@ -18,6 +18,8 @@ import { decrypt } from '../common/crypto/encryption.util'; /** How long a setup link handed to the user stays valid. */ export const SETUP_LINK_TTL_MS = 30 * 60 * 1000; +/** How long expired setup links stay in the table, for measurement only. */ +export const SETUP_LINK_RETENTION_MS = 7 * 24 * 60 * 60 * 1000; /** Installs through a chat, per user and hour. A model does not need more. */ const INSTALLS_PER_HOUR = 10; @@ -290,7 +292,11 @@ export class ConnectorSetupService implements SharedSetupProvider, OnModuleInit /** A fresh one-time link to finish this connector, for this user. */ async createLink(ctx: Pick, connectorId: string): Promise { const token = randomBytes(24).toString('base64url'); - await this.prisma.connectorSetupLink.deleteMany({ where: { expiresAt: { lt: new Date() } } }).catch(() => undefined); + // Expired links stop working at once (resolveLink checks expiresAt); the + // rows are kept a week so the share of links that get opened can be read. + await this.prisma.connectorSetupLink + .deleteMany({ where: { expiresAt: { lt: new Date(Date.now() - SETUP_LINK_RETENTION_MS) } } }) + .catch(() => undefined); await this.prisma.connectorSetupLink.create({ data: { tokenHash: this.hash(token), diff --git a/packages/backend/src/audit/security-event.service.spec.ts b/packages/backend/src/audit/security-event.service.spec.ts index 2b439258..efb21308 100644 --- a/packages/backend/src/audit/security-event.service.spec.ts +++ b/packages/backend/src/audit/security-event.service.spec.ts @@ -47,6 +47,29 @@ describe('SecurityEventService', () => { expect(dataOf().actorUserId).toBeNull(); }); + it('keeps an event whose user or organization was deleted, with the ids in metadata', async () => { + // A token rejected because its user is gone: the foreign key fails, and + // this is the row an investigation needs. + const fk = Object.assign(new Error('Foreign key constraint violated'), { code: 'P2003' }); + prisma.securityEvent.create.mockRejectedValueOnce(fk).mockResolvedValueOnce({}); + + await service.log({ + event: SecurityEvents.TOKEN_REJECTED, + actorType: 'USER', + organizationId: 'org-gone', + actorUserId: 'user-gone', + metadata: { reason: 'user_deleted' }, + }); + + expect(prisma.securityEvent.create).toHaveBeenCalledTimes(2); + const retried = prisma.securityEvent.create.mock.calls[1][0].data; + expect(retried).toMatchObject({ organizationId: null, actorUserId: null, targetUserId: null }); + expect(retried.metadata).toMatchObject({ + reason: 'user_deleted', + unresolved: { organizationId: 'org-gone', actorUserId: 'user-gone', targetUserId: null }, + }); + }); + it('never throws when the write fails', async () => { // An audit failure must not deny a legitimate request, nor give an // attacker a way to break the flow by breaking the write. diff --git a/packages/backend/src/audit/security-event.service.ts b/packages/backend/src/audit/security-event.service.ts index 4f2833fb..3b65b505 100644 --- a/packages/backend/src/audit/security-event.service.ts +++ b/packages/backend/src/audit/security-event.service.ts @@ -139,19 +139,40 @@ export class SecurityEventService { constructor(private readonly prisma: PrismaService) {} async log(input: SecurityEventInput): Promise { + const data = { + event: input.event, + actorType: input.actorType, + organizationId: input.organizationId ?? null, + actorUserId: input.actorUserId ?? null, + targetUserId: input.targetUserId ?? null, + metadata: (this.redact(input.metadata) ?? undefined) as any, + ip: input.ip ?? null, + userAgent: input.userAgent?.slice(0, MAX_STRING) ?? null, + }; try { - await this.prisma.securityEvent.create({ - data: { - event: input.event, - actorType: input.actorType, - organizationId: input.organizationId ?? null, - actorUserId: input.actorUserId ?? null, - targetUserId: input.targetUserId ?? null, - metadata: (this.redact(input.metadata) ?? undefined) as any, - ip: input.ip ?? null, - userAgent: input.userAgent?.slice(0, MAX_STRING) ?? null, - }, - }); + try { + await this.prisma.securityEvent.create({ data }); + } catch (error: any) { + // P2003: a referenced user or organization no longer exists, e.g. a + // token rejected because its user was deleted. That is exactly the + // event worth keeping, so store it without the links and keep the + // ids in the metadata instead of losing the row. + if (error?.code !== 'P2003') throw error; + const unresolved = { + organizationId: data.organizationId, + actorUserId: data.actorUserId, + targetUserId: data.targetUserId, + }; + await this.prisma.securityEvent.create({ + data: { + ...data, + organizationId: null, + actorUserId: null, + targetUserId: null, + metadata: { ...((data.metadata as Record) ?? {}), unresolved }, + }, + }); + } } catch (error: any) { // Never propagate: an audit failure must not deny a legitimate request, // and must not hand an attacker a way to break the flow by breaking the diff --git a/packages/backend/src/connectors/engines/mcp-client.engine.spec.ts b/packages/backend/src/connectors/engines/mcp-client.engine.spec.ts index 297adec5..6e469948 100644 --- a/packages/backend/src/connectors/engines/mcp-client.engine.spec.ts +++ b/packages/backend/src/connectors/engines/mcp-client.engine.spec.ts @@ -1,4 +1,4 @@ -import { McpClientEngine } from './mcp-client.engine'; +import { McpClientEngine, explainMcpConnectError } from './mcp-client.engine'; import { OAuth2TokenService } from './oauth2-token.service'; import { Client, StreamableHTTPClientTransport } from '@modelcontextprotocol/client'; import { assertSafeOutboundUrl } from '../../common/ssrf.util'; @@ -170,3 +170,21 @@ describe('McpClientEngine endpoint resolution', () => { }); }); }); + +describe('explainMcpConnectError', () => { + const url = new URL('https://soap-shipping.trycloudflare.com/mcp'); + + it.each([ + 'Error POSTing to endpoint: host not allowed', + 'Error POSTing to endpoint: Forbidden: invalid Host header', + ])('says which setting to change when the server refuses our Host: %s', (raw) => { + const out = explainMcpConnectError(new Error(raw), url); + expect(out.message).toContain(raw); + expect(out.message).toContain("Add 'soap-shipping.trycloudflare.com' to the server's allowed hosts"); + }); + + it('leaves other errors as they are', () => { + const err = new Error('Error POSTing to endpoint: 401 Unauthorized'); + expect(explainMcpConnectError(err, url)).toBe(err); + }); +}); diff --git a/packages/backend/src/connectors/engines/mcp-client.engine.ts b/packages/backend/src/connectors/engines/mcp-client.engine.ts index 88dc6244..7a71eb39 100644 --- a/packages/backend/src/connectors/engines/mcp-client.engine.ts +++ b/packages/backend/src/connectors/engines/mcp-client.engine.ts @@ -150,7 +150,11 @@ export class McpClientEngine { }); try { - await client.connect(transport); + try { + await client.connect(transport); + } catch (err) { + throw explainMcpConnectError(err, mcpUrl); + } const result = await client.listTools(); return (result.tools || []).map((tool) => ({ @@ -238,3 +242,25 @@ export class McpClientEngine { } } } + +/** + * A remote MCP server built on the MCP SDK with DNS-rebinding protection on + * answers 403 "Invalid Host header" / "host not allowed" to any hostname it + * was not told about, e.g. its own public tunnel (trycloudflare, ngrok). The + * raw message reads like our fault; say which setting on their side to change. + */ +export function explainMcpConnectError(err: unknown, mcpUrl: URL): Error { + const message = String((err as Error)?.message ?? err); + if (!/invalid host header|host not allowed|dns rebinding/i.test(message)) { + return err instanceof Error ? err : new Error(message); + } + const explained = new Error( + `${message}. The MCP server refused the hostname '${mcpUrl.hostname}': its DNS-rebinding ` + + `protection only accepts the hosts it was configured with. Add '${mcpUrl.hostname}' to the ` + + `server's allowed hosts (allowedHosts in the MCP SDK; with a tunnel, the tunnel's hostname) ` + + `or turn that check off behind the tunnel, then discover the tools again.`, + ); + (explained as Error & { cause?: unknown }).cause = err; + return explained; +} +