fix(security): keep the operator's ntfy token in and re-check every redirect hop

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

@@ -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<Response> {
   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<string, string>) ?? {}) },
   });
 }

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();

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);

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(/(?<!\/)\/+$/, '');
+    try {
+      const parsed = new URL(raw);
+      return `${parsed.protocol}//${parsed.host}${parsed.pathname.replace(/\/+$/, '')}`.toLowerCase();
+    } catch {
+      return raw.toLowerCase();
+    }
+  };
+  return normalize(server) === normalize(adminServer);
+}
+
+/**
+ * The token to send with a per-user ntfy request. The user's own token always
+ * wins; the operator's is only reachable when the request is actually going to
+ * the operator's server. Shared by the test route and the live send path so the
+ * two cannot drift — closing only one of them closes nothing.
+ */
+export function resolveNtfyToken(
+  adminCfg: NtfyConfig,
+  userCfg: NtfyConfig | null,
+  serverOverride?: string | null,
+): string | null {
+  if (userCfg?.token) return userCfg.token;
+  const target = serverOverride ?? userCfg?.server ?? adminCfg.server;
+  return isOperatorNtfyServer(target, adminCfg.server) ? adminCfg.token : null;
+}
+
 /** Resolve the ntfy POST URL for admin-scoped sends. Returns null if no admin topic. */
 export function resolveAdminNtfyUrl(adminCfg: NtfyConfig): string | null {
   if (!adminCfg.topic) return null;
@@ -106,12 +146,6 @@ export class NtfyService {
   ): Promise<boolean> {
     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;
     }

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<boolean> {
     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;
     }

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<string, unknown>;
     return {

server/src/utils/ssrfGuard.ts

@@ -190,6 +190,7 @@ export async function safeFetchAdminConfigured(
   responseTimeoutMs?: number,
 ): Promise<Response> {
   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<Response> {
-  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<typeof Headers>[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;
   }
 }