[VasiliyRad]

The bug (security)

skills/phone-conversation/scripts/conversation-server.ts exposes a POST /play-audio endpoint that does:

```ts
if (!body.path) { json(res, 400, ...); return; }
if (!existsSync(body.path)) { json(res, 404, ...); return; }
// ... spawn ffmpeg with body.path as -i
const ffmpegProc = spawn('ffmpeg', [...seekArgs, '-re', '-i', body.path, ...]);
ffmpegProc.stdout.on('data', (pcmBuf) => {
// mu-law encode + stream over Twilio WebSocket to active phone call
});
```

Two structural problems compound:

1. No authentication on the endpoint. The phone server binds to 0.0.0.0 (line 1720) to accommodate ngrok's local-tunnel client → any caller on the LAN can POST /play-audio.
2. No path allowlist on body.path — only existsSync(). The attacker chooses any local file the server's user can read; ffmpeg opens it; its audio (if any decodable track) streams over Twilio to whoever happens to be on the active phone call with the owner.

Concretely: if the owner is on a phone call with someone, anyone on the LAN can POST {"path": "/path/to/owner-private-recording.mov"} and the unrelated call participant suddenly hears the recording.

Why this is the right fix

Adopt the established _is_path_sendable pattern used in discord-bridge.py and telegram-bridge.py for [file: /path] marker delivery: an explicit allowlist of path prefixes + a realpath collapse to defeat symlink / .. escape.

New file skills/phone-conversation/scripts/audio_path_guard.ts owns the allowlist and helper:

```ts
export const AUDIO_ALLOWED_PREFIXES = [
'/tmp/sutando-',
'/private/tmp/sutando-',
];

export function isAllowedAudioPath(fpath: string): boolean {
// existsSync → realpathSync → prefix check
}
```

Matches the recording skill's /tmp/sutando-recording-*.mov convention used by src/recording-tools.ts:527 and :1677 (the legitimate callers).

The /play-audio handler replaces its existsSync() precheck with isAllowedAudioPath() and returns 403 (not 404) on rejected paths so the operator log distinguishes "missing file" from "policy violation."

Why this is a good-quality change

• Maintainability: allowlist + helper live in ONE file. Future legitimate sources (e.g. workspace-stored call recordings) are added by editing one constant. Mirrors the discord/telegram pattern.
• Testability: tests/phone-conversation-audio-path-guard.test.ts exercises the helper directly — 8 cases covering: allowed case, /etc/hosts as canonical disallowed file, prefix mismatch, non-existent file, null/undefined/non-string input, symlink-escape attempt (the precise reason for the realpath collapse), .. traversal, and a documented-set assertion that fails on silent allowlist widening.
• Robustness: closes the LAN-reachable file-exfil-via-audio primitive. Even with the 0.0.0.0 binding kept (ngrok dependency), the endpoint is now policy-gated.
• Low risk: the legitimate caller (src/recording-tools.ts) passes /tmp/sutando-recording-* paths — all under the allowlist. The 2nd spawn site in conversation-server.ts (the QuickTime-resume re-stream, line ~1677) uses a path derived from an internal ls -t /tmp/sutando-recording-*.mov shell glob — also under the allowlist by construction (NOT user-controlled), so no additional gate needed there.

Round totals

18 PRs, 14 real bugs (1 new security finding), 3 design fixes, ~133 tests, density ~1:9.5.

Test plan

• npx tsx --test tests/phone-conversation-audio-path-guard.test.ts — 8/8 pass
• npm run typecheck — clean
• Existing tests still pass (no regression to legit caller path)
• Manual: with phone server running, POST /play-audio with {"path": "/etc/hosts"} — verify 403 + no ffmpeg spawn. Then POST with a real /tmp/sutando-recording-*.mov during an active call — verify normal playback.

🤖 Generated with Claude Code

[bassilkhilo-ag2]

Cold review (proactive-loop pass)

Vulnerability confirmed, fix is correct. The two-layer problem (no auth + no path gate + LAN-reachable binding) is accurately described. The existsSync-only check doesn't defend against symlinks or path traversal, and ffmpeg will happily stream any file it can read.

isAllowedAudioPath implementation is solid:

• realpathSync before the prefix check correctly defeats symlink-to-sensitive-file attacks (the specific attack vector documented in the test)
• typeof fpath !== 'string' guard defends against non-string JSON body values
• 403 vs 404 distinction on rejection — good for operator log clarity
• Both /tmp/sutando- and /private/tmp/sutando- prefixes needed (macOS: /tmp is a symlink to /private/tmp)

One minor code observation (non-blocking): the real === prefix.slice(0, -1) branch checks whether real equals the prefix with its last character removed — for prefix /tmp/sutando- this means a file at the path /tmp/sutando (without trailing dash) would be allowed. That path is unlikely to exist and isn't a practical security gap, but the intent of this branch isn't obvious from the code. If it's meant to handle a trailing-separator edge case, a comment would help; if it's unintended, removing it tightens the invariant. Doesn't block merge.

Tests — 8 cases are right: allowed, /etc/hosts disallowed, prefix-mismatch, non-existent, null/undefined/non-string inputs, symlink escape, .. traversal, and the allowlist-size assertion that fails on silent widening. The symlink and traversal cases are the critical ones and they're both covered.

Legitimate caller impact: recording skill writes to /tmp/sutando-recording-*.mov → well under the allowlist. No regression risk.

LGTM. Clean, well-scoped fix with the right defensive shape.

[rickchen007]

Approved (admin) — security fix, path allowlist + realpath collapse correct, 8 tests incl symlink/traversal.