From 5138358d7bf72c431209adaa0dff8b622e820bf4 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 1 Oct 2026 12:00:01 +0200 Subject: [PATCH] feat(net): the SSRF-guarded requests go through HTTP_PROXY / HTTPS_PROXY too Place photos, link previews, webhooks and the integrations fetch through the guard's own dispatcher and so ignored the outbound proxy, which left a server that can only reach the internet through one without them. They now read HTTP_PROXY, HTTPS_PROXY and NO_PROXY themselves and connect through the proxy, after the guard has vetted the target. The Helm chart passes the three variables through. (#1754) --- charts/trek/templates/configmap.yaml | 9 +++ charts/trek/values.yaml | 4 + server/src/app-config/derive.ts | 10 +++ server/src/nest/collab/collab.service.ts | 4 +- server/src/utils/ssrfGuard.ts | 57 ++++++++++++- server/tests/unit/nest/collab.service.test.ts | 4 +- .../tests/unit/utils/ssrf-llm-timeout.test.ts | 6 +- server/tests/unit/utils/ssrf-proxy.test.ts | 81 +++++++++++++++++++ wiki/Environment-Variables.md | 10 ++- 9 files changed, 171 insertions(+), 14 deletions(-) create mode 100644 server/tests/unit/utils/ssrf-proxy.test.ts diff --git a/charts/trek/templates/configmap.yaml b/charts/trek/templates/configmap.yaml index b00270838..2961ca1b5 100644 --- a/charts/trek/templates/configmap.yaml +++ b/charts/trek/templates/configmap.yaml @@ -70,6 +70,15 @@ data: {{- if .Values.env.OIDC_SCOPE }} OIDC_SCOPE: {{ .Values.env.OIDC_SCOPE | quote }} {{- end }} + {{- if .Values.env.HTTP_PROXY }} + HTTP_PROXY: {{ .Values.env.HTTP_PROXY | quote }} + {{- end }} + {{- if .Values.env.HTTPS_PROXY }} + HTTPS_PROXY: {{ .Values.env.HTTPS_PROXY | quote }} + {{- end }} + {{- if .Values.env.NO_PROXY }} + NO_PROXY: {{ .Values.env.NO_PROXY | quote }} + {{- end }} {{- if .Values.env.OIDC_DISCOVERY_URL }} OIDC_DISCOVERY_URL: {{ .Values.env.OIDC_DISCOVERY_URL | quote }} {{- end }} diff --git a/charts/trek/values.yaml b/charts/trek/values.yaml index 74a4d319a..1c32275c9 100644 --- a/charts/trek/values.yaml +++ b/charts/trek/values.yaml @@ -77,6 +77,10 @@ env: # Space-separated OIDC scopes to request. Must include scopes for any claim used by OIDC_ADMIN_CLAIM. # OIDC_DISCOVERY_URL: "" # Override the OIDC discovery endpoint for providers with non-standard paths (e.g. Authentik). + # HTTP_PROXY: "" + # HTTPS_PROXY: "" + # NO_PROXY: "localhost,127.0.0.1" + # Outbound proxy for TREK's requests to the internet; NO_PROXY lists hosts that bypass it. # DEMO_MODE: "false" # Enable demo mode (hourly data resets). # MCP_RATE_LIMIT: "300" diff --git a/server/src/app-config/derive.ts b/server/src/app-config/derive.ts index 7c9fdcf84..419a1ff3f 100644 --- a/server/src/app-config/derive.ts +++ b/server/src/app-config/derive.ts @@ -343,6 +343,16 @@ export function deriveNet(raw: RawEnv) { return { allowInternalNetwork: parseBool(raw.ALLOW_INTERNAL_NETWORK) === true, allowLinkLocalIps: parseLinkLocalAllowList(raw.ALLOW_LINK_LOCAL_IPS).ips, + // The variables Node's own env proxy reads, both spellings, so the guarded + // requests below follow the same proxy as everything else (#1754). + proxy: { + http: (raw.HTTP_PROXY ?? raw.http_proxy)?.trim() || undefined, + https: (raw.HTTPS_PROXY ?? raw.https_proxy)?.trim() || undefined, + noProxy: ((raw.NO_PROXY ?? raw.no_proxy) || '') + .split(',') + .map(entry => entry.trim().toLowerCase()) + .filter(Boolean), + }, }; } diff --git a/server/src/nest/collab/collab.service.ts b/server/src/nest/collab/collab.service.ts index 67aca9369..8acb6bb94 100644 --- a/server/src/nest/collab/collab.service.ts +++ b/server/src/nest/collab/collab.service.ts @@ -5,7 +5,7 @@ import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; import { avatarUrl } from '../common/avatarUrl'; -import { checkSsrf, createPinnedDispatcher } from '../../utils/ssrfGuard'; +import { checkSsrf, createOutboundDispatcher } from '../../utils/ssrfGuard'; import { discardBody, exceedsDeclaredLength, readCappedText } from '../../utils/cappedFetch'; import type { CollabNote, CollabPoll, CollabMessage, TripFile, User } from '../../types'; import { NotificationsService } from '../notifications/notifications.service'; @@ -662,7 +662,7 @@ export class CollabService { return { ...fallback, error: 'URL not allowed' }; } - const dispatcher = createPinnedDispatcher(ssrf.resolvedIp!); + const dispatcher = createOutboundDispatcher(url, ssrf.resolvedIp!); try { // AbortSignal.timeout covers the body as well. The hand-rolled controller // this replaces was cleared as soon as the headers arrived, so a server that diff --git a/server/src/utils/ssrfGuard.ts b/server/src/utils/ssrfGuard.ts index 7e257fceb..3f48b4001 100644 --- a/server/src/utils/ssrfGuard.ts +++ b/server/src/utils/ssrfGuard.ts @@ -1,5 +1,5 @@ import dns from 'node:dns/promises'; -import { Agent } from 'undici'; +import { Agent, ProxyAgent, type Dispatcher } from 'undici'; import { readEnv } from '../app-config'; import { embeddedTransitionIpv4, expandIpv6 } from './ipv6'; @@ -9,6 +9,8 @@ const ALLOW_INTERNAL_NETWORK = readEnv().net.allowInternalNetwork; // host gateway being the reason (#2400). The parser never lets a cloud metadata // address in, so the rest of 169.254.0.0/16 stays blocked. Frozen the same way. const ALLOWED_LINK_LOCAL = new Set(readEnv().net.allowLinkLocalIps); +// HTTP_PROXY / HTTPS_PROXY / NO_PROXY (#1754). Frozen like the two above. +const PROXY = readEnv().net.proxy; export interface SsrfResult { allowed: boolean; @@ -291,7 +293,7 @@ export async function safeFetchAdminConfigured( throw new SsrfBlockedError('Requests to link-local / cloud-metadata addresses are not allowed'); } - const dispatcher = createPinnedDispatcher(usableIps, true, responseTimeoutMs); + const dispatcher = createOutboundDispatcher(currentUrl, usableIps, true, responseTimeoutMs); 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 @@ -485,7 +487,7 @@ export async function safeFetchFollow( throw new SsrfBlockedError(ssrf.error ?? 'Request blocked by SSRF guard'); } - const dispatcher = createPinnedDispatcher(ssrf.resolvedIps ?? [ssrf.resolvedIp!], rejectUnauthorized); + const dispatcher = createOutboundDispatcher(currentUrl, ssrf.resolvedIps ?? [ssrf.resolvedIp!], rejectUnauthorized); const response = await fetch(currentUrl, { ...hopInit, redirect: 'manual', @@ -520,6 +522,55 @@ export async function safeFetchFollow( } } +/** + * The proxy a guarded request to this URL goes through, or null (#1754). + * + * The guarded fetches below use their own dispatcher, so they never saw the + * environment proxy Node applies to everything else, and on a server that can + * only reach the internet through a proxy every place photo, link preview and + * webhook simply failed. NO_PROXY works the way curl and Node read it: `*` + * matches every host, an entry matches the host itself and its subdomains + * (a leading dot is optional), and `host:port` narrows it to one port. + */ +export function proxyFor(rawUrl: string, proxy = PROXY): string | null { + let url: URL; + try { url = new URL(rawUrl); } catch { return null; } + const target = url.protocol === 'https:' ? proxy.https ?? proxy.http : proxy.http; + if (!target) return null; + const host = url.hostname.toLowerCase().replace(/^\[|\]$/g, ''); + const port = url.port || (url.protocol === 'https:' ? '443' : '80'); + const bypass = proxy.noProxy.some((entry) => { + if (entry === '*') return true; + const [name, entryPort] = entry.replace(/^\[|\](?=:|$)/g, '').split(/:(?=\d+$)/); + if (entryPort && entryPort !== port) return false; + const bare = name.replace(/^\*?\./, ''); + return host === bare || host.endsWith(`.${bare}`); + }); + return bypass ? null : target; +} + +/** + * The dispatcher one guarded hop goes out through. Without a proxy it is the + * pinned agent below. With one, the proxy makes the connection, so the address + * cannot be pinned on our side: the guard has already refused a private or + * loopback target before this point, and the proxy the operator chose is the + * network boundary from there on. + */ +export function createOutboundDispatcher( + url: string, + resolved: string | readonly string[], + rejectUnauthorized = true, + responseTimeoutMs?: number, +): Dispatcher { + const proxy = proxyFor(url); + if (!proxy) return createPinnedDispatcher(resolved, rejectUnauthorized, responseTimeoutMs); + return new ProxyAgent({ + uri: proxy, + requestTls: { rejectUnauthorized }, + ...(responseTimeoutMs ? { headersTimeout: responseTimeoutMs, bodyTimeout: responseTimeoutMs } : {}), + }); +} + /** * Returns an undici Agent whose connect.lookup is pinned to the already-validated * addresses. This prevents DNS rebinding (TOCTOU) by ensuring the outbound diff --git a/server/tests/unit/nest/collab.service.test.ts b/server/tests/unit/nest/collab.service.test.ts index a3f6a7107..b1d9066f0 100644 --- a/server/tests/unit/nest/collab.service.test.ts +++ b/server/tests/unit/nest/collab.service.test.ts @@ -61,7 +61,7 @@ vi.mock('../../../src/utils/ssrfGuard', () => { } return { checkSsrf: mockCheckSsrf, - createPinnedDispatcher: mockCreatePinnedDispatcher, + createOutboundDispatcher: 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. @@ -545,7 +545,7 @@ describe('linkPreview hardening', () => { // the pin does not cover that hop: Node skips the pinned lookup for a literal IP. expect(init.redirect).toBe('error'); expect(init.signal).toBeInstanceOf(AbortSignal); - expect(mockCreatePinnedDispatcher).toHaveBeenCalledWith('93.184.216.34'); + expect(mockCreatePinnedDispatcher).toHaveBeenCalledWith('https://example.com/init', '93.184.216.34'); expect(init.dispatcher).toBe(dispatcher); // One Agent is built per preview; leaving it open leaks its sockets. expect(dispatcher.close).toHaveBeenCalled(); diff --git a/server/tests/unit/utils/ssrf-llm-timeout.test.ts b/server/tests/unit/utils/ssrf-llm-timeout.test.ts index 00b7200b8..210c7fe54 100644 --- a/server/tests/unit/utils/ssrf-llm-timeout.test.ts +++ b/server/tests/unit/utils/ssrf-llm-timeout.test.ts @@ -24,7 +24,7 @@ vi.mock('dns/promises', () => ({ default: { lookup: vi.fn() }, lookup: vi.fn() } // ssrfGuard reads env at module load, so the mock must answer before the import. const { readEnvMock } = vi.hoisted(() => ({ - readEnvMock: vi.fn(() => ({ net: { allowInternalNetwork: true }, integrations: { llmTimeoutMs: 900_000 } })), + readEnvMock: vi.fn(() => ({ net: { allowInternalNetwork: true, proxy: { noProxy: [] } }, integrations: { llmTimeoutMs: 900_000 } })), })); vi.mock('../../../src/app-config', () => ({ readEnv: readEnvMock })); @@ -36,7 +36,7 @@ const mockLookup = vi.mocked(dns.lookup); beforeEach(() => { AgentMock.mockClear(); readEnvMock.mockClear(); - readEnvMock.mockReturnValue({ net: { allowInternalNetwork: true }, integrations: { llmTimeoutMs: 900_000 } }); + readEnvMock.mockReturnValue({ net: { allowInternalNetwork: true, proxy: { noProxy: [] } }, integrations: { llmTimeoutMs: 900_000 } }); mockLookup.mockResolvedValue({ address: '203.0.113.10', family: 4 }); }); @@ -82,7 +82,7 @@ describe('createPinnedDispatcher — response ceiling', () => { describe('the ceiling belongs to the model lane only', () => { it('safeFetchLlm carries the configured ceiling to the dispatcher', async () => { - readEnvMock.mockReturnValue({ net: { allowInternalNetwork: true }, integrations: { llmTimeoutMs: 120_000 } }); + readEnvMock.mockReturnValue({ net: { allowInternalNetwork: true, proxy: { noProxy: [] } }, integrations: { llmTimeoutMs: 120_000 } }); vi.stubGlobal('fetch', vi.fn().mockResolvedValue(response({ status: 200 }))); await safeFetchLlm('https://api.provider.example/v1/chat/completions'); diff --git a/server/tests/unit/utils/ssrf-proxy.test.ts b/server/tests/unit/utils/ssrf-proxy.test.ts new file mode 100644 index 000000000..1d6049ad1 --- /dev/null +++ b/server/tests/unit/utils/ssrf-proxy.test.ts @@ -0,0 +1,81 @@ +/** + * Guarded requests and the outbound proxy (#1754). The SSRF guard fetches through + * its own dispatcher, so it used to skip HTTP_PROXY entirely; these pin when it + * goes through the proxy and when NO_PROXY keeps it direct. + */ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const built = vi.hoisted(() => ({ agents: [] as unknown[], proxies: [] as Array> })); + +vi.mock('undici', () => ({ + Agent: class { constructor(opts: unknown) { built.agents.push(opts); } }, + ProxyAgent: class { constructor(opts: Record) { built.proxies.push(opts); } }, +})); + +const PROXY_KEYS = ['HTTP_PROXY', 'HTTPS_PROXY', 'NO_PROXY', 'http_proxy', 'https_proxy', 'no_proxy'] as const; + +async function loadGuard(env: Partial>) { + for (const key of PROXY_KEYS) delete process.env[key]; + Object.assign(process.env, env); + vi.resetModules(); + return import('../../../src/utils/ssrfGuard'); +} + +afterEach(() => { + for (const key of PROXY_KEYS) delete process.env[key]; + built.agents.length = 0; + built.proxies.length = 0; +}); + +describe('proxyFor', () => { + it('SSRF-PROXY-001: no proxy configured means a direct connection', async () => { + const { proxyFor } = await loadGuard({}); + expect(proxyFor('https://example.com/a')).toBeNull(); + }); + + it('SSRF-PROXY-002: picks the proxy by scheme, HTTPS falling back to HTTP_PROXY', async () => { + const { proxyFor } = await loadGuard({ HTTP_PROXY: 'http://plain:3128' }); + expect(proxyFor('http://example.com/')).toBe('http://plain:3128'); + expect(proxyFor('https://example.com/')).toBe('http://plain:3128'); + const both = await loadGuard({ HTTP_PROXY: 'http://plain:3128', https_proxy: 'http://secure:3129' }); + expect(both.proxyFor('https://example.com/')).toBe('http://secure:3129'); + }); + + it('SSRF-PROXY-003: NO_PROXY matches the host, its subdomains, a port and the wildcard', async () => { + const { proxyFor } = await loadGuard({ + HTTPS_PROXY: 'http://proxy:3128', + NO_PROXY: 'localhost, .internal.lan, photos.example.com:8443', + }); + expect(proxyFor('https://localhost/health')).toBeNull(); + expect(proxyFor('https://immich.internal.lan/api')).toBeNull(); + expect(proxyFor('https://internal.lan/')).toBeNull(); + expect(proxyFor('https://photos.example.com:8443/x')).toBeNull(); + // Same host on another port, and a lookalike domain, still go through the proxy. + expect(proxyFor('https://photos.example.com/x')).toBe('http://proxy:3128'); + expect(proxyFor('https://notinternal.lan/')).toBe('http://proxy:3128'); + + const all = await loadGuard({ HTTPS_PROXY: 'http://proxy:3128', NO_PROXY: '*' }); + expect(all.proxyFor('https://example.com/')).toBeNull(); + }); +}); + +describe('createOutboundDispatcher', () => { + it('SSRF-PROXY-004: without a proxy it is the pinned agent', async () => { + const { createOutboundDispatcher } = await loadGuard({}); + createOutboundDispatcher('https://example.com/', ['93.184.216.34']); + expect(built.agents).toHaveLength(1); + expect(built.proxies).toHaveLength(0); + }); + + it('SSRF-PROXY-005: with a proxy it goes through it, keeping TLS checks and the timeout', async () => { + const { createOutboundDispatcher } = await loadGuard({ HTTPS_PROXY: 'http://proxy:3128' }); + createOutboundDispatcher('https://example.com/', ['93.184.216.34'], false, 30_000); + expect(built.agents).toHaveLength(0); + expect(built.proxies[0]).toEqual({ + uri: 'http://proxy:3128', + requestTls: { rejectUnauthorized: false }, + headersTimeout: 30_000, + bodyTimeout: 30_000, + }); + }); +}); diff --git a/wiki/Environment-Variables.md b/wiki/Environment-Variables.md index 4bdf01f27..7442c1a73 100644 --- a/wiki/Environment-Variables.md +++ b/wiki/Environment-Variables.md @@ -145,13 +145,15 @@ proxying is disabled by default. | `HTTPS_PROXY` | Proxy URL for outbound HTTPS requests | — | | `NO_PROXY` | Comma-separated hosts or domains that should bypass the proxy | — | -> **Note:** Proxy environment variables apply to requests made through Node.js's default HTTP dispatcher. Requests -> handled by TREK's SSRF protection use a dedicated dispatcher and do not use the environment proxy. +> **Note:** Requests that go through TREK's SSRF protection (place photos, link previews, webhooks, Immich, Synology, +> Dawarich and the other integrations) read the same three variables themselves and use the proxy as well, on any +> install. Everything else follows Node's own environment proxy, which needs `NODE_USE_ENV_PROXY=1` (see below). +> `NO_PROXY` takes `*`, a host (which also matches its subdomains, a leading dot is optional) or `host:port`. > **Container only.** Node ignores these variables unless it is started with `NODE_USE_ENV_PROXY=1`, and the official > image sets that for you. On a source or Proxmox install, set `NODE_USE_ENV_PROXY=1` alongside them or nothing will -> change. On Helm the image already has it, but the chart's ConfigMap does not declare `HTTP_PROXY`, `HTTPS_PROXY` or -> `NO_PROXY`, so a value under `env:` is dropped; patch the three onto the Deployment instead. +> change. On Helm the image already has it, and the chart passes `HTTP_PROXY`, `HTTPS_PROXY` and `NO_PROXY` from +> `env:` through to the container. > **Set `NO_PROXY`.** Without it every request goes to the proxy, including the ones TREK makes to itself, such as the > container health check. `localhost,127.0.0.1` is a sensible minimum; add your own hosts as needed.