diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index fb07ebd4e..90730c227 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -12354,7 +12354,8 @@ paths: Refuses with 400 when the instance has no SMTP transport, when the account has no email channel, or when the - channel carries no recipient — three distinguishable prose messages on one status. A transport failure is a 500. + channel carries no recipient — three distinguishable prose messages on one status. A failure at the mail server + or on the way to it is a 502 that names its cause; only a failure the server cannot name is a 500. responses: "200": description: The provider accepted the message. Not proof it was delivered. @@ -12365,6 +12366,21 @@ paths: "401": *a1 "422": *a4 "429": *a5 + "500": + description: The send failed in a way the server could not name. An internal fault, not a statement about the mail server. + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorEnvelope" + "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." + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorEnvelope" /api/settings/ntfy/test: post: tags: @@ -12372,9 +12388,10 @@ paths: summary: Send a test message to the saved ntfy topic description: Publishes one message to the account's configured ntfy server and topic. Takes no body. Rate-limited 5 per 5 minutes per user. Refuses with 400 when no ntfy channel is saved or when the saved config is missing a server - URL or topic; a publish failure is a 500. A server on a private network is refused with 422 and `meta.errorCode` - = `private_origin_not_approved` unless the operator listed its exact origin in `NOTIFICATION_PRIVATE_ORIGINS`; - the test and the scheduled delivery take the same decision. + URL or topic. A failure at the ntfy server or on the way to it is a 502 that carries the upstream status and, + when safe, what the server answered; only a failure the server cannot name is a 500. A server on a private + network is refused with 422 and `meta.errorCode` = `private_origin_not_approved` unless the operator listed its + exact origin in `NOTIFICATION_PRIVATE_ORIGINS`; the test and the scheduled delivery take the same decision. responses: "200": description: The provider accepted the message. Not proof it was delivered. @@ -12392,6 +12409,24 @@ paths: schema: $ref: "#/components/schemas/ErrorEnvelope" "429": *a5 + "500": + description: The send failed in a way the server could not name. An internal fault, not a statement about the ntfy server. + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorEnvelope" + "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)." + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorEnvelope" /api/settings/telegram/test: post: tags: @@ -12421,11 +12456,12 @@ paths: - Notifications summary: Send a test payload to the saved webhook description: Posts one payload to the account's configured webhook URL. Takes no body. Rate-limited 5 per 5 minutes per - user. Refuses with 400 when no webhook channel is saved or the saved config has no URL; a delivery failure is a - 500. The URL was checked against the SSRF floor when it was saved, so a test cannot be used to reach an internal - host. A URL on a private network is refused with 422 and `meta.errorCode` = `private_origin_not_approved` unless - the operator listed its exact origin in `NOTIFICATION_PRIVATE_ORIGINS`; the test and the scheduled delivery take - the same decision. + user. Refuses with 400 when no webhook channel is saved or the saved config has no URL. A failure at the relay + or on the way to it is a 502 that carries the upstream status and, when safe, what the relay answered; only a + failure the server cannot name is a 500. The URL was checked against the SSRF floor when it was saved, so a test + cannot be used to reach an internal host. A URL on a private network is refused with 422 and `meta.errorCode` = + `private_origin_not_approved` unless the operator listed its exact origin in `NOTIFICATION_PRIVATE_ORIGINS`; the + test and the scheduled delivery take the same decision. responses: "200": description: The provider accepted the message. Not proof it was delivered. @@ -12443,6 +12479,24 @@ paths: schema: $ref: "#/components/schemas/ErrorEnvelope" "429": *a5 + "500": + description: The send failed in a way the server could not name. An internal fault, not a statement about the relay. + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorEnvelope" + "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)." + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorEnvelope" /api/settings/data: delete: tags: 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 295e44f25..259a35461 100644 --- a/src/app/api/admin/notifications/test/__tests__/route.test.ts +++ b/src/app/api/admin/notifications/test/__tests__/route.test.ts @@ -65,6 +65,12 @@ vi.mock("@/lib/notifications/senders/web-push", () => ({ vi.mock("@/lib/notifications/senders/apns", () => ({ sendViaApns: vi.fn(), })); +vi.mock("@/lib/notifications/senders/webhook", () => ({ + sendViaWebhook: vi.fn(), +})); +vi.mock("@/lib/notifications/senders/email", () => ({ + sendViaEmail: vi.fn(), +})); vi.mock("@/lib/i18n/server-translator", () => ({ getServerTranslator: vi.fn(() => ({ @@ -78,6 +84,8 @@ import { requireAdmin } from "@/lib/api-handler"; import { sendViaApns } from "@/lib/notifications/senders/apns"; import { sendViaWebPush } from "@/lib/notifications/senders/web-push"; import { sendViaNtfy } from "@/lib/notifications/senders/ntfy"; +import { sendViaWebhook } from "@/lib/notifications/senders/webhook"; +import { sendViaEmail } from "@/lib/notifications/senders/email"; const ADMIN_USER_ID = "admin-1"; const ADMIN_CTX = { @@ -92,7 +100,7 @@ const ADMIN_CTX = { interface FakeChannelRow { id: string; userId: string; - type: "TELEGRAM" | "NTFY" | "WEB_PUSH" | "APNS"; + type: "TELEGRAM" | "NTFY" | "WEB_PUSH" | "APNS" | "WEBHOOK" | "EMAIL"; enabled: boolean; config: string; preferences: Array<{ eventType: string; enabled: boolean }>; @@ -119,6 +127,10 @@ interface ResultEnvelope { channel: string; success: boolean; error?: string; + errorCode?: string; + upstreamStatus?: number; + smtpCode?: number; + upstreamBody?: string; }>; }; } @@ -299,3 +311,127 @@ describe("POST /api/admin/notifications/test — preserved branches", () => { }); }); }); + +describe("POST /api/admin/notifications/test — a failure keeps its cause (#947)", () => { + function channel( + type: FakeChannelRow["type"], + config: string, + ): FakeChannelRow { + return { + id: `ch-${type}`, + userId: ADMIN_USER_ID, + type, + enabled: true, + config, + preferences: [], + }; + } + + it("carries the ntfy status and code instead of a bare sentence", async () => { + vi.mocked(prisma.notificationChannel.findMany).mockResolvedValue([ + channel("NTFY", 'enc({"topic":"hl-test"})'), + ] as never); + vi.mocked(sendViaNtfy).mockResolvedValue({ + ok: false, + hardReject: false, + reason: "ntfy_400", + statusCode: 400, + }); + + const body = (await (await POST()).json()) as ResultEnvelope; + + expect(body.data.results[0]).toMatchObject({ + channel: "NTFY", + success: false, + errorCode: "upstream_rejected", + upstreamStatus: 400, + }); + expect(body.data.results[0].error).toContain("HTTP 400"); + }); + + it("tests the webhook channel rather than calling it unknown", async () => { + vi.mocked(prisma.notificationChannel.findMany).mockResolvedValue([ + channel("WEBHOOK", 'enc({"url":"https://relay.example.com/message"})'), + ] as never); + vi.mocked(sendViaWebhook).mockResolvedValue({ + ok: false, + hardReject: false, + reason: "webhook_400", + statusCode: 400, + upstreamBody: "priority must be an integer", + }); + + const body = (await (await POST()).json()) as ResultEnvelope; + + expect(sendViaWebhook).toHaveBeenCalledWith( + { url: "https://relay.example.com/message" }, + expect.objectContaining({ eventType: "SYSTEM_ALERT" }), + ); + expect(body.data.results[0]).toMatchObject({ + channel: "WEBHOOK", + success: false, + errorCode: "upstream_rejected", + upstreamStatus: 400, + upstreamBody: "priority must be an integer", + }); + }); + + it("tests the email channel and forwards the SMTP code", async () => { + vi.mocked(prisma.notificationChannel.findMany).mockResolvedValue([ + channel("EMAIL", 'enc({"recipient":"admin@example.com"})'), + ] as never); + vi.mocked(sendViaEmail).mockResolvedValue({ + ok: false, + hardReject: true, + reason: "email_smtp_5xx", + failureCode: "upstream_rejected", + smtpCode: 550, + }); + + const body = (await (await POST()).json()) as ResultEnvelope; + + expect(body.data.results[0]).toMatchObject({ + channel: "EMAIL", + success: false, + errorCode: "upstream_rejected", + smtpCode: 550, + }); + expect(body.data.results[0].error).toContain("SMTP 550"); + }); + + it("forwards a private-origin refusal with its own code", async () => { + vi.mocked(prisma.notificationChannel.findMany).mockResolvedValue([ + channel("WEBHOOK", 'enc({"url":"http://10.0.0.5/message"})'), + ] as never); + vi.mocked(sendViaWebhook).mockResolvedValue({ + ok: false, + hardReject: false, + reason: "webhook_private_origin_refused", + errorCode: "private_origin_not_approved", + }); + + const body = (await (await POST()).json()) as ResultEnvelope; + + expect(body.data.results[0]).toMatchObject({ + channel: "WEBHOOK", + success: false, + errorCode: "private_origin_not_approved", + }); + }); + + it("reports a delivered webhook and email as successes", async () => { + vi.mocked(prisma.notificationChannel.findMany).mockResolvedValue([ + channel("WEBHOOK", 'enc({"url":"https://relay.example.com"})'), + channel("EMAIL", 'enc({"recipient":"admin@example.com"})'), + ] as never); + vi.mocked(sendViaWebhook).mockResolvedValue({ ok: true, statusCode: 200 }); + vi.mocked(sendViaEmail).mockResolvedValue({ ok: true }); + + const body = (await (await POST()).json()) as ResultEnvelope; + + expect(body.data.results).toEqual([ + { channel: "WEBHOOK", success: true }, + { channel: "EMAIL", success: true }, + ]); + }); +}); diff --git a/src/app/api/admin/notifications/test/route.ts b/src/app/api/admin/notifications/test/route.ts index 33492125f..0bf282198 100644 --- a/src/app/api/admin/notifications/test/route.ts +++ b/src/app/api/admin/notifications/test/route.ts @@ -7,9 +7,19 @@ import { sendViaTelegram } from "@/lib/notifications/senders/telegram"; import { sendViaNtfy } from "@/lib/notifications/senders/ntfy"; import { sendViaWebPush } from "@/lib/notifications/senders/web-push"; import { sendViaApns } from "@/lib/notifications/senders/apns"; +import { sendViaWebhook } from "@/lib/notifications/senders/webhook"; +import { sendViaEmail } from "@/lib/notifications/senders/email"; +import type { SendOutcome } from "@/lib/notifications/retry-policy"; +import { + testFailureDetail, + testFailureSentence, + type TestFailureDetail, +} from "@/lib/notifications/test-delivery-failure"; import type { TelegramChannelConfig, NtfyChannelConfig, + WebhookChannelConfig, + EmailChannelConfig, ChannelType, NotificationPayload, } from "@/lib/notifications/types"; @@ -62,6 +72,39 @@ async function ensureTelegramChannel(userId: string): Promise { return true; } +type TestResult = { + channel: string; + success: boolean; + error?: string; +} & TestFailureDetail; + +/** + * A failed HTTP or SMTP send, with the code, status and SMTP reply code the + * user-facing test routes answer with, instead of a sentence that drops + * them. A private-origin refusal forwards its own code. + */ +function failedSend( + channel: string, + label: string, + result: SendOutcome, + context?: string, +): TestResult { + const detail: TestFailureDetail = result.errorCode + ? { errorCode: result.errorCode } + : testFailureDetail(result); + const cause = result.errorCode + ? `refused by the private-origin policy (${result.errorCode})` + : detail.errorCode + ? testFailureSentence(label, detail) + : (result.reason ?? "send failed"); + return { + channel, + success: false, + error: context ? `${cause} (${context})` : cause, + ...detail, + }; +} + function isLocale(value: string | null | undefined): value is Locale { return ( value === "de" || @@ -121,11 +164,7 @@ export const POST = apiHandler(async () => { message: t("notifications.admin.testNotificationBody"), }; - const results: Array<{ - channel: string; - success: boolean; - error?: string; - }> = []; + const results: TestResult[] = []; for (const channel of channels) { const pref = channel.preferences[0]; @@ -166,11 +205,40 @@ export const POST = apiHandler(async () => { const ntfyResult = await sendViaNtfy(config, payload); success = ntfyResult.ok; if (!success) { - results.push({ - channel: "NTFY", - success: false, - error: `Send failed (topic: ${config.topic})`, - }); + results.push( + failedSend( + "NTFY", + "The ntfy server", + ntfyResult, + `topic: ${config.topic}`, + ), + ); + continue; + } + break; + } + // The generic webhook and email are first-class channels in the + // dispatcher; without these arms they reported "Unknown channel type". + case "WEBHOOK": { + const config = JSON.parse( + decrypt(channel.config), + ) as WebhookChannelConfig; + const webhookResult = await sendViaWebhook(config, payload); + success = webhookResult.ok; + if (!success) { + results.push(failedSend("WEBHOOK", "The webhook", webhookResult)); + continue; + } + break; + } + case "EMAIL": { + const config = JSON.parse( + decrypt(channel.config), + ) as EmailChannelConfig; + const emailResult = await sendViaEmail(config, payload); + success = emailResult.ok; + if (!success) { + results.push(failedSend("EMAIL", "The mail server", emailResult)); continue; } break; diff --git a/src/app/api/settings/__tests__/email-test-route.test.ts b/src/app/api/settings/__tests__/email-test-route.test.ts new file mode 100644 index 000000000..2bf5e0684 --- /dev/null +++ b/src/app/api/settings/__tests__/email-test-route.test.ts @@ -0,0 +1,131 @@ +/** + * POST /api/settings/email/test — a failed send names its cause. + * + * Before this the route answered every failure with a bare 500, so the card + * read "Test failed" whether the mail server refused the login, timed out, + * or rejected the recipient. + */ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@/lib/db", () => ({ + prisma: { notificationChannel: { findUnique: vi.fn() } }, +})); +vi.mock("@/lib/crypto", () => ({ + decrypt: vi.fn((value: string) => value.replace(/^encrypted:/, "")), +})); +const sendViaEmailMock = vi.fn(); +vi.mock("@/lib/notifications/senders/email", () => ({ + sendViaEmail: (...args: unknown[]) => sendViaEmailMock(...args), +})); +vi.mock("@/lib/notifications/senders/email-config", () => ({ + isEmailConfigured: () => true, +})); +vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() })); +vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/rate-limit", () => ({ + checkRateLimit: vi.fn().mockResolvedValue({ + allowed: true, + remaining: 4, + resetAt: Date.now() + 60_000, + }), + rateLimitHeaders: () => ({}), +})); +vi.mock("next/headers", () => ({ + headers: vi.fn(async () => ({ get: () => null })), + cookies: vi.fn(async () => ({ + get: () => undefined, + set: () => {}, + delete: () => {}, + })), +})); + +import { POST } from "../email/test/route"; +import { prisma } from "@/lib/db"; +import { getSession } from "@/lib/auth/session"; + +const SESSION_OK = { + session: { id: "sess-1", expiresAt: new Date(Date.now() + 3_600_000) }, + user: { id: "user-1", username: "testuser", role: "USER" as const }, +}; + +function post(): Promise { + return (POST as (request: Request) => Promise)( + new Request("http://localhost/api/settings/email/test", { + method: "POST", + }), + ); +} + +beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(prisma.notificationChannel.findUnique).mockResolvedValue({ + config: 'encrypted:{"recipient":"you@example.com"}', + } as never); +}); + +describe("POST /api/settings/email/test", () => { + it.each([ + ["credentials_rejected", 535, "rejected the credentials (SMTP 535)"], + ["upstream_rejected", 550, "refused the message (SMTP 550)"], + ])( + "answers an SMTP rejection with 502, %s and the reply code", + async (failureCode, smtpCode, sentence) => { + sendViaEmailMock.mockResolvedValue({ + ok: false, + hardReject: smtpCode >= 500, + reason: "email_smtp_5xx", + failureCode, + smtpCode, + }); + + const response = await post(); + + expect(response.status).toBe(502); + const json = await response.json(); + expect(json.meta).toEqual({ errorCode: failureCode, smtpCode }); + expect(json.error).toContain(sentence); + }, + ); + + it("answers a timeout with 502 and the timeout code", async () => { + sendViaEmailMock.mockResolvedValue({ + ok: false, + hardReject: false, + reason: "email_smtp_error", + failureCode: "timeout", + }); + + const response = await post(); + + expect(response.status).toBe(502); + await expect(response.json()).resolves.toMatchObject({ + meta: { errorCode: "timeout" }, + }); + }); + + it("keeps a 500 for a failure the sender could not name", async () => { + sendViaEmailMock.mockResolvedValue({ + ok: false, + hardReject: false, + reason: "email_smtp_error", + }); + + const response = await post(); + + expect(response.status).toBe(500); + const json = await response.json(); + expect(json.meta).toBeUndefined(); + }); + + it("answers a delivered test with 200", async () => { + sendViaEmailMock.mockResolvedValue({ ok: true }); + + const response = await post(); + + expect(response.status).toBe(200); + }); +}); 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 4bc8d5b77..7825c97b3 100644 --- a/src/app/api/settings/__tests__/private-origin-routes.test.ts +++ b/src/app/api/settings/__tests__/private-origin-routes.test.ts @@ -8,7 +8,9 @@ * of a generic "Invalid data", and a listed literal address saves although * the plain public floor would refuse it. The test routes forward the * sender's own refusal as a 422 with the same code instead of the bare 500 - * that made the reporter read the wide-event log to learn it was policy. + * that made the reporter read the wide-event log to learn it was policy, and + * answer a relay's error status with a 502 that carries the status and what + * the relay said. * * The last block is the structural half: the test button and the dispatcher * must reach the SAME sender function, so the decision cannot differ between @@ -351,20 +353,99 @@ describe("POST /api/settings/{webhook,ntfy}/test — the refusal names itself", it.each([ ["webhook", sendViaWebhookMock], ["ntfy", sendViaNtfyMock], - ])("%s: a plain delivery failure is still a 500", async (path, sender) => { - sender.mockResolvedValue({ - ok: false, - hardReject: false, - reason: `${path}_503`, - statusCode: 503, - }); + ])( + "%s: a relay that answered 400 is a 502 naming the status and what it said, not a bare 500 (#947: the card could only say Test failed)", + async (path, sender) => { + sender.mockResolvedValue({ + ok: false, + hardReject: false, + reason: `${path}_400`, + statusCode: 400, + upstreamBody: '{"error":"Bad Request","errorCode":400}', + }); - const response = await post(path); + const response = await post(path); - expect(response.status).toBe(500); - const json = await response.json(); - expect(json.meta).toBeUndefined(); - }); + expect(response.status).toBe(502); + const json = await response.json(); + expect(json.meta).toEqual({ + errorCode: "upstream_rejected", + upstreamStatus: 400, + upstreamBody: '{"error":"Bad Request","errorCode":400}', + }); + expect(json.error).toContain("HTTP 400"); + }, + ); + + it.each([ + [401, "credentials_rejected"], + [403, "credentials_rejected"], + [404, "endpoint_not_found"], + [410, "endpoint_not_found"], + [415, "upstream_rejected"], + [422, "upstream_rejected"], + [429, "rate_limited"], + [503, "upstream_error"], + [307, "redirected"], + ])( + "webhook: an upstream %i maps to %s, without a body when the sender dropped it", + async (status, code) => { + sendViaWebhookMock.mockResolvedValue({ + ok: false, + hardReject: false, + reason: `webhook_${status}`, + statusCode: status, + }); + + const response = await post("webhook"); + + expect(response.status).toBe(502); + const json = await response.json(); + expect(json.meta).toEqual({ errorCode: code, upstreamStatus: status }); + }, + ); + + it.each([ + ["webhook", sendViaWebhookMock], + ["ntfy", sendViaNtfyMock], + ])( + "%s: a timeout without a status is a 502 with the timeout code", + async (path, sender) => { + sender.mockResolvedValue({ + ok: false, + hardReject: false, + reason: `${path}_network_error`, + failureCode: "timeout", + }); + + const response = await post(path); + + expect(response.status).toBe(502); + await expect(response.json()).resolves.toMatchObject({ + meta: { errorCode: "timeout" }, + }); + }, + ); + + it.each([ + ["webhook", sendViaWebhookMock], + ["ntfy", sendViaNtfyMock], + ])( + "%s: only a failure the sender could not name stays a 500", + async (path, sender) => { + sender.mockResolvedValue({ + ok: false, + hardReject: false, + reason: `${path}_network_error`, + }); + + const response = await post(path); + + expect(response.status).toBe(500); + const json = await response.json(); + expect(json.meta).toBeUndefined(); + }, + ); it.each([ ["webhook", sendViaWebhookMock], diff --git a/src/app/api/settings/email/test/route.ts b/src/app/api/settings/email/test/route.ts index 4e40c8b9a..e6c1ecaa0 100644 --- a/src/app/api/settings/email/test/route.ts +++ b/src/app/api/settings/email/test/route.ts @@ -7,6 +7,7 @@ import { checkRateLimit } from "@/lib/rate-limit"; import { apiHandler, requireAuth } from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; import { isEmailConfigured } from "@/lib/notifications/senders/email-config"; +import { answerTestDeliveryFailure } from "@/lib/notifications/test-delivery-failure"; /** * POST: send a test email to the configured recipient. @@ -46,7 +47,13 @@ export const POST = apiHandler(async () => { }); if (!result.ok) { - return apiError("Failed to send test email", 500); + // The mail server, or the way to it, failed: 502 with the code and the + // SMTP reply code. Only an unnamed internal fault is a 500. + return answerTestDeliveryFailure(result, { + channel: "email", + label: "The mail server", + fallbackMessage: "Failed to send test email", + }); } annotate({ diff --git a/src/app/api/settings/ntfy/test/route.ts b/src/app/api/settings/ntfy/test/route.ts index b260f1643..feeaff9ea 100644 --- a/src/app/api/settings/ntfy/test/route.ts +++ b/src/app/api/settings/ntfy/test/route.ts @@ -6,6 +6,7 @@ import type { NtfyChannelConfig } from "@/lib/notifications/types"; import { checkRateLimit } from "@/lib/rate-limit"; import { apiHandler, requireAuth } from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; +import { answerTestDeliveryFailure } from "@/lib/notifications/test-delivery-failure"; /** * POST: Send a test notification via ntfy. @@ -58,7 +59,13 @@ export const POST = apiHandler(async () => { { errorCode: result.errorCode }, ); } - return apiError("Failed to send test message", 500); + // The server, or the way to it, failed: 502 with the upstream status + // and what it said (#947). Only an unnamed internal fault is a 500. + return answerTestDeliveryFailure(result, { + channel: "ntfy", + label: "The ntfy server", + fallbackMessage: "Failed to send test message", + }); } annotate({ action: { name: "settings.ntfy.test" }, meta: { success: true } }); diff --git a/src/app/api/settings/webhook/test/route.ts b/src/app/api/settings/webhook/test/route.ts index cc6bfa6dc..7021e2bfe 100644 --- a/src/app/api/settings/webhook/test/route.ts +++ b/src/app/api/settings/webhook/test/route.ts @@ -6,6 +6,7 @@ import type { WebhookChannelConfig } from "@/lib/notifications/types"; import { checkRateLimit } from "@/lib/rate-limit"; import { apiHandler, requireAuth } from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; +import { answerTestDeliveryFailure } from "@/lib/notifications/test-delivery-failure"; /** * POST: send a test notification via the configured generic webhook. @@ -57,7 +58,14 @@ export const POST = apiHandler(async () => { { errorCode: result.errorCode }, ); } - return apiError("Failed to send test message", 500); + // The relay, or the way to it, failed: 502 with the upstream status and + // what it said, so the card can show "the relay answered 400" instead + // of "Test failed" (#947). Only an unnamed internal fault is a 500. + return answerTestDeliveryFailure(result, { + channel: "webhook", + label: "The webhook", + fallbackMessage: "Failed to send test message", + }); } annotate({ diff --git a/src/lib/notifications/__tests__/test-delivery-failure.test.ts b/src/lib/notifications/__tests__/test-delivery-failure.test.ts new file mode 100644 index 000000000..69be323fb --- /dev/null +++ b/src/lib/notifications/__tests__/test-delivery-failure.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it } from "vitest"; + +import { + codeForUpstreamStatus, + testFailureDetail, + testFailureSentence, +} from "@/lib/notifications/test-delivery-failure"; + +describe("codeForUpstreamStatus", () => { + it.each([ + [400, "upstream_rejected"], + [401, "credentials_rejected"], + [403, "credentials_rejected"], + [404, "endpoint_not_found"], + [405, "upstream_rejected"], + [409, "upstream_rejected"], + [410, "endpoint_not_found"], + [413, "upstream_rejected"], + [415, "upstream_rejected"], + [422, "upstream_rejected"], + [429, "rate_limited"], + [500, "upstream_error"], + [502, "upstream_error"], + [301, "redirected"], + [308, "redirected"], + ])("%i → %s", (status, code) => { + expect(codeForUpstreamStatus(status)).toBe(code); + }); +}); + +describe("testFailureDetail", () => { + it("prefers the HTTP status over any transport code", () => { + expect( + testFailureDetail({ + ok: false, + statusCode: 400, + failureCode: "timeout", + upstreamBody: "bad", + }), + ).toEqual({ + errorCode: "upstream_rejected", + upstreamStatus: 400, + upstreamBody: "bad", + }); + }); + + it("uses the sender's code when there is no status", () => { + expect( + testFailureDetail({ + ok: false, + failureCode: "upstream_rejected", + smtpCode: 550, + }), + ).toEqual({ errorCode: "upstream_rejected", smtpCode: 550 }); + }); + + it("names nothing for an unnamed failure", () => { + expect(testFailureDetail({ ok: false, reason: "x" })).toEqual({}); + }); +}); + +describe("testFailureSentence", () => { + it("states the status, and falls back to a plain sentence for an unlisted code", () => { + expect( + testFailureSentence("The webhook", { + errorCode: "upstream_rejected", + upstreamStatus: 400, + }), + ).toBe("The webhook answered HTTP 400."); + expect( + testFailureSentence("The webhook", { errorCode: "something_new" }), + ).toBe("The webhook test failed."); + }); +}); diff --git a/src/lib/notifications/test-delivery-failure.ts b/src/lib/notifications/test-delivery-failure.ts new file mode 100644 index 000000000..1728f1678 --- /dev/null +++ b/src/lib/notifications/test-delivery-failure.ts @@ -0,0 +1,109 @@ +import { apiError } from "@/lib/api-response"; +import { annotate } from "@/lib/logging/context"; +import type { SendOutcome } from "@/lib/notifications/retry-policy"; + +/** + * The code for an HTTP status a relay answered, in the test card's + * vocabulary (`settings.testConnection.errors`). The same split the + * Nightscout and wearable test routes use, plus two the relays need: a 3xx + * (safeFetch never follows a redirect) and a 4xx that is not about the + * credentials or the path, where the relay accepted the connection and + * refused the payload. + */ +export function codeForUpstreamStatus(status: number): string { + if (status === 401 || status === 403) return "credentials_rejected"; + if (status === 404 || status === 410) return "endpoint_not_found"; + if (status === 429) return "rate_limited"; + if (status >= 500) return "upstream_error"; + if (status >= 300 && status < 400) return "redirected"; + return "upstream_rejected"; +} + +export interface TestFailureDetail { + /** Absent only for a failure the sender could not name: an internal fault. */ + errorCode?: string; + upstreamStatus?: number; + smtpCode?: number; + upstreamBody?: string; +} + +/** What a failed send outcome says, without the private-origin refusal. */ +export function testFailureDetail(result: SendOutcome): TestFailureDetail { + if (result.statusCode !== undefined) { + return { + errorCode: codeForUpstreamStatus(result.statusCode), + upstreamStatus: result.statusCode, + ...(result.upstreamBody ? { upstreamBody: result.upstreamBody } : {}), + }; + } + if (result.failureCode) { + return { + errorCode: result.failureCode, + ...(result.smtpCode !== undefined ? { smtpCode: result.smtpCode } : {}), + }; + } + return {}; +} + +/** One English sentence for the envelope's `error`, the card translates the code. */ +export function testFailureSentence( + label: string, + detail: TestFailureDetail, +): string { + if (detail.upstreamStatus !== undefined) { + return `${label} answered HTTP ${detail.upstreamStatus}.`; + } + const smtp = + detail.smtpCode !== undefined ? ` (SMTP ${detail.smtpCode})` : ""; + switch (detail.errorCode) { + case "timeout": + return `${label} did not answer in time.`; + case "connection_failed": + return `Could not connect to ${label}.`; + case "credentials_rejected": + return `${label} rejected the credentials${smtp}.`; + case "upstream_rejected": + return `${label} refused the message${smtp}.`; + case "upstream_error": + return `${label} reported a temporary error${smtp}.`; + default: + return `${label} test failed.`; + } +} + +/** + * The test routes' answer to a failed send that was not a private-origin + * refusal (those keep their own 422 arm). + * + * 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`. + */ +export function answerTestDeliveryFailure( + result: SendOutcome, + opts: { + channel: "webhook" | "ntfy" | "email"; + label: string; + fallbackMessage: string; + }, +): Response { + const detail = testFailureDetail(result); + annotate({ + action: { name: `settings.${opts.channel}.test` }, + meta: { + success: false, + [`${opts.channel}_test_code`]: detail.errorCode ?? "internal", + ...(detail.upstreamStatus !== undefined + ? { [`${opts.channel}_test_status`]: detail.upstreamStatus } + : {}), + ...(detail.smtpCode !== undefined + ? { [`${opts.channel}_test_smtp_code`]: detail.smtpCode } + : {}), + }, + }); + if (!detail.errorCode) { + return apiError(opts.fallbackMessage, 500); + } + return apiError(testFailureSentence(opts.label, detail), 502, { ...detail }); +} diff --git a/src/lib/openapi/routes/settings.ts b/src/lib/openapi/routes/settings.ts index 7b948794f..03264d50a 100644 --- a/src/lib/openapi/routes/settings.ts +++ b/src/lib/openapi/routes/settings.ts @@ -685,7 +685,7 @@ export const settingsPaths: NonNullable = { summary: "Send a test email to the saved recipient", description: "Fires one message through the operator's SMTP transport to the address saved on this account. Takes no body. Rate-limited 5 per 5 minutes per user.\n\n" + - "Refuses with 400 when the instance has no SMTP transport, when the account has no email channel, or when the channel carries no recipient — three distinguishable prose messages on one status. A transport failure is a 500.", + "Refuses with 400 when the instance has no SMTP transport, when the account has no email channel, or when the channel carries no recipient — three distinguishable prose messages on one status. A failure at the mail server or on the way to it is a 502 that names its cause; only a failure the server cannot name is a 500.", responses: { "200": { description: @@ -697,6 +697,16 @@ 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.", + content: { "application/json": { schema: errorEnvelope } }, + }, + "500": { + description: + "The send failed in a way the server could not name. An internal fault, not a statement about the mail server.", + content: { "application/json": { schema: errorEnvelope } }, + }, }, }, }, @@ -705,7 +715,7 @@ export const settingsPaths: NonNullable = { tags: ["Notifications"], summary: "Send a test message to the saved ntfy topic", description: - "Publishes one message to the account's configured ntfy server and topic. Takes no body. Rate-limited 5 per 5 minutes per user. Refuses with 400 when no ntfy channel is saved or when the saved config is missing a server URL or topic; a publish failure is a 500. A server on a private network is refused with 422 and `meta.errorCode` = `private_origin_not_approved` unless the operator listed its exact origin in `NOTIFICATION_PRIVATE_ORIGINS`; the test and the scheduled delivery take the same decision.", + "Publishes one message to the account's configured ntfy server and topic. Takes no body. Rate-limited 5 per 5 minutes per user. Refuses with 400 when no ntfy channel is saved or when the saved config is missing a server URL or topic. A failure at the ntfy server or on the way to it is a 502 that carries the upstream status and, when safe, what the server answered; only a failure the server cannot name is a 500. A server on a private network is refused with 422 and `meta.errorCode` = `private_origin_not_approved` unless the operator listed its exact origin in `NOTIFICATION_PRIVATE_ORIGINS`; the test and the scheduled delivery take the same decision.", responses: { "200": { description: @@ -717,6 +727,16 @@ 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).", + content: { "application/json": { schema: errorEnvelope } }, + }, + "500": { + description: + "The send failed in a way the server could not name. An internal fault, not a statement about the ntfy server.", + content: { "application/json": { schema: errorEnvelope } }, + }, "422": { description: "The saved server is on a private network the operator has not approved (`meta.errorCode` = `private_origin_not_approved`), or is a link-local, metadata or unspecified address that no grant can open (`private_origin_not_grantable`). The resolved address is never echoed.", @@ -751,7 +771,7 @@ export const settingsPaths: NonNullable = { tags: ["Notifications"], summary: "Send a test payload to the saved webhook", description: - "Posts one payload to the account's configured webhook URL. Takes no body. Rate-limited 5 per 5 minutes per user. Refuses with 400 when no webhook channel is saved or the saved config has no URL; a delivery failure is a 500. The URL was checked against the SSRF floor when it was saved, so a test cannot be used to reach an internal host. A URL on a private network is refused with 422 and `meta.errorCode` = `private_origin_not_approved` unless the operator listed its exact origin in `NOTIFICATION_PRIVATE_ORIGINS`; the test and the scheduled delivery take the same decision.", + "Posts one payload to the account's configured webhook URL. Takes no body. Rate-limited 5 per 5 minutes per user. Refuses with 400 when no webhook channel is saved or the saved config has no URL. A failure at the relay or on the way to it is a 502 that carries the upstream status and, when safe, what the relay answered; only a failure the server cannot name is a 500. The URL was checked against the SSRF floor when it was saved, so a test cannot be used to reach an internal host. A URL on a private network is refused with 422 and `meta.errorCode` = `private_origin_not_approved` unless the operator listed its exact origin in `NOTIFICATION_PRIVATE_ORIGINS`; the test and the scheduled delivery take the same decision.", responses: { "200": { description: @@ -763,6 +783,16 @@ 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).", + content: { "application/json": { schema: errorEnvelope } }, + }, + "500": { + description: + "The send failed in a way the server could not name. An internal fault, not a statement about the relay.", + content: { "application/json": { schema: errorEnvelope } }, + }, "422": { description: "The saved URL is on a private network the operator has not approved (`meta.errorCode` = `private_origin_not_approved`), or is a link-local, metadata or unspecified address that no grant can open (`private_origin_not_grantable`). The resolved address is never echoed.",