From 707773c2f784c657ca7a7f013e7d455298824f47 Mon Sep 17 00:00:00 2001 From: Vasiliy Radostev Date: Thu, 21 May 2026 12:50:19 -0700 Subject: [PATCH] fix(phone-conversation): gate non-/twilio control endpoints to loopback only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## The bug (security) `skills/phone-conversation/scripts/conversation-server.ts` binds to `0.0.0.0` to accommodate ngrok's local-tunnel client + the Twilio webhook path. That makes EVERY endpoint LAN-reachable. /twilio/* paths validate Twilio's signature; the rest are unauthenticated control endpoints: - POST /call - originate an outbound Twilio call - POST /concurrent-call - originate child call during a parent - POST /hangup - terminate an active call - POST /play-audio - stream audio through Twilio - POST /stop-audio - stop active playback - POST /meeting - start meeting-mode session - POST /meeting-approve - upgrade meeting to task-delegation Concrete attack: anyone on the LAN POSTs: curl -X POST http://:3100/call \ -d '{"to":"+1xxx","message":"go"}' Triggers an outbound Twilio call from the owner's account. This: - costs money (Twilio per-call charges), - reveals the owner's Twilio caller-ID, - can be abused for harassment / robocall through the owner's account, - combined with /concurrent-call, allows spam-dialing multiple targets in parallel. `/hangup` and `/stop-audio` let any LAN caller disrupt active owner calls. `/play-audio` lets them stream arbitrary `/tmp/sutando-*` audio during the owner's calls. ## Fix New module `loopback_guard.ts` exports `isLoopback(req)`, a one-line predicate against `req.socket.remoteAddress` covering IPv4 loopback (`127.0.0.1`), IPv6 loopback (`::1`), and the IPv4-mapped IPv6 form (`::ffff:127.0.0.1`) that some Node builds report on dual-stack sockets. In the request handler, BEFORE the endpoint dispatch: if (path !== '/health' && !path.startsWith('/twilio/') && !isLoopback(req)) { json(res, 403, { error: 'control endpoints are loopback-only' }); return; } - /twilio/* exempt: validated downstream by TWILIO_AUTH_TOKEN HMAC signature check. - /health exempt: read-only liveness probe useful for LAN-side monitoring. - Everything else is loopback-only. Logs the rejected remote address to stderr for operator triage. ## Why this is the right fix - Minimal blast radius: one predicate, one gate, before the dispatch table. Easy to audit. - Matches existing in-codebase pattern (`_is_path_sendable` realpath-collapse for files; this is the network-layer equivalent). - Defends against the IPv4-mapped IPv6 bypass class (same shape as `_is_safe_callback_url` SSRF fix). - Doesn't change the bind address — ngrok still works, Twilio webhooks still reach /twilio/* over the public tunnel, /health probes still work LAN-side. Alternative considered: switch bind from 0.0.0.0 to 127.0.0.1 and route ngrok separately. Rejected because it changes the deployment shape and would break installs where ngrok isn't the tunneling solution. The endpoint-level gate is cleaner. ## Tests `tests/phone-conversation-loopback-guard.test.ts` — 14 cases: 10 direct unit tests of `isLoopback` against various `remoteAddress` values (loopback v4/v6/mapped-v6 accepted; LAN v4, public v4, link-local v6, ULA v6, IPv4-mapped IPv6 to NON-loopback, empty/undefined all rejected). 4 source-grep architectural assertions on conversation-server.ts: - imports isLoopback from the guard module - gates non-/twilio non-/health endpoints with the predicate - returns 403 (not 404 or silent) on the rejection path - logs the rejected remote address for incident triage All 14 pass locally. `npm run typecheck` clean. Co-Authored-By: Vasiliy's Sutando --- .../scripts/conversation-server.ts | 23 +++++ .../scripts/loopback_guard.ts | 31 +++++++ .../phone-conversation-loopback-guard.test.ts | 92 +++++++++++++++++++ 3 files changed, 146 insertions(+) create mode 100644 skills/phone-conversation/scripts/loopback_guard.ts create mode 100644 tests/phone-conversation-loopback-guard.test.ts diff --git a/skills/phone-conversation/scripts/conversation-server.ts b/skills/phone-conversation/scripts/conversation-server.ts index 1a144eac8e..ebd23ca938 100644 --- a/skills/phone-conversation/scripts/conversation-server.ts +++ b/skills/phone-conversation/scripts/conversation-server.ts @@ -1163,6 +1163,20 @@ async function waitForWebhook(): Promise { // and a WebSocket endpoint for real-time audio streaming (Media Streams). // Claude also calls these endpoints to trigger actions (/call, /concurrent-call, /hangup). +// Control endpoints (anything not under /twilio/*) drive owner-account +// actions: originate outbound calls (/call), hang up active calls +// (/hangup), play audio through Twilio (/play-audio), etc. The server +// binds to 0.0.0.0 because ngrok's local-tunnel client + the Twilio +// webhook path need it (see server.listen at the bottom). That means +// every endpoint is LAN-reachable. /twilio/* paths validate Twilio's +// signature; the rest had no auth at all — any caller on the LAN could +// POST /call with an arbitrary `to` and trigger Twilio calls on the +// owner's account (cost + caller-ID exposure + harassment vector). +// `isLoopback` (see loopback_guard.ts) gates non-/twilio control +// endpoints to loopback only; LAN callers get 403. /health is exempt +// — it's read-only and useful for LAN-side liveness checks. +import { isLoopback } from './loopback_guard.js'; + const server = createServer(async (req, res) => { const url = new URL(req.url ?? '', `http://localhost:${PORT}`); const path = url.pathname; @@ -1172,6 +1186,15 @@ const server = createServer(async (req, res) => { res.end(); return; } + // Reject non-Twilio endpoints from non-loopback callers (see comment + // on `isLoopback` above). + if (path !== '/health' && !path.startsWith('/twilio/') && !isLoopback(req)) { + const remote = req.socket.remoteAddress ?? '?'; + console.log(`${ts()} [PhoneServer] REJECTED non-loopback ${req.method} ${path} from ${remote}`); + json(res, 403, { error: 'control endpoints are loopback-only' }); + return; + } + try { if (path === '/health' && req.method === 'GET') { json(res, 200, { status: 'ok', activeCalls: activeCalls.size, webhookUrl: WEBHOOK_BASE_URL }); diff --git a/skills/phone-conversation/scripts/loopback_guard.ts b/skills/phone-conversation/scripts/loopback_guard.ts new file mode 100644 index 0000000000..dff21e0b28 --- /dev/null +++ b/skills/phone-conversation/scripts/loopback_guard.ts @@ -0,0 +1,31 @@ +// Loopback-only access guard for the phone-conversation HTTP server. +// +// The server binds to `0.0.0.0` to accommodate ngrok's local-tunnel +// client + the Twilio webhook path. That makes every endpoint LAN- +// reachable. /twilio/* paths validate Twilio's signature; the rest are +// internal control endpoints (originate calls, hang up, play audio, +// etc.) that should never be exposed to the LAN. +// +// `isLoopback(req)` is the one-line predicate used by the request +// handler to gate non-/twilio control endpoints. Extracted to its own +// module so we can unit-test the predicate without loading +// conversation-server.ts (which has heavy module-load side effects: +// Gemini SDK init, Twilio client, env validation that `exit(1)`s when +// credentials are missing). + +import type { IncomingMessage } from 'node:http'; + +/** + * True iff the request originated from a loopback interface — IPv4 + * `127.0.0.1`, IPv6 `::1`, or the IPv4-mapped-IPv6 form + * `::ffff:127.0.0.1` that some Node builds report when listening on + * IPv4 via a dual-stack socket. + * + * Defensive against the same bypass class as `_is_safe_callback_url` + * in `src/agent-api.py`: a hostname/address that *resolves* to + * loopback via IPv4-mapping must be treated as loopback. + */ +export function isLoopback(req: IncomingMessage): boolean { + const addr = req.socket.remoteAddress ?? ''; + return addr === '127.0.0.1' || addr === '::1' || addr === '::ffff:127.0.0.1'; +} diff --git a/tests/phone-conversation-loopback-guard.test.ts b/tests/phone-conversation-loopback-guard.test.ts new file mode 100644 index 0000000000..716d6b30d7 --- /dev/null +++ b/tests/phone-conversation-loopback-guard.test.ts @@ -0,0 +1,92 @@ +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import type { IncomingMessage } from 'node:http'; +import { isLoopback } from '../skills/phone-conversation/scripts/loopback_guard.js'; + +// Security-relevant regression guard for the loopback-only gate on the +// phone-conversation server's control endpoints. +// +// Pre-fix: every endpoint (incl. `/call`, `/hangup`, `/concurrent-call`, +// `/play-audio`, etc.) was LAN-reachable because the server binds to +// `0.0.0.0` for ngrok. /twilio/* paths validate Twilio's signature; +// everything else was unauthenticated. Worst case: anyone on the LAN +// could POST `/call` with an arbitrary phone number and originate a +// Twilio call on the owner's account. + +function fakeReq(remoteAddress: string | undefined): IncomingMessage { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return { socket: { remoteAddress } } as any; +} + +describe('loopback_guard.isLoopback', () => { + it('accepts IPv4 loopback (127.0.0.1)', () => { + assert.equal(isLoopback(fakeReq('127.0.0.1')), true); + }); + it('accepts IPv6 loopback (::1)', () => { + assert.equal(isLoopback(fakeReq('::1')), true); + }); + it('accepts IPv4-mapped IPv6 loopback (::ffff:127.0.0.1)', () => { + assert.equal(isLoopback(fakeReq('::ffff:127.0.0.1')), true); + }); + it('rejects LAN IPv4 (192.168.x.x)', () => { + assert.equal(isLoopback(fakeReq('192.168.1.42')), false); + }); + it('rejects another LAN IPv4 (10.x.x.x)', () => { + assert.equal(isLoopback(fakeReq('10.0.0.5')), false); + }); + it('rejects public IPv4', () => { + assert.equal(isLoopback(fakeReq('1.1.1.1')), false); + }); + it('rejects link-local IPv6 (fe80::)', () => { + assert.equal(isLoopback(fakeReq('fe80::1')), false); + }); + it('rejects ULA IPv6 (fc00::)', () => { + assert.equal(isLoopback(fakeReq('fc00::1')), false); + }); + it('rejects IPv4-mapped IPv6 to a NON-loopback address', () => { + // Defensive: the SSRF-bypass shape (private IPv4 mapped into + // IPv6) must NOT be treated as loopback unless it actually maps + // to 127.0.0.1. + assert.equal(isLoopback(fakeReq('::ffff:192.168.1.42')), false); + assert.equal(isLoopback(fakeReq('::ffff:10.0.0.5')), false); + }); + it('rejects empty / undefined remoteAddress', () => { + assert.equal(isLoopback(fakeReq('')), false); + assert.equal(isLoopback(fakeReq(undefined)), false); + }); +}); + +describe('conversation-server.ts wiring', () => { + const SRC = readFileSync( + join(import.meta.dirname ?? '.', '..', 'skills/phone-conversation/scripts/conversation-server.ts'), + 'utf-8', + ); + + it('imports isLoopback from the guard module', () => { + assert.match(SRC, /from\s+['"]\.\/loopback_guard(?:\.js)?['"]/, ( + 'conversation-server.ts must import isLoopback from ./loopback_guard' + )); + }); + + it('gates non-/twilio non-/health endpoints with isLoopback', () => { + assert.match( + SRC, + /path\s*!==\s*['"]\/health['"][\s\S]{0,80}!\s*path\.startsWith\(['"]\/twilio\/['"]\)[\s\S]{0,80}!isLoopback\(req\)/, + 'conversation-server.ts must check `path !== "/health" && !path.startsWith("/twilio/") && !isLoopback(req)` before dispatching to control endpoints', + ); + }); + + it('returns 403 (not 200) on the rejection path', () => { + assert.match( + SRC, + /json\(res,\s*403,\s*\{\s*error:\s*['"]control endpoints are loopback-only['"]/, + 'conversation-server.ts must respond 403 with the documented error string', + ); + }); + + it('logs the rejected remote address for incident triage', () => { + assert.match(SRC, /REJECTED non-loopback/); + }); +});