fix(security): meter and throttle the deployed-chat TTS relay - #6212
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview The
Reviewed by Cursor Bugbot for commit 2ad92d7. Configure here. |
Greptile SummaryThe PR secures deployed-chat TTS by attributing and recording vendor usage, enforcing payer limits and request throttles, restricting accepted synthesis inputs, and centralizing deployed-chat caller resolution.
Confidence Score: 4/5The PR should not be considered fully safe to merge until the outstanding concurrent usage-limit overshoot is either prevented or explicitly accepted as bounded follow-up risk. The route still checks usage before the ElevenLabs request and records the charge afterward without a reservation, so concurrent requests can pass against the same stale balance and exceed the payer's configured limit; the new per-chat bucket bounds but does not eliminate that failure. Files Needing Attention: apps/sim/app/api/proxy/tts/stream/route.ts
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/proxy/tts/stream/route.ts | Adds throttling, payer-limit checks, vendor-cost metering, and fail-closed stream cancellation; the previously reported non-atomic usage-limit race remains. |
| apps/sim/lib/chat/deployed-chat-caller.ts | Centralizes deployed-chat authorization and payer lookup while rejecting inactive and archived chats. |
| apps/sim/lib/api/contracts/media/tts-stream.ts | Caps synthesis text and restricts anonymous callers to approved voice and model identifiers. |
| apps/sim/app/(interfaces)/chat/hooks/use-audio-streaming.ts | Splits long synthesis input into relay-compatible chunks and uses the shared default model. |
| apps/sim/lib/billing/core/usage-log.ts | Extends the usage-source type with metered voice output. |
| packages/db/migrations/0281_fixed_madame_web.sql | Adds voice-output to the persisted usage-log source enum. |
Sequence Diagram
sequenceDiagram
participant Client
participant TTS as TTS Relay
participant Chat as Chat Resolver
participant Billing
participant ElevenLabs
Client->>TTS: POST text, voice, model, chatId
TTS->>TTS: Enforce IP limit and validate body
TTS->>Chat: Resolve authorization and payer
Chat-->>TTS: Owner/workspace attribution
TTS->>TTS: Enforce per-chat limit
TTS->>Billing: Check payer usage limit
Billing-->>TTS: Allowed
TTS->>ElevenLabs: Request synthesis
ElevenLabs-->>TTS: Audio response stream
TTS->>Billing: Record voice-output charge
alt Usage recorded
TTS-->>Client: Stream audio
else Ledger failure
TTS->>ElevenLabs: Cancel response body
TTS-->>Client: 500 without audio
end
Reviews (4): Last reviewed commit: "fix(security): release the vendor stream..." | Re-trigger Greptile
a8b812f to
7bb2789
Compare
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 03f9bd8. Configure here.
POST /api/proxy/tts/stream treated "a live public chat exists" as authorization to spend the platform ElevenLabs key. A public chat id is handed to every visitor, so any anonymous caller could synthesize speech with no length cap, no rate limit and no usage accounting. Bring the relay in line with its STT sibling (/api/speech/token): - Resolve the chat's workspace and bill synthesized characters to that payer via a new `voice-output` usage source, so spend is attributable and counts against the plan's usage limit (402 once exceeded). - Throttle per IP before any database work, and per chat afterwards, to bound both one caller hammering many chats and many callers hammering one chat. - Cap `text` at 2000 characters and allowlist `voiceId`/`modelId`, so the caller can no longer choose an unbounded charge, a premium or cloned voice, or the billing model. - Drop `Access-Control-Allow-Origin: *`, which let any third-party page read the audio; deployed chat and the Office embed are same-origin.
Follow-up review of the previous commit found five defects in it: - Usage rows collided. `usage_log.event_key` is unique and inserts are conflict-do-nothing, and the key is derived from the entry's stable fields. With no explicit sourceReference, two synthesis calls of equal character count in the same workspace produced the same key, so every repeat length went unbilled — defeating the metering this change is for. Each call now carries a unique sourceReference. - Priced at $0.10 per 1k characters, twice the published ElevenLabs Flash/Turbo rate of $0.05, which would have overcharged customers 2x. - No body cap, so an anonymous caller could make the route buffer up to the shared 50 MB default before validation. Now 16 KB, as the STT sibling does. - Threshold settlement ran per sentence: several queries and a possible Stripe call on a realtime path. The workflow execution that produced the text already settles the payer. - The per-IP bucket was described as preventing database amplification. getClientIp trusts the leftmost X-Forwarded-For, so an attacker rotates past it; the comment now says the per-chat bucket is load-bearing.
Review of the previous commits surfaced duplication and one more gap: - The TTS and STT routes had grown near-identical copies of the chat auth + payer lookup. Extracted to resolveDeployedChatCaller, so the gate and the payer resolve together and cannot drift per route — that duplication is how the unmetered TTS path shipped in the first place. - Neither copy filtered chat.archivedAt, so an archived chat could still authorize spend against its former owner's workspace. The shared lookup now filters it, fixing both routes at once. Note: not covered by a test — the db chain mock does not evaluate WHERE clauses, so an assertion here could not fail. - Replaced the route's hand-rolled 429 builder with the existing enforceIpRateLimit helper, and added enforceChatRateLimit alongside the per-user/IP/workspace helpers. Gains the standard Retry-After and X-RateLimit-Reset headers plus throttle logging. - Dropped a test that asserted a module the route no longer imports was never called: it could not fail. - Narrowed the contract: unexported the single-use allowlists and dropped .passthrough() now that the body is a closed shape.
Review round 1 findings: - A ledger write failure previously logged and streamed the audio anyway, leaving the spend unrecorded and the payer's usage understated. The caller is anonymous, so serving audio we could not charge for is the unmetered spend this route exists to prevent — it now returns 500. - Use generateId() from @sim/utils/id rather than crypto.randomUUID, per the AGENTS.md ID rule. generateId returns a full UUID v4, so the per-call uniqueness the usage_log event_key depends on is unchanged.
The client sentence-splits on Western `.!?` only, so text that never matches — CJK punctuation, or a list with no terminal punctuation — accumulates and is flushed as one block at the end of the stream. Against the new 2000-character relay cap that block is rejected and the whole message plays no audio, a regression introduced by adding the cap. Split to cap-sized pieces at the single point that enqueues synthesis, so both the per-sentence path and the end-of-stream flush are covered. Prefers a whitespace or CJK punctuation boundary, falling back to a hard cut when a block has none. The server cap stays as the enforcement point.
…quest The fail-closed branch returned 500 with the ElevenLabs response body still open, so synthesis and download kept consuming vendor and runtime resources for a caller that was already rejected. Cancel it before returning, and assert the cancellation in the test.
03f9bd8 to
2ad92d7
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 2ad92d7. Configure here.
* refactor(chat): clean up the deployed chat surface Eight-angle cleanup pass over the full contents of the chat surface and the speech code that survived the voice-mode removal. Dead code - enforceChatRateLimit: added for the TTS relay in #6212, orphaned when #6215 deleted that route. Zero consumers. - ChatToolCallStatus, ChatErrorType, and six unused CHAT_ERROR_MESSAGES keys (only GENERIC_ERROR and CHAT_UNAVAILABLE are read). - scrollToMessage was declared and destructured by ChatMessageContainer but never used in its body; removing the prop also made the scrollToShowOnlyMessage branch unreachable, since the sole caller passed true. - permissionState and the language prop on useSpeechToText: both write-only across the repo. - The image branch in ChatFileDownload's renderIcon returned the same DefaultFileIcon at the same size as the fallback. - chatKeys.status/detail: aliases of deploymentKeys nothing imported, and misleading since they root under a different key namespace. Redundant state - password-auth and email-auth each kept a boolean in lockstep with `errors.length > 0`; email-auth also validated on every keystroke and then immediately hid the result. - file-download tracked hover in state to drive one opacity class; now group-hover. Verified emcn Button sets no `group` class of its own. Memoization - ChatMessageContainer's memo() could never bail: chat.tsx passes an inline arrow for scrollToBottom and displayMessages is a fresh array. Four of the five things that re-render ChatClient are its props anyway, so the memo is dropped rather than propped up. - ClientChatMessage keeps its memo — it blocks markdown re-parsing — but loses the custom comparator, which compared proxies (a key:status fingerprint, files by length) and ignored attachments and type entirely. Default shallow compare on its single prop is both simpler and stricter. - Six useCallbacks whose consumers are native DOM handlers or inline arrows, so nothing observed their identity. Effects - The scroll listener attached in an effect keyed on [chatConfig, authRequired] — values it never reads, standing in for "the container has mounted". It now attaches via a ref callback, so it no longer re-attaches on every config refetch. Design system and a11y - z-[100] -> z-[var(--z-dropdown)] (same value), shadow-lg -> shadow-medium, list styles from inline style to Tailwind classes, hover: -> hover-hover: on touch-reachable targets, Check sourced from emcn alongside its Duplicate pair. - Accessible names on the remove-attachment, stop, and send buttons, which announced only as "button". - Dropped a keyboard handler on a role='group' div with no tabIndex, where target === currentTarget was unreachable, and the Tooltip Provider wrappers and delayDuration, which emcn documents as no-op passthroughs. * fix(chat): restore markdown list markers The design-system pass swapped inline `listStyleType` for Tailwind classes, but the edit that added `list-disc`/`list-decimal` silently did not apply while the one removing the inline style did. With Preflight setting `list-style: none`, every bullet and number in an assistant response disappeared. `list-item` on the `li` sets display only, not the marker type.
Summary
POST /api/proxy/tts/streamtreated "a live public chat exists" as authorization to spend the platform ElevenLabs key. A public chat id is handed to every visitor, so any anonymous caller could synthesize speech with no length cap, no rate limit and no usage accounting.voice-outputusage source, so spend is attributable and counts against the plan's usage limit (402 once exceeded).textat 2000 characters, cap the request body at 16 KB, and allowlistvoiceId/modelIdso the caller can no longer pick an unbounded charge, a premium/cloned voice, or the billing model.Access-Control-Allow-Origin: *, which let any third-party page read the audio. Deployed chat and the Office embed are same-origin.resolveDeployedChatCaller, and filterchat.archivedAtthere — an archived chat could previously still authorize spend against its former owner's workspace, on both routes.Notes for review
TTS_COST_PER_1K_CHARS = 0.05is ElevenLabs' published Flash/Turbo API rate. Please confirm against our actual contract — if we're on a negotiated rate this constant needs updating. It's the vendor cost;getCostMultiplier()applies markup, matching the STT precedent.getClientIptrusts the leftmostX-Forwarded-For, which the caller controls. The per-chat bucket is the load-bearing control.usage_logrow per synthesized sentence, so a long answer produces ~10-30 rows. Accurate, but noisier than STT's one-row-per-session.chat.archivedAtfilter is not covered by a test — the db chain mock does not evaluate WHERE clauses, so an assertion there could not fail.Type of Change
Testing
13 route tests covering the length cap, voice/model allowlists, body cap, both rate-limit buckets, workspace attribution, the usage-limit 402, and unique
sourceReferenceper call. Each guard was verified to fail when its fix is reverted. Full sweep: 811 tests across billing/contracts/chat/rate-limiter, typecheck clean onapps/simandpackages/db,check:migrationsclean, no drizzle schema drift. Not live-tested against ElevenLabs.Checklist