fix: prevent pending stream cache exhaustion and stream eviction - #158
Conversation
|
@priyansh13-c is attempting to deploy a commit to the itzzavdhesh's projects Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
dco-missing
bug
✍️ DCO Sign-off NeededHey @priyansh13-c! 👋 One or more commits in this PR are missing a Warning
How to fix: For the latest commit: git commit --amend --signoff
git push --force-with-leaseFor multiple commits, replace git rebase --signoff HEAD~N
git push --force-with-leaseThis comment will update automatically after you push. 🤖 VoiceForge Automation · Updates automatically on edits |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR modifies the pending stream capacity enforcement in the ChangesPending Stream Capacity Enforcement and Rejection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎉 PR Ready for Mentor ReviewHey @priyansh13-c! 👋 Your PR passed all checks and is now in the GSSoC review queue. Note 🔗 Closing: #152 · 📐 134 lines across 2 file(s) · 📬 Already requested or no eligible reviewer found @sabeenaviklar @Anushreebasics @itsdakshjain @snehkris @1754riya @Mrigakshi-Rathore @Itzzavdheshh @vedhapprakashni, this PR is ready for your review — please confirm scope, check behavior and tests, then approve or request changes. Important This is not an approval. Please wait for mentor feedback before expecting a merge. If changes are requested, push them to this same branch and keep the PR focused on the linked issue. 🤖 VoiceForge Automation · Updates automatically on edits |
priyansh13-c
changed the title
github-actions
Bot
added
assignee-match
gssoc26
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/test/voiceController.secure-id.test.js (1)
87-104: 💤 Low valueWell-structured validation of non-eviction behavior.
The test correctly verifies that both the oldest and newest pending stream entries remain accessible when the cache is at capacity, confirming that the new implementation does not evict existing streams. This directly validates the fix for the eviction aspect of the DoS vulnerability.
Note: The test does not cover the scenario where invalid ElevenLabs API keys are rejected before creating pending stream entries, because that validation is not yet implemented (see comment on
voiceController.js:103-109).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/test/voiceController.secure-id.test.js` around lines 87 - 104, The test notes that invalid ElevenLabs API keys are not being rejected before creating pending stream entries; update the validation logic in voiceController.js (around the code handling creation of pending streams referenced at voiceController.js:103-109) to verify the ElevenLabs API key early and throw or return an error before adding any pending stream entry, ensuring functions that create or enqueue streams (e.g., the handler that calls createPendingStreamEntry/streamSpeech) validate the key, respond with an appropriate error, and never create pending entries when the key is invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/controllers/voiceController.js`:
- Around line 103-109: The pendingStreams DoS risk arises because requireApiKey
only checks for non-empty X-ElevenLabs-Api-Key and speak stores that key into
pendingStreams without verifying it; to fix, validate the provided ElevenLabs
key (e.g., by calling a lightweight ElevenLabs auth/validate endpoint or reusing
an existing verification helper) before inserting into pendingStreams in the
speak handler, cache successful key validations (TTL tied to
PENDING_STREAM_TTL_MS) to avoid repeated validation, and add rate limiting
middleware for the /api/voice/speak route (by IP and by API key) so malicious
clients cannot exhaust PENDING_STREAMS_MAX — update the functions/variables:
requireApiKey, speak, streamSpeech, pendingStreams, PENDING_STREAMS_MAX, and
PENDING_STREAM_TTL_MS accordingly.
---
Nitpick comments:
In `@server/test/voiceController.secure-id.test.js`:
- Around line 87-104: The test notes that invalid ElevenLabs API keys are not
being rejected before creating pending stream entries; update the validation
logic in voiceController.js (around the code handling creation of pending
streams referenced at voiceController.js:103-109) to verify the ElevenLabs API
key early and throw or return an error before adding any pending stream entry,
ensuring functions that create or enqueue streams (e.g., the handler that calls
createPendingStreamEntry/streamSpeech) validate the key, respond with an
appropriate error, and never create pending entries when the key is invalid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c2904b80-2c53-4a0f-b42f-b80a9e3103ff
📒 Files selected for processing (2)
server/controllers/voiceController.jsserver/test/voiceController.secure-id.test.js
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
itsdakshjain
left a comment
itsdakshjain
left a comment
There was a problem hiding this comment.
Resolve the comments
|
@priyansh13-c please resolve the branch conflicts |
|
@itzzavdhesh resolving conflicts for the third time, please merge the pr |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/controllers/voiceController.js (2)
284-284:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
randomUUID()is not defined — runtime ReferenceError.
randomUUIDis called without qualification, but it's not imported. It should becrypto.randomUUID().🐛 Proposed fix
- const speechId = randomUUID(); + const speechId = crypto.randomUUID();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/controllers/voiceController.js` at line 284, The randomUUID function is being called without the crypto module qualifier, causing a runtime ReferenceError. In the voiceController.js file where speechId is assigned using randomUUID(), change the unqualified randomUUID() call to crypto.randomUUID() to properly reference the function from the Node.js crypto module. Ensure the crypto module is already imported at the top of the file, or add the import if missing.
201-207:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
PENDING_STREAMS_MAXis undefined — runtime ReferenceError.The capacity guard uses
PENDING_STREAMS_MAXbut this constant is never defined or imported. The test setsprocess.env.PENDING_STREAMS_MAX, so you need to read it from the environment.🐛 Proposed fix: Add constant definitions at module level
const ALGORITHM = "aes-256-gcm"; + +const PENDING_STREAMS_MAX = parseInt(process.env.PENDING_STREAMS_MAX, 10) || 100; +const PENDING_STREAM_TTL_MS = parseInt(process.env.PENDING_STREAM_TTL_MS, 10) || 60000;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/controllers/voiceController.js` around lines 201 - 207, The constant PENDING_STREAMS_MAX is referenced in the capacity guard check (if statement comparing pendingStreams.size >= PENDING_STREAMS_MAX) but is never defined, causing a runtime ReferenceError. Add a constant definition at the module level in voiceController.js that reads PENDING_STREAMS_MAX from the environment variable process.env.PENDING_STREAMS_MAX with a sensible default fallback value to ensure the constant is available when the guard check executes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/controllers/voiceController.js`:
- Around line 298-299: The token expiry calculation on line 298 hardcodes 60000
milliseconds instead of using the PENDING_STREAM_TTL_MS environment variable
that is already being used elsewhere for the pending stream cleanup timeout.
Replace the hardcoded 60000 value in the expiresAt calculation with the
PENDING_STREAM_TTL_MS constant to ensure the token expiry and stream cleanup
timeout remain synchronized, preventing divergence when the TTL is configured
differently via environment variables.
- Around line 208-212: Remove the redundant if block (lines 208-211) that calls
requireApiKey(request) without capturing its result. Keep only the ternary
assignment on line 212 that properly assigns the result of the conditional
requireApiKey call to the apiKey variable. This eliminates the duplicate
function call and clarifies the flow.
- Around line 292-293: The variable `apiKey` is being redeclared with `const` at
line 292, but it was already declared at line 212 in the same scope, causing a
SyntaxError. Remove the `const` keyword and simply assign the value to the
existing `apiKey` variable. Additionally, when storing the pending stream data
in `pendingStreams.set()` at line 293, use the trimmed versions of `text` and
`voiceId` instead of the original values to maintain consistency with the
trimming that has already been performed earlier in the function.
---
Outside diff comments:
In `@server/controllers/voiceController.js`:
- Line 284: The randomUUID function is being called without the crypto module
qualifier, causing a runtime ReferenceError. In the voiceController.js file
where speechId is assigned using randomUUID(), change the unqualified
randomUUID() call to crypto.randomUUID() to properly reference the function from
the Node.js crypto module. Ensure the crypto module is already imported at the
top of the file, or add the import if missing.
- Around line 201-207: The constant PENDING_STREAMS_MAX is referenced in the
capacity guard check (if statement comparing pendingStreams.size >=
PENDING_STREAMS_MAX) but is never defined, causing a runtime ReferenceError. Add
a constant definition at the module level in voiceController.js that reads
PENDING_STREAMS_MAX from the environment variable
process.env.PENDING_STREAMS_MAX with a sensible default fallback value to ensure
the constant is available when the guard check executes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68b2374a-469d-4aa8-b5da-fcd8d8b89b6d
📒 Files selected for processing (2)
server/controllers/voiceController.jsserver/test/voiceController.secure-id.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- server/test/voiceController.secure-id.test.js
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@itsdakshjain @itzzavdhesh @Anushreebasics CI is failing during the frontend build (Build client). My changes are limited to server/controllers/voiceController.js and related tests. I also checked a recent workflow run on main (CI #476), which is failing during the same build stage. Could a maintainer confirm whether this is a pre-existing issue? |
@priyansh13-c You are completely correct that your PR didn't cause the frontend client build failure (useSpeechHistory.js). That's an upstream issue on main and you aren't responsible for fixing it here. However, the review comment is about a separate backend runtime error inside your changes to server/controllers/voiceController.js. In commit 7c0fd96, the capacity guard logic uses PENDING_STREAMS_MAX and randomUUID(). But because PENDING_STREAMS_MAX is never declared from process.env and the native crypto module isn't imported at the top of the file, Node.js will throw a ReferenceError and crash the server when the /speak endpoint is hit in production. |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
@itsdakshjain i have fixed the issue, please ask the admin to fix client CI issue and merger it, also can you change the level to intermediate |
Pls resolve the issue regarding pending stream max , and the ci check will be handled on our end , that is not blocking your pr |
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
|
@itsdakshjain done |
🎊 PR Merged SuccessfullyHey @priyansh13-c! 👋 Congratulations and thank you for your contribution to VoiceForge! Note 🔗 Linked issue(s): #152 · ✅ Marked as merged and complete Maintainers may still handle final cleanup, release notes, or follow-up tracking after the merge. 🤖 VoiceForge Automation · Updates automatically on edits |
5 participants
🚀 Program
GSSoC
📝 Description
Fixes a Denial of Service (DoS) vulnerability in the voice generation flow caused by uncontrolled allocation of entries in the
pendingStreamscache.Previously, the
/api/voice/speakendpoint could create pending stream entries before sufficient validation, allowing attackers to flood the cache with requests using invalid ElevenLabs API keys. Once the cache reachedPENDING_STREAMS_MAX, legitimate users' pending speech streams could be evicted before retrieval.This PR introduces safeguards to prevent cache exhaustion from abusive requests and ensures that legitimate pending streams are not evicted due to malicious traffic.
🔗 Related Issue
Closes #152
🔄 Type of Change
🧪 How to Test
Start the backend and frontend:
Open
http://localhost:5173in a browser.Use the app's TTS flow to generate a speech request with a valid
X-ElevenLabs-Api-Key.Confirm the first requests succeed and return a
speechId/audio URL.To verify the fix, send enough
/api/voice/speakrequests to fill the pending stream cache:PENDING_STREAMS_MAXconcurrent pending requests.PENDING_STREAMS_MAX=10in the backend environment.Once the cache is full, further
/speakrequests should return:Confirm existing pending stream URLs still work and are not evicted.
Run the automated backend tests:
npm test --workspace server📸 Screenshots (if applicable)
N/A (Backend security fix)
✅ Checklist
feat: add voice preview)Summary by cubic
Prevents DoS in TTS by rejecting new
/api/voice/speakrequests once the pending stream store hits capacity, without evicting active streams. Adds securespeechIds usingcrypto.randomUUID()and configurable TTL cleanup to address #152.PENDING_STREAMS_MAXandPENDING_STREAM_TTL_MS; pending entries auto-expire,deletePendingStreamclears unref’ed timers.text/voice_idand clampsvoice_settingswith safe defaults; tests cover overflow rejection, oldest/newest streams still completing, andspeechIdin audio URLs.Written for commit b2bc910. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests