fix(notifications): harden alert dispatch crash-safety and redact webhook secrets in logs (#1255)

* fix(notifications): harden alert dispatch crash-safety and redact webhook secrets in logs

Make NotificationService.dispatchAlert never reject so the many
fire-and-forget callers (monitors, event streams, policy and image-update
paths) cannot trigger an unhandledRejection on an unhealthy database. The
whole dispatch body now sits inside a guard covering node resolution, the
history insert, channel-table reads, and the WebSocket broadcast; a
failure logs and drops the notification instead of crashing the process.
An inner guard still splits the write-success and write-failure metrics.

Also:
- Redact webhook URLs in diagnostic logs via a new maskWebhookUrl helper;
  Discord/Slack/custom webhook URLs embed their token in the path, so only
  the origin is safe to emit.
- Add error logging to four notification-history route handlers that
  previously swallowed database errors silently before returning 500.
- Snapshot the subscriber set before broadcasting so a close/error handler
  firing mid-send cannot mutate the set during iteration.
- Sanitize the admin-supplied route name in dispatch log lines.

Adds tests for dispatch crash-safety (write failure, post-write routing
failure, broadcast send failure), the success-path write metric, webhook
URL masking, and history-route error logging.

* fix(notifications): sanitize route name and patterns in create logs

Apply sanitizeForLog to the admin-supplied route name and stack patterns
in the route-creation log lines, matching the dispatch-site sanitization
and closing the remaining log-injection path on this feature.

Also extend tests: userinfo-stripping in maskWebhookUrl, subscriber-set
snapshot behavior under an unsubscribe-during-send, and error logging on
the mark-read, delete-one, and clear-all notification-history handlers.
This commit is contained in:
Anso
2026-05-29 21:09:57 -04:00
committed by GitHub
parent 69edb0dcbb
commit 7d4e61625f
6 changed files with 340 additions and 72 deletions
@@ -0,0 +1,49 @@
/**
* Unit tests for notification channel helpers. Focused on maskWebhookUrl,
* which must never let a channel's embedded auth token reach a log line.
*/
import { describe, it, expect } from 'vitest';
import { maskWebhookUrl } from '../helpers/notificationChannels';
describe('maskWebhookUrl', () => {
it('redacts the token-bearing path of a Discord webhook URL', () => {
const masked = maskWebhookUrl('https://discord.com/api/webhooks/123456789/SuP3rS3cr3tT0k3n');
expect(masked).toBe('https://discord.com/<redacted>');
expect(masked).not.toContain('SuP3rS3cr3tT0k3n');
});
it('redacts the path of a Slack webhook URL', () => {
const masked = maskWebhookUrl('https://hooks.slack.com/services/T000/B000/XXXXSECRET');
expect(masked).toBe('https://hooks.slack.com/<redacted>');
expect(masked).not.toContain('XXXXSECRET');
});
it('redacts a secret carried in the query string', () => {
const masked = maskWebhookUrl('https://example.com/?token=abc123secret');
expect(masked).toBe('https://example.com/<redacted>');
expect(masked).not.toContain('abc123secret');
});
it('returns the bare origin when there is no path or query to hide', () => {
expect(maskWebhookUrl('https://example.com')).toBe('https://example.com');
expect(maskWebhookUrl('https://example.com/')).toBe('https://example.com');
});
it('strips embedded userinfo credentials (origin omits user:pass@)', () => {
const masked = maskWebhookUrl('https://user:s3cr3t@example.com/');
expect(masked).toBe('https://example.com');
expect(masked).not.toContain('s3cr3t');
expect(masked).not.toContain('user');
});
it('returns a placeholder for empty or non-string input', () => {
expect(maskWebhookUrl('')).toBe('<no url>');
expect(maskWebhookUrl(undefined)).toBe('<no url>');
expect(maskWebhookUrl(null)).toBe('<no url>');
expect(maskWebhookUrl(42)).toBe('<no url>');
});
it('returns a placeholder for an unparseable URL', () => {
expect(maskWebhookUrl('not a url')).toBe('<invalid url>');
});
});
@@ -528,3 +528,77 @@ describe('DELETE /api/notifications/:id - validation', () => {
expect(res.body.error).toContain('Invalid');
});
});
// --- Notification history endpoints ---
describe('GET /api/notifications - history', () => {
afterEach(() => {
vi.restoreAllMocks();
// Restore the license spies the suite relies on after a full mock reset.
vi.spyOn(licenseService, 'getTier').mockReturnValue('paid');
vi.spyOn(licenseService, 'getVariant').mockReturnValue('admiral');
});
it('returns 200 with an array for an authenticated user', async () => {
const res = await request(app).get('/api/notifications').set('Cookie', authCookie);
expect(res.status).toBe(200);
expect(Array.isArray(res.body)).toBe(true);
});
it('returns 401 without auth', async () => {
const res = await request(app).get('/api/notifications');
expect(res.status).toBe(401);
});
it('returns 500 and logs the error when the history read throws', async () => {
const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
vi.spyOn(DatabaseService.getInstance(), 'getNotificationHistory').mockImplementationOnce(() => {
throw new Error('database is locked');
});
const res = await request(app).get('/api/notifications').set('Cookie', authCookie);
expect(res.status).toBe(500);
expect(res.body.error).toBe('Failed to fetch notifications');
expect(consoleErrorSpy).toHaveBeenCalledWith('Failed to fetch notifications:', expect.any(Error));
});
it('POST /read returns 500 and logs the error when the mark-read write throws', async () => {
const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
vi.spyOn(DatabaseService.getInstance(), 'markAllNotificationsRead').mockImplementationOnce(() => {
throw new Error('database is locked');
});
const res = await request(app).post('/api/notifications/read').set('Cookie', authCookie);
expect(res.status).toBe(500);
expect(res.body.error).toBe('Failed to mark notifications read');
expect(consoleErrorSpy).toHaveBeenCalledWith('Failed to mark notifications read:', expect.any(Error));
});
it('DELETE /:id returns 500 and logs the error when the delete throws', async () => {
const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
vi.spyOn(DatabaseService.getInstance(), 'deleteNotification').mockImplementationOnce(() => {
throw new Error('database is locked');
});
const res = await request(app).delete('/api/notifications/1').set('Cookie', authCookie);
expect(res.status).toBe(500);
expect(res.body.error).toBe('Failed to delete notification');
expect(consoleErrorSpy).toHaveBeenCalledWith('Failed to delete notification:', expect.any(Error));
});
it('DELETE / returns 500 and logs the error when the clear-all write throws', async () => {
const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
vi.spyOn(DatabaseService.getInstance(), 'deleteAllNotifications').mockImplementationOnce(() => {
throw new Error('database is locked');
});
const res = await request(app).delete('/api/notifications').set('Cookie', authCookie);
expect(res.status).toBe(500);
expect(res.body.error).toBe('Failed to clear notifications');
expect(consoleErrorSpy).toHaveBeenCalledWith('Failed to clear notifications:', expect.any(Error));
});
});
@@ -2,7 +2,7 @@
* Unit tests for Notification Routing — CRUD operations on notification_routes,
* routing logic in NotificationService, and edge cases.
*/
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
// ── Hoisted mocks ──────────────────────────────────────────────────────
@@ -55,6 +55,7 @@ const mockFetch = vi.fn().mockResolvedValue({ ok: true });
vi.stubGlobal('fetch', mockFetch);
import { NotificationService } from '../services/NotificationService';
import { StackActivityMetricsService } from '../services/StackActivityMetricsService';
// ── Helpers ────────────────────────────────────────────────────────────
@@ -473,3 +474,100 @@ describe('NotificationService - routing logic', () => {
);
});
});
describe('NotificationService - crash safety (dispatchAlert never rejects)', () => {
let svc: NotificationService;
let consoleErrorSpy: ReturnType<typeof vi.spyOn>;
beforeEach(() => {
vi.clearAllMocks();
(NotificationService as any).instance = undefined;
svc = NotificationService.getInstance();
consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
});
afterEach(() => {
consoleErrorSpy.mockRestore();
});
it('resolves (does not reject) and skips dispatch when the history write throws', async () => {
const recordSpy = vi.spyOn(StackActivityMetricsService.getInstance(), 'record');
mockAddNotificationHistory.mockImplementationOnce(() => {
throw new Error('database is locked');
});
mockGetEnabledAgents.mockReturnValue([makeAgent()]);
await expect(
svc.dispatchAlert('error', 'monitor_alert', 'Container crashed', { stackName: 'my-app' }),
).resolves.toBeUndefined();
// No external dispatch and no routing read when there is no persisted row.
expect(mockFetch).not.toHaveBeenCalled();
expect(mockGetEnabledNotificationRoutes).not.toHaveBeenCalled();
// The write failure is recorded for metrics and logged.
expect(recordSpy).toHaveBeenCalledWith(1, 'write', expect.any(Number), false);
expect(consoleErrorSpy).toHaveBeenCalledWith('[Notify] Failed to persist notification:', expect.any(Error));
recordSpy.mockRestore();
});
it('records a successful write metric on the happy path', async () => {
const recordSpy = vi.spyOn(StackActivityMetricsService.getInstance(), 'record');
await svc.dispatchAlert('info', 'system', 'Host rebooted');
expect(recordSpy).toHaveBeenCalledWith(1, 'write', expect.any(Number), true);
recordSpy.mockRestore();
});
it('resolves (does not reject) when reading routes throws after a successful write', async () => {
mockGetEnabledNotificationRoutes.mockImplementationOnce(() => {
throw new Error('database read failed');
});
await expect(
svc.dispatchAlert('error', 'monitor_alert', 'Container crashed', { stackName: 'my-app' }),
).resolves.toBeUndefined();
// The history row was still written; only routing failed, and it was logged.
expect(mockAddNotificationHistory).toHaveBeenCalledTimes(1);
expect(consoleErrorSpy).toHaveBeenCalledWith('[Notify] dispatchAlert failed:', expect.any(Error));
});
it('resolves (does not reject) when a subscriber send throws during broadcast', async () => {
// A subscriber whose socket reports OPEN but throws on send exercises the
// broadcast leg of the outer guard.
const throwingWs = {
readyState: 1, // WebSocket.OPEN
send: () => { throw new Error('socket write failed'); },
} as unknown as import('ws').WebSocket;
svc.subscribe(throwingWs);
await expect(
svc.dispatchAlert('info', 'system', 'Host rebooted'),
).resolves.toBeUndefined();
expect(mockAddNotificationHistory).toHaveBeenCalledTimes(1);
expect(consoleErrorSpy).toHaveBeenCalledWith('[Notify] dispatchAlert failed:', expect.any(Error));
});
it('snapshots subscribers so an unsubscribe during a send does not skip later subscribers', async () => {
const sent: string[] = [];
let unsubscribeB: () => void = () => {};
const wsB = {
readyState: 1, // WebSocket.OPEN
send: () => { sent.push('B'); },
} as unknown as import('ws').WebSocket;
// A's send removes B mid-iteration, mimicking a 'close' handler firing.
const wsA = {
readyState: 1,
send: () => { sent.push('A'); unsubscribeB(); },
} as unknown as import('ws').WebSocket;
svc.subscribe(wsA);
unsubscribeB = svc.subscribe(wsB);
await svc.dispatchAlert('info', 'system', 'Host rebooted');
// B still receives the broadcast because iteration runs over a snapshot.
expect(sent).toEqual(['A', 'B']);
});
});
@@ -9,3 +9,22 @@ export function validateHttpsUrl(value: unknown): string | null {
try { new URL(value); } catch { return 'is not a valid URL'; }
return null;
}
/**
* Mask a channel webhook URL for logging. Discord/Slack/custom webhook URLs
* embed their auth token in the path (and sometimes the query), so only the
* origin is safe to emit. Returns `https://host/<redacted>` or a generic
* placeholder if the value is not a parseable URL.
*/
export function maskWebhookUrl(value: unknown): string {
if (typeof value !== 'string' || value.length === 0) return '<no url>';
try {
const { origin, pathname, search } = new URL(value);
// pathname is normalized to '/' for an origin-only URL, so anything else
// (or any query string) is a token-bearing segment we must not log.
const hasSecret = pathname !== '/' || search !== '';
return hasSecret ? `${origin}/<redacted>` : origin;
} catch {
return '<invalid url>';
}
}
+13 -7
View File
@@ -9,9 +9,11 @@ import {
NOTIFICATION_CHANNEL_TYPES,
validateHttpsUrl,
cleanStackPatterns,
maskWebhookUrl,
} from '../helpers/notificationChannels';
import { isDebugEnabled } from '../utils/debug';
import { getErrorMessage } from '../utils/errors';
import { sanitizeForLog } from '../utils/safeLog';
import { parseIntParam } from '../utils/parseIntParam';
const VALID_CATEGORIES: ReadonlySet<NotificationCategory> = new Set(ALL_NOTIFICATION_CATEGORIES);
@@ -56,7 +58,8 @@ notificationsRouter.get('/', authMiddleware, async (req: Request, res: Response)
const category = typeof req.query.category === 'string' ? req.query.category : undefined;
const history = DatabaseService.getInstance().getNotificationHistory(nodeId, 50, category);
res.json(history);
} catch {
} catch (error) {
console.error('Failed to fetch notifications:', error);
res.status(500).json({ error: 'Failed to fetch notifications' });
}
});
@@ -66,7 +69,8 @@ notificationsRouter.post('/read', authMiddleware, async (req: Request, res: Resp
const nodeId = req.nodeId ?? 0;
DatabaseService.getInstance().markAllNotificationsRead(nodeId);
res.json({ success: true });
} catch {
} catch (error) {
console.error('Failed to mark notifications read:', error);
res.status(500).json({ error: 'Failed to mark notifications read' });
}
});
@@ -78,7 +82,8 @@ notificationsRouter.delete('/:id', authMiddleware, async (req: Request, res: Res
const nodeId = req.nodeId ?? 0;
DatabaseService.getInstance().deleteNotification(nodeId, id);
res.json({ success: true });
} catch {
} catch (error) {
console.error('Failed to delete notification:', error);
res.status(500).json({ error: 'Failed to delete notification' });
}
});
@@ -88,7 +93,8 @@ notificationsRouter.delete('/', authMiddleware, async (req: Request, res: Respon
const nodeId = req.nodeId ?? 0;
DatabaseService.getInstance().deleteAllNotifications(nodeId);
res.json({ success: true });
} catch {
} catch (error) {
console.error('Failed to clear notifications:', error);
res.status(500).json({ error: 'Failed to clear notifications' });
}
});
@@ -172,8 +178,8 @@ notificationRoutesRouter.post('/', authMiddleware, async (req: Request, res: Res
created_at: now,
updated_at: now,
});
console.log(`[Routes] Route "${route.name}" created (id=${route.id})`);
if (isDebugEnabled()) console.log(`[Routes:diag] Route "${route.name}" created with patterns=[${cleanedPatterns}], channel=${channel_type}`);
console.log(`[Routes] Route "${sanitizeForLog(route.name)}" created (id=${route.id})`);
if (isDebugEnabled()) console.log(`[Routes:diag] Route "${sanitizeForLog(route.name)}" created with patterns=[${sanitizeForLog(cleanedPatterns.join(', '))}], channel=${channel_type}`);
res.status(201).json(route);
} catch (error) {
console.error('Failed to create notification route:', error);
@@ -284,7 +290,7 @@ notificationRoutesRouter.post('/:id/test', authMiddleware, async (req: Request,
const route = DatabaseService.getInstance().getNotificationRoute(id);
if (!route) { res.status(404).json({ error: 'Route not found' }); return; }
if (isDebugEnabled()) console.log(`[Routes:diag] Test dispatch for route ${id} (${route.channel_type} -> ${route.channel_url})`);
if (isDebugEnabled()) console.log(`[Routes:diag] Test dispatch for route ${id} (${route.channel_type} -> ${maskWebhookUrl(route.channel_url)})`);
await NotificationService.getInstance().testDispatch(route.channel_type, route.channel_url);
res.json({ success: true });
} catch (error) {
+86 -64
View File
@@ -74,7 +74,9 @@ export class NotificationService {
private broadcastToSubscribers(notification: NotificationHistory): void {
if (this.subscribers.size === 0) return;
const msg = JSON.stringify({ type: 'notification', payload: notification });
for (const ws of this.subscribers) {
// Snapshot first: a 'close'/'error' handler firing during a send would
// otherwise mutate the Set mid-iteration.
for (const ws of [...this.subscribers]) {
if (ws.readyState === WebSocket.OPEN) {
ws.send(msg);
}
@@ -93,7 +95,9 @@ export class NotificationService {
public broadcastEvent(envelope: { type: string; [key: string]: unknown }): void {
if (this.subscribers.size === 0) return;
const msg = JSON.stringify(envelope);
for (const ws of this.subscribers) {
// Snapshot first: a 'close'/'error' handler firing during a send would
// otherwise mutate the Set mid-iteration.
for (const ws of [...this.subscribers]) {
if (ws.readyState === WebSocket.OPEN) {
ws.send(msg);
}
@@ -104,9 +108,14 @@ export class NotificationService {
* Dispatch an alert: log to history, push via WebSocket, and route to
* external channels.
*
* Routing uses two tiers that coexist intentionally:
* - notification_routes (Admiral tier): per-stack pattern-based routing
* with priority ordering. If any route matches, global agents are skipped.
* Never rejects. Callers fire this off without awaiting, so any failure
* (node resolution, history insert, channel-table read, broadcast) is
* caught and logged internally rather than propagated.
*
* Routing uses two layers that coexist intentionally:
* - notification_routes (paid tier, admin-managed): per-stack
* pattern/label/category routing with priority ordering. If any route
* matches, global agents are skipped.
* - agents table (all tiers): global fallback channels used when no
* notification_routes match or when no stackName is provided.
*/
@@ -118,48 +127,59 @@ export class NotificationService {
) {
const t0 = Date.now();
const { stackName, containerName, actor } = options ?? {};
// Internal writes use the middleware default so they share a row key
// with user-initiated requests; otherwise the UI and monitors split
// between different node_id buckets.
const localNodeId = NodeRegistry.getInstance().getDefaultNodeId();
// Use the full resolution chain (node.compose_dir, env, default)
// so messages mentioning a per-node compose override get collapsed.
const sanitized = sanitizeNotificationMessage(message, {
composeDir: NodeRegistry.getInstance().getComposeDir(localNodeId),
});
let notification: NotificationHistory;
// dispatchAlert is called fire-and-forget from monitors, event streams,
// and request handlers across the app. It must never reject: node
// resolution, the history insert, channel-table reads, and the
// WebSocket broadcast can all throw on an unhealthy DB, which would
// otherwise surface as an unhandledRejection and take the process down.
// The whole body is wrapped so the worst case is a dropped notification.
try {
notification = this.dbService.addNotificationHistory(localNodeId, {
level,
category,
message: sanitized,
timestamp: Date.now(),
stack_name: stackName,
container_name: containerName,
actor_username: actor ?? null,
// Internal writes use the middleware default so they share a row key
// with user-initiated requests; otherwise the UI and monitors split
// between different node_id buckets.
const localNodeId = NodeRegistry.getInstance().getDefaultNodeId();
// Use the full resolution chain (node.compose_dir, env, default)
// so messages mentioning a per-node compose override get collapsed.
const sanitized = sanitizeNotificationMessage(message, {
composeDir: NodeRegistry.getInstance().getComposeDir(localNodeId),
});
} catch (err) {
StackActivityMetricsService.getInstance().record(localNodeId, 'write', Date.now() - t0, false);
throw err;
}
StackActivityMetricsService.getInstance().record(localNodeId, 'write', Date.now() - t0, true);
// Separate [StackActivity:diag] namespace from the [Notify:diag] lines
// below so a single grep can pull every per-stack timeline write across
// route reads and dispatch writes.
if (isDebugEnabled()) {
console.log('[StackActivity:diag] write', {
category, stackName, nodeId: localNodeId, actor: actor ?? null, messageLen: sanitized.length,
});
}
// 2. Push to connected browser clients via WebSocket
this.broadcastToSubscribers(notification);
// The inner try only distinguishes a write success from a write
// failure for metrics; on failure there is no row to dispatch, so
// we log and stop.
let notification: NotificationHistory;
try {
notification = this.dbService.addNotificationHistory(localNodeId, {
level,
category,
message: sanitized,
timestamp: Date.now(),
stack_name: stackName,
container_name: containerName,
actor_username: actor ?? null,
});
StackActivityMetricsService.getInstance().record(localNodeId, 'write', Date.now() - t0, true);
} catch (err) {
StackActivityMetricsService.getInstance().record(localNodeId, 'write', Date.now() - t0, false);
console.error('[Notify] Failed to persist notification:', err);
return;
}
// Separate [StackActivity:diag] namespace from the [Notify:diag] lines
// below so a single grep can pull every per-stack timeline write across
// route reads and dispatch writes.
if (isDebugEnabled()) {
console.log('[StackActivity:diag] write', {
category, stackName, nodeId: localNodeId, actor: actor ?? null, messageLen: sanitized.length,
});
}
// 3. Check notification routing rules — always evaluated, matchers compose AND
const errors: string[] = [];
// 2. Push to connected browser clients via WebSocket
this.broadcastToSubscribers(notification);
// 3. Check notification routing rules — always evaluated, matchers compose AND
const errors: string[] = [];
{
const routes = this.dbService.getEnabledNotificationRoutes();
const needsLabels = stackName !== undefined && routes.some(r => r.label_ids != null && r.label_ids.length > 0);
const stackLabelIds = needsLabels ? this.dbService.getStackLabelIds(localNodeId, stackName!) : [];
@@ -177,10 +197,10 @@ export class NotificationService {
matched.map(route =>
this.sendToChannel(route.channel_type, route.channel_url, level, sanitized)
.then(() => {
if (isDebugEnabled()) console.log(`[Notify:diag] Dispatched ${level} via route "${route.name}" (${route.channel_type})`);
if (isDebugEnabled()) console.log(`[Notify:diag] Dispatched ${level} via route "${sanitizeForLog(route.name)}" (${route.channel_type})`);
})
.catch(error => {
console.error(`Failed to dispatch notification via route "${route.name}":`, error);
console.error(`Failed to dispatch notification via route "${sanitizeForLog(route.name)}":`, error);
errors.push(`Route "${route.name}": ${getErrorMessage(error, String(error))}`);
})
)
@@ -188,29 +208,31 @@ export class NotificationService {
this.recordDispatchErrors(notification.id!, errors);
return;
}
}
// 4. Fall back to this instance's agents (keyed by this instance's default node id).
const agents = this.dbService.getEnabledAgents(localNodeId);
if (agents.length === 0) {
if (isDebugEnabled()) console.log('[Notify:diag] No routes or agents matched; skipping external dispatch');
return;
}
// 4. Fall back to this instance's agents (keyed by this instance's default node id).
const agents = this.dbService.getEnabledAgents(localNodeId);
if (agents.length === 0) {
if (isDebugEnabled()) console.log('[Notify:diag] No routes or agents matched; skipping external dispatch');
return;
}
if (isDebugEnabled()) console.log(`[Notify:diag] Falling back to ${agents.length} global agent(s)`);
await Promise.allSettled(
agents.map(agent =>
this.sendToChannel(agent.type, agent.url, level, sanitized)
.then(() => {
if (isDebugEnabled()) console.log(`[Notify:diag] Dispatched ${level} via global agent (${agent.type})`);
})
.catch(error => {
console.error(`Failed to dispatch notification to ${agent.type}:`, error);
errors.push(`${agent.type}: ${getErrorMessage(error, String(error))}`);
})
)
);
this.recordDispatchErrors(notification.id!, errors);
if (isDebugEnabled()) console.log(`[Notify:diag] Falling back to ${agents.length} global agent(s)`);
await Promise.allSettled(
agents.map(agent =>
this.sendToChannel(agent.type, agent.url, level, sanitized)
.then(() => {
if (isDebugEnabled()) console.log(`[Notify:diag] Dispatched ${level} via global agent (${agent.type})`);
})
.catch(error => {
console.error(`Failed to dispatch notification to ${agent.type}:`, error);
errors.push(`${agent.type}: ${getErrorMessage(error, String(error))}`);
})
)
);
this.recordDispatchErrors(notification.id!, errors);
} catch (err) {
console.error('[Notify] dispatchAlert failed:', err);
}
}
/** Persist dispatch errors to the notification record for user visibility. */