From 43e972be845f496a44fe66e246197d8b0692b77f Mon Sep 17 00:00:00 2001 From: Brennan Neoh <497569+brennanneoh@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:36:07 +0800 Subject: [PATCH] fix: SSH-login alerts silently dropped (channel load + auth middleware ordering) (#1083) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: load notification channels on mount in AlertsPanel Channels only loaded when the Channels tab was visited, so opening Edit Alert Rule before ever switching to that tab showed the channel picker as empty even when channels existed. (cherry picked from commit caed913ee91990a853f5a048849c67ed3f7c329e) * fix: register login-alert route before auth middleware Global JWT auth middleware ran before this internal service-to-service route, rejecting it with 401 before its own IP+token check ever ran — silently dropped every SSH-login alert. Also surface non-OK responses instead of swallowing them. Co-Authored-By: Claude Sonnet 5 * test: add coverage for alert-notification fixes Channel-load-on-mount, login-alert non-OK handling, and a source-order guard for the route/auth-middleware regression. Co-Authored-By: Claude Sonnet 5 * format AlertsPanel test with prettier --------- Co-authored-by: Claude Sonnet 5 Co-authored-by: ZacharyZcR --- src/backend/hosts/metrics/index.ts | 4 +- .../metrics/login-alert-route-order.test.ts | 27 ++++ src/backend/tests/utils/alert-trigger.test.ts | 46 ++++++ src/ui/sidebar/AlertsPanel.tsx | 3 +- src/ui/tests/sidebar/AlertsPanel.test.tsx | 131 ++++++++++++++++++ 5 files changed, 209 insertions(+), 2 deletions(-) create mode 100644 src/backend/tests/hosts/metrics/login-alert-route-order.test.ts create mode 100644 src/ui/tests/sidebar/AlertsPanel.test.tsx diff --git a/src/backend/hosts/metrics/index.ts b/src/backend/hosts/metrics/index.ts index 5084922e..9dacffdb 100644 --- a/src/backend/hosts/metrics/index.ts +++ b/src/backend/hosts/metrics/index.ts @@ -842,7 +842,9 @@ app.use((_req, res, next) => { next(); }); -// Internal endpoint — only accepts calls from localhost. +// Internal endpoint — only accepts calls from localhost. Registered before +// the auth middleware since it's a service-to-service call authenticated by +// IP + shared secret, not a user JWT. // Used by the main backend to notify the metrics service of SSH login events. app.post("/internal/login-alert", async (req, res) => { const remoteIp = req.socket.remoteAddress; diff --git a/src/backend/tests/hosts/metrics/login-alert-route-order.test.ts b/src/backend/tests/hosts/metrics/login-alert-route-order.test.ts new file mode 100644 index 00000000..11f4c422 --- /dev/null +++ b/src/backend/tests/hosts/metrics/login-alert-route-order.test.ts @@ -0,0 +1,27 @@ +import { describe, it, expect } from "vitest"; +import fs from "fs"; +import path from "path"; + +// Regression guard for: the /internal/login-alert route was registered +// after the global JWT auth middleware, so every service-to-service login +// alert got rejected with 401 before the route's own IP+token check ever +// ran. Spinning up the full metrics-service Express app (DB, SSH clients, +// polling managers, etc.) just to hit this one route is out of scope, so +// this asserts the registration order directly against the source instead. +describe("metrics service /internal/login-alert route order", () => { + it("is registered before the global auth middleware", () => { + const source = fs.readFileSync( + path.resolve(__dirname, "../../../hosts/metrics/index.ts"), + "utf8", + ); + + const routeIndex = source.indexOf('app.post("/internal/login-alert"'); + const authMiddlewareIndex = source.indexOf( + "app.use(authManager.createAuthMiddleware())", + ); + + expect(routeIndex).toBeGreaterThan(-1); + expect(authMiddlewareIndex).toBeGreaterThan(-1); + expect(routeIndex).toBeLessThan(authMiddlewareIndex); + }); +}); diff --git a/src/backend/tests/utils/alert-trigger.test.ts b/src/backend/tests/utils/alert-trigger.test.ts index d4ed63c1..1edf42db 100644 --- a/src/backend/tests/utils/alert-trigger.test.ts +++ b/src/backend/tests/utils/alert-trigger.test.ts @@ -45,4 +45,50 @@ describe("triggerLoginAlert", () => { expect(warn).not.toHaveBeenCalled(); }); + + it("sends the internal auth token and login details the metrics service expects", async () => { + vi.spyOn(SystemCrypto, "getInstance").mockReturnValue({ + getInternalAuthToken: vi.fn().mockResolvedValue("internal-token"), + } as never); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response('{"ok":true}', { status: 200 })); + + await triggerLoginAlert(42, "user-1", "root", "10.0.0.5"); + + expect(fetchSpy).toHaveBeenCalledWith( + "http://localhost:30005/internal/login-alert", + expect.objectContaining({ + method: "POST", + headers: expect.objectContaining({ + "x-internal-auth": "internal-token", + }), + body: JSON.stringify({ + hostId: 42, + userId: "user-1", + sshUser: "root", + fromIp: "10.0.0.5", + }), + }), + ); + }); + + it("logs a warning if the fetch itself throws, instead of propagating", async () => { + vi.spyOn(SystemCrypto, "getInstance").mockReturnValue({ + getInternalAuthToken: vi.fn().mockResolvedValue("internal-token"), + } as never); + vi.spyOn(globalThis, "fetch").mockRejectedValue( + new Error("connect ECONNREFUSED"), + ); + const warn = vi.spyOn(sshLogger, "warn").mockImplementation(() => {}); + + await expect( + triggerLoginAlert(1, "user-1", "root", "127.0.0.1"), + ).resolves.toBeUndefined(); + + expect(warn).toHaveBeenCalledWith( + "Failed to trigger login alert", + expect.objectContaining({ hostId: 1 }), + ); + }); }); diff --git a/src/ui/sidebar/AlertsPanel.tsx b/src/ui/sidebar/AlertsPanel.tsx index 4c4b77fa..aac8ed5f 100644 --- a/src/ui/sidebar/AlertsPanel.tsx +++ b/src/ui/sidebar/AlertsPanel.tsx @@ -96,7 +96,8 @@ export function AlertsPanel() { useEffect(() => { loadFirings(); - }, [loadFirings]); + loadChannels(); + }, [loadFirings, loadChannels]); useEffect(() => { if (tab === "rules") loadRules(); diff --git a/src/ui/tests/sidebar/AlertsPanel.test.tsx b/src/ui/tests/sidebar/AlertsPanel.test.tsx new file mode 100644 index 00000000..2e1c1a28 --- /dev/null +++ b/src/ui/tests/sidebar/AlertsPanel.test.tsx @@ -0,0 +1,131 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { + render, + screen, + waitFor, + cleanup, + fireEvent, +} from "@testing-library/react"; +import type { NotificationChannel } from "@/api/alerts-api"; + +const alertsApi = vi.hoisted(() => ({ + getAlertFirings: vi.fn(async () => []), + acknowledgeAlertFiring: vi.fn(async () => {}), + acknowledgeAllAlertFirings: vi.fn(async () => {}), + getAlertRules: vi.fn(async () => []), + getNotificationChannels: vi.fn(async () => [] as NotificationChannel[]), + deleteAlertRule: vi.fn(async () => {}), + deleteNotificationChannel: vi.fn(async () => {}), + testNotificationChannel: vi.fn(async () => {}), +})); + +vi.mock("@/api/alerts-api", () => alertsApi); + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ + t: (_key: string, fallback?: string) => fallback ?? _key, + }), +})); + +// A real (non-null) fake so tests can see what channels actually reach the +// rule dialog, instead of only asserting the API call count. +vi.mock("../../sidebar/AlertRuleDialog", () => ({ + AlertRuleDialog: ({ + open, + channels, + }: { + open: boolean; + channels: NotificationChannel[]; + }) => + open ? ( +
+ {channels.length === 0 + ? "no channels" + : channels.map((c) => c.name).join(",")} +
+ ) : null, +})); +vi.mock("../../sidebar/NotificationChannelDialog", () => ({ + NotificationChannelDialog: () => null, +})); + +import { AlertsPanel } from "../../sidebar/AlertsPanel"; + +function channel( + overrides: Partial = {}, +): NotificationChannel { + return { + id: 1, + userId: "user-1", + name: "ntfy", + type: "ntfy", + config: "{}", + enabled: true, + createdAt: new Date().toISOString(), + ...overrides, + }; +} + +beforeEach(() => { + alertsApi.getAlertFirings.mockReset().mockResolvedValue([]); + alertsApi.getAlertRules.mockReset().mockResolvedValue([]); + alertsApi.getNotificationChannels.mockReset().mockResolvedValue([]); +}); + +afterEach(() => { + cleanup(); +}); + +describe("AlertsPanel - notification channels", () => { + it("loads notification channels on mount, before the Channels tab is ever opened", async () => { + alertsApi.getNotificationChannels.mockResolvedValue([channel()]); + + render(); + + // Regression: channels used to only load once the user switched to the + // "channels" tab, so editing a rule before ever visiting that tab showed + // an empty channel picker even though channels existed. + await waitFor(() => { + expect(alertsApi.getNotificationChannels).toHaveBeenCalledTimes(1); + }); + }); + + it("populates the rule dialog's channel picker without ever visiting the Channels tab", async () => { + alertsApi.getNotificationChannels.mockResolvedValue([ + channel({ id: 1, name: "ntfy-home" }), + channel({ id: 2, name: "webhook-slack" }), + ]); + + render(); + + await waitFor(() => { + expect(alertsApi.getNotificationChannels).toHaveBeenCalledTimes(1); + }); + + // Go straight to Rules and open the "new rule" dialog, skipping Channels + // entirely, this is exactly the sequence the original bug broke. + fireEvent.click(screen.getByText("Rules")); + fireEvent.click(screen.getByText("Add")); + + expect(screen.getByTestId("rule-dialog-channels").textContent).toBe( + "ntfy-home,webhook-slack", + ); + }); + + it("does not refetch channels again when switching to the Rules tab", async () => { + render(); + + await waitFor(() => { + expect(alertsApi.getNotificationChannels).toHaveBeenCalledTimes(1); + }); + + // Switching tabs only reloads rules here, not channels, since channels + // were already fetched on mount by the fix under test. + fireEvent.click(screen.getByText("Rules")); + + await waitFor(() => { + expect(alertsApi.getAlertRules).toHaveBeenCalledTimes(1); + }); + expect(alertsApi.getNotificationChannels).toHaveBeenCalledTimes(1); + }); +});