Skip to content

fix(security): fail closed when an agent-conversation webhook secret is unset - #208

Open
massimoalbarello wants to merge 1 commit into
mainfrom
security/agent-webhook-fail-closed
Open

fix(security): fail closed when an agent-conversation webhook secret is unset#208
massimoalbarello wants to merge 1 commit into
mainfrom
security/agent-webhook-fail-closed

Conversation

@massimoalbarello

Copy link
Copy Markdown
Contributor

The agent-conversations webhook verified its shared secret only when one happened to be configured:

// nango-integrations/agent-conversations/syncs/conversations.ts:58 (before)
if (metadata.webhookSecret && payload.secret !== metadata.webhookSecret) {
  throw new Error('Invalid agent conversation webhook secret');
}

If a connection's metadata carried no webhookSecret, the guard short-circuited and every payload was accepted. MetadataSchema declared the field .optional(), so that state was well-formed rather than a parse error.

Why it matters — this sync is reachable from Nango's public ingress: nango/packages/server/lib/routes.public.ts:183 registers /webhook/:environmentUuid/:providerConfigKey on publicAPI behind only a rate limiter. Against a secret-less connection, anyone who knows the environment UUID and provider config key can push arbitrary AgentConversation records straight into brain.records, which then feed the knowledge-distillation pipeline — so this is a knowledge-poisoning path, not just spurious rows.

Scope of exposure — the deployed connections are not currently affected. _scripts/lib/bootstrap-connections.ts:27 reads the secret via requiredValue(), which throws on a missing or empty AGENT_SYNC_WEBHOOK_SECRET, so every bootstrapped connection has one. The gap opens only for connections created out-of-band (Nango UI or API). I'm treating "the deploy script happens to set it" as an accidental mitigation rather than a control, which is why this fixes the check itself.

FixwebhookSecret becomes required (.min(1)), so parseMetadata throws before any record is written when it is absent, and the comparison is now unconditional.

Verified by reverting the source change and re-running the new tests: the no-secret case saves records against the old code and rejects against the new one. The wrong-secret test passes either way and is there as a regression guard.

Note this is intentionally a breaking change for any existing secret-less connection — it will now error on webhook delivery until a secret is set, which is the fail-closed behaviour. Constant-time comparison was deliberately left out: node:crypto is unused anywhere in nango-integrations and a remote timing attack on this value isn't practical, so pulling it in would have widened the diff for no real gain.

…is unset

The webhook secret check was skipped entirely when a connection carried no
webhookSecret in its metadata, so a connection created outside the bootstrap
script accepted unauthenticated writes into the knowledge pipeline.

The secret is now required by MetadataSchema and always compared.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant