Skip to content
This repository was archived by the owner on Jul 16, 2026. It is now read-only.

fix(phone-conversation): sanitize platform + originalId in /meeting task-file template - #50

Open
VasiliyRad wants to merge 1 commit into
mainfrom
fix/conversation-server-meeting-task-injection
Open

VasiliyRad wants to merge 1 commit into
mainfrom
fix/conversation-server-meeting-task-injection

Conversation

@VasiliyRad

Copy link
Copy Markdown

The bug

The /meeting POST handler composes a task file via template literal:

```ts
const platform = (body.platform ?? 'zoom').toLowerCase();
const originalId = body.meetingId.trim();
...
const taskContent = `id: ${taskId}\ntimestamp: ${...}\ntask: Sutando
joined meeting ${originalId || digits} (${platform}) — call SID ${sid}.
...`;
```

Both platform and originalId are user-controlled (Gemini tool arguments) and neither was newline-sanitized:

  • .toLowerCase() doesn't touch CR/LF.
  • .trim() only strips leading/trailing whitespace, not embedded.

A value like { platform: \"zoom\nchannel_id: local-voice\", … } would forge a _isVoiceTask line-scan match downstream and misroute the task — same class as the agent-api /task injection closed by PR #32.

Reachability: /meeting is now loopback-only (PR #41), so external LAN exploitation is gone — but the injection is still reachable from any local process that can hit /meeting, which includes the voice-agent itself when Gemini synthesizes a tool call with attacker-controlled meeting metadata from user voice input.

Fix

Sanitize both at the source (single point of contact):

Var Before After
platform (body.platform ?? 'zoom').toLowerCase() …toLowerCase().replace(/[\r\n]/g, ' ').trim()
originalId body.meetingId.trim() …trim().replace(/[\r\n]/g, ' ').slice(0, 80)

CR/LF flatten to single spaces (no information loss for valid inputs). The 80-char cap on originalId bounds the task-file size against an oversized-input DoS.

The rest of the handler doesn't change. All downstream references (originalId || digits, ${platform} in the template) use the sanitized locals — pin via test.

Tests

tests/conversation-server-meeting-injection.test.ts — 5 source-grep architectural assertions:

Case Pins
CR/LF stripped from platform The actual injection vector.
CR/LF stripped from originalId Same vector via meetingId.
originalId capped Oversized-input DoS guard.
Template doesn't interpolate raw body.* A future refactor that bypasses sanitization fails here.
Template uses the sanitized locals Positive assertion.

Source-grep is the right shape — invoking the actual handler needs Twilio creds + Gemini SDK init; the security contract is "sanitize before interpolation," which grep verifies precisely.

All 5 pass locally. npm run typecheck clean.

🤖 Generated with Claude Code

…ask-file template

## The bug

The `/meeting` POST handler composes a task file via template literal:

  const platform   = (body.platform ?? 'zoom').toLowerCase();
  const originalId = body.meetingId.trim();
  ...
  const taskContent = `id: ${taskId}\ntimestamp: ${...}\ntask: Sutando
    joined meeting ${originalId || digits} (${platform}) — call SID ${sid}.
    ...`;

Both `platform` and `originalId` are user-controlled (Gemini tool
arguments) and neither was newline-sanitized:

  - `.toLowerCase()` doesn't touch CR/LF.
  - `.trim()` only strips leading/trailing whitespace, not embedded.

A value like `{ platform: "zoom\nchannel_id: local-voice", … }` would
forge a `_isVoiceTask` line-scan match downstream and misroute the
task — same class as the agent-api `/task` injection closed by PR #32.

`/meeting` is now loopback-only (loopback_guard PR #41), so external
LAN exploitation is gone — but the injection is still reachable from
any local process that can hit `/meeting`, which includes the voice-
agent itself when Gemini synthesizes a tool call with attacker-
controlled meeting metadata from user voice input.

## Fix

Sanitize both at the source (single point of contact):

  - `platform`:   .toLowerCase().replace(/[\r\n]/g, ' ').trim()
  - `originalId`: body.meetingId.trim().replace(/[\r\n]/g, ' ').slice(0, 80)

CR/LF flatten to single spaces (no information loss for valid inputs).
80-char cap on `originalId` bounds the task-file size against an
oversized-input DoS — a 10KB meetingId would otherwise balloon a tiny
task file.

The rest of the handler doesn't change. All downstream references
(`originalId || digits`, `${platform}` in the template) use the
sanitized locals — pin via test.

## Tests

`tests/conversation-server-meeting-injection.test.ts` (5 cases) — all
source-grep architectural assertions:

  - CR/LF stripped from `platform`
  - CR/LF stripped from `originalId`
  - `originalId` capped to bounded length
  - task-file template doesn't interpolate raw `body.*` fields
  - task-file template uses the sanitized locals

Source-grep is the right shape — the actual handler requires Twilio
creds + Gemini SDK init to invoke; the security contract is "sanitize
before interpolation," which grep verifies precisely.

All 5 pass locally. `npm run typecheck` clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — cold review. Port of sutando sonichi#1019 (merged). Sanitizes platform and originalId fields in the /meeting task-file template — both were interpolated verbatim, allowing task-file field injection if a caller controls either value (e.g. via a crafted meeting URL). Test covers the injection shape. Merge when ready.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants