Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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') {
Expand All @@ -36,7 +59,7 @@ export class IsValidSignal implements ValidatorConstraintInterface {
}

defaultMessage(): string {
return 'metadata signals require key + value; trigger signals require workflowId.';
return 'metadata signals require a safe alphanumeric key (letters, digits, "-", "_", ":") and a value; trigger signals require workflowId.';
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
}

Expand Down
8 changes: 7 additions & 1 deletion apps/api/src/app/agents/services/bridge-executor.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,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';

Expand Down Expand Up @@ -104,7 +105,12 @@ export class BridgeExecutorService {
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,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -271,7 +272,17 @@ export class HandleAgentReply {
channel: ConversationChannel,
signals: Array<{ type: 'metadata'; key: string; value: unknown }>
): Promise<void> {
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 and any key that wouldn't survive a clean
// round-trip through downstream consumers.
for (const signal of signals) {
if (!isValidMetadataSignalKey(signal.key)) {
throw new BadRequestException(`Invalid metadata signal key: "${signal.key}"`);
}
}

const merged: Record<string, unknown> = { ...(conversation.metadata ?? {}) };
for (const signal of signals) {
merged[signal.key] = signal.value;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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');
Comment thread
coderabbitai[bot] marked this conversation as resolved.

const existing = await this.agentRepository.findOne(
{
identifier: command.identifier,
Expand Down Expand Up @@ -118,4 +126,13 @@ export class UpdateAgent {
throw new ForbiddenException('Dev bridge cannot be activated on production environments.');
}
}

private async assertSafeBridgeUrl(url: string | undefined | null, field: string): Promise<void> {
if (!url) return;

const ssrfError = await validateUrlSsrf(url);
if (ssrfError) {
throw new BadRequestException(`${field}: ${ssrfError}`);
}
}
}
Loading