diff --git a/Dockerfile b/Dockerfile index 0de6f3ccb..42284699a 100644 --- a/Dockerfile +++ b/Dockerfile @@ -184,7 +184,7 @@ RUN mkdir -p /opt/prisma-cli && \ cd /opt/prisma-cli && \ npm init -y && \ npm pkg set 'overrides.deepmerge-ts=^8.0.0' && \ - npm install --omit=dev prisma@7.8 @prisma/engines@7.8 tsx@4.23.1 dotenv@17.4.2 && \ + npm install --omit=dev prisma@7.8.0 @prisma/engines@7.8.0 tsx@4.23.1 dotenv@17.4.2 && \ ln -sfn /opt/prisma-cli/node_modules/.bin/tsx /usr/local/bin/healthlog-tsx && \ ln -sfn /opt/prisma-cli/node_modules/dotenv /app/node_modules/dotenv && \ ln -sfn /opt/prisma-cli/node_modules/prisma /app/node_modules/prisma && \ diff --git a/eslint-plugins/healthlog/spacing-scale.js b/eslint-plugins/healthlog/spacing-scale.js index 6c5e032fa..cb5f9418c 100644 --- a/eslint-plugins/healthlog/spacing-scale.js +++ b/eslint-plugins/healthlog/spacing-scale.js @@ -61,8 +61,15 @@ const ENFORCED_ROOTS = ["src/components/", "src/app/"]; // A pt-/pb- utility with a numeric step, `px`, or an arbitrary value, // optionally behind modifier prefixes (`md:`, `hover:`, `[.border-b]:`). // `pb-safe` (safe-area helpers) and word-suffixed utilities never match. +// +// The modifier chain excludes `:` from its chunk class so each chunk ends at +// exactly one colon. With `:` inside the class the star was ambiguous — +// `(?:[^\s"']*:)*` could carve the same prefix into chunks many ways — and a +// long non-matching class string made the engine try them all +// (exponential backtracking, CodeQL js/redos). Same strings accepted, +// one parse each, linear time. const SLOT_PADDING_RE = - /(?:^|\s)(?:[^\s"']*:)*(p[tb]-(?:\d+(?:\.\d+)?|px|\[[^\]]+\]))(?=\s|$)/; + /(?:^|\s)(?:[^\s"':]*:)*(p[tb]-(?:\d+(?:\.\d+)?|px|\[[^\]]+\]))(?=\s|$)/; const SLOT_NAMES = new Set(["CardHeader", "CardContent"]); @@ -83,8 +90,10 @@ const SPACING_PREFIX = // `5` or `7`; or a half-step whose integer part is anything but `1` // (`0.5`, `2.5`…`9.5`, `10.5`+), so the sanctioned `1.5` is left alone. const OFF_SCALE_STEP = "5|7|(?:[02-9]|\\d\\d+)\\.5"; +// Modifier chain shaped like SLOT_PADDING_RE's, and linear for the same +// reason: `:` is excluded from the chunk class so the star has one parse. const SHELL_OFFSCALE_RE = new RegExp( - `(?:^|\\s)(?:[^\\s"']*:)*((?:${SPACING_PREFIX})-(?:${OFF_SCALE_STEP}))(?=\\s|$)`, + `(?:^|\\s)(?:[^\\s"':]*:)*((?:${SPACING_PREFIX})-(?:${OFF_SCALE_STEP}))(?=\\s|$)`, ); function toPosix(filename) { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 271314a1c..32915255a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -13,6 +13,9 @@ overrides: picomatch@<4.0.4: ^4.0.4 body-parser@<2.3.0: ^2.3.0 fast-uri@<3.1.5: ^3.1.5 + flatted@<3.4.2: ^3.4.2 + js-yaml@>=4.0.0 <4.3.1: ^4.3.1 + minimatch@<3.1.4: ^3.1.4 ip-address@<10.3.1: ^10.3.1 sharp@<0.35.0: ^0.35.3 pdfjs-dist@>=5.6.83 <6.2.108: 6.2.108 @@ -3696,9 +3699,6 @@ packages: bowser@2.14.1: resolution: {integrity: sha512-tzPjzCxygAKWFOJP011oxFHs57HzIhOEracIgAePE4pqB3LikALKnSzUyU4MGs9/iCEUuHlAJTjTc5M+u7YEGg==} - brace-expansion@1.1.12: - resolution: {integrity: sha512-9T9UjW3r0UW5c1Q7GTwllptXwhvYmEzFhzMfZ9H7FQWt+uZePjZPjBP/W1ZEyZ1twGWom5/56TF4lPcqjnDHcg==} - brace-expansion@1.1.18: resolution: {integrity: sha512-Edep/X9fGqVNmzKBVsDYIOtD+z1tuezV70LBjdCst9Tqu76lsnvRiZ6oTic1n+/BIwX6QDGAO94PN4N2SADvtw==} @@ -4554,8 +4554,8 @@ packages: resolution: {integrity: sha512-f7ccFPK3SXFHpx15UIGyRJ/FJQctuKZ0zVuN3frBo4HnK3cay9VEW0R6yPYFHC0AgqhukPzKjq22t5DmAyqGyw==} engines: {node: '>=16'} - flatted@3.3.3: - resolution: {integrity: sha512-GX+ysw4PBCz0PzosHDepZGANEuFCMLrnRTiEy9McGjmkCQYwRq4A/X786G/fjM/+OjsWSU1ZrY5qyARZmO/uwg==} + flatted@3.4.4: + resolution: {integrity: sha512-5+ybhBZANEJxaH3X5evAFatUxLfEHSr7n6kYJ+1Qd0mUqr4eu9gIf6GDbWHf8RJijHrjjO8G+la14SlL2SeS1Q==} for-each@0.3.5: resolution: {integrity: sha512-dKx12eRCVIzqCxFGplyFKJMPvLEWgmNtUrpTiJIR5u97zEhRG8ySrtboPHZXx7daLxQVrl643cTzbab2tkQjxg==} @@ -5093,10 +5093,6 @@ packages: js-tokens@4.0.0: resolution: {integrity: sha512-RdJUflcE3cUzKiMqQgsCu06FPu9UdIJO0beYbPhHN4k6apgJtifcoCtT9bcxOpYBtpD2kCM6Sbzg4CausW/PKQ==} - js-yaml@4.1.1: - resolution: {integrity: sha512-qQKT4zQxXl8lLwBtHMWwaTcGfFOZviOJet3Oy/xmGk2gZH677CJM9EvtfdSkgWcATZhj/55JZ0rmy3myCT5lsA==} - hasBin: true - js-yaml@4.3.1: resolution: {integrity: sha512-CY6crGq313MX8GkwvB7tzgp99vjQxY1++5y10/BKN/GUfHqWaOGQMNZkBvqSzsZKWk/ijwHlWzzkLulsGHhjWQ==} hasBin: true @@ -5458,9 +5454,6 @@ packages: resolution: {integrity: sha512-vpLQEs+VLCr1nU0BXS07maYoFwlDAH0gngQuuttxIwutDFEMHq2blX+8vpgxDdK3J1PwjCJiep77OitTZ4Ll1A==} engines: {node: 18 || 20 || >=22} - minimatch@3.1.3: - resolution: {integrity: sha512-M2GCs7Vk83NxkUyQV1bkABc4yxgz9kILhHImZiBPAZ9ybuvCb0/H7lEl5XvIg3g+9d4eNotkZA5IWwYl0tibaA==} - minimatch@3.1.5: resolution: {integrity: sha512-VgjWUsnnT6n+NUk6eZq77zeFdpW2LWDzP6zFGrCbHXiYNul5Dzqk2HHQ5uFH2DNW5Xbp8+jVzaeNt94ssEEl4w==} @@ -7818,7 +7811,7 @@ snapshots: dependencies: '@eslint/object-schema': 2.1.7 debug: 4.4.3(supports-color@7.2.0) - minimatch: 3.1.3 + minimatch: 3.1.5 transitivePeerDependencies: - supports-color @@ -7838,8 +7831,8 @@ snapshots: globals: 14.0.0 ignore: 5.3.2 import-fresh: 3.3.1 - js-yaml: 4.1.1 - minimatch: 3.1.3 + js-yaml: 4.3.1 + minimatch: 3.1.5 strip-json-comments: 3.1.1 transitivePeerDependencies: - supports-color @@ -10633,11 +10626,6 @@ snapshots: bowser@2.14.1: {} - brace-expansion@1.1.12: - dependencies: - balanced-match: 1.0.2 - concat-map: 0.0.1 - brace-expansion@1.1.18: dependencies: balanced-match: 1.0.2 @@ -11453,7 +11441,7 @@ snapshots: is-glob: 4.0.3 json-stable-stringify-without-jsonify: 1.0.1 lodash.merge: 4.6.2 - minimatch: 3.1.3 + minimatch: 3.1.5 natural-compare: 1.4.0 optionator: 0.9.4 optionalDependencies: @@ -11698,10 +11686,10 @@ snapshots: flat-cache@4.0.1: dependencies: - flatted: 3.3.3 + flatted: 3.4.4 keyv: 4.5.4 - flatted@3.3.3: {} + flatted@3.4.4: {} for-each@0.3.5: dependencies: @@ -12187,10 +12175,6 @@ snapshots: js-tokens@4.0.0: {} - js-yaml@4.1.1: - dependencies: - argparse: 2.0.1 - js-yaml@4.3.1: dependencies: argparse: 2.0.1 @@ -12509,10 +12493,6 @@ snapshots: dependencies: brace-expansion: 5.0.9 - minimatch@3.1.3: - dependencies: - brace-expansion: 1.1.12 - minimatch@3.1.5: dependencies: brace-expansion: 1.1.18 diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 24652c777..e25a0b26e 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -7,6 +7,13 @@ overrides: "picomatch@<4.0.4": "^4.0.4" "body-parser@<2.3.0": "^2.3.0" "fast-uri@<3.1.5": "^3.1.5" + "flatted@<3.4.2": "^3.4.2" + # The advisory covers 4.x only; the bound keeps a hypothetical 3.x consumer + # off a major it never asked for. + "js-yaml@>=4.0.0 <4.3.1": "^4.3.1" + # The scanner suggests 10.x (latest), but the ReDoS fix is backported: only + # <3.1.4 is vulnerable, so the 3.x line stays where its consumers expect it. + "minimatch@<3.1.4": "^3.1.4" # Address4 decoded leading-zero octets as decimal, so `010.0.0.1` parsed as # 10.0.0.1 rather than being rejected. That is the exact alt-notation class # `isPublicUrl` exists to catch, and this copy arrives under the MCP SDK's diff --git a/src/lib/__tests__/geo-offline-detection.test.ts b/src/lib/__tests__/geo-offline-detection.test.ts index 178439514..2379c9c6b 100644 --- a/src/lib/__tests__/geo-offline-detection.test.ts +++ b/src/lib/__tests__/geo-offline-detection.test.ts @@ -1,32 +1,45 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; -import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + mkdtempSync, + readFileSync, + rmSync, + unlinkSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; /** - * v1.4.27 R5 — runtime detection + one-shot admin notification for the - * offline GeoLite2 tier. + * Runtime detection + once-per-state admin notification for the offline + * GeoLite2 tier. * * The CI workflow drops an `.empty` marker into `assets/geolite2/` * whenever `MAXMIND_LICENSE_KEY` is unset, so the Docker COPY still * lands a non-empty directory in `/opt/geolite2/`. The runtime resolver - * detects the marker on first fallback, fires a single localised - * notification to every admin, and stays silent for the rest of the - * process lifetime. + * detects the marker on first fallback and fires a localised + * notification to every admin. * - * The tests cover the three states the design pins: + * Issue #851: the sent-latch used to be a module-level boolean, so every + * worker boot re-sent the same notice while the configuration never + * changed. The anchor now persists in the `notification_events` ledger + * (via `claimNotificationEvent`) and is released when the state exits — + * databases configured, or online lookups disabled. The tests pin the + * full state machine: * - * 1. `.empty` marker present, City MMDB absent → notification fires - * on first public-IP lookup. - * 2. No marker, City MMDB present → no notification fires. - * 3. Marker present, repeat lookups → notification fires once and - * never again. + * 1. Entering the unconfigured state sends, once, to every admin. + * 2. Staying in the state — further lookups, and simulated worker + * restarts — stays silent. + * 3. Leaving the state releases the anchor; re-entering sends again. + * 4. `IP_GEO_LOOKUP_DISABLED=1` counts as having left the state: with + * no egress there is nothing to warn about. * * `mmdb-lib` is stubbed via the same shape the existing * `geo-asn.test.ts` uses; `fs` is left real because `offlineGeoReady` - * reads real files at the temp-dir path picked per test, and the - * `dispatchLocalisedNotification` + `prisma.user.findMany` are mocked - * so the test never reaches a database or a sender. + * reads real files at the temp-dir path picked per test. `@/lib/db` is + * mocked with an in-memory `notification_events` store that outlives + * `vi.resetModules()`, which is what lets a test simulate a restart + * while the "database" keeps its rows. `safeFetch` is stubbed so the + * online-fallback path never leaves the process. */ vi.mock("mmdb-lib", () => ({ @@ -41,15 +54,76 @@ vi.mock("mmdb-lib", () => ({ }, })); +vi.mock("@/lib/safe-fetch", () => ({ + safeFetch: vi.fn(async () => ({ ok: false, status: 503 })), +})); + const dispatchSpy = vi.fn(async () => undefined); vi.mock("@/lib/notifications/dispatch-localised", () => ({ dispatchLocalisedNotification: dispatchSpy, })); +interface EventRow { + recordUserId: string; + eventType: string; + dedupKey: string; + createdAt: Date; +} + +/** In-memory `notification_events` — survives `vi.resetModules()`. */ +const eventStore: EventRow[] = []; + const findManySpy = vi.fn(async () => [{ id: "admin-1" }]); + +const txStub = { + $queryRaw: async () => [{ locked: 1 }], + notificationEvent: { + findFirst: async (args: { + where: { + recordUserId: string; + eventType: string; + dedupKey: string; + createdAt: { gte: Date }; + }; + }) => + eventStore.find( + (row) => + row.recordUserId === args.where.recordUserId && + row.eventType === args.where.eventType && + row.dedupKey === args.where.dedupKey && + row.createdAt >= args.where.createdAt.gte, + ) ?? null, + create: async (args: { + data: { recordUserId: string; eventType: string; dedupKey: string }; + }) => { + const row: EventRow = { ...args.data, createdAt: new Date() }; + eventStore.push(row); + return row; + }, + }, +}; + vi.mock("@/lib/db", () => ({ prisma: { user: { findMany: findManySpy }, + $transaction: async (fn: (tx: typeof txStub) => Promise) => + fn(txStub), + notificationEvent: { + deleteMany: async (args: { + where: { eventType: string; dedupKey: string }; + }) => { + const before = eventStore.length; + for (let i = eventStore.length - 1; i >= 0; i--) { + if ( + eventStore[i].eventType === args.where.eventType && + eventStore[i].dedupKey === args.where.dedupKey + ) { + eventStore.splice(i, 1); + } + } + return { count: before - eventStore.length }; + }, + }, }, })); @@ -57,7 +131,7 @@ const ORIGINAL_ENV = { ...process.env }; let tmpRoot: string; async function flushMicrotasks(): Promise { - // The notification path is fire-and-forget — `void notify…()` — + // The notification path is fire-and-forget — `void evaluate…()` — // and pulls in `@/lib/db` + `@/lib/notifications/dispatch-localised` // through dynamic `import()` calls, so each dispatch needs several // microtask + macrotask flushes before the spy registers the call. @@ -75,6 +149,7 @@ beforeEach(() => { dispatchSpy.mockClear(); findManySpy.mockClear(); findManySpy.mockResolvedValue([{ id: "admin-1" }]); + eventStore.length = 0; vi.resetModules(); }); @@ -85,7 +160,7 @@ afterEach(() => { vi.unstubAllGlobals(); }); -describe("offline-geo runtime detection (v1.4.27 R5)", () => { +describe("offline-geo runtime detection", () => { it("offlineGeoReady() is false when the .empty marker is present", async () => { writeFileSync(join(tmpRoot, ".empty"), ""); const { offlineGeoReady } = await import("../geo"); @@ -125,10 +200,8 @@ describe("offline-geo runtime detection (v1.4.27 R5)", () => { expect(offlineGeoReady()).toBe(true); }); - it("fires the one-shot admin notification when the marker is present and lookupIpLocation falls back", async () => { + it("fires the admin notification when the marker is present and lookupIpLocation falls back", async () => { writeFileSync(join(tmpRoot, ".empty"), ""); - // Disable the online fallback so the test does not hit the network. - process.env.IP_GEO_LOOKUP_DISABLED = "1"; const { lookupIpLocation } = await import("../geo"); await lookupIpLocation("8.8.8.8"); @@ -152,7 +225,6 @@ describe("offline-geo runtime detection (v1.4.27 R5)", () => { it("names the real online provider in the notification params", async () => { writeFileSync(join(tmpRoot, ".empty"), ""); - process.env.IP_GEO_LOOKUP_DISABLED = "1"; process.env.IP_GEO_LOOKUP_URL = "https://ip-api.example/json"; const { lookupIpLocation } = await import("../geo"); @@ -204,6 +276,20 @@ describe("offline-geo runtime detection (v1.4.27 R5)", () => { // The stubbed reader returns null for every IP, which is fine — the // lookup will still go through the online path, but the offline // check sees a healthy directory so the notification stays silent. + const { lookupIpLocation } = await import("../geo"); + + await lookupIpLocation("8.8.8.8"); + await flushMicrotasks(); + + expect(dispatchSpy).not.toHaveBeenCalled(); + expect(findManySpy).not.toHaveBeenCalled(); + }); + + it("does NOT fire the notification when online lookups are disabled", async () => { + // With IP_GEO_LOOKUP_DISABLED=1 no address leaves the host, so there + // is no egress for the notice to warn about — the disabled host has + // left the state exactly like one that mounted the databases. + writeFileSync(join(tmpRoot, ".empty"), ""); process.env.IP_GEO_LOOKUP_DISABLED = "1"; const { lookupIpLocation } = await import("../geo"); @@ -214,9 +300,8 @@ describe("offline-geo runtime detection (v1.4.27 R5)", () => { expect(findManySpy).not.toHaveBeenCalled(); }); - it("fires the notification exactly once across repeated fallbacks (process-level latch)", async () => { + it("fires the notification exactly once across repeated fallbacks in one process", async () => { writeFileSync(join(tmpRoot, ".empty"), ""); - process.env.IP_GEO_LOOKUP_DISABLED = "1"; const { lookupIpLocation, lookupIpAsn } = await import("../geo"); await lookupIpLocation("8.8.8.8"); @@ -232,7 +317,6 @@ describe("offline-geo runtime detection (v1.4.27 R5)", () => { it("falls back silently when no admin user is configured", async () => { writeFileSync(join(tmpRoot, ".empty"), ""); - process.env.IP_GEO_LOOKUP_DISABLED = "1"; findManySpy.mockResolvedValueOnce([]); const { lookupIpLocation } = await import("../geo"); @@ -253,4 +337,72 @@ describe("offline-geo runtime detection (v1.4.27 R5)", () => { expect(result).toBeNull(); expect(dispatchSpy).toHaveBeenCalledTimes(1); }); + + it("is once per configuration state: silent across restarts, resent only after exit + re-entry (issue #851)", async () => { + const cityDb = join(tmpRoot, "GeoLite2-City.mmdb"); + + // ── Boot 1: unconfigured → the notice goes out and anchors durably. + writeFileSync(join(tmpRoot, ".empty"), ""); + let geo = await import("../geo"); + await geo.lookupIpLocation("8.8.8.8"); + await flushMicrotasks(); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + expect(eventStore).toHaveLength(1); + + // ── Boot 2: worker restart, state unchanged → the persisted anchor + // keeps it silent. This is the #851 repeat, pinned closed. + vi.resetModules(); + geo = await import("../geo"); + await geo.lookupIpLocation("8.8.8.8"); + await flushMicrotasks(); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + expect(eventStore).toHaveLength(1); + + // ── Boot 3: the operator mounts the databases → state exits, the + // anchor is released, still no new send. + unlinkSync(join(tmpRoot, ".empty")); + writeFileSync(cityDb, Buffer.from("city-stub")); + vi.resetModules(); + geo = await import("../geo"); + await geo.lookupIpLocation("8.8.8.8"); + await flushMicrotasks(); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + expect(eventStore).toHaveLength(0); + + // ── Boot 4: the databases are gone again → genuine re-entry into the + // unconfigured state, and the notice fires afresh. + unlinkSync(cityDb); + writeFileSync(join(tmpRoot, ".empty"), ""); + vi.resetModules(); + geo = await import("../geo"); + await geo.lookupIpLocation("8.8.8.8"); + await flushMicrotasks(); + expect(dispatchSpy).toHaveBeenCalledTimes(2); + expect(eventStore).toHaveLength(1); + }); + + it("re-evaluates after a runtime database fetch resets the reader cache", async () => { + // The runtime GeoLite2 fetch calls `resetGeoLite2ReaderCache()` after + // placing fresh databases. The next lookup must observe the healthy + // state and release the anchor without waiting for a restart. + writeFileSync(join(tmpRoot, ".empty"), ""); + const geo = await import("../geo"); + + await geo.lookupIpLocation("8.8.8.8"); + await flushMicrotasks(); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + expect(eventStore).toHaveLength(1); + + unlinkSync(join(tmpRoot, ".empty")); + writeFileSync( + join(tmpRoot, "GeoLite2-City.mmdb"), + Buffer.from("city-stub"), + ); + geo.resetGeoLite2ReaderCache(); + + await geo.lookupIpLocation("1.1.1.1"); + await flushMicrotasks(); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + expect(eventStore).toHaveLength(0); + }); }); diff --git a/src/lib/geo.ts b/src/lib/geo.ts index 3efa3cd5a..6c7b2f9a6 100644 --- a/src/lib/geo.ts +++ b/src/lib/geo.ts @@ -76,15 +76,25 @@ * `MAXMIND_LICENSE_KEY` secret — the CI workflow drops an `.empty` * marker into the geo asset directory when the key is unset. * `offlineGeoReady()` is the canonical check used by `/api/version` - * and the admin status surface, and the lookup paths fire a one-shot - * admin notification on the first fallback so the maintainer hears - * about the gap from the running app. + * and the admin status surface, and the lookup paths fire an admin + * notification on the first fallback so the maintainer hears about the + * gap from the running app. The notification is once per CONFIGURATION + * STATE, not once per process: the sent-anchor persists in the + * `notification_events` ledger and is released when the state exits + * (databases configured, or lookups disabled), so a worker restart does + * not re-send while nothing changed (issue #851) and a genuine re-entry + * into the unconfigured state does. */ import fs from "node:fs"; import path from "node:path"; import { Reader as MmdbReader } from "mmdb-lib"; import type { AsnResponse, CityResponse } from "mmdb-lib/lib/reader/response"; import { getEvent } from "@/lib/logging/context"; +import { + claimNotificationEvent, + releaseNotificationEvent, + CONFIG_NOTICE_EVENT_TYPE, +} from "@/lib/notifications/reminder-dedup"; import { safeFetch } from "@/lib/safe-fetch"; interface IpwhoIsResponse { @@ -257,13 +267,16 @@ function getAsnReader(): MmdbReader | null { * calls this after it places freshly downloaded databases, so a running worker * picks them up without a restart — the readers latch `null` on the first miss, * so without this a process that booted before the files landed would keep - * resolving online forever. Also clears the one-shot "offline unavailable" - * admin-notification latch, since the offline tier may now be present. + * resolving online forever. Also clears the "offline unavailable" + * notice-evaluation latch, so the next lookup re-evaluates the + * configuration state — and, finding the offline tier present, releases + * the persisted sent-anchor so a later loss of the databases notifies + * again. */ export function resetGeoLite2ReaderCache(): void { cache.city = undefined; cache.asn = undefined; - notifiedThisProcess = false; + noticeEvaluatedThisProcess = false; } /** @@ -277,7 +290,7 @@ export function __resetGeoLite2CacheForTests(): void { GEO_CACHE.clear(); } -// ── Offline readiness + one-shot admin notification ───────────────── +// ── Offline readiness + once-per-state admin notification ──────────── // // The CI workflow drops an `.empty` marker into the geo asset directory // when the maintainer has not configured `MAXMIND_LICENSE_KEY`, in @@ -286,12 +299,36 @@ export function __resetGeoLite2CacheForTests(): void { // `/api/version`, the admin status surface, and the resolver's own // offline-first branch, so the three cannot disagree. // -// `notifiedThisProcess` is a module-level latch so the admin only -// receives the "offline geo is disabled" notification once per worker -// boot — every subsequent fallback hit is silent. Test-only reset is -// folded into the cache reset above. +// The notification is once per CONFIGURATION STATE. A module-level latch +// alone re-sent it on every worker boot while the state never changed +// (issue #851: same notice again and again on a host that has simply not +// mounted the databases). The durable anchor is a per-admin +// `notification_events` claim — the same ledger every other repeat-prone +// notice anchors in — released when the state exits, i.e. when the +// databases are configured or the online lookup is disabled. Entering the +// unconfigured state sends once; staying in it is silent across restarts; +// leaving and re-entering sends again. +// +// `noticeEvaluatedThisProcess` remains as a cheap gate so the ledger is +// consulted once per worker boot (and again after a runtime database +// fetch resets it), not on every lookup. Test-only reset is folded into +// the cache reset above. -let notifiedThisProcess = false; +let noticeEvaluatedThisProcess = false; + +/** Durable anchor identity for the offline-geo notice in `notification_events`. */ +const OFFLINE_GEO_NOTICE_DEDUP_KEY = "geo:offline-unavailable"; + +/** + * Whether the host is in the state the notice describes: the offline tier + * is not configured AND lookups still egress to the online provider. With + * `IP_GEO_LOOKUP_DISABLED=1` there is no egress to warn about, so the + * disabled host counts as having left the state — exactly like a host + * that mounted the databases. + */ +function offlineGeoNoticeStateActive(): boolean { + return !offlineGeoReady() && process.env.IP_GEO_LOOKUP_DISABLED !== "1"; +} /** * Where the admin notification sends someone who wants the offline tier. @@ -311,17 +348,28 @@ export function offlineGeoReady(): boolean { } } -async function notifyOfflineGeoUnavailable(): Promise { - if (notifiedThisProcess) return; - notifiedThisProcess = true; +async function evaluateOfflineGeoNotice(): Promise { + if (noticeEvaluatedThisProcess) return; + noticeEvaluatedThisProcess = true; // Defer the resolve so we don't pay the cost of pulling the - // Prisma client into the import graph until the first fallback — + // Prisma client into the import graph until the first evaluation — // most calls are cache hits or private-IP short-circuits. try { - const [{ prisma }, { dispatchLocalisedNotification }] = await Promise.all([ - import("@/lib/db"), - import("@/lib/notifications/dispatch-localised"), - ]); + const { prisma } = await import("@/lib/db"); + + if (!offlineGeoNoticeStateActive()) { + // The state the notice describes does not hold. Release any + // persisted sent-anchor so a future re-entry into the unconfigured + // state notifies again — this is the "state exit" edge. + await releaseNotificationEvent(prisma, { + eventType: CONFIG_NOTICE_EVENT_TYPE, + dedupKey: OFFLINE_GEO_NOTICE_DEDUP_KEY, + }); + return; + } + + const { dispatchLocalisedNotification } = + await import("@/lib/notifications/dispatch-localised"); const admins = await prisma.user.findMany({ where: { role: "ADMIN" }, select: { id: true }, @@ -332,10 +380,21 @@ async function notifyOfflineGeoUnavailable(): Promise { ); return; } - getEvent()?.addWarning( - `geo: offline databases unavailable, every lookup goes online to ${resolveGeoProviderHost()} — notifying admins`, - ); for (const admin of admins) { + // The anchor is unwindowed (`since` at epoch): it holds until the + // state exit above releases it, however many restarts happen in + // between. An admin who already has the anchor claimed nothing and + // hears nothing. + const claimed = await claimNotificationEvent(prisma, { + recordUserId: admin.id, + eventType: CONFIG_NOTICE_EVENT_TYPE, + dedupKey: OFFLINE_GEO_NOTICE_DEDUP_KEY, + since: new Date(0), + }); + if (!claimed) continue; + getEvent()?.addWarning( + `geo: offline databases unavailable, every lookup goes online to ${resolveGeoProviderHost()} — notifying admin`, + ); await dispatchLocalisedNotification({ userId: admin.id, titleKey: "notifications.admin.offlineGeoUnavailableTitle", @@ -610,15 +669,17 @@ export function lookupIpAsn( ip: string | null, ): { asn: number; carrier: string | null } | null { if (!ip || PRIVATE_IP.test(ip)) return null; + // First public-IP lookup of the process evaluates the offline-geo + // configuration state: unconfigured-with-egress sends the admin notice + // (once per state, anchored durably), configured-or-disabled releases + // the anchor so a later re-entry notifies again. Runs on the healthy + // path too — that is where the state-exit edge is observed. + void evaluateOfflineGeoNotice(); const reader = getAsnReader(); if (!reader) { - // No offline ASN data — alert once per process. The online tier can - // still fill the carrier from its ISP field, but the ASN number and - // the authoritative carrier come from the MMDB, so the admin is told - // once that the offline tier is not being read. - if (!offlineGeoReady()) { - void notifyOfflineGeoUnavailable(); - } + // No offline ASN data. The online tier can still fill the carrier + // from its ISP field, but the ASN number and the authoritative + // carrier come from the MMDB. return null; } try { diff --git a/src/lib/jobs/reminder/__tests__/notification-retention.test.ts b/src/lib/jobs/reminder/__tests__/notification-retention.test.ts index 2573d6698..4314ad55d 100644 --- a/src/lib/jobs/reminder/__tests__/notification-retention.test.ts +++ b/src/lib/jobs/reminder/__tests__/notification-retention.test.ts @@ -49,8 +49,14 @@ describe("notification retention", () => { expect(h.pushDeleteMany).toHaveBeenCalledWith({ where: { createdAt: { lt: expect.any(Date) } }, }); + // State-scoped admin-config-notice anchors live as long as the state + // they describe; the horizon prune must leave them alone or the + // once-per-state notice re-fires every 90 days. expect(h.eventDeleteMany).toHaveBeenCalledWith({ - where: { createdAt: { lt: expect.any(Date) } }, + where: { + createdAt: { lt: expect.any(Date) }, + eventType: { not: "ADMIN_CONFIG_NOTICE" }, + }, }); expect(h.authorizationDeleteMany).toHaveBeenCalledWith({ where: { authorizedAt: { lt: expect.any(Date) } }, diff --git a/src/lib/jobs/reminder/cleanup-handlers.ts b/src/lib/jobs/reminder/cleanup-handlers.ts index e793bce69..98ad16be9 100644 --- a/src/lib/jobs/reminder/cleanup-handlers.ts +++ b/src/lib/jobs/reminder/cleanup-handlers.ts @@ -38,6 +38,7 @@ import { cleanupExpiredMoodTombstones, cleanupExpiredIntakeTombstones, } from "@/lib/jobs/measurement-tombstone-cleanup"; +import { CONFIG_NOTICE_EVENT_TYPE } from "@/lib/notifications/reminder-dedup"; import { getWorkerPrisma } from "./shared"; const MOOD_REMINDER_RETENTION_DAYS = 90; @@ -125,7 +126,12 @@ export async function handleMoodReminderCleanup( * so anything older than the 90-day retention window is dead weight * inflating the table and the `(user_id, created_at DESC)` index. Record-only * notification events are claims rather than delivery diagnostics, but after - * the same horizon their dedup windows have elapsed and they have no reader. + * the same horizon their dedup windows have elapsed and they have no reader — + * with one carve-out: state-scoped admin-config-notice anchors + * (`CONFIG_NOTICE_EVENT_TYPE`) hold for as long as the configuration state + * they describe and are released explicitly on state exit, so the horizon + * delete must not touch them or the once-per-state notice re-fires every + * 90 days. * * The DELETE is unbounded by user. The event and attempt ledgers carry direct * `created_at` indexes, and final egress claims carry `authorized_at`, so each @@ -151,7 +157,10 @@ export async function handlePushAttemptCleanup( where: { createdAt: { lt: cutoff } }, }); const deletedEvents = await p.notificationEvent.deleteMany({ - where: { createdAt: { lt: cutoff } }, + where: { + createdAt: { lt: cutoff }, + eventType: { not: CONFIG_NOTICE_EVENT_TYPE }, + }, }); const deletedAuthorizations = await p.notificationEgressAuthorization.deleteMany({ diff --git a/src/lib/notifications/__tests__/strip-html.test.ts b/src/lib/notifications/__tests__/strip-html.test.ts new file mode 100644 index 000000000..063d0ae3b --- /dev/null +++ b/src/lib/notifications/__tests__/strip-html.test.ts @@ -0,0 +1,50 @@ +import { describe, it, expect } from "vitest"; +import { stripHtml } from "@/lib/notifications/strip-html"; + +/** + * The five plain-text senders used to strip tags with a local single-pass + * regex replace (CodeQL: incomplete multi-character sanitization). The + * hardened shared helper re-applies the strip until the string stops + * changing, so a tag re-formed by an earlier removal cannot survive. These + * cases pin the invariant that matters — no tag-shaped sequence in any + * output — and identity on plain input. + */ +describe("stripHtml", () => { + it("leaves plain input unchanged", () => { + expect(stripHtml("Blood pressure logged: 120/80")).toBe( + "Blood pressure logged: 120/80", + ); + expect(stripHtml("")).toBe(""); + }); + + it("strips simple tags exactly like the old single pass", () => { + expect(stripHtml("urgent reading")).toBe("urgent reading"); + expect(stripHtml("line
break")).toBe("linebreak"); + expect(stripHtml("a < b and b > a")).toBe("a a"); + }); + + it("removes the classic nested-tag bypass shapes", () => { + expect(stripHtml("<script>alert(1)")).toBe("script>alert(1)"); + expect(stripHtml("<script>")).toBe("script>"); + expect(stripHtml("<<script>>payload")).toBe("script>>payload"); + }); + + it("never lets a tag-shaped sequence survive any adversarial input", () => { + const hostile = [ + "<script>alert(1)</script>", + "ipt>alert(1)ipt>", + "<".repeat(64) + "b>".repeat(64) + "script>alert(1)", + '', + "e>f>g>", + ]; + for (const input of hostile) { + expect(stripHtml(input), input).not.toMatch(/<[^>]*>/); + } + }); + + it("handles unterminated brackets without stripping legitimate text", () => { + // No closing `>` anywhere after the `<` — nothing tag-shaped to remove. + expect(stripHtml(" { + try { + await prisma.notificationEvent.deleteMany({ + where: { + eventType: input.eventType, + dedupKey: input.dedupKey, + }, + }); + } catch { + // Swallowed by design; see above. + } +} diff --git a/src/lib/notifications/senders/apns.ts b/src/lib/notifications/senders/apns.ts index 6ea4afdc7..118195217 100644 --- a/src/lib/notifications/senders/apns.ts +++ b/src/lib/notifications/senders/apns.ts @@ -31,6 +31,7 @@ import { getEvent } from "@/lib/logging/context"; import type { SendOutcome } from "@/lib/notifications/retry-policy"; import { recordPushAttemptForPayload } from "@/lib/notifications/senders/push-attempt-record"; import { plainPushText } from "@/lib/notifications/strip-emoji"; +import { stripHtml } from "@/lib/notifications/strip-html"; import { isManagedGuardianDelivery } from "@/lib/notifications/managed-delivery"; export interface ApnsPayload { @@ -998,10 +999,6 @@ export async function sendApnsRawPush( }; } -function stripHtml(s: string): string { - return s.replace(/<[^>]*>/g, ""); -} - /** * Whitelist the metadata keys we send through to iOS. The dispatcher * payload's `metadata` field is shared across every channel — Telegram diff --git a/src/lib/notifications/senders/email.ts b/src/lib/notifications/senders/email.ts index 813a1660a..86afb748e 100644 --- a/src/lib/notifications/senders/email.ts +++ b/src/lib/notifications/senders/email.ts @@ -10,6 +10,7 @@ import { getEvent } from "@/lib/logging/context"; import { recordPushAttemptForPayload } from "@/lib/notifications/senders/push-attempt-record"; import { loadEmailConfig } from "@/lib/notifications/senders/email-config"; import { plainPushText } from "@/lib/notifications/strip-emoji"; +import { stripHtml } from "@/lib/notifications/strip-html"; /** * SMTP / email sender (v1.17.1). @@ -126,10 +127,7 @@ export async function sendViaEmail( // Plain text only — strip HTML + decorative emoji on routine reminders. const subject = plainPushText(payload.title, payload.eventType); - const text = plainPushText( - payload.message.replace(/<[^>]*>/g, ""), - payload.eventType, - ); + const text = plainPushText(stripHtml(payload.message), payload.eventType); try { await transport.transporter.sendMail({ diff --git a/src/lib/notifications/senders/ntfy.ts b/src/lib/notifications/senders/ntfy.ts index ade424ac6..ea966adac 100644 --- a/src/lib/notifications/senders/ntfy.ts +++ b/src/lib/notifications/senders/ntfy.ts @@ -8,6 +8,7 @@ import { getEvent } from "@/lib/logging/context"; import { recordPushAttemptForPayload } from "@/lib/notifications/senders/push-attempt-record"; import { safeFetch } from "@/lib/safe-fetch"; import { plainPushText } from "@/lib/notifications/strip-emoji"; +import { stripHtml } from "@/lib/notifications/strip-html"; import { isUrgentPayload } from "@/lib/notifications/types"; /** @@ -57,10 +58,7 @@ export async function sendViaNtfy( } // Strip HTML tags for ntfy (plain text only) + emoji on routine reminders - const body = plainPushText( - payload.message.replace(/<[^>]*>/g, ""), - payload.eventType, - ); + const body = plainPushText(stripHtml(payload.message), payload.eventType); // requirePublicHost adds the DNS-rebinding pin (issue #217) on top // of the input-time isPublicUrl guard already enforced when the diff --git a/src/lib/notifications/senders/web-push.ts b/src/lib/notifications/senders/web-push.ts index 753f793a4..cf26df4f3 100644 --- a/src/lib/notifications/senders/web-push.ts +++ b/src/lib/notifications/senders/web-push.ts @@ -9,6 +9,7 @@ import { recordPushAttemptForPayload } from "@/lib/notifications/senders/push-at import { isPublicUrl } from "@/lib/validations/notifications"; import { safeFetch } from "@/lib/safe-fetch"; import { plainPushText } from "@/lib/notifications/strip-emoji"; +import { stripHtml } from "@/lib/notifications/strip-html"; import { isManagedGuardianDelivery } from "@/lib/notifications/managed-delivery"; /** @@ -102,10 +103,7 @@ export async function sendViaWebPush( const pushPayload = JSON.stringify({ title: plainPushText(payload.title, payload.eventType), - body: plainPushText( - payload.message.replace(/<[^>]*>/g, ""), - payload.eventType, - ), + body: plainPushText(stripHtml(payload.message), payload.eventType), // Discreet mode (cycle privacy): the coalescing `tag` must not name the // event. Web-Push payloads are VAPID-encrypted (only the SW sees this), // but the discreet contract still requires no event name on the wire — diff --git a/src/lib/notifications/senders/webhook.ts b/src/lib/notifications/senders/webhook.ts index fd01eeac8..34de62f72 100644 --- a/src/lib/notifications/senders/webhook.ts +++ b/src/lib/notifications/senders/webhook.ts @@ -8,6 +8,7 @@ import { getEvent } from "@/lib/logging/context"; import { recordPushAttemptForPayload } from "@/lib/notifications/senders/push-attempt-record"; import { safeFetch } from "@/lib/safe-fetch"; import { plainPushText } from "@/lib/notifications/strip-emoji"; +import { stripHtml } from "@/lib/notifications/strip-html"; /** * Send a notification via a generic outbound webhook (v1.17.1). @@ -49,10 +50,7 @@ export async function sendViaWebhook( // a relay rule can route without leaking the cycle event name. const body = JSON.stringify({ title: plainPushText(payload.title, payload.eventType), - message: plainPushText( - payload.message.replace(/<[^>]*>/g, ""), - payload.eventType, - ), + message: plainPushText(stripHtml(payload.message), payload.eventType), eventType: payload.discreet ? "reminder" : payload.eventType, // v1.18.4 — an explicitly urgent event maps to `urgent` so a relay // rule (Gotify priority, Discord mention, Home Assistant automation) diff --git a/src/lib/notifications/strip-html.ts b/src/lib/notifications/strip-html.ts new file mode 100644 index 000000000..2c1645945 --- /dev/null +++ b/src/lib/notifications/strip-html.ts @@ -0,0 +1,37 @@ +/** + * Strip HTML tags from notification content, hardened against + * multi-character bypasses. + * + * Every plain-text sender (APNs, Web Push, webhook, ntfy, email) used to + * carry its own single-pass `replace(/<[^>]*>/g, "")` — the shape CodeQL + * flags as incomplete multi-character sanitization, because a removal can + * re-form the very pattern it removed (`<script>` loses `` and the + * remaining characters close ranks). Whether a given regex happens to be + * idempotent is an argument about that regex; five copies of the argument + * across five senders is how the next edit quietly breaks one of them. + * + * This helper makes the guarantee structural instead: re-apply the strip + * until the string stops changing, so a tag re-formed by an earlier + * removal is removed on the next pass no matter how deep the nesting. The + * iteration cap bounds CPU on adversarial input; a string still changing + * at the cap is pathological by construction, and for that remnant the + * angle brackets themselves are dropped so no `<...>` sequence can ever + * survive. Plain input without tags returns unchanged, same as the old + * single pass. + */ + +const TAG_RE = /<[^>]*>/g; +const MAX_STRIP_PASSES = 10; + +/** Remove HTML tags, re-applying until stable so stripped tags cannot re-form. */ +export function stripHtml(text: string): string { + let out = text; + for (let i = 0; i < MAX_STRIP_PASSES; i++) { + const next = out.replace(TAG_RE, ""); + if (next === out) return next; + out = next; + } + // Still changing after the cap — nesting this deep is adversarial. + // Drop the brackets outright so nothing tag-shaped remains. + return out.replace(/[<>]/g, ""); +}