mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(plugins): repair plain-HTTP egress and forward the private-egress opt-out
Two pre-existing bugs in the plugin egress guard, found by running a plugin against a real service end to end. 1. Every plain-HTTP request a plugin made was refused, whatever host it had declared. Node pre-normalises `net.connect()` args into an [options, cb] array and passes THAT array as the single argument; undici's plain-HTTP connector takes this path, its TLS connector does not. classifyConnect read `host` off the array, got undefined, and fell back to 'localhost' — so a fetch to a declared, public host was rejected with the nonsense message "localhost is not in the plugin's declared hosts". It failed closed, so it was never a security hole, and it went unnoticed because the only shipped egress plugin uses HTTPS. unwrapConnectArgs() unwraps the normalised form before anything reads host/path. 2. TREK_PLUGIN_ALLOW_PRIVATE_EGRESS could never have any effect. The guard that reads it runs INSIDE the child, whose env is scrubbed to a four-entry whitelist that never included it — so a documented setting (wiki/ Environment-Variables.md) was wired to nothing, and no plugin could reach a self-hoster's LAN service no matter what the operator set. Forwarded only when set, so the default stays the secure block-private policy. Regression tests cover the normalised form in both directions: the real host is now resolved, and an undeclared host, a private IP and a unix socket are all still refused when passed that way.
This commit is contained in:
@@ -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 };
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user