fix(alerts): harden with security fixes, design compliance, and test coverage (#570)

* fix(alerts): harden with security fixes, design compliance, and test coverage

Add authMiddleware to all alert endpoints, validate notification test
dispatch inputs, fix restart_count metric via Docker inspect, correct
network metric units, replace any types with DockerContainerStats
interface, add webhook timeouts and dispatch error tracking.

Frontend: migrate Select to Combobox, add ScrollArea and delete
confirmation AlertDialog, fix icon strokeWidth to 1.5.

Add update availability notifications for both Sencho version updates
(6-hour check in MonitorService) and stack image updates (state
transition detection in ImageUpdateService). Extract shared version
fetch logic into utils/version-check.ts.

Add diagnostic logging gated behind developer_mode for MonitorService
breach state machine and NotificationService dispatch routing.

Tests: 24 new alert API integration tests, restart_count and version
check unit tests (688 total passing). Docs updated with HTTPS
requirement, update notifications section, and troubleshooting guide.

* fix(alerts): remove unused TEST_USERNAME import in alerts-api tests

backend/src/__tests__/alerts-api.test.ts

@@ -0,0 +1,282 @@
+/**
+ * Integration tests for Alert CRUD endpoints, notification test dispatch
+ * validation, and auth enforcement on all alert/notification routes.
+ */
+import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest';
+import request from 'supertest';
+import bcrypt from 'bcrypt';
+import { setupTestDb, cleanupTestDb, loginAsTestAdmin } from './helpers/setupTestDb';
+
+let tmpDir: string;
+let app: import('express').Express;
+let DatabaseService: typeof import('../services/DatabaseService').DatabaseService;
+let authCookie: string;
+let viewerCookie: string;
+
+beforeAll(async () => {
+  tmpDir = await setupTestDb();
+  ({ DatabaseService } = await import('../services/DatabaseService'));
+
+  // Mock LicenseService so Admiral-gated routes are accessible
+  const { LicenseService } = await import('../services/LicenseService');
+  vi.spyOn(LicenseService.getInstance(), 'getTier').mockReturnValue('paid');
+  vi.spyOn(LicenseService.getInstance(), 'getVariant').mockReturnValue('admiral');
+  vi.spyOn(LicenseService.getInstance(), 'getSeatLimits').mockReturnValue({ maxAdmins: null, maxViewers: null });
+
+  ({ app } = await import('../index'));
+  authCookie = await loginAsTestAdmin(app);
+
+  // Create a viewer user for non-admin tests
+  const viewerHash = await bcrypt.hash('viewerpass', 1);
+  DatabaseService.getInstance().addUser({ username: 'viewer', password_hash: viewerHash, role: 'viewer' });
+  const viewerRes = await request(app)
+    .post('/api/auth/login')
+    .send({ username: 'viewer', password: 'viewerpass' });
+  const cookies = viewerRes.headers['set-cookie'] as string | string[];
+  viewerCookie = Array.isArray(cookies) ? cookies[0] : cookies;
+});
+
+afterAll(() => {
+  cleanupTestDb(tmpDir);
+});
+
+// --- GET /api/alerts ---
+
+describe('GET /api/alerts', () => {
+  it('rejects unauthenticated requests with 401', async () => {
+    const res = await request(app).get('/api/alerts');
+    expect(res.status).toBe(401);
+  });
+
+  it('returns empty array when no alerts exist', async () => {
+    const res = await request(app)
+      .get('/api/alerts')
+      .set('Cookie', authCookie);
+    expect(res.status).toBe(200);
+    expect(res.body).toEqual([]);
+  });
+
+  it('filters alerts by stackName query param', async () => {
+    // Seed two alerts for different stacks
+    const db = DatabaseService.getInstance();
+    db.addStackAlert({ stack_name: 'web', metric: 'cpu_percent', operator: '>', threshold: 80, duration_mins: 5, cooldown_mins: 60 });
+    db.addStackAlert({ stack_name: 'api', metric: 'memory_percent', operator: '>', threshold: 90, duration_mins: 5, cooldown_mins: 60 });
+
+    const res = await request(app)
+      .get('/api/alerts?stackName=web')
+      .set('Cookie', authCookie);
+    expect(res.status).toBe(200);
+    expect(res.body.length).toBe(1);
+    expect(res.body[0].stack_name).toBe('web');
+  });
+});
+
+// --- POST /api/alerts ---
+
+describe('POST /api/alerts', () => {
+  it('rejects unauthenticated requests with 401', async () => {
+    const res = await request(app)
+      .post('/api/alerts')
+      .send({ stack_name: 'test', metric: 'cpu_percent', operator: '>', threshold: 80, duration_mins: 5, cooldown_mins: 60 });
+    expect(res.status).toBe(401);
+  });
+
+  it('rejects non-admin users with 403', async () => {
+    const res = await request(app)
+      .post('/api/alerts')
+      .set('Cookie', viewerCookie)
+      .send({ stack_name: 'test', metric: 'cpu_percent', operator: '>', threshold: 80, duration_mins: 5, cooldown_mins: 60 });
+    expect(res.status).toBe(403);
+  });
+
+  it('creates alert and returns 201 with created resource', async () => {
+    const payload = {
+      stack_name: 'new-stack',
+      metric: 'memory_percent',
+      operator: '>=',
+      threshold: 85,
+      duration_mins: 10,
+      cooldown_mins: 30,
+    };
+
+    const res = await request(app)
+      .post('/api/alerts')
+      .set('Cookie', authCookie)
+      .send(payload);
+
+    expect(res.status).toBe(201);
+    expect(res.body.id).toBeDefined();
+    expect(res.body.stack_name).toBe('new-stack');
+    expect(res.body.metric).toBe('memory_percent');
+    expect(res.body.threshold).toBe(85);
+  });
+
+  it('validates required fields and returns 400 for missing data', async () => {
+    const res = await request(app)
+      .post('/api/alerts')
+      .set('Cookie', authCookie)
+      .send({ stack_name: 'test' }); // missing metric, operator, threshold, etc.
+
+    expect(res.status).toBe(400);
+    expect(res.body.error).toBe('Invalid alert data');
+  });
+
+  it('rejects invalid metric values', async () => {
+    const res = await request(app)
+      .post('/api/alerts')
+      .set('Cookie', authCookie)
+      .send({ stack_name: 'test', metric: 'invalid_metric', operator: '>', threshold: 80, duration_mins: 5, cooldown_mins: 60 });
+
+    expect(res.status).toBe(400);
+  });
+
+  it('rejects negative threshold', async () => {
+    const res = await request(app)
+      .post('/api/alerts')
+      .set('Cookie', authCookie)
+      .send({ stack_name: 'test', metric: 'cpu_percent', operator: '>', threshold: -1, duration_mins: 5, cooldown_mins: 60 });
+
+    expect(res.status).toBe(400);
+  });
+
+  it('rejects empty stack_name', async () => {
+    const res = await request(app)
+      .post('/api/alerts')
+      .set('Cookie', authCookie)
+      .send({ stack_name: '', metric: 'cpu_percent', operator: '>', threshold: 80, duration_mins: 5, cooldown_mins: 60 });
+
+    expect(res.status).toBe(400);
+  });
+
+  it('rejects stack_name exceeding 255 characters', async () => {
+    const res = await request(app)
+      .post('/api/alerts')
+      .set('Cookie', authCookie)
+      .send({ stack_name: 'a'.repeat(256), metric: 'cpu_percent', operator: '>', threshold: 80, duration_mins: 5, cooldown_mins: 60 });
+
+    expect(res.status).toBe(400);
+  });
+
+  it('rejects duration_mins exceeding 1440', async () => {
+    const res = await request(app)
+      .post('/api/alerts')
+      .set('Cookie', authCookie)
+      .send({ stack_name: 'test', metric: 'cpu_percent', operator: '>', threshold: 80, duration_mins: 1441, cooldown_mins: 60 });
+
+    expect(res.status).toBe(400);
+  });
+
+  it('rejects cooldown_mins exceeding 10080', async () => {
+    const res = await request(app)
+      .post('/api/alerts')
+      .set('Cookie', authCookie)
+      .send({ stack_name: 'test', metric: 'cpu_percent', operator: '>', threshold: 80, duration_mins: 5, cooldown_mins: 10081 });
+
+    expect(res.status).toBe(400);
+  });
+
+  it('rejects invalid operator', async () => {
+    const res = await request(app)
+      .post('/api/alerts')
+      .set('Cookie', authCookie)
+      .send({ stack_name: 'test', metric: 'cpu_percent', operator: '!=', threshold: 80, duration_mins: 5, cooldown_mins: 60 });
+
+    expect(res.status).toBe(400);
+  });
+});
+
+// --- DELETE /api/alerts/:id ---
+
+describe('DELETE /api/alerts/:id', () => {
+  it('rejects unauthenticated requests with 401', async () => {
+    const res = await request(app).delete('/api/alerts/1');
+    expect(res.status).toBe(401);
+  });
+
+  it('rejects non-admin users with 403', async () => {
+    const res = await request(app)
+      .delete('/api/alerts/1')
+      .set('Cookie', viewerCookie);
+    expect(res.status).toBe(403);
+  });
+
+  it('deletes existing alert rule', async () => {
+    // Create an alert to delete
+    const created = DatabaseService.getInstance().addStackAlert({
+      stack_name: 'delete-me',
+      metric: 'cpu_percent',
+      operator: '>',
+      threshold: 90,
+      duration_mins: 0,
+      cooldown_mins: 0,
+    });
+
+    const res = await request(app)
+      .delete(`/api/alerts/${created.id}`)
+      .set('Cookie', authCookie);
+
+    expect(res.status).toBe(200);
+    expect(res.body.success).toBe(true);
+  });
+});
+
+// --- POST /api/notifications/test ---
+
+describe('POST /api/notifications/test', () => {
+  it('rejects unauthenticated requests with 401', async () => {
+    const res = await request(app)
+      .post('/api/notifications/test')
+      .send({ type: 'discord', url: 'https://discord.com/api/webhooks/123/abc' });
+    expect(res.status).toBe(401);
+  });
+
+  it('rejects non-admin users with 403', async () => {
+    const res = await request(app)
+      .post('/api/notifications/test')
+      .set('Cookie', viewerCookie)
+      .send({ type: 'discord', url: 'https://discord.com/api/webhooks/123/abc' });
+    expect(res.status).toBe(403);
+  });
+
+  it('rejects invalid type with 400', async () => {
+    const res = await request(app)
+      .post('/api/notifications/test')
+      .set('Cookie', authCookie)
+      .send({ type: 'telegram', url: 'https://example.com' });
+    expect(res.status).toBe(400);
+    expect(res.body.error).toContain('discord, slack, or webhook');
+  });
+
+  it('rejects missing type with 400', async () => {
+    const res = await request(app)
+      .post('/api/notifications/test')
+      .set('Cookie', authCookie)
+      .send({ url: 'https://example.com' });
+    expect(res.status).toBe(400);
+  });
+
+  it('rejects non-HTTPS url with 400', async () => {
+    const res = await request(app)
+      .post('/api/notifications/test')
+      .set('Cookie', authCookie)
+      .send({ type: 'discord', url: 'http://example.com' });
+    expect(res.status).toBe(400);
+    expect(res.body.error).toContain('HTTPS');
+  });
+
+  it('rejects missing url with 400', async () => {
+    const res = await request(app)
+      .post('/api/notifications/test')
+      .set('Cookie', authCookie)
+      .send({ type: 'discord' });
+    expect(res.status).toBe(400);
+  });
+
+  it('rejects malformed url with 400', async () => {
+    const res = await request(app)
+      .post('/api/notifications/test')
+      .set('Cookie', authCookie)
+      .send({ type: 'discord', url: 'https://' });
+    expect(res.status).toBe(400);
+  });
+});

backend/src/__tests__/monitor-service.test.ts

@@ -10,9 +10,11 @@ const { mockGetGlobalSettings, mockGetNodes, mockGetStackAlerts, mockAddContaine
   mockCleanupOldMetrics, mockCleanupOldNotifications, mockCleanupOldAuditLogs,
   mockUpdateStackAlertLastFired, mockGetSystemState, mockSetSystemState,
   mockGetRunningContainers, mockGetAllContainers, mockGetContainerStatsStream,
+  mockGetContainerRestartCount,
   mockDispatchAlert,
   mockCurrentLoad, mockMem, mockFsSize,
   mockExecAsync,
+  mockFetchLatestSenchoVersion,
 } = vi.hoisted(() => ({
   mockGetGlobalSettings: vi.fn().mockReturnValue({}),
   mockGetNodes: vi.fn().mockReturnValue([]),
@@ -27,11 +29,13 @@ const { mockGetGlobalSettings, mockGetNodes, mockGetStackAlerts, mockAddContaine
   mockGetRunningContainers: vi.fn().mockResolvedValue([]),
   mockGetAllContainers: vi.fn().mockResolvedValue([]),
   mockGetContainerStatsStream: vi.fn().mockResolvedValue('{}'),
+  mockGetContainerRestartCount: vi.fn().mockResolvedValue(0),
   mockDispatchAlert: vi.fn().mockResolvedValue(undefined),
   mockCurrentLoad: vi.fn().mockResolvedValue({ currentLoad: 10 }),
   mockMem: vi.fn().mockResolvedValue({ used: 4e9, total: 16e9 }),
   mockFsSize: vi.fn().mockResolvedValue([{ mount: '/', use: 30 }]),
   mockExecAsync: vi.fn().mockResolvedValue({ stdout: '' }),
+  mockFetchLatestSenchoVersion: vi.fn().mockRejectedValue(new Error('not configured')),
 }));
 
 vi.mock('../services/DatabaseService', () => ({
@@ -57,10 +61,15 @@ vi.mock('../services/DockerController', () => ({
       getRunningContainers: mockGetRunningContainers,
       getAllContainers: mockGetAllContainers,
       getContainerStatsStream: mockGetContainerStatsStream,
+      getContainerRestartCount: mockGetContainerRestartCount,
     }),
   },
 }));
 
+vi.mock('../utils/version-check', () => ({
+  fetchLatestSenchoVersion: (...args: unknown[]) => mockFetchLatestSenchoVersion(...args),
+}));
+
 vi.mock('../services/NotificationService', () => ({
   NotificationService: {
     getInstance: () => ({
@@ -522,3 +531,135 @@ describe('MonitorService - isProcessing guard', () => {
     expect((svc as any).isProcessing).toBe(false);
   });
 });
+
+// ── restart_count metric ──────────────────────────────────────────────
+
+describe('MonitorService - restart_count metric', () => {
+  function setupRestartScenario(restartCount: number, hasRestartRule: boolean) {
+    mockGetNodes.mockReturnValue([{ id: 1, name: 'local', type: 'local' }]);
+    mockGetRunningContainers.mockResolvedValue([{
+      Id: 'c1',
+      Labels: { 'com.docker.compose.project': 'my-stack' },
+    }]);
+    mockGetContainerStatsStream.mockResolvedValue(JSON.stringify({
+      cpu_stats: { cpu_usage: { total_usage: 1000 }, system_cpu_usage: 5000, online_cpus: 1 },
+      precpu_stats: { cpu_usage: { total_usage: 1000 }, system_cpu_usage: 5000 },
+      memory_stats: { usage: 100e6, limit: 1e9 },
+    }));
+    mockGetContainerRestartCount.mockResolvedValue(restartCount);
+    const alerts = [];
+    if (hasRestartRule) {
+      alerts.push({
+        id: 100,
+        stack_name: 'my-stack',
+        metric: 'restart_count',
+        operator: '>',
+        threshold: 3,
+        duration_mins: 0,
+        cooldown_mins: 60,
+        last_fired_at: 0,
+      });
+    }
+    mockGetStackAlerts.mockReturnValue(alerts);
+    mockGetGlobalSettings.mockReturnValue({});
+  }
+
+  it('fetches restart count from Docker when a restart_count rule exists', async () => {
+    setupRestartScenario(5, true);
+
+    const svc = MonitorService.getInstance();
+    await (svc as any).evaluate();
+
+    expect(mockGetContainerRestartCount).toHaveBeenCalledWith('c1');
+    // restart_count=5 > threshold=3, should fire
+    expect(mockDispatchAlert).toHaveBeenCalledWith('warning', expect.stringContaining('Restart count'), 'my-stack');
+  });
+
+  it('skips Docker inspect when no restart_count rules exist', async () => {
+    setupRestartScenario(5, false);
+
+    const svc = MonitorService.getInstance();
+    await (svc as any).evaluate();
+
+    expect(mockGetContainerRestartCount).not.toHaveBeenCalled();
+  });
+
+  it('does not fire when restart count is below threshold', async () => {
+    setupRestartScenario(2, true);
+
+    const svc = MonitorService.getInstance();
+    await (svc as any).evaluate();
+
+    expect(mockGetContainerRestartCount).toHaveBeenCalledWith('c1');
+    expect(mockDispatchAlert).not.toHaveBeenCalledWith('warning', expect.stringContaining('Restart count'), expect.anything());
+  });
+});
+
+// ── Sencho version update check ───────────────────────────────────────
+
+describe('MonitorService - Sencho version check', () => {
+  it('dispatches notification when newer version available', async () => {
+    // Set current version
+    process.env.npm_package_version = '0.45.0';
+    mockFetchLatestSenchoVersion.mockResolvedValue('0.46.0');
+    mockGetSystemState.mockReturnValue(null); // No previous notification
+    mockGetGlobalSettings.mockReturnValue({});
+    mockGetNodes.mockReturnValue([]);
+    mockGetStackAlerts.mockReturnValue([]);
+
+    const svc = MonitorService.getInstance();
+    // Reset the version check timer so it runs immediately
+    (svc as any).lastVersionCheckAt = 0;
+    await (svc as any).evaluate();
+
+    expect(mockDispatchAlert).toHaveBeenCalledWith('info', expect.stringContaining('0.46.0'));
+    expect(mockSetSystemState).toHaveBeenCalledWith('last_sencho_update_notified_version', '0.46.0');
+  });
+
+  it('does not re-notify for the same version', async () => {
+    process.env.npm_package_version = '0.45.0';
+    mockFetchLatestSenchoVersion.mockResolvedValue('0.46.0');
+    mockGetSystemState.mockReturnValue('0.46.0'); // Already notified for this version
+    mockGetGlobalSettings.mockReturnValue({});
+    mockGetNodes.mockReturnValue([]);
+    mockGetStackAlerts.mockReturnValue([]);
+
+    const svc = MonitorService.getInstance();
+    (svc as any).lastVersionCheckAt = 0;
+    await (svc as any).evaluate();
+
+    expect(mockDispatchAlert).not.toHaveBeenCalledWith('info', expect.stringContaining('0.46.0'));
+  });
+
+  it('handles version check failure gracefully', async () => {
+    process.env.npm_package_version = '0.45.0';
+    mockFetchLatestSenchoVersion.mockRejectedValue(new Error('Network down'));
+    mockGetGlobalSettings.mockReturnValue({});
+    mockGetNodes.mockReturnValue([]);
+    mockGetStackAlerts.mockReturnValue([]);
+
+    const svc = MonitorService.getInstance();
+    (svc as any).lastVersionCheckAt = 0;
+
+    // Should not throw
+    await expect((svc as any).evaluate()).resolves.toBeUndefined();
+    expect(mockDispatchAlert).not.toHaveBeenCalledWith('info', expect.stringContaining('available'));
+  });
+
+  it('respects the 6-hour cooldown interval', async () => {
+    process.env.npm_package_version = '0.45.0';
+    mockFetchLatestSenchoVersion.mockResolvedValue('0.46.0');
+    mockGetSystemState.mockReturnValue(null);
+    mockGetGlobalSettings.mockReturnValue({});
+    mockGetNodes.mockReturnValue([]);
+    mockGetStackAlerts.mockReturnValue([]);
+
+    const svc = MonitorService.getInstance();
+    // Simulate the check ran 1 hour ago (within 6-hour window)
+    (svc as any).lastVersionCheckAt = Date.now() - 1 * 60 * 60 * 1000;
+    await (svc as any).evaluate();
+
+    // fetchLatestSenchoVersion should not have been called since we're within cooldown
+    expect(mockFetchLatestSenchoVersion).not.toHaveBeenCalled();
+  });
+});