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/docs/self-hosting/notifications.md b/docs/self-hosting/notifications.md index 49f198adf..c631f5138 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/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/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/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")); } 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); } 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/__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/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 } : {}), 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 }); } 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. */ 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": {