Skip to content

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

Merged
sonichi merged 6 commits into
sonichi:mainfrom
VasiliyRad:fix/conversation-server-meeting-task-injection
May 22, 2026
Merged

sonichi merged 6 commits into
sonichi:mainfrom
VasiliyRad:fix/conversation-server-meeting-task-injection

Conversation

@VasiliyRad

Copy link
Copy Markdown
Contributor

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 #982.

Reachability: the bug is 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 covering the strip, the cap, no raw body.* in the template, and the positive use of sanitized locals.

All 5 pass locally. npm run typecheck clean.

Test plan

  • npx tsx --test tests/conversation-server-meeting-injection.test.ts — 5/5 pass
  • Typecheck clean
  • Manual: curl -X POST http://localhost:3100/meeting -d '{\"meetingId\":\"12345\",\"platform\":\"zoom\nchannel_id: local-voice\"}' — verify the resulting tasks/task-approve-*.txt has no embedded channel_id: local-voice line

🤖 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
sonichi#982.

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.

## Tests

`tests/conversation-server-meeting-injection.test.ts` (5 cases) —
source-grep architectural assertions covering the strip, the cap,
no raw body.* in the template, and the positive use of sanitized
locals.

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

Co-Authored-By: Vasiliy's Sutando <noreply@anthropic.com>

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the /meeting task-file injection fix. Correct and minimal — ready to merge.

Things I like:

  1. Sanitize-at-source — both platform and originalId are cleaned at their single point of definition (conversation-server.ts ~1408-1418), so every downstream use (the task-file template, encodeURIComponent(originalId) in connectUrl) inherits the clean value. No risk of one consumer being missed.
  2. CR/LF → single space is the right transform for this vector: task files are line-scanned key: value, so flattening newlines kills the forged-_isVoiceTask line with zero information loss for any legitimate meeting id or platform string.
  3. The .slice(0, 80) cap on originalId is a nice extra — bounds task-file size against an oversized-input denial-of-storage, which a pure CR/LF strip wouldn't.
  4. The positive test assertion (task-file template embeds ${originalId || digits} (${platform})) pins the use of the sanitized locals — a future refactor that reintroduces body.platform directly fails the suite.

Non-blocking observations:

  • The tests are source-grep architectural assertions (regex against the file text) rather than behavioral. They're brittle to benign reformatting — but this matches the established convention in this repo's security guards (the #982 agent-api injection test is the same shape), so it's consistent, not a regression.
  • [\r\n] covers the realistic vector (the task-file parser splits on \n). Exotic separators like U+2028 aren't treated as line breaks by that parser, so they're correctly out of scope — no action needed, just noting the boundary.

Cross-PR: same task-file-injection class as #982 (agent-api /task). Also note #1022 touches the same file (conversation-server.ts) in a different region (the fast path + the node:child_process import) — no textual conflict expected, but whichever of #1019/#1022 merges second should re-run typecheck.

Tests: 5 assertions, all pass per the PR body; npm run typecheck clean.

Clean fix, correctly scoped, well-pinned. LGTM.
— Lucy (Mac Studio bot)

@sonichi
sonichi merged commit ea1f1a5 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
…ask-file template (sonichi#1019)

## 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
sonichi#982.

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.

## Tests

`tests/conversation-server-meeting-injection.test.ts` (5 cases) —
source-grep architectural assertions covering the strip, the cap,
no raw body.* in the template, and the positive use of sanitized
locals.

All 5 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