Skip to content

fix(phone-conversation): gate non-/twilio control endpoints to loopback only - #992

Merged
sonichi merged 2 commits into
sonichi:mainfrom
VasiliyRad:fix/phone-server-localhost-only-control
May 22, 2026
Merged

sonichi merged 2 commits into
sonichi:mainfrom
VasiliyRad:fix/phone-server-localhost-only-control

Conversation

@VasiliyRad

Copy link
Copy Markdown
Contributor

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:

Endpoint What an unauth'd LAN caller can do
POST /call Originate an outbound Twilio call from the owner's account (costs money, reveals caller-ID, harassment vector)
POST /concurrent-call Spam-dial multiple targets in parallel using the owner's account
POST /hangup Terminate any active owner call
POST /play-audio Stream /tmp/sutando-* audio during owner's active calls
POST /stop-audio Stop owner's audio playback mid-call
POST /meeting, /meeting-approve Start meeting-mode / upgrade to task-delegation

Concrete repro:

```bash
curl -X POST http://:3100/call
-d '{"to":"+1xxxxxxxxxx","message":"go"}'
```

Triggers an outbound call from the owner's Twilio account. No auth check, no rate limit, no logging of the caller.

Fix

New loopback_guard.ts exports isLoopback(req) — a one-line predicate against req.socket.remoteAddress covering IPv4 (127.0.0.1), IPv6 (::1), and IPv4-mapped IPv6 loopback (::ffff:127.0.0.1).

In the request handler, BEFORE the endpoint dispatch:

```ts
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.
  • Everything else is loopback-only.

Logs the rejected remote address to stderr for operator triage.

Alternative considered: switch bind from 0.0.0.0 to 127.0.0.1 and route ngrok separately. Rejected because it changes the server's deployment shape — would break installs where ngrok isn't the tunneling solution. The endpoint-level gate is cleaner and preserves the deployment surface.

Why this is a good-quality change

  • Maintainability: one predicate, one gate, before the dispatch table. Easy to audit. Matches the existing in-codebase pattern (_is_path_sendable for files; this is the network-layer equivalent).
  • Testability: isLoopback extracted to its own tiny module so unit tests don't have to load conversation-server.ts (which has heavy module-load side effects).
  • Robustness: closes a real unauth'd-RCE-shape attack on the owner's Twilio account.
  • Low risk: doesn't change the bind address. ngrok still works. Twilio webhooks still reach /twilio/* over the public tunnel. /health LAN-side probes still work. Legit owner-side callers always hit from loopback.

Tests

tests/phone-conversation-loopback-guard.test.ts — 14 cases:

10 unit tests of isLoopback:

Input Loopback?
127.0.0.1 ✅
::1 ✅
::ffff:127.0.0.1 (IPv4-mapped IPv6) ✅
192.168.1.42 (LAN v4) ❌
10.0.0.5 (private v4) ❌
1.1.1.1 (public v4) ❌
fe80::1 (link-local v6) ❌
fc00::1 (ULA v6) ❌
::ffff:192.168.1.42 (mapped non-loopback v4) ❌
"" / undefined ❌

4 source-grep architectural assertions on conversation-server.ts confirming the gate is wired in, returns 403, logs the rejected remote, and imports from the guard module.

All 14 pass locally. npm run typecheck clean.

Test plan

  • npx tsx --test tests/phone-conversation-loopback-guard.test.ts — 14/14 pass
  • Typecheck clean
  • Manual: from another machine on the LAN, curl -X POST http://<owner-host>:3100/call -d '{"to":"+1xxx","message":"x"}' — verify 403 + log. Then from localhost — verify normal handling.

🤖 Generated with Claude Code

…ck only

## 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://<host>: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 <noreply@anthropic.com>
@CLAassistant

CLAassistant commented May 21, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@sonichi

sonichi commented May 21, 2026

Copy link
Copy Markdown
Owner

LGTM. Most directly weaponizable LAN attack in the batch — unauth'd POST /call from any LAN device originates an outbound Twilio call billed to the server owner's account. Concrete repro from a phone on the same Wi-Fi.

Allowlist shape is correct: /health and /twilio/* prefix stay public (Twilio webhooks arrive via ngrok and are signature-validated by the existing handler); everything else requires loopback. Single chokepoint before endpoint dispatch — no per-endpoint forgetting risk.

isLoopback(req) correctly handles IPv4 127.0.0.1, IPv6 ::1, AND IPv4-mapped IPv6 ::ffff:127.0.0.1 (same vector class as #981 — without it, a request arriving via IPv6 loopback would falsely look non-loopback). Source is req.socket.remoteAddress, not header-derived (correct — headers are forgeable). Composes well with #980's path allowlist: defense in depth on the same surface.

@sonichi
sonichi merged commit 563f63c into sonichi:main May 22, 2026
2 checks passed
liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
…ck only (sonichi#992)

## 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://<host>: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 <noreply@anthropic.com>
Co-authored-by: Chi Wang <4250911+sonichi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants