From 2d315df8bf06b8dc9ac1e37dde08018afdca4855 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 6 Sep 2026 13:49:29 +0200 Subject: [PATCH] fix(security): keep the operator's ntfy token in and re-check every redirect hop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two reported advisories, and a third path neither of them covers. GHSA-7pqc-fj3c-9346 — the ntfy token fell back to the operator's whenever a request did not carry one, so an authenticated user who named their own server was handed the operator's decrypted token as `Authorization: Bearer`. The report covers `test-ntfy`, but the live send path in channels/builtins.ts does the same thing off a per-user `ntfy_server` setting: no test route needed, just wait for the next notification. Gating the route on `role === 'admin'` would have closed one of the two and broken the documented shared-server setup, where the operator supplies server plus token and users only set a topic. The invariant is the target, not the role: the operator's token travels to the operator's server and nowhere else, and that rule fits both paths unchanged. Scheme is part of the comparison — an http twin of an https server would put it on the wire in clear. GHSA-8mw6-xphx-886m — both transports checked only the first URL and then let the platform follow redirects on a dispatcher pinned to that first IP. Pinning does not help: Node skips the pinned lookup for an IP-literal host, so a 307 to 127.0.0.1 or 169.254.169.254 connected straight through. They go through safeFetchFollow now, which re-checks and re-pins per hop. safeFetch itself had the same shape and carried it to sixteen more callers, several with a URL out of a per-user setting, and one (pipeAsset) streams the response back — which would have made a redirect into an internal service readable rather than blind. It now routes through the same follower. The third: following by hand opts out of the platform's own rules, and both manual followers replayed the caller's init verbatim. undici drops `Authorization` across origins itself, so converting ntfy to safeFetchFollow would have re-opened the first advisory through a redirect. Credential headers are now dropped on an origin change (same-host http to https stays, self-hosted setups depend on it), and a 303 — or a 301/302 on a non-idempotent method — drops the body, so a plugin's client_secret is not re-POSTed at a host the first one merely pointed at. plugin-oauth pins maxRedirects to 0 like its OIDC twin, and the Google Places calls get the timeout none of their nine call sites passed. --- server/src/nest/maps/maps.service.ts | 8 ++ .../nest/notifications/channels/builtins.ts | 7 +- .../notifications/notifications.controller.ts | 8 +- .../notifications/transports/ntfy.service.ts | 65 +++++++++++-- .../transports/webhook.service.ts | 25 +++-- .../plugins/oauth/plugin-oauth.service.ts | 7 +- server/src/utils/ssrfGuard.ts | 92 +++++++++++++++++-- .../tests/unit/nest/channel-override.test.ts | 24 ++++- server/tests/unit/nest/collab.service.test.ts | 26 +++++- .../unit/nest/notification-transports.test.ts | 76 +++++++++++++-- .../nest/notifications.controller.test.ts | 36 ++++++++ .../unit/nest/notifications.service.test.ts | 27 +++++- 12 files changed, 355 insertions(+), 46 deletions(-) diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index d1da18539..59cdab5ca 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -76,6 +76,9 @@ function logKeyFailure(label: string, status: number, userId: number, source: Ap console.error(`[Maps] ${label} failed with ${status} userId=${userId} keySource=${source}`); } +/** Ceiling for one Google Places call. Generous — the photo download is the slow one. */ +const GOOGLE_FETCH_TIMEOUT_MS = 20000; + function googleFetch(rawEndpoint: string, label: string, init?: RequestInit): Promise { const endpoint = placesEndpoint(rawEndpoint); googleApiCallCount++; @@ -83,6 +86,11 @@ function googleFetch(rawEndpoint: string, label: string, init?: RequestInit): Pr const referer = readEnv().app.appUrl ? getAppUrl() : undefined; return fetch(endpoint, { ...init, + // A default ceiling here rather than at each of the nine call sites, none of + // which passed one: a hung upstream held the request handler open for as + // long as it liked. A caller that needs longer still wins, it only has to + // say so. + signal: init?.signal ?? AbortSignal.timeout(GOOGLE_FETCH_TIMEOUT_MS), headers: { ...(referer ? { Referer: referer } : {}), ...((init?.headers as Record) ?? {}) }, }); } diff --git a/server/src/nest/notifications/channels/builtins.ts b/server/src/nest/notifications/channels/builtins.ts index d55e13d9d..1f713c722 100644 --- a/server/src/nest/notifications/channels/builtins.ts +++ b/server/src/nest/notifications/channels/builtins.ts @@ -1,7 +1,7 @@ import { registerChannel } from '../channel-registry'; import type { ChannelMessage, ExternalChannel } from '../notification-events'; import type { MailerService } from '../mailer/mailer.service'; -import { resolveAdminNtfyUrl, resolveNtfyUrl, type NtfyService } from '../transports/ntfy.service'; +import { resolveAdminNtfyUrl, resolveNtfyToken, resolveNtfyUrl, type NtfyService } from '../transports/ntfy.service'; import type { WebhookService } from '../transports/webhook.service'; // The three built-in external channels, wrapping the transports that were free @@ -92,7 +92,10 @@ export function buildBuiltinChannels({ mailer, webhook, ntfy }: BuiltinChannelDe const adminCfg = ntfy.getAdminNtfyConfig(); const url = resolveNtfyUrl(adminCfg, userCfg); if (!url) return false; - return ntfy.sendNtfy(url, userCfg?.token ?? adminCfg.token, { event: msg.event, title: msg.title, body: msg.body, link: msg.url }); + // Not `?? adminCfg.token`: the user picks their own ntfy_server, so that + // handed the operator's decrypted token to whatever host they named, on + // every ordinary send and with no test route involved (GHSA-7pqc-fj3c-9346). + return ntfy.sendNtfy(url, resolveNtfyToken(adminCfg, userCfg), { event: msg.event, title: msg.title, body: msg.body, link: msg.url }); }, async sendGlobal(msg: ChannelMessage) { const adminCfg = ntfy.getAdminNtfyConfig(); diff --git a/server/src/nest/notifications/notifications.controller.ts b/server/src/nest/notifications/notifications.controller.ts index 29c4d4a88..c0935452f 100644 --- a/server/src/nest/notifications/notifications.controller.ts +++ b/server/src/nest/notifications/notifications.controller.ts @@ -14,6 +14,7 @@ import { import type { ChannelTestResult, UnreadCountResult } from '@trek/shared'; import type { User } from '../../types'; import { NotificationsService } from './notifications.service'; +import { resolveNtfyToken } from './transports/ntfy.service'; import { PreferencesUpdateDto, TestSmtpDto, @@ -102,9 +103,14 @@ export class NotificationsController { const resolvedTopic = topic || userCfg?.topic || undefined; const resolvedServer = server || userCfg?.server || adminCfg.server || undefined; // Reuse the saved token when the request sends null, empty, or the masked placeholder. + // Not `?? adminCfg.token`: the caller picks `server`, so that handed the + // operator's decrypted token to any host an authenticated user named + // (GHSA-7pqc-fj3c-9346). Same rule as the live send path, and target-based + // rather than role-based on purpose — an admin-only gate here would take a + // working button away from every user with their own ntfy config. const resolvedToken = (token && token !== MASKED) ? token - : (userCfg?.token ?? adminCfg.token ?? null); + : resolveNtfyToken(adminCfg, userCfg, resolvedServer ?? null); if (!resolvedTopic) { throw new HttpException({ error: 'No ntfy topic configured' }, 400); diff --git a/server/src/nest/notifications/transports/ntfy.service.ts b/server/src/nest/notifications/transports/ntfy.service.ts index cf76033b4..0bfe4c293 100644 --- a/server/src/nest/notifications/transports/ntfy.service.ts +++ b/server/src/nest/notifications/transports/ntfy.service.ts @@ -2,7 +2,7 @@ import { Injectable } from '@nestjs/common'; import { logDebug, logError, logInfo } from '../../audit/audit-log.logger'; import { decrypt_api_key } from '../../common/crypto/apiKeyCrypto'; import { DatabaseService } from '../../database/database.service'; -import { checkSsrf, createPinnedDispatcher } from '../../../utils/ssrfGuard'; +import { safeFetchFollow, SsrfBlockedError } from '../../../utils/ssrfGuard'; import type { NotifEventType } from '../notification-events'; export interface NtfyConfig { @@ -44,6 +44,46 @@ export function resolveNtfyUrl(adminCfg: NtfyConfig, userCfg: NtfyConfig | null) return `${base}/${encodeURIComponent(topic)}`; } +/** + * Whether an ntfy target is the operator's own server. + * + * The admin token is the operator's credential for the operator's server, so it + * may only travel there. Anyone can save their own `ntfy_server`, and without + * this the decrypted admin token went out as `Authorization: Bearer` to whatever + * host they named — on the test route and on every ordinary send + * (GHSA-7pqc-fj3c-9346). The scheme is part of the comparison: an http:// twin + * of an https:// server would put the token on the wire in the clear. + * Both sides empty means ntfy.sh against ntfy.sh, which is still a match. + */ +export function isOperatorNtfyServer(server: string | null | undefined, adminServer: string | null): boolean { + const normalize = (value: string | null | undefined): string => { + const raw = (value || 'https://ntfy.sh').replace(/(? { if (!url) return false; - const ssrf = await checkSsrf(url); - if (!ssrf.allowed) { - logError(`Ntfy blocked by SSRF guard event=${payload.event} url=${url} reason=${ssrf.error}`); - return false; - } - const meta = NTFY_EVENT_META[payload.event as NotifEventType] ?? NTFY_DEFAULT_META; // ntfy header-based API: POST to topic URL, body = plain text message, metadata in headers @@ -124,13 +158,17 @@ export class NtfyService { if (token) headers['Authorization'] = `Bearer ${token}`; try { - const res = await fetch(url, { + // Every hop re-checked and re-pinned. Checking only the first URL was not + // enough: Node skips the pinned lookup for an IP-literal host, so a 307 to + // 127.0.0.1 or 169.254.169.254 connected straight through + // (GHSA-8mw6-xphx-886m). No dispatcher or redirect here — safeFetchFollow + // sets both per hop and would overwrite them. + const res = await safeFetchFollow(url, { method: 'POST', headers, body: payload.body, signal: AbortSignal.timeout(10000), - dispatcher: createPinnedDispatcher(ssrf.resolvedIp!), - } as RequestInit); + }); if (!res.ok) { const errBody = await res.text().catch(() => ''); @@ -142,6 +180,13 @@ export class NtfyService { logDebug(`Ntfy url=${url} priority=${meta.priority} tags=${meta.tags.join(',')}`); return true; } catch (err) { + // The guard used to run before the try, with its own line and its own + // `return false`. safeFetchFollow throws instead, so the same line and the + // same result are reproduced here rather than changing what callers see. + if (err instanceof SsrfBlockedError) { + logError(`Ntfy blocked by SSRF guard event=${payload.event} url=${url} reason=${err.message}`); + return false; + } logError(`Ntfy failed event=${payload.event}: ${err instanceof Error ? err.message : err}`); return false; } diff --git a/server/src/nest/notifications/transports/webhook.service.ts b/server/src/nest/notifications/transports/webhook.service.ts index 2647e929c..48d065174 100644 --- a/server/src/nest/notifications/transports/webhook.service.ts +++ b/server/src/nest/notifications/transports/webhook.service.ts @@ -2,7 +2,7 @@ import { Injectable } from '@nestjs/common'; import { logDebug, logError, logInfo } from '../../audit/audit-log.logger'; import { decrypt_api_key } from '../../common/crypto/apiKeyCrypto'; import { DatabaseService } from '../../database/database.service'; -import { checkSsrf, createPinnedDispatcher } from '../../../utils/ssrfGuard'; +import { safeFetchFollow, SsrfBlockedError } from '../../../utils/ssrfGuard'; /** * Renders the outgoing body. Discord and Slack get their native shapes; anything @@ -66,20 +66,18 @@ export class WebhookService { ): Promise { if (!url) return false; - const ssrf = await checkSsrf(url); - if (!ssrf.allowed) { - logError(`Webhook blocked by SSRF guard event=${payload.event} url=${url} reason=${ssrf.error}`); - return false; - } - try { - const res = await fetch(url, { + // Every hop re-checked and re-pinned. Checking only the first URL was not + // enough: Node skips the pinned lookup for an IP-literal host, so a 307 to + // 127.0.0.1 or 169.254.169.254 connected straight through + // (GHSA-8mw6-xphx-886m). No dispatcher or redirect here — safeFetchFollow + // sets both per hop and would overwrite them. + const res = await safeFetchFollow(url, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: buildWebhookBody(url, payload), signal: AbortSignal.timeout(10000), - dispatcher: createPinnedDispatcher(ssrf.resolvedIp!), - } as RequestInit); + }); if (!res.ok) { const errBody = await res.text().catch(() => ''); @@ -91,6 +89,13 @@ export class WebhookService { logDebug(`Webhook url=${url} payload=${buildWebhookBody(url, payload).substring(0, 500)}`); return true; } catch (err) { + // The guard used to run before the try, with its own line and its own + // `return false`. safeFetchFollow throws instead, so the same line and the + // same result are reproduced here rather than changing what callers see. + if (err instanceof SsrfBlockedError) { + logError(`Webhook blocked by SSRF guard event=${payload.event} url=${url} reason=${err.message}`); + return false; + } logError(`Webhook failed event=${payload.event}: ${err instanceof Error ? err.message : err}`); return false; } diff --git a/server/src/nest/plugins/oauth/plugin-oauth.service.ts b/server/src/nest/plugins/oauth/plugin-oauth.service.ts index 5008f6e76..3a19bc91f 100644 --- a/server/src/nest/plugins/oauth/plugin-oauth.service.ts +++ b/server/src/nest/plugins/oauth/plugin-oauth.service.ts @@ -201,11 +201,16 @@ export class PluginOAuthService { // pinning the connection to the resolved IP, so a token_url that is a DNS name // (or IPv6 literal) pointing at metadata can't reach it and can't DNS-rebind. // Loopback/LAN stay reachable so a self-hosted internal IdP keeps working. + // maxRedirects 0, same as the OIDC twin: following one would hand + // client_secret to a second host, and a token endpoint has no legitimate + // reason to redirect. The timeout is not optional either — without it a + // hanging provider pins the request handler open indefinitely. const resp = await safeFetchAdminConfigured(cfg.tokenUrl, { method: 'POST', headers: { 'content-type': 'application/x-www-form-urlencoded', accept: 'application/json' }, body: body.toString(), - }); + signal: AbortSignal.timeout(15000), + }, 0); if (!resp.ok) throw new Error(`token endpoint returned ${resp.status}`); const json = (await resp.json()) as Record; return { diff --git a/server/src/utils/ssrfGuard.ts b/server/src/utils/ssrfGuard.ts index a29199e1a..35fde52a4 100644 --- a/server/src/utils/ssrfGuard.ts +++ b/server/src/utils/ssrfGuard.ts @@ -190,6 +190,7 @@ export async function safeFetchAdminConfigured( responseTimeoutMs?: number, ): Promise { let currentUrl = url; + let hopInit = init; for (let hop = 0; ; hop++) { let parsed: URL; @@ -213,7 +214,7 @@ export async function safeFetchAdminConfigured( } const dispatcher = createPinnedDispatcher(resolvedIp, true, responseTimeoutMs); - const response = await fetch(currentUrl, { ...init, redirect: 'manual', dispatcher } as any); + const response = await fetch(currentUrl, { ...hopInit, redirect: 'manual', dispatcher } as any); // Only a 3xx WITH a Location header is a redirect we follow; anything else // (2xx/4xx/5xx, or a 3xx with no Location) is the final response. @@ -234,6 +235,7 @@ export async function safeFetchAdminConfigured( // Drain the redirect body so the connection can be reused/closed, then loop // to re-resolve + re-check + re-pin the next hop. void response.body?.cancel().catch(() => {}); + hopInit = nextHopInit(hopInit, currentUrl, nextUrl, status); currentUrl = nextUrl; } } @@ -275,19 +277,93 @@ export interface SafeFetchOptions { * Pass `{ rejectUnauthorized: false }` for targets that use self-signed TLS * certificates (e.g. a Synology NAS on a local network). The SSRF guard still * applies — only the TLS certificate check is relaxed. + * + * Redirects are followed through safeFetchFollow, so every hop is re-checked and + * re-pinned. It used to hand the platform a `redirect: 'follow'` with a + * dispatcher pinned to the FIRST hop only — the same shape as + * GHSA-8mw6-xphx-886m, and pinning does not help there because Node skips the + * pinned lookup for an IP-literal host. Sixteen callers ride on this, several + * with a URL out of a per-user setting (Immich, Synology, AirTrail), and one + * (pipeAsset) streams the response back to the caller, which would have made a + * redirect to an internal service readable rather than blind. */ export async function safeFetch(url: string, init?: RequestInit, options?: SafeFetchOptions): Promise { - const ssrf = await checkSsrf(url); - if (!ssrf.allowed) { - throw new SsrfBlockedError(ssrf.error ?? 'Request blocked by SSRF guard'); + return safeFetchFollow(url, init, options); +} + +/** + * Headers that must not survive a hop to another origin. Following a redirect + * by hand means the platform's own protection does not apply: undici drops + * `Authorization` across origins itself (Fetch, "HTTP-redirect fetch" step 13), + * so a manual follower that replays the caller's headers verbatim is strictly + * weaker than the fetch it replaced. The list is credentials-only on purpose — + * `User-Agent` has to survive, or the goo.gl chain in maps.service resolves to + * a different page than the one its coordinates are parsed out of. + */ +const CREDENTIAL_HEADERS = [ + 'authorization', 'proxy-authorization', 'cookie', 'cookie2', + 'x-api-key', 'api-key', 'x-auth-token', +]; + +/** Headers that describe the body, and go when the body does. */ +const BODY_HEADERS = ['content-type', 'content-length', 'content-encoding', 'content-language', 'content-location']; + +/** + * Cross-origin per Fetch, with one carve-out: the same hostname moving from + * http to https is an upgrade, not a host change. A self-hosted IdP or ntfy + * behind a redirecting proxy does exactly that, and a strict origin compare + * would strip the credential those setups depend on. + */ +function isCrossOriginHop(from: string, to: string): boolean { + let a: URL, b: URL; + try { a = new URL(from); b = new URL(to); } catch { return true; } + if (a.origin === b.origin) return false; + return !(a.hostname === b.hostname && a.protocol === 'http:' && b.protocol === 'https:'); +} + +function stripHeaders(init: RequestInit | undefined, names: string[]): RequestInit | undefined { + if (!init?.headers) return init; + const headers = new Headers(init.headers as ConstructorParameters[0]); + for (const name of names) headers.delete(name); + return { ...init, headers }; +} + +/** + * The init for the next hop of a manual redirect follow. Both followers below + * go through this: following by hand opts out of the platform's own rules, so + * they have to be restated once rather than forgotten twice. + */ +function nextHopInit( + init: RequestInit | undefined, + currentUrl: string, + nextUrl: string, + status: number, + keepCredentials = false, +): RequestInit | undefined { + let next = init; + if (!keepCredentials && isCrossOriginHop(currentUrl, nextUrl)) { + next = stripHeaders(next, CREDENTIAL_HEADERS); } - const dispatcher = createPinnedDispatcher(ssrf.resolvedIp!, options?.rejectUnauthorized ?? true); - return fetch(url, { ...init, dispatcher } as any); + // RFC 9110 15.4.4 for 303, and 15.4.2/15.4.3 plus what every client actually + // does for 301/302 on a POST: the next hop is a GET with no body. Replaying + // the body would re-POST it — a plugin's client_secret included — at a host + // the first one merely pointed at. + const method = (next?.method ?? 'GET').toUpperCase(); + if (status === 303 || ((status === 301 || status === 302) && method !== 'GET' && method !== 'HEAD')) { + next = { ...stripHeaders(next, BODY_HEADERS), method: 'GET', body: undefined }; + } + return next; } export interface SafeFetchFollowOptions extends SafeFetchOptions { /** Maximum number of redirects to follow before giving up. Defaults to 5. */ maxRedirects?: number; + /** + * Keep credential headers across an origin change. Off by default, and no + * caller needs it today — it exists so a future one that genuinely does has + * to say so here rather than route around the guard. + */ + keepCredentialsOnRedirect?: boolean; /** * When true, private/internal IPs that ALLOW_INTERNAL_NETWORK would normally * permit are still blocked (matches `checkSsrf(url, true)`). Loopback and @@ -323,6 +399,7 @@ export async function safeFetchFollow( const bypassInternalIpAllowed = options?.bypassInternalIpAllowed ?? false; let currentUrl = url; + let hopInit = init; for (let hop = 0; ; hop++) { const ssrf = await checkSsrf(currentUrl, bypassInternalIpAllowed); @@ -332,7 +409,7 @@ export async function safeFetchFollow( const dispatcher = createPinnedDispatcher(ssrf.resolvedIp!, rejectUnauthorized); const response = await fetch(currentUrl, { - ...init, + ...hopInit, redirect: 'manual', dispatcher, } as any); @@ -360,6 +437,7 @@ export async function safeFetchFollow( throw new SsrfBlockedError('Invalid redirect location'); } void response.body?.cancel().catch(() => {}); + hopInit = nextHopInit(hopInit, currentUrl, nextUrl, status, options?.keepCredentialsOnRedirect); currentUrl = nextUrl; } } diff --git a/server/tests/unit/nest/channel-override.test.ts b/server/tests/unit/nest/channel-override.test.ts index 5d180bbee..c4aad493d 100644 --- a/server/tests/unit/nest/channel-override.test.ts +++ b/server/tests/unit/nest/channel-override.test.ts @@ -21,7 +21,29 @@ const { sendMailMock } = vi.hoisted(() => ({ sendMailMock: vi.fn().mockResolvedV vi.mock('nodemailer', () => ({ default: { createTransport: vi.fn(() => ({ sendMail: sendMailMock, verify: vi.fn() })) } })); vi.stubGlobal('fetch', vi.fn()); vi.mock('../../../src/websocket', () => ({ broadcast: vi.fn(), broadcastToUser: vi.fn() })); -vi.mock('../../../src/utils/ssrfGuard', () => ({ checkSsrf: vi.fn(async () => ({ allowed: true, resolvedIp: '1.2.3.4' })), createPinnedDispatcher: vi.fn(() => ({})) })); +vi.mock('../../../src/utils/ssrfGuard', () => { + class SsrfBlockedError extends Error { + constructor(message: string) { + super(message); + this.name = 'SsrfBlockedError'; + } + } + const checkSsrf = vi.fn(async (_url: string) => ({ allowed: true, resolvedIp: '1.2.3.4' })); + return { + checkSsrf, + createPinnedDispatcher: vi.fn(() => ({})), + SsrfBlockedError, + // The notification transports go through safeFetchFollow now, so the fake + // has to guard and then hand over to the stubbed fetch the way it does. + safeFetchFollow: vi.fn(async (url: string, init?: RequestInit) => { + const verdict = await checkSsrf(url); + if (!verdict.allowed) { + throw new SsrfBlockedError((verdict as { error?: string }).error ?? 'Request blocked by SSRF guard'); + } + return fetch(url, init); + }), + }; +}); import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; diff --git a/server/tests/unit/nest/collab.service.test.ts b/server/tests/unit/nest/collab.service.test.ts index e36a51a6c..a3f6a7107 100644 --- a/server/tests/unit/nest/collab.service.test.ts +++ b/server/tests/unit/nest/collab.service.test.ts @@ -52,10 +52,28 @@ const { mockCheckSsrf, mockCreatePinnedDispatcher } = vi.hoisted(() => ({ ), mockCreatePinnedDispatcher: vi.fn(() => ({})), })); -vi.mock('../../../src/utils/ssrfGuard', () => ({ - checkSsrf: mockCheckSsrf, - createPinnedDispatcher: mockCreatePinnedDispatcher, -})); +vi.mock('../../../src/utils/ssrfGuard', () => { + class SsrfBlockedError extends Error { + constructor(message: string) { + super(message); + this.name = 'SsrfBlockedError'; + } + } + return { + checkSsrf: mockCheckSsrf, + createPinnedDispatcher: mockCreatePinnedDispatcher, + SsrfBlockedError, + // The notification transports go through safeFetchFollow now, so the fake + // has to guard and then hand over to the stubbed fetch the way it does. + safeFetchFollow: vi.fn(async (url: string, init?: RequestInit) => { + const verdict = await (mockCheckSsrf)(url); + if (!verdict.allowed) { + throw new SsrfBlockedError((verdict as { error?: string }).error ?? 'Request blocked by SSRF guard'); + } + return fetch(url, init); + }), + }; +}); import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; diff --git a/server/tests/unit/nest/notification-transports.test.ts b/server/tests/unit/nest/notification-transports.test.ts index ca08abb14..6a48ac339 100644 --- a/server/tests/unit/nest/notification-transports.test.ts +++ b/server/tests/unit/nest/notification-transports.test.ts @@ -18,16 +18,37 @@ vi.mock('../../../src/nest/audit/audit-log.logger', () => ({ vi.mock('nodemailer', () => ({ default: { createTransport: vi.fn(() => ({ sendMail: vi.fn() })) } })); vi.stubGlobal('fetch', vi.fn()); -// ssrfGuard is mocked per-test in the SSRF describe block; default passes all -vi.mock('../../../src/utils/ssrfGuard', () => ({ - checkSsrf: vi.fn(async () => ({ allowed: true, isPrivate: false, resolvedIp: '1.2.3.4' })), - createPinnedDispatcher: vi.fn(() => ({})), -})); +// ssrfGuard is mocked per-test in the SSRF describe block; default passes all. +// safeFetchFollow is modelled rather than stubbed away: the transports call it +// instead of fetch now, and the assertions below still speak in terms of the +// stubbed global fetch, so the fake has to guard first and then hand the request +// over exactly like the real one does. +vi.mock('../../../src/utils/ssrfGuard', () => { + class SsrfBlockedError extends Error { + constructor(message: string) { + super(message); + this.name = 'SsrfBlockedError'; + } + } + const checkSsrf = vi.fn(async (_url: string) => ({ allowed: true, isPrivate: false, resolvedIp: '1.2.3.4' })); + return { + checkSsrf, + createPinnedDispatcher: vi.fn(() => ({})), + SsrfBlockedError, + safeFetchFollow: vi.fn(async (url: string, init?: RequestInit) => { + const verdict = await checkSsrf(url); + if (!verdict.allowed) { + throw new SsrfBlockedError((verdict as { error?: string }).error ?? 'Request blocked by SSRF guard'); + } + return fetch(url, init); + }), + }; +}); import { DatabaseService } from '../../../src/nest/database/database.service'; import { getEventText, buildEmailHtml } from '../../../src/nest/notifications/mailer/email-html'; import { WebhookService, buildWebhookBody } from '../../../src/nest/notifications/transports/webhook.service'; -import { NtfyService, resolveNtfyUrl, resolveAdminNtfyUrl, type NtfyConfig } from '../../../src/nest/notifications/transports/ntfy.service'; +import { NtfyService, resolveNtfyUrl, resolveAdminNtfyUrl, resolveNtfyToken, type NtfyConfig } from '../../../src/nest/notifications/transports/ntfy.service'; // The transports are providers now; the db stub below is the same one the // module-level `db` mock used to supply, handed in instead of imported. @@ -333,6 +354,49 @@ afterAll(() => vi.unstubAllGlobals()); // ── resolveNtfyUrl ──────────────────────────────────────────────────────────── +// GHSA-7pqc-fj3c-9346 — this is the live send path, reachable with no test route +// involved at all: anyone can save their own ntfy_server, and the token used to +// fall back to the operator's whatever host that named. +describe('resolveNtfyToken', () => { + const adminCfg: NtfyConfig = { server: 'https://ntfy.operator.example', topic: 'ops', token: 'operator-token' }; + + it('withholds the operator token when the user points at their own server', () => { + const user: NtfyConfig = { server: 'https://listener.attacker.example', topic: 't', token: null }; + expect(resolveNtfyToken(adminCfg, user)).toBeNull(); + }); + + it('sends it when the user rides the operator server, which is the shared setup', () => { + const user: NtfyConfig = { server: null, topic: 't', token: null }; + expect(resolveNtfyToken(adminCfg, user)).toBe('operator-token'); + }); + + it('matches on a trailing slash, which either side may carry', () => { + const user: NtfyConfig = { server: 'https://ntfy.operator.example/', topic: 't', token: null }; + expect(resolveNtfyToken(adminCfg, user)).toBe('operator-token'); + }); + + it('does not treat the http twin as the same server', () => { + const user: NtfyConfig = { server: 'http://ntfy.operator.example', topic: 't', token: null }; + expect(resolveNtfyToken(adminCfg, user)).toBeNull(); + }); + + it("the user's own token always wins, wherever it is going", () => { + const user: NtfyConfig = { server: 'https://elsewhere.example', topic: 't', token: 'mine' }; + expect(resolveNtfyToken(adminCfg, user)).toBe('mine'); + }); + + it('both sides unset means ntfy.sh against ntfy.sh, still a match', () => { + const noServer: NtfyConfig = { server: null, topic: 'ops', token: 'operator-token' }; + expect(resolveNtfyToken(noServer, { server: null, topic: 't', token: null })).toBe('operator-token'); + }); + + it('honours an explicit server override, which is what the test route passes', () => { + const user: NtfyConfig = { server: null, topic: 't', token: null }; + expect(resolveNtfyToken(adminCfg, user, 'https://listener.attacker.example')).toBeNull(); + expect(resolveNtfyToken(adminCfg, user, 'https://ntfy.operator.example')).toBe('operator-token'); + }); +}); + describe('resolveNtfyUrl', () => { const adminCfg: NtfyConfig = { server: 'https://ntfy.sh', topic: 'admin-topic', token: null }; diff --git a/server/tests/unit/nest/notifications.controller.test.ts b/server/tests/unit/nest/notifications.controller.test.ts index f037db0d2..c44dfacbb 100644 --- a/server/tests/unit/nest/notifications.controller.test.ts +++ b/server/tests/unit/nest/notifications.controller.test.ts @@ -102,6 +102,42 @@ describe('NotificationsController (parity with the legacy /api/notifications rou await makeController({ testNtfy, userNtfyConfig, adminNtfyConfig }).testNtfy(user, { token: MASKED }); expect(testNtfy).toHaveBeenCalledWith({ topic: 'saved-topic', server: 'https://ntfy.me', token: 'saved-token' }); }); + + // GHSA-7pqc-fj3c-9346 — the caller picks `server`, so falling back to the + // admin token unconditionally handed the operator's decrypted credential to + // any host an authenticated user named. + it("withholds the operator token when the request names a server that is not the operator's", async () => { + const testNtfy = vi.fn().mockResolvedValue({ success: true }); + const userNtfyConfig = vi.fn().mockReturnValue({ topic: 'mine', server: null, token: null }); + const adminNtfyConfig = vi.fn().mockReturnValue({ server: 'https://ntfy.operator.example', token: 'operator-token' }); + await makeController({ testNtfy, userNtfyConfig, adminNtfyConfig }).testNtfy(user, { server: 'https://listener.attacker.example' }); + expect(testNtfy).toHaveBeenCalledWith({ topic: 'mine', server: 'https://listener.attacker.example', token: null }); + }); + + it("still sends the operator token to the operator's own server", async () => { + const testNtfy = vi.fn().mockResolvedValue({ success: true }); + const userNtfyConfig = vi.fn().mockReturnValue({ topic: 'mine', server: null, token: null }); + const adminNtfyConfig = vi.fn().mockReturnValue({ server: 'https://ntfy.operator.example/', token: 'operator-token' }); + // Trailing slash on one side only: the shared setup, and it must still match. + await makeController({ testNtfy, userNtfyConfig, adminNtfyConfig }).testNtfy(user, { server: 'https://ntfy.operator.example' }); + expect(testNtfy).toHaveBeenCalledWith({ topic: 'mine', server: 'https://ntfy.operator.example', token: 'operator-token' }); + }); + + it('a plain http twin of the operator server does not pull the token', async () => { + const testNtfy = vi.fn().mockResolvedValue({ success: true }); + const userNtfyConfig = vi.fn().mockReturnValue({ topic: 'mine', server: null, token: null }); + const adminNtfyConfig = vi.fn().mockReturnValue({ server: 'https://ntfy.operator.example', token: 'operator-token' }); + await makeController({ testNtfy, userNtfyConfig, adminNtfyConfig }).testNtfy(user, { server: 'http://ntfy.operator.example' }); + expect(testNtfy).toHaveBeenCalledWith({ topic: 'mine', server: 'http://ntfy.operator.example', token: null }); + }); + + it('a user with their own token keeps using it anywhere', async () => { + const testNtfy = vi.fn().mockResolvedValue({ success: true }); + const userNtfyConfig = vi.fn().mockReturnValue({ topic: 'mine', server: null, token: 'my-own-token' }); + const adminNtfyConfig = vi.fn().mockReturnValue({ server: 'https://ntfy.operator.example', token: 'operator-token' }); + await makeController({ testNtfy, userNtfyConfig, adminNtfyConfig }).testNtfy(user, { server: 'https://somewhere.else.example' }); + expect(testNtfy).toHaveBeenCalledWith({ topic: 'mine', server: 'https://somewhere.else.example', token: 'my-own-token' }); + }); }); describe('in-app list + counts', () => { diff --git a/server/tests/unit/nest/notifications.service.test.ts b/server/tests/unit/nest/notifications.service.test.ts index 050a94cc2..afadb1f04 100644 --- a/server/tests/unit/nest/notifications.service.test.ts +++ b/server/tests/unit/nest/notifications.service.test.ts @@ -64,10 +64,29 @@ vi.stubGlobal('fetch', fetchMock); // RealtimeService imports both names from src/websocket, so the mock must // export both even though only broadcastToUser is asserted here. vi.mock('../../../src/websocket', () => ({ broadcast: vi.fn(), broadcastToUser: broadcastMock })); -vi.mock('../../../src/utils/ssrfGuard', () => ({ - checkSsrf: vi.fn(async () => ({ allowed: true, isPrivate: false, resolvedIp: '1.2.3.4' })), - createPinnedDispatcher: vi.fn(() => ({})), -})); +vi.mock('../../../src/utils/ssrfGuard', () => { + class SsrfBlockedError extends Error { + constructor(message: string) { + super(message); + this.name = 'SsrfBlockedError'; + } + } + const checkSsrf = vi.fn(async (_url: string) => ({ allowed: true, isPrivate: false, resolvedIp: '1.2.3.4' })); + return { + checkSsrf, + createPinnedDispatcher: vi.fn(() => ({})), + SsrfBlockedError, + // The notification transports go through safeFetchFollow now, so the fake + // has to guard and then hand over to the stubbed fetch the way it does. + safeFetchFollow: vi.fn(async (url: string, init?: RequestInit) => { + const verdict = await checkSsrf(url); + if (!verdict.allowed) { + throw new SsrfBlockedError((verdict as { error?: string }).error ?? 'Request blocked by SSRF guard'); + } + return fetch(url, init); + }), + }; +}); import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations';