diff --git a/server/src/nest/plugins/runtime/egress-policy.ts b/server/src/nest/plugins/runtime/egress-policy.ts index d37ea4fc2..168c7056a 100644 --- a/server/src/nest/plugins/runtime/egress-policy.ts +++ b/server/src/nest/plugins/runtime/egress-policy.ts @@ -106,13 +106,29 @@ export function dgramConnectTarget(args: unknown[]): string | null { return typeof args[1] === 'string' ? (args[1] as string) : null; } +/** + * Node's `net.connect()` pre-normalises its arguments into an `[options, callback]` + * array and then calls `Socket.prototype.connect` with THAT ARRAY as the single + * argument. undici's plain-HTTP connector takes this path; its TLS connector does + * not — which is why HTTPS egress worked and plain HTTP silently did not. + * + * Unwrap it before anything reads `host`/`path`, or those come back undefined off an + * array, every plain-HTTP request a plugin makes is misclassified as `localhost`, and + * it is rejected with a nonsense "localhost is not in the plugin's declared hosts". + */ +export function unwrapConnectArgs(args: unknown[]): unknown[] { + return Array.isArray(args[0]) ? (args[0] as unknown[]) : args; +} + /** * Classify a `net.Socket.connect(...)` argument list into what we must check: * a unix-socket/pipe (local, allowed), a literal IP (checked synchronously), or * a hostname (allowlist + a DNS-resolving guard). Mirrors Node's connect - * overloads: connect(options[,cb]) | connect(port[,host][,cb]) | connect(path[,cb]). + * overloads: connect(options[,cb]) | connect(port[,host][,cb]) | connect(path[,cb]), + * plus the pre-normalised array form above. */ -export function classifyConnect(args: unknown[], isIP: (s: string) => boolean): ConnectTarget { +export function classifyConnect(rawArgs: unknown[], isIP: (s: string) => boolean): ConnectTarget { + const args = unwrapConnectArgs(rawArgs); const first = args[0]; if (first && typeof first === 'object') { const o = first as { host?: string; path?: string }; diff --git a/server/src/nest/plugins/runtime/plugin-host-entry.ts b/server/src/nest/plugins/runtime/plugin-host-entry.ts index 6bd7e592b..a60b82039 100644 --- a/server/src/nest/plugins/runtime/plugin-host-entry.ts +++ b/server/src/nest/plugins/runtime/plugin-host-entry.ts @@ -16,7 +16,7 @@ import dns from 'node:dns'; import dgram from 'node:dgram'; import { createRequire } from 'node:module'; import { createPluginContext, definePlugin, PLUGIN_API_VERSION, type ChildTransport, type PluginContext, type PluginDefinition } from './plugin-sdk'; -import { isBlockedIp, makeHostAllow, classifyConnect, dgramSendTarget, dgramConnectTarget } from './egress-policy'; +import { isBlockedIp, makeHostAllow, classifyConnect, unwrapConnectArgs, dgramSendTarget, dgramConnectTarget } from './egress-policy'; import type { Envelope, RpcError } from '../protocol/envelope'; const pluginId = process.argv[2] || process.env.TREK_PLUGIN_ID || 'unknown'; @@ -434,7 +434,11 @@ function installEgressGuard(egress: string[]): void { const proto = net.Socket.prototype as unknown as { connect: (...a: unknown[]) => unknown }; const realConnect = proto.connect; - proto.connect = function (this: unknown, ...args: unknown[]): unknown { + proto.connect = function (this: unknown, ...rawArgs: unknown[]): unknown { + // Node hands the pre-normalised [options, cb] array in as a single argument on the + // net.connect() path (undici uses it for plain HTTP). Work from the unwrapped list, + // or the rebuild below spreads an array into {0: options, 1: cb} and Node rejects it. + const args = unwrapConnectArgs(rawArgs); const target = classifyConnect(args, (s) => net.isIP(s) !== 0); if (target.kind === 'local') { // A unix-socket / named-pipe connect is a host-local pivot — a malicious plugin @@ -443,7 +447,7 @@ function installEgressGuard(egress: string[]): void { // Refuse it under the SAME policy: blocked by default, allowed only when the // operator explicitly opted into private egress. if (blockPrivate) throw new Error(`egress: connecting to a local socket/pipe (${target.host}) is not allowed`); - return realConnect.apply(this, args); + return realConnect.apply(this, rawArgs); } if (!allowed(target.host)) { throw new Error(`egress: ${target.host} is not in the plugin's declared hosts`); @@ -452,7 +456,7 @@ function installEgressGuard(egress: string[]): void { if (blockPrivate && isBlockedIp(target.host)) { throw new Error(`egress: ${target.host} is a blocked address`); } - return realConnect.apply(this, args); + return realConnect.apply(this, rawArgs); } // Hostname: inject the resolving guard. Preserve an existing lookup by // wrapping the args' options object. diff --git a/server/src/nest/plugins/supervisor/plugin-supervisor.ts b/server/src/nest/plugins/supervisor/plugin-supervisor.ts index ea4329d5f..ecc87642f 100644 --- a/server/src/nest/plugins/supervisor/plugin-supervisor.ts +++ b/server/src/nest/plugins/supervisor/plugin-supervisor.ts @@ -469,6 +469,14 @@ export class PluginSupervisor { TZ: process.env.TZ ?? '', PATH: process.env.PATH ?? '', TREK_PLUGIN_ID: sup.id, + // The egress guard that reads this runs INSIDE the child, and the child's env + // is scrubbed — so without forwarding it here the documented opt-out is dead + // code and a plugin can never reach a self-hoster's LAN service (a Gotify, an + // ntfy, an Ollama) no matter what the admin sets. Forwarded only when set, so + // the default stays the secure block-private policy. + ...(process.env.TREK_PLUGIN_ALLOW_PRIVATE_EGRESS + ? { TREK_PLUGIN_ALLOW_PRIVATE_EGRESS: process.env.TREK_PLUGIN_ALLOW_PRIVATE_EGRESS } + : {}), }, }); sup.child = child; diff --git a/server/tests/unit/plugins/egress-policy.test.ts b/server/tests/unit/plugins/egress-policy.test.ts index 4c67865f6..15db1c3a1 100644 --- a/server/tests/unit/plugins/egress-policy.test.ts +++ b/server/tests/unit/plugins/egress-policy.test.ts @@ -6,7 +6,7 @@ import { describe, it, expect } from 'vitest'; import net from 'node:net'; import { - isBlockedIp, makeHostAllow, classifyConnect, dgramSendTarget, dgramConnectTarget, + isBlockedIp, makeHostAllow, classifyConnect, unwrapConnectArgs, dgramSendTarget, dgramConnectTarget, } from '../../../src/nest/plugins/runtime/egress-policy'; const isIP = (s: string) => net.isIP(s) !== 0; @@ -116,3 +116,57 @@ describe('dgramConnectTarget', () => { expect(dgramConnectTarget([53, () => {}])).toBeNull(); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// Node's pre-normalised connect args (the plain-HTTP egress bug) +// ───────────────────────────────────────────────────────────────────────────── +// +// net.connect() normalises its arguments into an [options, callback] array and calls +// Socket.prototype.connect with THAT ARRAY as the single argument. undici's plain-HTTP +// connector goes through this path; its TLS connector does not. +// +// Before unwrapConnectArgs, `host` read as undefined off the array and classifyConnect +// fell back to 'localhost' — so EVERY plain-HTTP request a plugin made was misclassified +// and refused with "localhost is not in the plugin's declared hosts", no matter which +// host it had declared. It only escaped notice because the sole shipped egress plugin +// uses HTTPS. + +describe('unwrapConnectArgs (pre-normalised connect args)', () => { + // Exactly what undici's plain-HTTP connector produces. + const normalised = [[{ highWaterMark: 65536, localAddress: null, port: '18080', host: 'gotify.example.com' }, null]]; + + it('unwraps the [options, cb] array Node passes as a single argument', () => { + expect(unwrapConnectArgs(normalised)[0]).toEqual( + expect.objectContaining({ host: 'gotify.example.com', port: '18080' }), + ); + }); + + it('leaves a conventional argument list alone', () => { + expect(unwrapConnectArgs([{ host: 'a.example.com', port: 443 }])).toEqual([{ host: 'a.example.com', port: 443 }]); + expect(unwrapConnectArgs([443, 'a.example.com'])).toEqual([443, 'a.example.com']); + }); + + it('classifyConnect reads the REAL host out of it, not the localhost fallback', () => { + expect(classifyConnect(normalised, isIP)).toEqual({ kind: 'hostname', host: 'gotify.example.com' }); + }); + + it('a normalised literal-IP target is still classified as an IP (so the private-IP block applies)', () => { + expect(classifyConnect([[{ port: 80, host: '169.254.169.254' }, null]], isIP)).toEqual({ + kind: 'literal-ip', + host: '169.254.169.254', + }); + }); + + it('a normalised unix-socket target is still classified as local', () => { + expect(classifyConnect([[{ path: '/var/run/docker.sock' }, null]], isIP)).toEqual({ + kind: 'local', + host: '/var/run/docker.sock', + }); + }); + + it('an undeclared host in normalised form is still refused by the allow-list', () => { + const allow = makeHostAllow(['gotify.example.com']); + const target = classifyConnect([[{ port: 80, host: 'evil.example.com' }, null]], isIP); + expect(allow(target.host)).toBe(false); + }); +});