From 761a5a54c150ed90598b05d4cacffde9cfc115d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 16:51:57 +0200 Subject: [PATCH] fix(notifications): keep the egress mark from clobbering a set action The mark named its action unconditionally, so a job that had already named its own (the low-stock underflow branch) or a settings route whose test then failed at the relay lost that name on the wide event. Set the action only when the event has none, and always land the channel and origin meta pair, through a small hasAction() on the builder. Note the parse-time floor on the Nightscout page, which shares the grammar. Refs #947 --- docs/integrations/nightscout.md | 7 +++ docs/self-hosting/notifications.md | 16 +++---- src/lib/logging/event-builder.ts | 10 +++++ .../__tests__/egress-policy.test.ts | 43 +++++++++++++++++++ src/lib/notifications/egress-policy.ts | 16 ++++--- .../senders/__tests__/ntfy.test.ts | 8 +++- .../senders/__tests__/webhook.test.ts | 8 +++- 7 files changed, 93 insertions(+), 15 deletions(-) diff --git a/docs/integrations/nightscout.md b/docs/integrations/nightscout.md index 37f32f340..3a75f8ef4 100644 --- a/docs/integrations/nightscout.md +++ b/docs/integrations/nightscout.md @@ -28,9 +28,16 @@ Each entry is an exact normalized `scheme://host:port` trust unit: rejected; - approving one hostname does not approve subdomains, sibling hosts, or a different port; and +- loopback (`127.0.0.0/8`, `::1`, `localhost`), the unspecified address, + link-local and the cloud-metadata range cannot be listed, and a listed + name that resolves there is refused at connect time — the grant opens + your network, never the container or the host it runs on; and - any malformed non-empty entry fails evaluation instead of silently weakening the configured policy. +The grammar is shared with `NOTIFICATION_PRIVATE_ORIGINS`, the same grant +for webhook and ntfy targets (`docs/self-hosting/notifications.md`). + Do not put a Nightscout API token, capability URL, health data, or a URL containing credentials in the allowlist. Restart the application after changing the environment. diff --git a/docs/self-hosting/notifications.md b/docs/self-hosting/notifications.md index ade4cd6ae..0dc298b51 100644 --- a/docs/self-hosting/notifications.md +++ b/docs/self-hosting/notifications.md @@ -254,14 +254,14 @@ lists it. Restart the app after changing it. What is refused where, so you know which lever to pull: -| Target | Without the grant | With the exact origin listed | -| ----------------------------------------------------------- | ----------------- | ---------------------------- | -| Public name resolving to a public address | works | works, public pin | -| Public name with a private A record (LAN reverse proxy) | refused at send | works | -| `gotify.lan`, `gotify.home.arpa`, a Docker service name | refused at send | works | -| `gotify.local` (mDNS) | refused at save | works | -| Literal `192.168.x.x`, `10.x.x.x`, `100.64.x.x`, `fd00::/8` | refused at save | works | -| `localhost`, `127.0.0.1`, `169.254.169.254`, `::1` | refused at save | still refused | +| Target | Without the grant | With the exact origin listed | +| ----------------------------------------------------------- | ----------------- | --------------------------------------- | +| Public name resolving to a public address | works | works, public pin | +| Public name with a private A record (LAN reverse proxy) | refused at send | works | +| `gotify.lan`, `gotify.home.arpa`, a Docker service name | refused at send | works | +| `gotify.local` (mDNS) | refused at save | allowed, if the container resolves mDNS | +| Literal `192.168.x.x`, `10.x.x.x`, `100.64.x.x`, `fd00::/8` | refused at save | works | +| `localhost`, `127.0.0.1`, `169.254.169.254`, `::1` | refused at save | still refused | A private target that is not listed is refused when the card is saved (`422`, `meta.errorCode` = `private_origin_not_approved`, the message diff --git a/src/lib/logging/event-builder.ts b/src/lib/logging/event-builder.ts index 13347ae3b..439d65b5b 100644 --- a/src/lib/logging/event-builder.ts +++ b/src/lib/logging/event-builder.ts @@ -150,6 +150,16 @@ export class WideEventBuilder { return this; } + /** + * Whether a caller has already named this event's action. A leaf that + * wants to mark the event (the private-origin egress mark in the + * notification senders) checks this first so it never overwrites the + * route's or job's own action name, which dashboards pin on. + */ + hasAction(): boolean { + return this.event.action !== undefined; + } + setBackground(bg: WideEvent["background"]): this { this.event.background = bg; return this; diff --git a/src/lib/notifications/__tests__/egress-policy.test.ts b/src/lib/notifications/__tests__/egress-policy.test.ts index 1818abd7b..50631967e 100644 --- a/src/lib/notifications/__tests__/egress-policy.test.ts +++ b/src/lib/notifications/__tests__/egress-policy.test.ts @@ -12,7 +12,11 @@ */ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { eventStorage } from "@/lib/logging/context"; +import { WideEventBuilder } from "@/lib/logging/event-builder"; + import { + annotatePrivateOriginEgress, configuredNotificationPrivateOrigins, evaluateNotificationTarget, isAllowedNotificationTarget, @@ -166,3 +170,42 @@ describe("evaluateNotificationTarget", () => { }); }); }); + +describe("annotatePrivateOriginEgress", () => { + it("names the action when the event has none and pins the meta pair", () => { + const event = new WideEventBuilder("background"); + eventStorage.run(event, () => { + annotatePrivateOriginEgress("webhook", "https://gotify.example.com"); + }); + + const json = event.toJSON(); + expect(json.action).toEqual({ name: "notification.egress.private_origin" }); + expect(json.meta).toEqual({ + channel: "webhook", + origin: "https://gotify.example.com", + }); + }); + + it("keeps an action the route or job already set, and still lands the meta", () => { + // A settings route or a job names its own action; dashboards pin on it. + // The egress mark must not overwrite that name. + const event = new WideEventBuilder("http"); + event.setAction({ name: "settings.webhook.test" }); + eventStorage.run(event, () => { + annotatePrivateOriginEgress("ntfy", "http://ntfy.lan:8080"); + }); + + const json = event.toJSON(); + expect(json.action).toEqual({ name: "settings.webhook.test" }); + expect(json.meta).toEqual({ + channel: "ntfy", + origin: "http://ntfy.lan:8080", + }); + }); + + it("is a no-op without an active event", () => { + expect(() => + annotatePrivateOriginEgress("webhook", "https://gotify.example.com"), + ).not.toThrow(); + }); +}); diff --git a/src/lib/notifications/egress-policy.ts b/src/lib/notifications/egress-policy.ts index 8afce08ed..aab9a14f5 100644 --- a/src/lib/notifications/egress-policy.ts +++ b/src/lib/notifications/egress-policy.ts @@ -6,7 +6,7 @@ import { PRIVATE_ORIGIN_NOT_APPROVED, type OriginVerdict, } from "@/lib/private-origin-policy"; -import { annotate } from "@/lib/logging/context"; +import { annotate, getEvent } from "@/lib/logging/context"; /** * `NOTIFICATION_PRIVATE_ORIGINS` (#947) — the operator grant that lets the @@ -93,13 +93,19 @@ export { PRIVATE_ORIGIN_NOT_APPROVED }; * an operator can see which grant is actually in use. The origin is the * canonical grant string, never the full target URL, which may carry a * token in its path or query. + * + * The action name is set only when the event has none yet: a settings route + * or a job that already named its action keeps it, and the meta pair still + * lands on the same event, so the mark is never lost and never clobbers. */ export function annotatePrivateOriginEgress( channel: "webhook" | "ntfy", origin: string, ): void { - annotate({ - action: { name: "notification.egress.private_origin" }, - meta: { channel, origin }, - }); + const event = getEvent(); + if (!event) return; + if (!event.hasAction()) { + annotate({ action: { name: "notification.egress.private_origin" } }); + } + annotate({ meta: { channel, origin } }); } diff --git a/src/lib/notifications/senders/__tests__/ntfy.test.ts b/src/lib/notifications/senders/__tests__/ntfy.test.ts index a80a8a202..770a6fdae 100644 --- a/src/lib/notifications/senders/__tests__/ntfy.test.ts +++ b/src/lib/notifications/senders/__tests__/ntfy.test.ts @@ -25,7 +25,11 @@ vi.mock("@/lib/safe-fetch", () => ({ })); vi.mock("@/lib/logging/context", () => ({ - getEvent: () => ({ addWarning: vi.fn(), addExternalCall: vi.fn() }), + getEvent: () => ({ + addWarning: vi.fn(), + addExternalCall: vi.fn(), + hasAction: () => false, + }), annotate: (...args: unknown[]) => annotateMock(...args), })); @@ -165,6 +169,8 @@ describe("sendViaNtfy", () => { expect(opts.operatorApprovedPrivateOrigin).toBe("http://ntfy.lan:8080"); expect(annotateMock).toHaveBeenCalledWith({ action: { name: "notification.egress.private_origin" }, + }); + expect(annotateMock).toHaveBeenCalledWith({ meta: { channel: "ntfy", origin: "http://ntfy.lan:8080" }, }); }); diff --git a/src/lib/notifications/senders/__tests__/webhook.test.ts b/src/lib/notifications/senders/__tests__/webhook.test.ts index 6655238cb..410ce3896 100644 --- a/src/lib/notifications/senders/__tests__/webhook.test.ts +++ b/src/lib/notifications/senders/__tests__/webhook.test.ts @@ -26,7 +26,11 @@ vi.mock("@/lib/safe-fetch", () => ({ const annotateMock = vi.fn(); vi.mock("@/lib/logging/context", () => ({ - getEvent: () => ({ addWarning: vi.fn(), addExternalCall: vi.fn() }), + getEvent: () => ({ + addWarning: vi.fn(), + addExternalCall: vi.fn(), + hasAction: () => false, + }), annotate: (...args: unknown[]) => annotateMock(...args), })); @@ -198,6 +202,8 @@ describe("sendViaWebhook", () => { ); expect(annotateMock).toHaveBeenCalledWith({ action: { name: "notification.egress.private_origin" }, + }); + expect(annotateMock).toHaveBeenCalledWith({ meta: { channel: "webhook", origin: "https://gotify.example.com" }, }); });