diff --git a/apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts b/apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts index 52199223a2a..f86793aebb3 100644 --- a/apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts +++ b/apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts @@ -19,13 +19,36 @@ export type { FileRef } from '@novu/framework'; const SIGNAL_TYPES = ['metadata', 'trigger'] as const; +/** + * Allowed characters for a metadata signal key. + * + * Metadata is merged into `conversation.metadata` (a plain object) and re-hydrated by + * every downstream consumer, so we forbid anything that could produce a prototype + * pollution gadget (`__proto__`, `constructor`, `prototype`) or break key handling + * for storage/serialization (dots, brackets, control chars). The shape mirrors + * SLUG_IDENTIFIER_REGEX with an additional `:` for namespacing (e.g. `crm:ticketId`). + */ +const METADATA_SIGNAL_KEY_REGEX = /^[a-zA-Z0-9]+(?:[-_:][a-zA-Z0-9]+)*$/; +const FORBIDDEN_METADATA_SIGNAL_KEYS = new Set(['__proto__', 'constructor', 'prototype']); +const MAX_METADATA_SIGNAL_KEY_LENGTH = 128; + +export function isValidMetadataSignalKey(key: unknown): key is string { + if (typeof key !== 'string' || key.length === 0 || key.length > MAX_METADATA_SIGNAL_KEY_LENGTH) { + return false; + } + + if (FORBIDDEN_METADATA_SIGNAL_KEYS.has(key)) return false; + + return METADATA_SIGNAL_KEY_REGEX.test(key); +} + @ValidatorConstraint({ name: 'isValidSignal', async: false }) export class IsValidSignal implements ValidatorConstraintInterface { validate(signal: SignalDto): boolean { if (!signal?.type) return false; if (signal.type === 'metadata') { - return typeof signal.key === 'string' && signal.key.length > 0 && signal.value !== undefined; + return isValidMetadataSignalKey(signal.key) && signal.value !== undefined; } if (signal.type === 'trigger') { @@ -36,7 +59,11 @@ export class IsValidSignal implements ValidatorConstraintInterface { } defaultMessage(): string { - return 'metadata signals require key + value; trigger signals require workflowId.'; + return ( + 'metadata signals require a key 1-128 chars of letters, digits and "-", "_", ":" separators ' + + '(no leading, trailing or consecutive separators) plus a defined value; ' + + 'trigger signals require workflowId.' + ); } } diff --git a/apps/api/src/app/agents/e2e/agents.e2e.ts b/apps/api/src/app/agents/e2e/agents.e2e.ts index 1b5cdc5b0b2..246e54085e5 100644 --- a/apps/api/src/app/agents/e2e/agents.e2e.ts +++ b/apps/api/src/app/agents/e2e/agents.e2e.ts @@ -307,51 +307,51 @@ describe('Agents API - /agents #novu-v2', () => { it('should update bridgeUrl via PATCH', async () => { const res = await session.testAgent.patch(`/v1/agents/${encodeURIComponent(identifier)}`).send({ - bridgeUrl: 'https://prod.example.com/api/novu', + bridgeUrl: 'https://example.com/api/novu', }); expect(res.status).to.equal(200); - expect(res.body.data.bridgeUrl).to.equal('https://prod.example.com/api/novu'); + expect(res.body.data.bridgeUrl).to.equal('https://example.com/api/novu'); }); it('should update devBridgeUrl and devBridgeActive via PUT bridge endpoint', async () => { const res = await session.testAgent.put(`/v1/agents/${encodeURIComponent(identifier)}/bridge`).send({ - devBridgeUrl: 'https://tunnel.example.com', + devBridgeUrl: 'https://example.org', devBridgeActive: true, }); expect(res.status).to.equal(200); - expect(res.body.data.devBridgeUrl).to.equal('https://tunnel.example.com'); + expect(res.body.data.devBridgeUrl).to.equal('https://example.org'); expect(res.body.data.devBridgeActive).to.equal(true); }); it('should set bridgeUrl via PUT bridge endpoint', async () => { const res = await session.testAgent.put(`/v1/agents/${encodeURIComponent(identifier)}/bridge`).send({ - bridgeUrl: 'https://prod.example.com/novu', + bridgeUrl: 'https://example.com/novu', }); expect(res.status).to.equal(200); - expect(res.body.data.bridgeUrl).to.equal('https://prod.example.com/novu'); + expect(res.body.data.bridgeUrl).to.equal('https://example.com/novu'); }); it('should return bridge fields on GET', async () => { await session.testAgent.patch(`/v1/agents/${encodeURIComponent(identifier)}`).send({ - bridgeUrl: 'https://prod.example.com/api/novu', - devBridgeUrl: 'https://tunnel.example.com', + bridgeUrl: 'https://example.com/api/novu', + devBridgeUrl: 'https://example.org', devBridgeActive: true, }); const res = await session.testAgent.get(`/v1/agents/${encodeURIComponent(identifier)}`); expect(res.status).to.equal(200); - expect(res.body.data.bridgeUrl).to.equal('https://prod.example.com/api/novu'); - expect(res.body.data.devBridgeUrl).to.equal('https://tunnel.example.com'); + expect(res.body.data.bridgeUrl).to.equal('https://example.com/api/novu'); + expect(res.body.data.devBridgeUrl).to.equal('https://example.org'); expect(res.body.data.devBridgeActive).to.equal(true); }); it('should deactivate devBridgeActive', async () => { await session.testAgent.patch(`/v1/agents/${encodeURIComponent(identifier)}`).send({ - devBridgeUrl: 'https://tunnel.example.com', + devBridgeUrl: 'https://example.org', devBridgeActive: true, }); @@ -361,7 +361,28 @@ describe('Agents API - /agents #novu-v2', () => { expect(res.status).to.equal(200); expect(res.body.data.devBridgeActive).to.equal(false); - expect(res.body.data.devBridgeUrl).to.equal('https://tunnel.example.com'); + expect(res.body.data.devBridgeUrl).to.equal('https://example.org'); + }); + + // Locks in the SSRF guard — see UpdateAgent.assertSafeBridgeUrl. + // localhost / private IPs / link-local must be rejected so an authenticated + // AGENT_WRITE caller can't repoint the bridge at internal hosts. + it('should reject bridgeUrl pointing at loopback', async () => { + const res = await session.testAgent.patch(`/v1/agents/${encodeURIComponent(identifier)}`).send({ + bridgeUrl: 'http://localhost:4000/api/novu', + }); + + expect(res.status).to.equal(400); + expect(JSON.stringify(res.body)).to.match(/bridgeUrl/i); + }); + + it('should reject devBridgeUrl pointing at link-local cloud metadata', async () => { + const res = await session.testAgent.put(`/v1/agents/${encodeURIComponent(identifier)}/bridge`).send({ + devBridgeUrl: 'http://169.254.169.254/latest/meta-data/', + }); + + expect(res.status).to.equal(400); + expect(JSON.stringify(res.body)).to.match(/devBridgeUrl/i); }); }); @@ -406,7 +427,7 @@ describe('Agents API - /agents #novu-v2', () => { await prodSession.testAgent.post('/v1/agents').send({ name: 'Prod Agent 2', identifier: `${identifier}-prod2` }); const res = await prodSession.testAgent.patch(`/v1/agents/${encodeURIComponent(`${identifier}-prod2`)}`).send({ - devBridgeUrl: 'https://tunnel.example.com', + devBridgeUrl: 'https://example.org', }); expect(res.status).to.equal(403); @@ -420,11 +441,11 @@ describe('Agents API - /agents #novu-v2', () => { await prodSession.testAgent.post('/v1/agents').send({ name: 'Prod Agent 3', identifier: `${identifier}-prod3` }); const res = await prodSession.testAgent.patch(`/v1/agents/${encodeURIComponent(`${identifier}-prod3`)}`).send({ - bridgeUrl: 'https://prod.example.com/novu', + bridgeUrl: 'https://example.com/novu', }); expect(res.status).to.equal(200); - expect(res.body.data.bridgeUrl).to.equal('https://prod.example.com/novu'); + expect(res.body.data.bridgeUrl).to.equal('https://example.com/novu'); await prodSession.testAgent.delete(`/v1/agents/${encodeURIComponent(`${identifier}-prod3`)}`); }); diff --git a/apps/api/src/app/agents/services/bridge-executor.service.ts b/apps/api/src/app/agents/services/bridge-executor.service.ts index afdc317441f..91d776ed0d7 100644 --- a/apps/api/src/app/agents/services/bridge-executor.service.ts +++ b/apps/api/src/app/agents/services/bridge-executor.service.ts @@ -4,6 +4,7 @@ import { GetDecryptedSecretKey, GetDecryptedSecretKeyCommand, PinoLogger, + validateUrlSsrf, } from '@novu/application-generic'; import { ConversationActivityEntity, ConversationEntity, SubscriberEntity } from '@novu/dal'; import type { @@ -17,6 +18,7 @@ import type { AgentSubscriber, } from '@novu/framework'; import { AgentEventEnum } from '@novu/framework'; +import { HttpHeaderKeysEnum } from '@novu/framework/internal'; import type { Message } from 'chat'; import { ResolvedAgentConfig } from './agent-config-resolver.service'; @@ -99,12 +101,27 @@ export class BridgeExecutorService { let lastError: Error | undefined; for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) { + // Re-validate per attempt to defend against (a) agents whose bridgeUrl was + // stored before the update-time SSRF guard was added, and (b) DNS rebinding + // — a hostname that resolved to a public IP at update-time can later resolve + // to a private one. validateUrlSsrf caches DNS lookups for 5 minutes, so + // the per-attempt cost is amortized across retries. + const ssrfError = await validateUrlSsrf(url); + if (ssrfError) { + throw new Error(`Bridge URL blocked by SSRF protection: ${ssrfError}`); + } + try { const response = await fetch(url, { method: 'POST', headers: { 'Content-Type': 'application/json', - 'x-novu-signature': signatureHeader, + // Must match HttpHeaderKeysEnum.NOVU_SIGNATURE — the framework SDK reads + // this exact header to verify the HMAC. Sending any other name (e.g. + // `x-novu-signature`) silently disables signature verification on the + // bridge and lets a forged AgentBridgeRequest exfiltrate the secret key + // via an attacker-controlled `replyUrl`. + [HttpHeaderKeysEnum.NOVU_SIGNATURE]: signatureHeader, }, body, }); diff --git a/apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts b/apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts index a73325864df..0c6ecab9980 100644 --- a/apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts +++ b/apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts @@ -19,6 +19,7 @@ import { } from '@novu/dal'; import type { SentMessageInfo } from '@novu/framework'; import { AgentEventEnum } from '../../dtos/agent-event.enum'; +import { isValidMetadataSignalKey } from '../../dtos/agent-reply-payload.dto'; import type { EditPayloadDto, ReplyContentDto } from '../../dtos/agent-reply-payload.dto'; import { AgentConfigResolver, ResolvedAgentConfig } from '../../services/agent-config-resolver.service'; import { AgentConversationService } from '../../services/agent-conversation.service'; @@ -271,8 +272,19 @@ export class HandleAgentReply { channel: ConversationChannel, signals: Array<{ type: 'metadata'; key: string; value: unknown }> ): Promise { - const merged = { ...(conversation.metadata ?? {}) }; + // Defense in depth: the DTO validator already enforces this for HTTP callers, + // but commands can also be constructed internally (e.g. by the inbound handler). + // Reject prototype-pollution gadgets, keys that wouldn't survive a clean + // round-trip through downstream consumers, and undefined values (which would + // be silently dropped by JSON.stringify and never reach storage). + const merged: Record = { ...(conversation.metadata ?? {}) }; for (const signal of signals) { + if (!isValidMetadataSignalKey(signal.key)) { + throw new BadRequestException(`Invalid metadata signal key: "${signal.key}"`); + } + if (signal.value === undefined) { + throw new BadRequestException(`Metadata signal "${signal.key}" must have a defined value`); + } merged[signal.key] = signal.value; } diff --git a/apps/api/src/app/agents/usecases/update-agent/update-agent.usecase.ts b/apps/api/src/app/agents/usecases/update-agent/update-agent.usecase.ts index 6b952f03010..74e5affd639 100644 --- a/apps/api/src/app/agents/usecases/update-agent/update-agent.usecase.ts +++ b/apps/api/src/app/agents/usecases/update-agent/update-agent.usecase.ts @@ -1,4 +1,5 @@ import { BadRequestException, ForbiddenException, Injectable, NotFoundException } from '@nestjs/common'; +import { validateUrlSsrf } from '@novu/application-generic'; import { AgentRepository, EnvironmentRepository } from '@novu/dal'; import { EnvironmentTypeEnum } from '@novu/shared'; import type { AgentResponseDto } from '../../dtos'; @@ -35,6 +36,13 @@ export class UpdateAgent { await this.assertNotProductionEnvironment(command.environmentId, command.organizationId); } + // The bridge executor `fetch()`s these URLs from inside the API process on every + // inbound chat event with a Novu HMAC and sensitive payload (subscriber + history). + // Without an SSRF guard, an authenticated AGENT_WRITE caller can repoint the bridge + // at internal hosts (loopback, RFC1918, link-local 169.254.169.254, cloud metadata). + await this.assertSafeBridgeUrl(command.bridgeUrl, 'bridgeUrl'); + await this.assertSafeBridgeUrl(command.devBridgeUrl, 'devBridgeUrl'); + const existing = await this.agentRepository.findOne( { identifier: command.identifier, @@ -118,4 +126,15 @@ export class UpdateAgent { throw new ForbiddenException('Dev bridge cannot be activated on production environments.'); } } + + private async assertSafeBridgeUrl(url: string | undefined | null, field: string): Promise { + if (!url) { + return; + } + + const ssrfError = await validateUrlSsrf(url); + if (ssrfError) { + throw new BadRequestException(`${field}: ${ssrfError}`); + } + } }