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 1/6] 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 }); } From a11e5991cfcdc471e2a50d64aff51bd0cc0282de 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 2/6] test(settings): read a real failure response through the test button The fetch and envelope reading move into runConnectionTest, which the button calls. A test stubs the fetch with the 502 envelopes the notification test routes send and asserts the rendered code, HTTP status, SMTP code and relay reply, so dropping the meta extraction fails it. Refs #947 --- .../test-connection-button-response.test.tsx | 122 ++++++++++++++++++ .../settings/test-connection-button.tsx | 82 ++++++------ 2 files changed, 166 insertions(+), 38 deletions(-) create mode 100644 src/components/settings/__tests__/test-connection-button-response.test.tsx diff --git a/src/components/settings/__tests__/test-connection-button-response.test.tsx b/src/components/settings/__tests__/test-connection-button-response.test.tsx new file mode 100644 index 000000000..97ab62800 --- /dev/null +++ b/src/components/settings/__tests__/test-connection-button-response.test.tsx @@ -0,0 +1,122 @@ +/** + * How the test-connection button reads a real failure response (#947). + * + * The component suite is SSR-only, so the button's probe is exercised as the + * function the button calls: `apiFetchRaw` is stubbed with the envelope a + * notification test route sends, and the result is rendered through the same + * callout the button renders. Removing the meta extraction turns this red. + */ +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { renderToStaticMarkup } from "react-dom/server"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const apiFetchRawMock = vi.fn(); +vi.mock("@/lib/api/api-fetch", () => ({ + apiFetchRaw: (...args: unknown[]) => apiFetchRawMock(...args), +})); + +import { + runConnectionTest, + TestConnectionFailure, +} from "../test-connection-button"; + +function envelope(status: number, body: unknown): Response { + return new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json" }, + }); +} + +function renderFailure( + result: Awaited>, +): string { + if (result.kind !== "error") throw new Error("expected a failure"); + return renderToStaticMarkup( + , + ); +} + +beforeEach(() => { + apiFetchRawMock.mockReset(); +}); + +describe("runConnectionTest", () => { + it("reads the code, the HTTP status and what the relay said from a 502", async () => { + apiFetchRawMock.mockResolvedValue( + envelope(502, { + data: null, + error: "The webhook answered HTTP 400.", + meta: { + errorCode: "upstream_rejected", + upstreamStatus: 400, + upstreamBody: "priority must be an integer", + }, + }), + ); + + const result = await runConnectionTest("/api/settings/webhook/test"); + + expect(apiFetchRawMock).toHaveBeenCalledWith("/api/settings/webhook/test", { + method: "POST", + }); + const html = renderFailure(result); + expect(html).toContain("code:upstream_rejected (HTTP 400)"); + expect(html).toContain("priority must be an integer"); + }); + + it("reads the SMTP reply code from an email failure", async () => { + apiFetchRawMock.mockResolvedValue( + envelope(502, { + data: null, + error: "The mail server rejected the credentials (SMTP 535).", + meta: { errorCode: "credentials_rejected", smtpCode: 535 }, + }), + ); + + const html = renderFailure( + await runConnectionTest("/api/settings/email/test"), + ); + + expect(html).toContain("code:credentials_rejected (SMTP 535)"); + expect(html).not.toContain("test-connection-upstream-body"); + }); + + it("falls back to the generic code when a proxy replaced the body", async () => { + apiFetchRawMock.mockResolvedValue( + new Response("Bad Gateway", { status: 502 }), + ); + + expect(await runConnectionTest("/api/settings/ntfy/test")).toEqual({ + kind: "error", + errorCode: "generic", + upstreamStatus: undefined, + smtpCode: undefined, + upstreamBody: undefined, + }); + }); + + it("reports success with the latency", async () => { + apiFetchRawMock.mockResolvedValue( + envelope(200, { data: { ok: true, latencyMs: 42 }, error: null }), + ); + + expect(await runConnectionTest("/api/nightscout/test")).toEqual({ + kind: "ok", + latency: 42, + }); + }); + + it("is what the button calls", () => { + const src = readFileSync( + join(process.cwd(), "src/components/settings/test-connection-button.tsx"), + "utf8", + ); + expect(src).toMatch(/setResult\(await runConnectionTest\(endpoint\)\)/); + }); +}); diff --git a/src/components/settings/test-connection-button.tsx b/src/components/settings/test-connection-button.tsx index e741a6233..ed9f63b08 100644 --- a/src/components/settings/test-connection-button.tsx +++ b/src/components/settings/test-connection-button.tsx @@ -82,6 +82,48 @@ export function TestConnectionFailure({ ); } +export type ConnectionTestResult = + | { kind: "ok"; latency: number } + | { + kind: "error"; + errorCode: string; + upstreamStatus?: number; + smtpCode?: number; + upstreamBody?: string; + }; + +/** + * The button's probe: POST the endpoint and read the envelope into what the + * button shows. Exported so the reading of a real failure response can be + * tested without a DOM. + */ +export async function runConnectionTest( + endpoint: string, +): Promise { + try { + const res = await apiFetchRaw(endpoint, { method: "POST" }); + const json = (await res.json().catch(() => ({}))) as TestResponse; + + if (res.ok && json.data?.ok !== false) { + return { kind: "ok", latency: json.data?.latencyMs ?? 0 }; + } + const meta = json.meta; + return { + kind: "error", + errorCode: meta?.errorCode ?? "generic", + upstreamStatus: + typeof meta?.upstreamStatus === "number" + ? meta.upstreamStatus + : undefined, + smtpCode: typeof meta?.smtpCode === "number" ? meta.smtpCode : undefined, + upstreamBody: + typeof meta?.upstreamBody === "string" ? meta.upstreamBody : undefined, + }; + } catch { + return { kind: "error", errorCode: "connection_failed" }; + } +} + export function TestConnectionButton({ endpoint, disabled = false, @@ -89,49 +131,13 @@ export function TestConnectionButton({ }: TestConnectionButtonProps) { const { t } = useTranslations(); const [testing, setTesting] = useState(false); - const [result, setResult] = useState< - | { kind: "ok"; latency: number } - | { - kind: "error"; - errorCode: string; - upstreamStatus?: number; - smtpCode?: number; - upstreamBody?: string; - } - | null - >(null); + const [result, setResult] = useState(null); async function handleClick() { setTesting(true); setResult(null); try { - const res = await apiFetchRaw(endpoint, { method: "POST" }); - const json = (await res.json().catch(() => ({}))) as TestResponse; - - if (res.ok && json.data?.ok !== false) { - setResult({ - kind: "ok", - latency: json.data?.latencyMs ?? 0, - }); - } else { - const meta = json.meta; - setResult({ - kind: "error", - errorCode: meta?.errorCode ?? "generic", - upstreamStatus: - typeof meta?.upstreamStatus === "number" - ? meta.upstreamStatus - : undefined, - smtpCode: - typeof meta?.smtpCode === "number" ? meta.smtpCode : undefined, - upstreamBody: - typeof meta?.upstreamBody === "string" - ? meta.upstreamBody - : undefined, - }); - } - } catch { - setResult({ kind: "error", errorCode: "connection_failed" }); + setResult(await runConnectionTest(endpoint)); } finally { setTesting(false); } From f81cdf8768763a6bb33c6c5646614e4a4a9f1770 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 13 Sep 2026 14:50:33 +0200 Subject: [PATCH 3/6] fix(notifications): name TLS, protocol and envelope faults in email tests A STARTTLS failure (port 465 set up as 587) and a protocol error now read as connection_failed, and an envelope or message refused without a reply code as upstream_rejected. They used to fall through to the 500 for an unnamed failure. Refs #947 --- .../senders/__tests__/email.test.ts | 40 +++++++++++++++++++ src/lib/notifications/senders/email.ts | 11 ++++- 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/lib/notifications/senders/__tests__/email.test.ts b/src/lib/notifications/senders/__tests__/email.test.ts index 83f12cec3..b785b1130 100644 --- a/src/lib/notifications/senders/__tests__/email.test.ts +++ b/src/lib/notifications/senders/__tests__/email.test.ts @@ -237,3 +237,43 @@ describe("sendViaEmail — the failure names itself for the test button", () => expect(result.smtpCode).toBeUndefined(); }); }); + +describe("sendViaEmail — transport and envelope faults are named too", () => { + it.each([ + ["a TLS handshake failure", "ETLS", undefined, "connection_failed"], + ["a protocol error", "EPROTOCOL", undefined, "connection_failed"], + [ + "an envelope refused without a code", + "EENVELOPE", + undefined, + "upstream_rejected", + ], + [ + "a message refused without a code", + "EMESSAGE", + undefined, + "upstream_rejected", + ], + [ + "an envelope refused with a temporary code", + "EENVELOPE", + 451, + "upstream_error", + ], + ])("%s", async (_label, code, responseCode, failureCode) => { + loadEmailConfigMock.mockReturnValue(transport); + sendMailMock.mockRejectedValue( + Object.assign(new Error(String(code)), { + code, + ...(responseCode !== undefined ? { responseCode } : {}), + }), + ); + + const result = await sendViaEmail( + { recipient: "you@example.com" }, + payload(), + ); + + expect(result.failureCode).toBe(failureCode); + }); +}); diff --git a/src/lib/notifications/senders/email.ts b/src/lib/notifications/senders/email.ts index 2b518638d..22011e45e 100644 --- a/src/lib/notifications/senders/email.ts +++ b/src/lib/notifications/senders/email.ts @@ -106,13 +106,20 @@ function classifySmtpError(err: unknown): { : code === "ECONNECTION" || code === "ESOCKET" || code === "EDNS" || - code === "ECONNREFUSED" + code === "ECONNREFUSED" || + // STARTTLS or TLS handshake failure (typically port 465 set up + // as 587 or the reverse) and a server that does not speak SMTP. + code === "ETLS" || + code === "EPROTOCOL" ? "connection_failed" : smtpCode !== undefined && smtpCode >= 500 ? "upstream_rejected" : smtpCode !== undefined && smtpCode >= 400 ? "upstream_error" - : undefined; + : // An envelope or message refused without a reply code. + code === "EENVELOPE" || code === "EMESSAGE" + ? "upstream_rejected" + : undefined; const extras = { ...(failureCode ? { failureCode } : {}), ...(smtpCode !== undefined ? { smtpCode } : {}), From 2480c9fa5544f01037d9f48a77e76c600dad2003 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 13 Sep 2026 14:50:33 +0200 Subject: [PATCH 4/6] fix(notifications): refuse encoded and path-borne secrets in replies Userinfo and each query value are decoded on their own, so one malformed escape no longer discards every other secret in the URL. A reply is compared with each secret in literal, JSON-escaped and percent-encoded form, and URL path segments of 16 or more characters count as secrets, which covers the token in a Discord or Slack webhook URL and an n8n webhook id. Refs #947 --- .../__tests__/upstream-body.test.ts | 64 +++++++++++++++++- src/lib/notifications/upstream-body.ts | 67 ++++++++++++++++--- 2 files changed, 122 insertions(+), 9 deletions(-) diff --git a/src/lib/notifications/__tests__/upstream-body.test.ts b/src/lib/notifications/__tests__/upstream-body.test.ts index 021c6c1a0..c7ade0172 100644 --- a/src/lib/notifications/__tests__/upstream-body.test.ts +++ b/src/lib/notifications/__tests__/upstream-body.test.ts @@ -95,7 +95,7 @@ describe("secret extraction", () => { secretsInUrl( "https://user:pa%20ss@gotify.example.com/message?token=Abc123", ), - ).toEqual(["Abc123", "user", "pa ss"]); + ).toEqual(expect.arrayContaining(["Abc123", "user", "pa%20ss", "pa ss"])); expect(secretsInUrl("not a url")).toEqual([]); }); @@ -108,3 +108,65 @@ describe("secret extraction", () => { expect(secretsInHeaderValue(undefined)).toEqual([]); }); }); + +describe("secret matching survives encoding and malformed parts (review L4)", () => { + it("keeps the query token when the userinfo holds a malformed escape", () => { + const found = secretsInUrl( + "https://us%ZZer:pw@gotify.example.com/message?token=Abc12345", + ); + expect(found).toContain("Abc12345"); + expect(found).toContain("us%ZZer"); + }); + + it("keeps the other query values when one holds a malformed escape", () => { + const found = secretsInUrl( + "https://gotify.example.com/message?a=%ZZ&token=Abc12345", + ); + expect(found).toContain("Abc12345"); + expect(found).toContain("%ZZ"); + }); + + it("treats long path segments as secrets and short ones not", () => { + const found = secretsInUrl( + "https://discord.com/api/webhooks/123456789012345678/AbCdEfGhIjKlMnOpQrSt", + ); + expect(found).toContain("123456789012345678"); + expect(found).toContain("AbCdEfGhIjKlMnOpQrSt"); + expect(found).not.toContain("webhooks"); + expect(found).not.toContain("api"); + }); + + it("refuses a body echoing a path token", async () => { + await expect( + readUpstreamBody( + res('{"message":"Unknown Webhook AbCdEfGhIjKlMnOpQrSt"}', 404), + secretsInUrl( + "https://discord.com/api/webhooks/123/AbCdEfGhIjKlMnOpQrSt", + ), + ), + ).resolves.toBeUndefined(); + }); + + it("refuses a JSON-escaped echo with an escaped slash", async () => { + await expect( + readUpstreamBody(res('{"auth":"Basic dXNlcj\\/wYXNz"}'), [ + "dXNlcj/wYXNz", + ]), + ).resolves.toBeUndefined(); + }); + + it("refuses a JSON-escaped echo with an escaped quote", async () => { + await expect( + readUpstreamBody(res('{"got":"ab\\"cd1234"}'), ['ab"cd1234']), + ).resolves.toBeUndefined(); + }); + + it("refuses a percent-encoded echo, in either hex case", async () => { + await expect( + readUpstreamBody(res("bad token p%40ss%20word!"), ["p@ss word!"]), + ).resolves.toBeUndefined(); + await expect( + readUpstreamBody(res("bad token a%2fb%2Bc1234"), ["a/b+c1234"]), + ).resolves.toBeUndefined(); + }); +}); diff --git a/src/lib/notifications/upstream-body.ts b/src/lib/notifications/upstream-body.ts index dbf57bad2..60646c681 100644 --- a/src/lib/notifications/upstream-body.ts +++ b/src/lib/notifications/upstream-body.ts @@ -52,6 +52,36 @@ async function readBounded(res: Response): Promise { return undefined; } +/** + * The ways a relay can echo a secret back: literally, JSON-escaped (with and + * without the `\/` form some serialisers write for a slash, common inside a + * base64 value), and percent-encoded. + */ +function echoForms(secret: string): string[] { + const json = JSON.stringify(secret).slice(1, -1); + return [ + ...new Set([ + secret, + json, + json.replaceAll("/", "\\/"), + encodeURIComponent(secret), + ]), + ]; +} + +/** A value and its decoded form; a malformed escape keeps the raw value. */ +function withDecoded(part: string): string[] { + try { + const decoded = decodeURIComponent(part.replaceAll("+", " ")); + return decoded === part ? [part] : [part, decoded]; + } catch { + return [part]; + } +} + +/** Path segments this long are credentials in Discord, Slack and n8n URLs. */ +const MIN_SECRET_PATH_SEGMENT_LENGTH = 16; + /** * Read what a relay answered on a non-2xx, in a form safe to hand back to * the person who pressed the test button. @@ -87,9 +117,16 @@ export async function readUpstreamBody( if (!cleaned) return undefined; if (looksSecretShaped(raw) || looksSecretShaped(cleaned)) return undefined; + const rawLower = raw.toLowerCase(); for (const secret of knownSecrets) { if (!secret || secret.length < MIN_KNOWN_SECRET_LENGTH) continue; - if (raw.includes(secret) || cleaned.includes(secret)) return undefined; + for (const form of echoForms(secret)) { + if (raw.includes(form) || cleaned.includes(form)) return undefined; + } + // Percent-encoding is case-insensitive in its hex digits. + if (rawLower.includes(encodeURIComponent(secret).toLowerCase())) { + return undefined; + } } const characters = Array.from(cleaned); @@ -98,19 +135,33 @@ export async function readUpstreamBody( } /** - * The secrets a URL can carry on its own: userinfo and every query value - * (Gotify's `?token=` form). + * The secrets a URL can carry on its own: userinfo, every query value + * (Gotify's `?token=` form) and every path segment of 16 or more characters + * (a Discord or Slack webhook token, an n8n webhook id). Each part is decoded + * on its own, so one malformed escape cannot drop the others. */ export function secretsInUrl(url: string): string[] { + let parsed: URL; try { - const parsed = new URL(url); - const found = [...parsed.searchParams.values()]; - if (parsed.username) found.push(decodeURIComponent(parsed.username)); - if (parsed.password) found.push(decodeURIComponent(parsed.password)); - return found; + parsed = new URL(url); } catch { return []; } + const found: string[] = []; + if (parsed.username) found.push(...withDecoded(parsed.username)); + if (parsed.password) found.push(...withDecoded(parsed.password)); + for (const pair of parsed.search.replace(/^\?/, "").split("&")) { + const eq = pair.indexOf("="); + if (eq < 0) continue; + const value = pair.slice(eq + 1); + if (value) found.push(...withDecoded(value)); + } + for (const segment of parsed.pathname.split("/")) { + if (segment.length >= MIN_SECRET_PATH_SEGMENT_LENGTH) { + found.push(...withDecoded(segment)); + } + } + return [...new Set(found)]; } /** A header value and, for the ` ` form, the token alone. */ From 32eaf7a29e41999bd58d3148e4360cbd1c479a3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 13 Sep 2026 14:50:33 +0200 Subject: [PATCH 5/6] fix(admin): show each channel's cause in the notification test toast The admin test response carried a code and status per channel that nothing rendered. The toast now lists each failing channel with the translated cause and the HTTP status or SMTP code. The relay reply is no longer part of that response, and the ntfy error no longer echoes the topic, which the sender treats as a secret. Refs #947 --- .../test/__tests__/route.test.ts | 4 +- src/app/api/admin/notifications/test/route.ts | 32 ++++---- .../notification-test-failures.test.ts | 76 +++++++++++++++++++ .../admin/notification-test-failures.ts | 46 +++++++++++ src/components/admin/reminders-section.tsx | 24 ++++-- 5 files changed, 157 insertions(+), 25 deletions(-) create mode 100644 src/components/admin/__tests__/notification-test-failures.test.ts create mode 100644 src/components/admin/notification-test-failures.ts diff --git a/src/app/api/admin/notifications/test/__tests__/route.test.ts b/src/app/api/admin/notifications/test/__tests__/route.test.ts index 259a35461..796464b39 100644 --- a/src/app/api/admin/notifications/test/__tests__/route.test.ts +++ b/src/app/api/admin/notifications/test/__tests__/route.test.ts @@ -347,6 +347,7 @@ describe("POST /api/admin/notifications/test — a failure keeps its cause (#947 upstreamStatus: 400, }); expect(body.data.results[0].error).toContain("HTTP 400"); + expect(body.data.results[0].error).not.toContain("hl-test"); }); it("tests the webhook channel rather than calling it unknown", async () => { @@ -372,8 +373,9 @@ describe("POST /api/admin/notifications/test — a failure keeps its cause (#947 success: false, errorCode: "upstream_rejected", upstreamStatus: 400, - upstreamBody: "priority must be an integer", }); + // The relay body belongs to the settings card, not the admin toast. + expect(body.data.results[0].upstreamBody).toBeUndefined(); }); it("tests the email channel and forwards the SMTP code", async () => { diff --git a/src/app/api/admin/notifications/test/route.ts b/src/app/api/admin/notifications/test/route.ts index 0bf282198..4c71a805e 100644 --- a/src/app/api/admin/notifications/test/route.ts +++ b/src/app/api/admin/notifications/test/route.ts @@ -72,11 +72,12 @@ async function ensureTelegramChannel(userId: string): Promise { return true; } +/** The admin toast renders `errorCode` and the status; the relay body stays on the settings card. */ type TestResult = { channel: string; success: boolean; error?: string; -} & TestFailureDetail; +} & Omit; /** * A failed HTTP or SMTP send, with the code, status and SMTP reply code the @@ -87,21 +88,26 @@ function failedSend( channel: string, label: string, result: SendOutcome, - context?: string, ): TestResult { - const detail: TestFailureDetail = result.errorCode - ? { errorCode: result.errorCode } + const { errorCode, upstreamStatus, smtpCode } = result.errorCode + ? { + errorCode: result.errorCode, + upstreamStatus: undefined, + smtpCode: undefined, + } : testFailureDetail(result); const cause = result.errorCode ? `refused by the private-origin policy (${result.errorCode})` - : detail.errorCode - ? testFailureSentence(label, detail) + : errorCode + ? testFailureSentence(label, { errorCode, upstreamStatus, smtpCode }) : (result.reason ?? "send failed"); return { channel, success: false, - error: context ? `${cause} (${context})` : cause, - ...detail, + error: cause, + ...(errorCode ? { errorCode } : {}), + ...(upstreamStatus !== undefined ? { upstreamStatus } : {}), + ...(smtpCode !== undefined ? { smtpCode } : {}), }; } @@ -205,14 +211,8 @@ export const POST = apiHandler(async () => { const ntfyResult = await sendViaNtfy(config, payload); success = ntfyResult.ok; if (!success) { - results.push( - failedSend( - "NTFY", - "The ntfy server", - ntfyResult, - `topic: ${config.topic}`, - ), - ); + // The topic is not echoed: on a public server it is the secret. + results.push(failedSend("NTFY", "The ntfy server", ntfyResult)); continue; } break; diff --git a/src/components/admin/__tests__/notification-test-failures.test.ts b/src/components/admin/__tests__/notification-test-failures.test.ts new file mode 100644 index 000000000..b2fd9f403 --- /dev/null +++ b/src/components/admin/__tests__/notification-test-failures.test.ts @@ -0,0 +1,76 @@ +/** + * The admin notification test toast names each failing channel's cause + * (review L5): the code the route sends is translated and the status is + * shown, so the per-channel fields in the response have a reader. + */ +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; + +import { describeChannelTestFailures } from "../notification-test-failures"; + +const MESSAGES: Record = { + "settings.testConnection.errors.upstream_rejected": + "The other end refused the message", + "settings.testConnection.errors.credentials_rejected": "Credentials rejected", + "admin.notificationTestFailed": "Failed to send test notification", +}; +const t = (key: string) => MESSAGES[key] ?? key; + +describe("describeChannelTestFailures", () => { + it("lists only failing channels, with the translated cause and the status", () => { + expect( + describeChannelTestFailures( + [ + { channel: "APNS", success: true }, + { + channel: "WEBHOOK", + success: false, + error: "The webhook answered HTTP 400.", + errorCode: "upstream_rejected", + upstreamStatus: 400, + }, + { + channel: "EMAIL", + success: false, + error: "The mail server rejected the credentials (SMTP 535).", + errorCode: "credentials_rejected", + smtpCode: 535, + }, + ], + t, + ), + ).toEqual([ + "Webhook: The other end refused the message (HTTP 400)", + "Email: Credentials rejected (SMTP 535)", + ]); + }); + + it("falls back to the route's sentence when there is no known code", () => { + expect( + describeChannelTestFailures( + [ + { + channel: "NTFY", + success: false, + error: "SYSTEM_ALERT disabled in settings", + }, + { channel: "MYSTERY", success: false, errorCode: "unlisted" }, + ], + t, + ), + ).toEqual([ + "ntfy: SYSTEM_ALERT disabled in settings", + "MYSTERY: Failed to send test notification", + ]); + }); + + it("is what the admin toast renders", () => { + const src = readFileSync( + join(process.cwd(), "src/components/admin/reminders-section.tsx"), + "utf8", + ); + expect(src).toMatch(/describeChannelTestFailures\(data\?\.results/); + expect(src).toMatch(/description:/); + }); +}); diff --git a/src/components/admin/notification-test-failures.ts b/src/components/admin/notification-test-failures.ts new file mode 100644 index 000000000..caf578b50 --- /dev/null +++ b/src/components/admin/notification-test-failures.ts @@ -0,0 +1,46 @@ +import { + CHANNEL_TYPE_LABELS, + type ChannelType, +} from "@/lib/notifications/types"; + +/** One channel's row in the admin notification test response. */ +export interface ChannelTestResult { + channel: string; + success: boolean; + error?: string; + errorCode?: string; + upstreamStatus?: number; + smtpCode?: number; +} + +/** + * One line per failing channel for the admin test toast: the channel name, + * the cause in the admin's language (the same `settings.testConnection.errors` + * vocabulary the settings cards use), and the HTTP status or SMTP reply code. + * A row without a known code falls back to the route's own sentence. + */ +export function describeChannelTestFailures( + results: ReadonlyArray, + t: (key: string) => string, +): string[] { + return results + .filter((r) => !r.success) + .map((r) => { + const label = CHANNEL_TYPE_LABELS[r.channel as ChannelType] ?? r.channel; + const key = r.errorCode + ? `settings.testConnection.errors.${r.errorCode}` + : null; + const translated = key ? t(key) : null; + const cause = + translated && translated !== key + ? translated + : (r.error ?? t("admin.notificationTestFailed")); + const code = + typeof r.upstreamStatus === "number" + ? ` (HTTP ${r.upstreamStatus})` + : typeof r.smtpCode === "number" + ? ` (SMTP ${r.smtpCode})` + : ""; + return `${label}: ${cause}${code}`; + }); +} diff --git a/src/components/admin/reminders-section.tsx b/src/components/admin/reminders-section.tsx index ff4f28818..2a6d892d3 100644 --- a/src/components/admin/reminders-section.tsx +++ b/src/components/admin/reminders-section.tsx @@ -22,6 +22,10 @@ import { useTranslations } from "@/lib/i18n/context"; import { queryKeys } from "@/lib/query-keys"; import { useAdminSettings, useUpdateSettings } from "./_shared"; import { apiGet, apiPost } from "@/lib/api/api-fetch"; +import { + describeChannelTestFailures, + type ChannelTestResult, +} from "./notification-test-failures"; interface NotificationHealth { windowHours: number; @@ -132,19 +136,23 @@ export function RemindersSection() { return apiPost< | { message?: string; - results?: Array<{ - channel: string; - success: boolean; - error?: string; - }>; + results?: ChannelTestResult[]; } | undefined >("/api/admin/notifications/test"); }, onSuccess: (data) => { - const hasFailures = data?.results?.some((r) => !r.success); - if (hasFailures) { - toast.error(data?.message ?? t("admin.notificationTestFailed")); + const failures = describeChannelTestFailures(data?.results ?? [], t); + if (failures.length > 0) { + toast.error(data?.message ?? t("admin.notificationTestFailed"), { + description: ( +
    + {failures.map((line) => ( +
  • {line}
    • + ))} +
+ ), + }); } else { toast.success(data?.message ?? t("admin.notificationTestSuccess")); } From 60dfffc7adeb057eb57f5357f45ce0ae63ea3970 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 13 Sep 2026 14:50:33 +0200 Subject: [PATCH 6/6] docs(api): name each channel's own credentials in test errors The ntfy and webhook 502 descriptions shared one list of refused credentials that neither channel fully has. Each now names its own, including the userinfo and long path segments of a webhook URL. The email description names the TLS, protocol and envelope cases. Refs #947 --- docs/api/openapi.yaml | 13 ++++++++----- src/lib/openapi/routes/settings.ts | 6 +++--- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 71260c858..891e9ce15 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -12375,7 +12375,8 @@ paths: "502": description: "The mail server, or the way to it, failed. `meta.errorCode` names the cause: `credentials_rejected` (the server refused the SMTP login), `upstream_rejected` (a permanent 5xx rejection, e.g. an unknown mailbox or - relay denied), `upstream_error` (a temporary 4xx rejection), `timeout`, or `connection_failed`. + relay denied, or an envelope or message refused without a reply code), `upstream_error` (a temporary 4xx + rejection), `timeout`, or `connection_failed` (including a TLS handshake or protocol failure). `meta.smtpCode` carries the SMTP reply code when the mail server sent one." content: application/json: @@ -12421,8 +12422,9 @@ paths: (404/410), `rate_limited` (429), `upstream_error` (5xx), `redirected` (3xx, never followed), `timeout`, or `connection_failed`. `meta.upstreamStatus` carries the HTTP status whenever one arrived, and `meta.upstreamBody` up to 200 characters of what the other end answered, with control characters removed; it - is omitted when the body was empty or could carry a credential (a token-shaped string, or the saved header - value, auth token, topic or URL token)." + is omitted when the body was empty or could carry a credential: a token-shaped string, or the saved auth + token, the topic, or a credential in the server URL (userinfo, a query value, a path segment of 16 or more + characters), literal, JSON-escaped or percent-encoded." content: application/json: schema: @@ -12491,8 +12493,9 @@ paths: (404/410), `rate_limited` (429), `upstream_error` (5xx), `redirected` (3xx, never followed), `timeout`, or `connection_failed`. `meta.upstreamStatus` carries the HTTP status whenever one arrived, and `meta.upstreamBody` up to 200 characters of what the other end answered, with control characters removed; it - is omitted when the body was empty or could carry a credential (a token-shaped string, or the saved header - value, auth token, topic or URL token)." + is omitted when the body was empty or could carry a credential: a token-shaped string, or the saved header + value (and its token part), the URL's userinfo, a query value, or a path segment of 16 or more characters + (the token in a Discord or Slack webhook URL), literal, JSON-escaped or percent-encoded." content: application/json: schema: diff --git a/src/lib/openapi/routes/settings.ts b/src/lib/openapi/routes/settings.ts index 13315f1b7..a837dbdba 100644 --- a/src/lib/openapi/routes/settings.ts +++ b/src/lib/openapi/routes/settings.ts @@ -705,7 +705,7 @@ export const settingsPaths: NonNullable = { ...stdResponses, "502": { description: - "The mail server, or the way to it, failed. `meta.errorCode` names the cause: `credentials_rejected` (the server refused the SMTP login), `upstream_rejected` (a permanent 5xx rejection, e.g. an unknown mailbox or relay denied), `upstream_error` (a temporary 4xx rejection), `timeout`, or `connection_failed`. `meta.smtpCode` carries the SMTP reply code when the mail server sent one.", + "The mail server, or the way to it, failed. `meta.errorCode` names the cause: `credentials_rejected` (the server refused the SMTP login), `upstream_rejected` (a permanent 5xx rejection, e.g. an unknown mailbox or relay denied, or an envelope or message refused without a reply code), `upstream_error` (a temporary 4xx rejection), `timeout`, or `connection_failed` (including a TLS handshake or protocol failure). `meta.smtpCode` carries the SMTP reply code when the mail server sent one.", content: { "application/json": { schema: errorEnvelope } }, }, "500": { @@ -735,7 +735,7 @@ export const settingsPaths: NonNullable = { ...stdResponses, "502": { description: - "The ntfy server, or the way to it, failed. `meta.errorCode` names the cause: `upstream_rejected` (the other end refused the payload, e.g. 400/409/415/422), `credentials_rejected` (401/403), `endpoint_not_found` (404/410), `rate_limited` (429), `upstream_error` (5xx), `redirected` (3xx, never followed), `timeout`, or `connection_failed`. `meta.upstreamStatus` carries the HTTP status whenever one arrived, and `meta.upstreamBody` up to 200 characters of what the other end answered, with control characters removed; it is omitted when the body was empty or could carry a credential (a token-shaped string, or the saved header value, auth token, topic or URL token).", + "The ntfy server, or the way to it, failed. `meta.errorCode` names the cause: `upstream_rejected` (the other end refused the payload, e.g. 400/409/415/422), `credentials_rejected` (401/403), `endpoint_not_found` (404/410), `rate_limited` (429), `upstream_error` (5xx), `redirected` (3xx, never followed), `timeout`, or `connection_failed`. `meta.upstreamStatus` carries the HTTP status whenever one arrived, and `meta.upstreamBody` up to 200 characters of what the other end answered, with control characters removed; it is omitted when the body was empty or could carry a credential: a token-shaped string, or the saved auth token, the topic, or a credential in the server URL (userinfo, a query value, a path segment of 16 or more characters), literal, JSON-escaped or percent-encoded.", content: { "application/json": { schema: errorEnvelope } }, }, "500": { @@ -791,7 +791,7 @@ export const settingsPaths: NonNullable = { ...stdResponses, "502": { description: - "The relay, or the way to it, failed. `meta.errorCode` names the cause: `upstream_rejected` (the other end refused the payload, e.g. 400/409/415/422), `credentials_rejected` (401/403), `endpoint_not_found` (404/410), `rate_limited` (429), `upstream_error` (5xx), `redirected` (3xx, never followed), `timeout`, or `connection_failed`. `meta.upstreamStatus` carries the HTTP status whenever one arrived, and `meta.upstreamBody` up to 200 characters of what the other end answered, with control characters removed; it is omitted when the body was empty or could carry a credential (a token-shaped string, or the saved header value, auth token, topic or URL token).", + "The relay, or the way to it, failed. `meta.errorCode` names the cause: `upstream_rejected` (the other end refused the payload, e.g. 400/409/415/422), `credentials_rejected` (401/403), `endpoint_not_found` (404/410), `rate_limited` (429), `upstream_error` (5xx), `redirected` (3xx, never followed), `timeout`, or `connection_failed`. `meta.upstreamStatus` carries the HTTP status whenever one arrived, and `meta.upstreamBody` up to 200 characters of what the other end answered, with control characters removed; it is omitted when the body was empty or could carry a credential: a token-shaped string, or the saved header value (and its token part), the URL's userinfo, a query value, or a path segment of 16 or more characters (the token in a Discord or Slack webhook URL), literal, JSON-escaped or percent-encoded.", content: { "application/json": { schema: errorEnvelope } }, }, "500": {