From 6a663dfc27f2de3aa76c376e06df09ff8c7c1fb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 13 Sep 2026 14:50:32 +0200 Subject: [PATCH] fix(notifications): record a relay refusal at warn, not error A 502 from a channel test reports the user's relay or mail setup, not a fault on this server, yet the status alone raised every press to an error-level event. The event builder gains a ceiling for the status-derived level, and the test routes cap it at warn on the 502 path. The 500 for an unnamed failure stays at error, and explicit elevations are unaffected. The notifications guide notes that a reverse proxy which replaces 5xx pages hides the detail, so the card falls back to Test failed. Refs #947 --- docs/self-hosting/notifications.md | 7 +++++ .../__tests__/private-origin-routes.test.ts | 12 +++++++- .../logging/__tests__/event-builder.test.ts | 30 +++++++++++++++++++ src/lib/logging/event-builder.ts | 25 ++++++++++++++-- .../notifications/test-delivery-failure.ts | 9 ++++-- 5 files changed, 78 insertions(+), 5 deletions(-) diff --git a/docs/self-hosting/notifications.md b/docs/self-hosting/notifications.md index e0416e86c..65b7e1fa1 100644 --- a/docs/self-hosting/notifications.md +++ b/docs/self-hosting/notifications.md @@ -274,6 +274,13 @@ short, Gotify's own error text. A `401` means the token is wrong. A `400` means Gotify could not read the body; check that the format is set to Gotify. A Gotify on your own network also needs the operator grant below. +The detail reaches the card inside the server's `502` response. A reverse +proxy that replaces error pages for 5xx responses, such as Traefik's +`errors` middleware or nginx with `proxy_intercept_errors on`, swaps that +response for its own page, and the card can then only say "Test failed". +Exclude the `/api/` paths from the error-page rule to keep the detail. This +applies to the ntfy and email tests too. + ## Notification targets on a private network Every webhook and ntfy target is resolved at send time and refused when diff --git a/src/app/api/settings/__tests__/private-origin-routes.test.ts b/src/app/api/settings/__tests__/private-origin-routes.test.ts index 7825c97b3..edcc96bd4 100644 --- a/src/app/api/settings/__tests__/private-origin-routes.test.ts +++ b/src/app/api/settings/__tests__/private-origin-routes.test.ts @@ -52,7 +52,10 @@ vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() })); vi.mock("@/lib/auth/audit", () => ({ auditLog: vi.fn().mockResolvedValue(undefined), })); -vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); +const emitIfSampledMock = vi.fn(); +vi.mock("@/lib/logging/transports", () => ({ + emitIfSampled: (...args: unknown[]) => emitIfSampledMock(...args), +})); vi.mock("@/lib/db-compat", () => ({ ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), })); @@ -374,6 +377,10 @@ describe("POST /api/settings/{webhook,ntfy}/test — the refusal names itself", upstreamBody: '{"error":"Bad Request","errorCode":400}', }); expect(json.error).toContain("HTTP 400"); + // A user's relay refusing is not an operator error: the event is warn. + expect(emitIfSampledMock).toHaveBeenCalledWith( + expect.objectContaining({ level: "warn" }), + ); }, ); @@ -444,6 +451,9 @@ describe("POST /api/settings/{webhook,ntfy}/test — the refusal names itself", expect(response.status).toBe(500); const json = await response.json(); expect(json.meta).toBeUndefined(); + expect(emitIfSampledMock).toHaveBeenCalledWith( + expect.objectContaining({ level: "error" }), + ); }, ); diff --git a/src/lib/logging/__tests__/event-builder.test.ts b/src/lib/logging/__tests__/event-builder.test.ts index ae12c5227..2ff65ce65 100644 --- a/src/lib/logging/__tests__/event-builder.test.ts +++ b/src/lib/logging/__tests__/event-builder.test.ts @@ -110,3 +110,33 @@ describe("WideEventBuilder.setHttp", () => { expect(evt.toJSON().http).toBeUndefined(); }); }); + +describe("WideEventBuilder.capStatusLevel", () => { + it("derives error from a 5xx by default", () => { + const evt = new WideEventBuilder("http"); + evt.finish(502); + expect(evt.toJSON().level).toBe("error"); + }); + + it("keeps a capped 5xx at warn", () => { + const evt = new WideEventBuilder("http"); + evt.capStatusLevel("warn"); + evt.finish(502); + expect(evt.toJSON().level).toBe("warn"); + }); + + it("does not lower an explicit elevation", () => { + const evt = new WideEventBuilder("http"); + evt.capStatusLevel("warn"); + evt.elevateLevel("error"); + evt.finish(502); + expect(evt.toJSON().level).toBe("error"); + }); + + it("leaves a 4xx at warn under the cap", () => { + const evt = new WideEventBuilder("http"); + evt.capStatusLevel("warn"); + evt.finish(422); + expect(evt.toJSON().level).toBe("warn"); + }); +}); diff --git a/src/lib/logging/event-builder.ts b/src/lib/logging/event-builder.ts index 439d65b5b..9a89a6dd4 100644 --- a/src/lib/logging/event-builder.ts +++ b/src/lib/logging/event-builder.ts @@ -43,6 +43,7 @@ export class WideEventBuilder { private event: Partial; private startTime: number; private highestLevel: LogLevel = "info"; + private statusLevelCeiling: LogLevel = "error"; constructor(kind: EventKind = "http") { this.startTime = performance.now(); @@ -239,6 +240,18 @@ export class WideEventBuilder { return this; } + /** + * Ceiling for the level `finish()` derives from the HTTP status. A route + * whose 5xx reports someone else's refusal (a user's relay answering a + * test with 400) caps it at `warn`, so a user's configuration does not + * raise an operator-facing error. `elevateLevel` calls and thrown errors + * are unaffected. + */ + capStatusLevel(level: LogLevel): this { + this.statusLevelCeiling = level; + return this; + } + /** Request/Operation beenden, Duration berechnen */ finish(httpStatus?: number): this { this.event.duration_ms = Math.round(performance.now() - this.startTime); @@ -256,8 +269,16 @@ export class WideEventBuilder { this.event.http.status = httpStatus; } if (httpStatus !== undefined) { - if (httpStatus >= 500) this.elevateLevel("error"); - else if (httpStatus >= 400) this.elevateLevel("warn"); + const derived: LogLevel | null = + httpStatus >= 500 ? "error" : httpStatus >= 400 ? "warn" : null; + if (derived) { + this.elevateLevel( + LOG_LEVEL_PRIORITY[derived] > + LOG_LEVEL_PRIORITY[this.statusLevelCeiling] + ? this.statusLevelCeiling + : derived, + ); + } } this.event.level = this.highestLevel; return this; diff --git a/src/lib/notifications/test-delivery-failure.ts b/src/lib/notifications/test-delivery-failure.ts index 1728f1678..4612f1d72 100644 --- a/src/lib/notifications/test-delivery-failure.ts +++ b/src/lib/notifications/test-delivery-failure.ts @@ -1,5 +1,5 @@ import { apiError } from "@/lib/api-response"; -import { annotate } from "@/lib/logging/context"; +import { annotate, getEvent } from "@/lib/logging/context"; import type { SendOutcome } from "@/lib/notifications/retry-policy"; /** @@ -78,7 +78,8 @@ export function testFailureSentence( * The other end, or the way to it, failed: 502 with `meta.errorCode`, the * upstream status or SMTP code, and what the relay said when it is short * and holds no secret. A failure the sender could not name is an internal - * fault and stays a 500 with `fallbackMessage`. + * fault and stays a 500 with `fallbackMessage`. The 502 is recorded at + * `warn`, the 500 at `error`. */ export function answerTestDeliveryFailure( result: SendOutcome, @@ -105,5 +106,9 @@ export function answerTestDeliveryFailure( if (!detail.errorCode) { return apiError(opts.fallbackMessage, 500); } + // 502 is the honest status: the other end failed, not this request. But + // the cause is the user's relay or mail setup, so the wide event stays at + // warn rather than raising an operator-facing error on every test press. + getEvent()?.capStatusLevel("warn"); return apiError(testFailureSentence(opts.label, detail), 502, { ...detail }); }