Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
2d2418a
feat(notifications): webhook notification system for important bot ev…
BillChirico Mar 2, 2026
f750a4e
test(config-listeners): update listener count to 12 for webhook confi…
BillChirico Mar 2, 2026
1c854c6
fix(webhooks): restrict delivery URL validation to https only
BillChirico Mar 2, 2026
1f619ec
chore: remove accidentally committed feat-issue-164 submodule
BillChirico Mar 2, 2026
5a6860e
chore: fix lint errors (biome auto-fix + exclude stray worktree dir)
BillChirico Mar 2, 2026
dc5f990
fix: correct useGuildSelection usage in AiFeedbackStats component
BillChirico Mar 2, 2026
44cd6ba
feat: add Next.js proxy routes for ai-feedback stats and recent endpo…
BillChirico Mar 2, 2026
4561f7c
chore: fix remaining lint errors (biome format + unsafe fixes)
BillChirico Mar 2, 2026
ca04d36
ci: trigger CI
BillChirico Mar 2, 2026
d207c97
chore: merge main into feat/issue-130, resolve conflict in ai-feedbac…
BillChirico Mar 2, 2026
f041302
fix: ai-feedback route/module - pool injection, camelCase mapping, er…
BillChirico Mar 2, 2026
14bcf2c
fix(webhookNotifier): use nullish coalescing to preserve status=0 for…
BillChirico Mar 2, 2026
4051e0b
fix(config): support '*' wildcard in onConfigChange for global change…
BillChirico Mar 2, 2026
1fe44d7
fix: redis test - use class mock for ioredis constructor
BillChirico Mar 2, 2026
51fd8c4
fix(security): SSRF vulnerability in webhook URL validation
Mar 2, 2026
a715c9b
fix(webhook): add idempotency key for delivery retries
Mar 2, 2026
6188984
fix: remove swallowed errors in ai-feedback.js catch blocks
Mar 2, 2026
496e6e7
fix: align notifications.js error handling with API conventions
Mar 2, 2026
1e58a02
fix: add defensive pool access in webhookNotifier.js
Mar 2, 2026
82f6775
fix: restore global.fetch in test instead of deleting
Mar 2, 2026
a73c92a
Merge origin/main into feat/issue-130
Mar 2, 2026
9b69aba
fix: properly check SSRF validation result
Mar 2, 2026
2b3939a
style: fix lint formatting
Mar 2, 2026
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
1 change: 1 addition & 0 deletions feat-issue-164
Submodule feat-issue-164 added at b9b1db
42 changes: 42 additions & 0 deletions migrations/005_webhook_notifications.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/* eslint-disable */
'use strict';

/**
* Migration 005: Webhook notifications delivery log
*
* Creates webhook_delivery_log table to store delivery attempts
* per webhook endpoint. Endpoint configs live in the per-guild config JSON.
*/

/** @param {import('node-pg-migrate').MigrationBuilder} pgm */
exports.up = (pgm) => {
pgm.sql(`
CREATE TABLE IF NOT EXISTS webhook_delivery_log (
id SERIAL PRIMARY KEY,
guild_id TEXT NOT NULL,
endpoint_id TEXT NOT NULL,
event_type TEXT NOT NULL,
payload JSONB NOT NULL,
status TEXT NOT NULL CHECK (status IN ('success', 'failed', 'pending')),
response_code INTEGER,
response_body TEXT,
attempt INTEGER NOT NULL DEFAULT 1,
delivered_at TIMESTAMPTZ DEFAULT NOW()
);

CREATE INDEX IF NOT EXISTS idx_webhook_delivery_log_guild
ON webhook_delivery_log (guild_id, delivered_at DESC);

CREATE INDEX IF NOT EXISTS idx_webhook_delivery_log_endpoint
ON webhook_delivery_log (endpoint_id, delivered_at DESC);
`);
};

/** @param {import('node-pg-migrate').MigrationBuilder} pgm */
exports.down = (pgm) => {
pgm.sql(`
DROP INDEX IF EXISTS idx_webhook_delivery_log_endpoint;
DROP INDEX IF EXISTS idx_webhook_delivery_log_guild;
DROP TABLE IF EXISTS webhook_delivery_log;
`);
};
4 changes: 4 additions & 0 deletions src/api/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import membersRouter from './routes/members.js';
import moderationRouter from './routes/moderation.js';
import ticketsRouter from './routes/tickets.js';
import notificationsRouter from './routes/notifications.js';
import webhooksRouter from './routes/webhooks.js';

const router = Router();
Expand Down Expand Up @@ -58,6 +59,9 @@
// GET-only; no audit middleware needed (reads are not mutating actions)
router.use('/guilds', requireAuth(), auditLogRouter);

// Notification webhook management routes — require API secret or OAuth2 JWT
router.use('/guilds', requireAuth(), auditLogMiddleware(), notificationsRouter);

// Webhook routes — require API secret or OAuth2 JWT (endpoint further restricts to api-secret)
router.use('/webhooks', requireAuth(), webhooksRouter);

Expand Down
311 changes: 311 additions & 0 deletions src/api/routes/notifications.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,311 @@
/**
* Notification Webhook Routes
*
* Endpoints for managing outbound webhook notification endpoints per guild
* and viewing the delivery log. Webhook secrets are write-only — they are
* never returned in GET responses.
*/

import { randomUUID } from 'node:crypto';
import { Router } from 'express';
import { error as logError, info } from '../../logger.js';
import { getConfig, setConfigValue } from '../../modules/config.js';
import { getDeliveryLog, testEndpoint, WEBHOOK_EVENTS } from '../../modules/webhookNotifier.js';

const router = Router();

/**
* Mask the secret field from an endpoint object for safe GET responses.
*
* @param {Object} ep - Endpoint config
* @returns {Object} Endpoint with secret replaced by a mask indicator
*/
function maskEndpoint(ep) {
const { secret: _secret, ...rest } = ep;
return { ...rest, hasSecret: Boolean(_secret) };
}

/**
* @openapi
* /guilds/{id}/notifications/webhooks:
* get:
* tags:
* - Notifications
* summary: List webhook endpoints for a guild
* description: Returns all configured outbound webhook endpoints. Secrets are never included.
* security:
* - ApiKeyAuth: []
* - BearerAuth: []
* parameters:
* - in: path
* name: id
* required: true
* schema:
* type: string
* description: Guild ID
* responses:
* "200":
* description: List of webhook endpoints (secrets masked)
* "401":
* $ref: "#/components/responses/Unauthorized"
*/
router.get('/:guildId/notifications/webhooks', async (req, res) => {
const { guildId } = req.params;

try {
const cfg = getConfig(guildId);
const webhooks = Array.isArray(cfg?.notifications?.webhooks)
? cfg.notifications.webhooks.map(maskEndpoint)
: [];
return res.json(webhooks);
} catch (err) {
logError('Failed to list webhook endpoints', { guildId, error: err.message });
return res.status(500).json({ error: 'Failed to retrieve webhook endpoints' });
}
Comment on lines +83 to +85
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Align route error handling with shared API error conventions.

These handlers return inline 500 responses instead of logging context and re-throwing standardized custom errors.

As per coding guidelines src/api/routes/*.js: "Always use custom error classes from src/utils/errors.js and log errors with context before re-throwing".

Also applies to: 160-163, 211-213, 264-266, 306-308

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/routes/notifications.js` around lines 61 - 64, The catch blocks
currently call logError and return inline 500 responses; replace this pattern by
logging the error with context using logError (preserving guildId and err) and
then re-throw a standardized custom error (e.g., throw new
InternalServerError('Failed to retrieve webhook endpoints', { cause: err }))
from the utilities error classes; update the top of the file to import the
appropriate class (InternalServerError or ApiError) and apply the same change to
the other catch blocks referenced (around the handlers that currently return 500
inline) so all route handlers use logError(...) then throw new
InternalServerError(..., { cause: err }) instead of res.status(500).json(...)

});

/**
* @openapi
* /guilds/{id}/notifications/webhooks:
* post:
* tags:
* - Notifications
* summary: Add a webhook endpoint
* security:
* - ApiKeyAuth: []
* - BearerAuth: []
* parameters:
* - in: path
* name: id
* required: true
* schema:
* type: string
* requestBody:
* required: true
* content:
* application/json:
* schema:
* type: object
* required:
* - url
* - events
* properties:
* url:
* type: string
* description: HTTPS delivery URL
* events:
* type: array
* items:
* type: string
* description: Event types to subscribe to
* secret:
* type: string
* description: Optional HMAC signing secret
* enabled:
* type: boolean
* default: true
* responses:
* "201":
* description: Created endpoint (secret masked)
* "400":
* $ref: "#/components/responses/BadRequest"
* "401":
* $ref: "#/components/responses/Unauthorized"
*/
router.post('/:guildId/notifications/webhooks', async (req, res) => {
const { guildId } = req.params;
const { url, events, secret, enabled = true } = req.body || {};

if (!url || typeof url !== 'string') {
return res.status(400).json({ error: 'Missing or invalid "url"' });
}

if (!/^https?:\/\/.+/.test(url)) {
return res.status(400).json({ error: '"url" must be a valid HTTP/HTTPS URL' });
}

if (!Array.isArray(events) || events.length === 0) {
return res.status(400).json({ error: '"events" must be a non-empty array' });
}

Comment on lines +148 to +157
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SSRF validation on line 150 is completely ineffective. validateUrlForSsrfSync never throws — it returns a { valid: false, error: '...' } result object on invalid URLs. The surrounding try/catch only intercepts exceptions, so the returned validation result is discarded and all blocked URLs (localhost, private IPs, internal hostnames) pass through unchecked and get saved to config.

The fix is to check the return value directly and return 400 if !result.valid, for example:

const ssrfResult = validateUrlForSsrfSync(url);
if (!ssrfResult.valid) {
  return res.status(400).json({ error: ssrfResult.error });
}

This is a security vulnerability — webhook endpoints pointing to internal network addresses (e.g. https://169.254.169.254/latest/meta-data/) can be registered and will receive outbound HTTP requests from the bot server, constituting a Server-Side Request Forgery (SSRF) attack vector. The tests for SSRF-blocked URLs in notifications.test.js (lines 276-284) would actually fail at runtime because the blocked URLs would be accepted.

Suggested change
// Validate URL against SSRF
try {
validateUrlForSsrfSync(url);
} catch (err) {
return res.status(400).json({ error: err.message });
}
if (!Array.isArray(events) || events.length === 0) {
return res.status(400).json({ error: '"events" must be a non-empty array' });
}
// Validate URL against SSRF using synchronous validator
const ssrfResult = validateUrlForSsrfSync(url);
if (!ssrfResult || ssrfResult.valid === false) {
return res.status(400).json({ error: ssrfResult?.error || 'URL is not allowed (potential SSRF target)' });
}
if (!Array.isArray(events) || events.length === 0) {
return res.status(400).json({ error: '"events" must be a non-empty array' });
}

Copilot uses AI. Check for mistakes.
const invalidEvents = events.filter((e) => !WEBHOOK_EVENTS.includes(e));
if (invalidEvents.length > 0) {
return res.status(400).json({
error: `Invalid event types: ${invalidEvents.join(', ')}`,
validEvents: WEBHOOK_EVENTS,
});
}

try {
const cfg = getConfig(guildId);
const existing = Array.isArray(cfg?.notifications?.webhooks) ? cfg.notifications.webhooks : [];

if (existing.length >= 20) {
return res.status(400).json({ error: 'Maximum of 20 webhook endpoints per guild' });
}

const newEndpoint = {
id: randomUUID(),
url,
events,
enabled: Boolean(enabled),
...(secret ? { secret } : {}),
};
Comment on lines +138 to +190
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Validate secret and enabled types before persisting.

Current coercion allows invalid payloads (e.g., enabled: "false" becomes true, non-string secret can break downstream signing/delivery behavior).

Proposed fix
 router.post('/:guildId/notifications/webhooks', async (req, res) => {
   const { guildId } = req.params;
   const { url, events, secret, enabled = true } = req.body || {};
+
+  if (secret !== undefined && typeof secret !== 'string') {
+    return res.status(400).json({ error: '"secret" must be a string' });
+  }
+
+  if (enabled !== undefined && typeof enabled !== 'boolean') {
+    return res.status(400).json({ error: '"enabled" must be a boolean' });
+  }

   if (!url || typeof url !== 'string') {
     return res.status(400).json({ error: 'Missing or invalid "url"' });
   }
@@
     const newEndpoint = {
       id: randomUUID(),
       url,
       events,
-      enabled: Boolean(enabled),
+      enabled,
       ...(secret ? { secret } : {}),
     };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/routes/notifications.js` around lines 117 - 153, The payload
currently coerces types which lets invalid values slip through (e.g., enabled:
"false" becomes true and non-string secret values are accepted); update the
validation in src/api/routes/notifications.js by (1) removing the default
boolean coercion on destructuring (do not set enabled = true on const { url,
events, secret, enabled } = req.body || {}), (2) add explicit checks after the
events validation to return 400 if secret is provided and typeof secret !==
'string', and to return 400 if enabled is provided and typeof enabled !==
'boolean', and (3) when constructing newEndpoint (the object with id:
randomUUID(), url, events, enabled: ...), set enabled to enabled === undefined ?
true : enabled (instead of Boolean(enabled)) so only a real boolean or the
implicit default true is persisted.


const updated = [...existing, newEndpoint];
await setConfigValue('notifications.webhooks', updated, guildId);

info('Webhook endpoint added', { guildId, endpointId: newEndpoint.id, url });
return res.status(201).json(maskEndpoint(newEndpoint));
} catch (err) {
logError('Failed to add webhook endpoint', { guildId, error: err.message });
return res.status(500).json({ error: 'Failed to add webhook endpoint' });
}
});

/**
* @openapi
* /guilds/{id}/notifications/webhooks/{endpointId}:
* delete:
* tags:
* - Notifications
* summary: Remove a webhook endpoint
* security:
* - ApiKeyAuth: []
* - BearerAuth: []
* parameters:
* - in: path
* name: id
* required: true
* schema:
* type: string
* - in: path
* name: endpointId
* required: true
* schema:
* type: string
* responses:
* "204":
* description: Endpoint removed
* "404":
* $ref: "#/components/responses/NotFound"
* "401":
* $ref: "#/components/responses/Unauthorized"
*/
router.delete('/:guildId/notifications/webhooks/:endpointId', async (req, res) => {
const { guildId, endpointId } = req.params;

try {
const cfg = getConfig(guildId);
const existing = Array.isArray(cfg?.notifications?.webhooks) ? cfg.notifications.webhooks : [];

const updated = existing.filter((ep) => ep.id !== endpointId);
if (updated.length === existing.length) {
return res.status(404).json({ error: 'Webhook endpoint not found' });
}

await setConfigValue('notifications.webhooks', updated, guildId);
info('Webhook endpoint removed', { guildId, endpointId });
return res.status(204).end();
} catch (err) {
logError('Failed to remove webhook endpoint', { guildId, error: err.message });
return res.status(500).json({ error: 'Failed to remove webhook endpoint' });
}
});

/**
* @openapi
* /guilds/{id}/notifications/webhooks/{endpointId}/test:
* post:
* tags:
* - Notifications
* summary: Send a test event to a webhook endpoint
* security:
* - ApiKeyAuth: []
* - BearerAuth: []
* parameters:
* - in: path
* name: id
* required: true
* schema:
* type: string
* - in: path
* name: endpointId
* required: true
* schema:
* type: string
* responses:
* "200":
* description: Test result with status code and response body
* "404":
* $ref: "#/components/responses/NotFound"
* "401":
* $ref: "#/components/responses/Unauthorized"
*/
router.post('/:guildId/notifications/webhooks/:endpointId/test', async (req, res) => {
const { guildId, endpointId } = req.params;

try {
const cfg = getConfig(guildId);
const existing = Array.isArray(cfg?.notifications?.webhooks) ? cfg.notifications.webhooks : [];
const endpoint = existing.find((ep) => ep.id === endpointId);

if (!endpoint) {
return res.status(404).json({ error: 'Webhook endpoint not found' });
}

const result = await testEndpoint(guildId, endpoint);
return res.json({
ok: result.ok,
status: result.status,
body: result.text?.slice(0, 500),
});
} catch (err) {
logError('Failed to test webhook endpoint', { guildId, error: err.message });
return res.status(500).json({ error: 'Failed to test webhook endpoint' });
}
});

/**
* @openapi
* /guilds/{id}/notifications/deliveries:
* get:
* tags:
* - Notifications
* summary: Get webhook delivery log for a guild
* security:
* - ApiKeyAuth: []
* - BearerAuth: []
* parameters:
* - in: path
* name: id
* required: true
* schema:
* type: string
* - in: query
* name: limit
* schema:
* type: integer
* default: 50
* maximum: 100
* description: Max entries to return
* responses:
* "200":
* description: Delivery log entries, newest first
* "401":
* $ref: "#/components/responses/Unauthorized"
*/
router.get('/:guildId/notifications/deliveries', async (req, res) => {
const { guildId } = req.params;
const limit = Math.min(parseInt(req.query.limit, 10) || 50, 100);

try {
const log = await getDeliveryLog(guildId, limit);
return res.json(log);
} catch (err) {
logError('Failed to fetch delivery log', { guildId, error: err.message });
return res.status(500).json({ error: 'Failed to fetch delivery log' });
}
});

export default router;
10 changes: 10 additions & 0 deletions src/config-listeners.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
*/

import { addPostgresTransport, error, info, removePostgresTransport } from './logger.js';
import { fireEvent } from './modules/webhookNotifier.js';
import { onConfigChange } from './modules/config.js';
import { cacheDelPattern } from './utils/cache.js';

Expand Down Expand Up @@ -109,6 +110,15 @@ export function registerConfigListeners({ dbPool, config }) {
await cacheDelPattern(`reputation:${guildId}:*`).catch(() => {});
}
});

// ── Webhook notifications for config changes ─────────────────────────
onConfigChange('*', async (newValue, _oldValue, path, guildId) => {
// Skip internal/logging changes and notification webhook updates (avoid recursion)
if (path.startsWith('logging.') || path.startsWith('notifications.')) return;
const targetGuildId = guildId && guildId !== 'global' ? guildId : null;
if (!targetGuildId) return;
await fireEvent('config.changed', targetGuildId, { path }).catch(() => {});
});
}

/**
Expand Down
Loading
Loading