Add control server to accept Go Live handoff and drive ffmpeg-based streaming #2
Conversation
…age Streamer sessions, ffmpeg demuxing and streaming Co-authored-by: Genie <[email protected]>
WalkthroughImplements a new TypeScript control server exporting Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant HTTP as Control HTTP
participant Manager as Session Manager
participant Streamer as Discord Streamer
participant FFmpeg as FFmpeg
participant Demux as NUT Demuxer
participant UDP as RTP/UDP Sockets
Client->>HTTP: POST /go-live/start {stream_key,...}
HTTP->>Manager: validate & create GoLiveSession
Manager->>Streamer: init/login & allocate stream connection
Manager-->>HTTP: 202 {state: starting}
Manager->>FFmpeg: prepareStream (nvenc|software) & spawn
FFmpeg->>Demux: output NUT -> video/audio streams
Demux->>Manager: provide video/audio attrs / demux results
Manager->>UDP: configure packetizers & open sockets
Manager->>Streamer: await select_protocol_ack
Streamer-->>Manager: select_protocol_ack
Manager->>Streamer: enable UDP, set speaking, negotiate video attrs
Manager-->>HTTP: session running
HTTP-->>Client: 200 {state: running}
Client->>HTTP: GET /go-live/status?stream_key=...
HTTP->>Manager: query session
Manager-->>HTTP: {state,lastError}
HTTP-->>Client: 200 {status}
Client->>HTTP: POST /go-live/stop {stream_key}
HTTP->>Manager: stop session
Manager->>UDP: close sockets
Manager->>FFmpeg: terminate process
Manager->>Streamer: cleanup stream connection
Manager->>Manager: run cleanup handlers -> stopped
Manager-->>HTTP: {state: stopped}
HTTP-->>Client: 200 {stopped}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/controlServer.ts (3)
112-119: Resolve ffmpeg name collision and remove redundant code.payload.ffmpeg shadows the module; also duplicate ternaries. Simplify and rename to ff.
@@ - video, encryptionPreference, ffmpeg + video, encryptionPreference, ffmpeg: ff } = payload; @@ - const { default: ffmpegLib } = await import("fluent-ffmpeg"); + const { default: ffmpegLib } = await import("fluent-ffmpeg"); @@ - // We reuse the demux + VideoStream/AudioStream path instead of playStream() to avoid gateway signaling - const input = ffmpeg.input; - const inputSpec = ffmpeg.input; // Not used directly; we build command via newApi-like logic + // We reuse the demux + VideoStream/AudioStream path instead of playStream() to avoid gateway signaling @@ - const command = ffmpegLib(ffmpeg.input ? ffmpeg.input : ffmpeg.input) + const command = ffmpegLib(ff.input) .addOption("-loglevel", "info") - .input(ffmpeg.input ? ffmpeg.input : ffmpeg.input); + .input(ff.input); @@ - const merged = { - width: ffmpeg.options?.width, - height: ffmpeg.options?.height, - frameRate: ffmpeg.options?.frameRate, - bitrateVideo: ffmpeg.options?.bitrateVideo ?? 5000, - bitrateVideoMax: ffmpeg.options?.bitrateVideoMax ?? 7000, - bitrateAudio: ffmpeg.options?.bitrateAudio ?? 128, - includeAudio: ffmpeg.options?.includeAudio ?? true, - hardwareAcceleratedDecoding: ffmpeg.options?.hardwareAcceleratedDecoding ?? false, - minimizeLatency: ffmpeg.options?.minimizeLatency ?? false, - customHeaders: ffmpeg.options?.customHeaders ?? { + const merged = { + width: ff.options?.width, + height: ff.options?.height, + frameRate: ff.options?.frameRate, + bitrateVideo: ff.options?.bitrateVideo ?? 5000, + bitrateVideoMax: ff.options?.bitrateVideoMax ?? 7000, + bitrateAudio: ff.options?.bitrateAudio ?? 128, + includeAudio: ff.options?.includeAudio ?? true, + hardwareAcceleratedDecoding: ff.options?.hardwareAcceleratedDecoding ?? false, + minimizeLatency: ff.options?.minimizeLatency ?? false, + customHeaders: ff.options?.customHeaders ?? { "User-Agent": "Mozilla/5.0", "Connection": "keep-alive", }, - customFfmpegFlags: ffmpeg.options?.customFfmpegFlags ?? [], - encoder: ffmpeg.options?.encoder ?? "software", + customFfmpegFlags: ff.options?.customFfmpegFlags ?? [], + encoder: ff.options?.encoder ?? "software", }; @@ - if (ffmpeg.input.startsWith("http")) { + if (ff.input.startsWith("http")) {Also applies to: 185-197, 223-227, 230-247, 255-262, 264-266
350-355: Populate status.metrics (packets, encryption mode).You defined metrics but never return them. Optional, but useful for ops.
- Include encryption mode from session/udp.
- If MediaUdp exposes counters/timestamps, add them to metrics. I can wire this up if you confirm the fields.
229-253: Video pipeline defaults: small tweaks for stability (optional).
- Consider keyint and scenecut to stabilize IDR cadence: -g <2*fps> -keyint_min -sc_threshold 0.
- For NVENC path, map encoder option to h264_nvenc and set rc=vbr_hq with -cq.
I can add conditional codec flagging if you confirm NVENC is expected on the target hosts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controlServer.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/controlServer.ts (6)
src/client/voice/StreamConnection.ts (1)
StreamConnection(4-33)src/client/Streamer.ts (1)
Streamer(26-279)src/utils.ts (2)
isFiniteNonZero(29-31)SupportedVideoCodec(24-24)src/media/LibavDemuxer.ts (1)
demux(103-274)src/media/VideoStream.ts (1)
VideoStream(4-15)src/media/AudioStream.ts (1)
AudioStream(4-15)
🔇 Additional comments (2)
src/controlServer.ts (2)
301-311: Confirm stream type compatibility (Packet vs Buffer).demux() emits object-mode Packet streams; VideoStream/AudioStream _sendFrame signature uses Buffer. Ensure BaseMediaStream consumes Packet objects or adapt via a transform.
Would you like me to add a tiny Transform that maps {data, pts} to Buffer+frametime when needed?
112-162: Event flow confirmed—startup path is sound.The startup sequence is properly structured:
conn.once("select_protocol_ack", ...)listener is registered at line 172 before WebSocket connection initiatessetSession()+setTokens()trigger WebSocket connection in the correct order- Discord voice protocol delivers opcodes in sequence: READY → SELECT_PROTOCOL_ACK
handleReady()initializeswebRtcParamsand callssendVoice()(which creates UDP)handleProtocolAck()emitsselect_protocol_ackonly after both webRtcParams and UDP are readysetVideoAttributes(true, {...})at line 182 has required webRtcParams and UDP availableNo issues found with the handshake/startup path.
| async function parseJsonBody(req: http.IncomingMessage): Promise<any> { | ||
| return new Promise((resolve, reject) => { | ||
| const chunks: Buffer[] = []; | ||
| req.on("data", (c) => chunks.push(c)); | ||
| req.on("end", () => { | ||
| try { | ||
| const raw = Buffer.concat(chunks).toString("utf-8"); | ||
| const obj = raw ? JSON.parse(raw) : {}; | ||
| resolve(obj); | ||
| } catch (e) { | ||
| reject(e); | ||
| } | ||
| }); | ||
| req.on("error", (e) => reject(e)); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Harden JSON parsing: cap body size and validate content-type.
Prevent DoS and malformed input.
async function parseJsonBody(req: http.IncomingMessage): Promise<any> {
- return new Promise((resolve, reject) => {
+ return new Promise((resolve, reject) => {
const chunks: Buffer[] = [];
+ const max = 1 * 1024 * 1024; // 1MB
+ let total = 0;
+ const ctype = (req.headers["content-type"] || "").toLowerCase();
+ if (req.method !== "GET" && !ctype.includes("application/json")) {
+ reject(new Error("invalid content-type"));
+ return;
+ }
req.on("data", (c) => {
- chunks.push(c));
+ total += c.length;
+ if (total > max) {
+ reject(new Error("payload too large"));
+ req.destroy();
+ return;
+ }
+ chunks.push(c);
+ });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/controlServer.ts around lines 75 to 90, the JSON body parser currently
reads and parses the entire request without bounds or content-type checks;
update it to first validate that req.headers['content-type'] exists and begins
with 'application/json' (rejecting with a clear error if not), and enforce a
configurable maximum body size (e.g., MAX_BODY_BYTES) by tracking accumulated
byte length during "data" events and immediately rejecting with a 413 Payload
Too Large and destroying the connection if the limit is exceeded; ensure you
still parse the accumulated bytes as UTF-8 JSON on "end", and normalize error
handling to reject with descriptive errors for invalid JSON, missing/invalid
content-type, and payload-too-large cases.
| export async function startControlServer() { | ||
| const token = process.env.DISCORD_TOKEN; | ||
| if (!token) { | ||
| throw new Error("DISCORD_TOKEN env var not set"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Secure the control plane: require auth and bind to localhost.
Unauthenticated endpoints exposed on 0.0.0.0 are a critical risk. Enforce a bearer secret and bind to 127.0.0.1 by default.
@@
export async function startControlServer() {
const token = process.env.DISCORD_TOKEN;
if (!token) {
throw new Error("DISCORD_TOKEN env var not set");
}
+ const controlSecret = process.env.CONTROL_AUTH_TOKEN;
+ if (!controlSecret) {
+ throw new Error("CONTROL_AUTH_TOKEN env var not set");
+ }
@@
- const server = http.createServer(async (req, res) => {
+ const server = http.createServer(async (req, res) => {
try {
const url = new URL(req.url ?? "/", `http://${req.headers.host ?? "localhost"}`);
+ // Auth: Bearer <CONTROL_AUTH_TOKEN>
+ const auth = req.headers.authorization ?? "";
+ const ok = auth.startsWith("Bearer ") && auth.slice(7) === controlSecret;
+ if (!ok) {
+ jsonResponse(res, 401, { ok: false, error: "unauthorized" });
+ return;
+ }
@@
- const port = Number(process.env.PORT ?? 3000);
- server.listen(port, () => {
- console.log(`Control server listening on http://localhost:${port}`);
- });
+ const port = Number(process.env.PORT ?? 3000);
+ const host = process.env.HOST ?? "127.0.0.1";
+ server.listen(port, host, () => {
+ console.log(`Control server listening on http://${host}:${port}`);
+ });Also applies to: 321-361, 363-367
🤖 Prompt for AI Agents
In src/controlServer.ts around lines 96-101 (and also update sections 321-361,
363-367), the control server starts without authentication and binds to all
interfaces; enforce a bearer secret and bind to localhost by default: read a
CONTROL_SECRET (or similarly named) env var at startup and throw if missing, add
a middleware that validates Authorization: Bearer <secret> on all control
endpoints returning 401 on mismatch, and change the server listen address to use
process.env.CONTROL_BIND_ADDR || "127.0.0.1" (update any other listen calls in
the other ranges). Ensure error messages are clear and tests/docs mention the
new env vars.
| const streamer = new Streamer(new (await import("discord.js-selfbot-v13")).Client(), { | ||
| // Allow external preference; default follows library behavior | ||
| forceChacha20Encryption: false, | ||
| rtcpSenderReportEnabled: true, | ||
| }); | ||
|
|
||
| await streamer.client.login(token); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Self-bot usage risk.
Logging in via discord.js-selfbot-v13 can violate Discord ToS and lead to account bans. Confirm risk acceptance and keep tokens out of logs.
If desired, I can guard startup with an env flag like REQUIRE_SELF_BOT_ACK=1 to avoid accidental misuse.
I'll verify this review comment by checking the ToS concerns and examining the codebase for existing safeguards.
🏁 Script executed:
#!/bin/bash
# Search for token handling, logging, and existing env variable guards
echo "=== Checking for token logging risks ==="
rg -n -C3 "token" src/controlServer.ts
echo "=== Checking for env variables related to safeguards ==="
rg -n "REQUIRE_SELF_BOT|process.env" src/controlServer.ts
echo "=== Checking for error handling and documentation ==="
rg -n "streamer|login|discord" src/controlServer.ts -C2
echo "=== Checking for any README or docs about self-bot risks ==="
fd -e md | head -20 | xargs grep -l "self-bot\|selfbot" 2>/dev/null || echo "No markdown files mention self-bot"
echo "=== Checking project configuration and dependencies ==="
fd -e json -e ts | grep -E "package.json|tsconfig" | head -5Length of output: 2889
🌐 Web query:
discord.js-selfbot-v13 Discord Terms of Service violation risk
💡 Result:
Short answer: Yes — using discord.js-selfbot-v13 (a library to run a "self-bot"/user-bot) violates Discord rules and risks account suspension or termination.
Why: Discord's Community Guidelines and Terms explicitly forbid self-bots / user-bots and automated user accounts; they treat such activity as misuse of the service and a grounds for enforcement. Using unofficial self-bot libraries also exposes your account token (risking compromise). [1][2][3][4]
Risks you should expect: warnings, temporary or permanent account bans, loss of servers/DM access, and potential token/theft or other security problems if you share/run tokens with third‑party code. [1][2][5]
Sources:
- Discord Community Guidelines — “Do not use self-bots or user-bots.” [1]
- Discord Terms of Service (restrictions & enforcement). [2]
- Example GitHub repo for discord.js-selfbot-v13 (unofficial self‑bot library). [3]
- Discussion/experience notes on enforcement and warnings for selfbots. [4]
If you want safer alternatives, consider building a proper bot account via the Discord developer portal (bot tokens + bot account) — I can point to docs for that if you want.
Add guard to prevent accidental self-bot misuse and document ToS risk.
discord.js-selfbot-v13 violates Discord's Community Guidelines and Terms of Service, risking account suspension or termination. Token is securely read from environment variables, but there is no guard to prevent accidental deployment or misuse. Add REQUIRE_SELF_BOT_ACK=1 environment variable check at startup (lines 96–100) to require explicit opt-in, and document this risk prominently in your README.
🤖 Prompt for AI Agents
In src/controlServer.ts around lines 96 to 109, add a startup guard that checks
process.env.REQUIRE_SELF_BOT_ACK === "1" before importing or instantiating
discord.js-selfbot-v13 and creating the Streamer; if the variable is not set,
exit with a clear error message that the self-bot feature is disabled and
requires explicit opt-in. Move the dynamic import and streamer/client creation
behind that guard so the library is never loaded unless acknowledged, and update
README.md to prominently document the ToS/Community Guidelines risk and the
REQUIRE_SELF_BOT_ACK opt-in requirement.
src/controlServer.ts
Outdated
| // Once the SELECT_PROTOCOL_ACK is received, configure attributes | ||
| conn.once("select_protocol_ack", async () => { | ||
| try { | ||
| // Mark speaking as Go Live (speaking: 2) | ||
| conn.setSpeaking(true); | ||
|
|
||
| // Set video attributes | ||
| const w = isFiniteNonZero(video?.width) ? Math.round(video!.width!) : 1280; | ||
| const h = isFiniteNonZero(video?.height) ? Math.round(video!.height!) : 720; | ||
| const fps = isFiniteNonZero(video?.fps) ? Math.round(video!.fps!) : 30; | ||
|
|
||
| conn.setVideoAttributes(true, { width: w, height: h, fps }); | ||
|
|
There was a problem hiding this comment.
Add handshake timeout for select_protocol_ack.
Without a timeout, sessions can hang in "starting" forever if the ACK never arrives.
- // Once the SELECT_PROTOCOL_ACK is received, configure attributes
- conn.once("select_protocol_ack", async () => {
+ // Timeout if ACK never arrives
+ const ackTimer = setTimeout(() => {
+ const session = sessions.get(stream_key);
+ if (session) {
+ session.state = "error";
+ session.lastError = "Timed out waiting for select_protocol_ack";
+ }
+ }, 10_000);
+ // Once the SELECT_PROTOCOL_ACK is received, configure attributes
+ conn.once("select_protocol_ack", async () => {
+ clearTimeout(ackTimer);🤖 Prompt for AI Agents
In src/controlServer.ts around lines 171 to 183, the
conn.once("select_protocol_ack", ...) handler needs a handshake timeout to avoid
sessions hanging when the ACK never arrives; add a timeout (e.g., 5–10s) started
immediately before registering the once listener that will reject the handshake:
on timeout clear the once listener, set the connection/session to an
error/failed state (or emit an error) and close or cleanup the conn, and ensure
the timeout is cleared inside the ack handler so normal flows are unaffected.
Ensure any resources allocated for the attempted start are cleaned up on timeout
to avoid leaks.
src/controlServer.ts
Outdated
| const w = isFiniteNonZero(video?.width) ? Math.round(video!.width!) : 1280; | ||
| const h = isFiniteNonZero(video?.height) ? Math.round(video!.height!) : 720; | ||
| const fps = isFiniteNonZero(video?.fps) ? Math.round(video!.fps!) : 30; | ||
|
|
There was a problem hiding this comment.
Reject negative/zero video dimensions and fps.
isFiniteNonZero allows negatives. Use a positive-int guard.
- const w = isFiniteNonZero(video?.width) ? Math.round(video!.width!) : 1280;
- const h = isFiniteNonZero(video?.height) ? Math.round(video!.height!) : 720;
- const fps = isFiniteNonZero(video?.fps) ? Math.round(video!.fps!) : 30;
+ const toPosInt = (n: unknown, d: number) =>
+ typeof n === "number" && Number.isFinite(n) && n > 0 ? Math.round(n) : d;
+ const w = toPosInt(video?.width, 1280);
+ const h = toPosInt(video?.height, 720);
+ const fps = toPosInt(video?.fps, 30);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const w = isFiniteNonZero(video?.width) ? Math.round(video!.width!) : 1280; | |
| const h = isFiniteNonZero(video?.height) ? Math.round(video!.height!) : 720; | |
| const fps = isFiniteNonZero(video?.fps) ? Math.round(video!.fps!) : 30; | |
| const toPosInt = (n: unknown, d: number) => | |
| typeof n === "number" && Number.isFinite(n) && n > 0 ? Math.round(n) : d; | |
| const w = toPosInt(video?.width, 1280); | |
| const h = toPosInt(video?.height, 720); | |
| const fps = toPosInt(video?.fps, 30); |
🤖 Prompt for AI Agents
In src/controlServer.ts around lines 178 to 181, the current checks use
isFiniteNonZero which permits negative values; replace those guards with a
positive-integer check (e.g., value !== undefined && Number.isFinite(value) &&
value > 0) for width, height and fps, then round or Math.floor the value before
use and fall back to the existing defaults (1280, 720, 30) when the check fails;
ensure fps is converted to an integer > 0 as well.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
src/controlServer.ts (1)
267-280: Critical: FFmpeg process not saved for cleanup and error handler incomplete.The
commandobject fromfluent-ffmpegis started at line 280 but never added tosession.cleanupFns, meaning it won't be killed when the session stops. Additionally, theonErrorhandler (lines 267-270) sets error state but doesn't destroy the output stream or stop the command.This was flagged in past comments but remains unaddressed. Apply this diff:
const onError = (e: unknown) => { session.state = "error"; session.lastError = e instanceof Error ? e.message : String(e); + try { command.kill("SIGKILL"); } catch {} + try { output.destroy(); } catch {} }; command.on("error", onError); command.on("end", () => { // finished naturally if (session.state !== "stopping" && session.state !== "stopped") { session.state = "stopped"; } }); + session.cleanupFns.push(() => { + try { command.kill("SIGKILL"); } catch {} + }); + session.cleanupFns.push(() => { + try { output.destroy(); } catch {} + }); + command.run();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controlServer.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/controlServer.ts (6)
src/client/voice/StreamConnection.ts (1)
StreamConnection(4-33)src/client/Streamer.ts (1)
Streamer(26-279)src/utils.ts (2)
isFiniteNonZero(29-31)SupportedVideoCodec(24-24)src/media/LibavDemuxer.ts (1)
demux(103-274)src/media/VideoStream.ts (1)
VideoStream(4-15)src/media/AudioStream.ts (1)
AudioStream(4-15)
🔇 Additional comments (1)
src/controlServer.ts (1)
223-227: Minor: Extend URL check to include HTTPS.The check for HTTP headers only covers "http" protocol, missing "https" URLs which are far more common.
Apply this diff:
- if (ffmpeg.input.startsWith("http")) { + if (ffmpeg.input.startsWith("http://") || ffmpeg.input.startsWith("https://")) { command.inputOption("-headers", Object.entries(merged.customHeaders).map(([k, v]) => `${k}: ${v}`).join("\r\n") ); }Likely an incorrect or invalid review comment.
src/controlServer.ts
Outdated
| // Prepare UDP and packetizers | ||
| const udp = conn.udp; | ||
| udp.setPacketizer(codec); |
There was a problem hiding this comment.
Critical: Null check missing for conn.udp.
conn.udp is accessed without verifying it exists. If the connection hasn't fully initialized, this will throw a runtime error that crashes the handler.
Apply this diff:
// Prepare UDP and packetizers
const udp = conn.udp;
+ if (!udp) {
+ throw new Error("UDP connection not ready");
+ }
udp.setPacketizer(codec);🤖 Prompt for AI Agents
In src/controlServer.ts around lines 296 to 298, the code accesses conn.udp
without a null check which can throw if the connection hasn't initialized; add a
guard before using conn.udp (e.g., if conn.udp is falsy) and handle that case by
logging a clear warning/error and exiting the function early (or otherwise
skipping packetizer setup) so the handler doesn't crash when udp is missing.
| video.stream.pipe(vStream); | ||
|
|
||
| if (audio) { | ||
| const aStream = new AudioStream(udp); | ||
| session.cleanupFns.push(() => aStream.destroy()); | ||
| audio.stream.pipe(aStream); | ||
| vStream.syncStream = aStream; | ||
| } |
There was a problem hiding this comment.
Major: Add error handlers for stream pipes.
The video and audio streams are piped (lines 303, 308) without registering error handlers. If either pipe encounters an error (network issues, codec mismatches, etc.), the error will bubble up uncaught.
Apply this diff to handle pipe errors:
const vStream = new VideoStream(udp);
session.cleanupFns.push(() => vStream.destroy());
- video.stream.pipe(vStream);
+ video.stream.on("error", onError).pipe(vStream);
+ vStream.on("error", onError);
if (audio) {
const aStream = new AudioStream(udp);
session.cleanupFns.push(() => aStream.destroy());
- audio.stream.pipe(aStream);
+ audio.stream.on("error", onError).pipe(aStream);
+ aStream.on("error", onError);
vStream.syncStream = aStream;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| video.stream.pipe(vStream); | |
| if (audio) { | |
| const aStream = new AudioStream(udp); | |
| session.cleanupFns.push(() => aStream.destroy()); | |
| audio.stream.pipe(aStream); | |
| vStream.syncStream = aStream; | |
| } | |
| const vStream = new VideoStream(udp); | |
| session.cleanupFns.push(() => vStream.destroy()); | |
| video.stream.on("error", onError).pipe(vStream); | |
| vStream.on("error", onError); | |
| if (audio) { | |
| const aStream = new AudioStream(udp); | |
| session.cleanupFns.push(() => aStream.destroy()); | |
| audio.stream.on("error", onError).pipe(aStream); | |
| aStream.on("error", onError); | |
| vStream.syncStream = aStream; | |
| } |
🤖 Prompt for AI Agents
In src/controlServer.ts around lines 303 to 310, the video and audio streams are
piped without error handlers which can cause uncaught exceptions; add 'error'
listeners on both source and destination streams (video.stream, vStream,
audio.stream, aStream) that log the error, unpipe the streams, run the session
cleanup (or destroy the affected streams) and remove the listeners to avoid
leaks; ensure any cleanupFns already pushed are invoked on stream error and that
the aStream/vStream are destroyed safely to prevent the error from bubbling.
| if (req.method === "POST" && url.pathname === "/go-live/start") { | ||
| const body = await parseJsonBody(req) as StartGoLiveRequest; | ||
| await startGoLive(body); | ||
| jsonResponse(res, 200, { ok: true }); | ||
| return; |
There was a problem hiding this comment.
Major: Validate request body structure before use.
The handler casts the parsed body to StartGoLiveRequest without validating that required fields exist or have the correct types. Invalid payloads will cause cryptic errors deeper in startGoLive.
Add validation before processing:
if (req.method === "POST" && url.pathname === "/go-live/start") {
const body = await parseJsonBody(req) as StartGoLiveRequest;
+ // Validate required fields
+ const required = ["guild_id", "channel_id", "user_id", "session_id",
+ "stream_key", "rtc_server_id", "endpoint", "token"];
+ for (const field of required) {
+ if (!body[field] || typeof body[field] !== "string") {
+ jsonResponse(res, 400, { ok: false, error: `Missing or invalid field: ${field}` });
+ return;
+ }
+ }
+ if (!body.ffmpeg?.input) {
+ jsonResponse(res, 400, { ok: false, error: "Missing ffmpeg.input" });
+ return;
+ }
await startGoLive(body);
jsonResponse(res, 200, { ok: true });
return;
}🤖 Prompt for AI Agents
In src/controlServer.ts around lines 323 to 327, the code casts the parsed body
to StartGoLiveRequest without validating required fields or types; add explicit
runtime validation of the request shape (either a small type-guard function or a
lightweight schema check) to ensure required properties exist and have expected
types before calling startGoLive; if validation fails, return a 400 JSON
response with an error message and do not call startGoLive; keep the success
path unchanged when validation passes.
| if (req.method === "POST" && url.pathname === "/go-live/stop") { | ||
| const body = await parseJsonBody(req) as StopGoLiveRequest; | ||
| const session = sessions.get(body.stream_key); | ||
| if (!session) { | ||
| jsonResponse(res, 404, { ok: false, error: "not found" }); | ||
| return; | ||
| } | ||
| session.state = "stopping"; | ||
| await session.stop(); | ||
| sessions.delete(body.stream_key); | ||
| jsonResponse(res, 200, { ok: true }); | ||
| return; |
There was a problem hiding this comment.
Major: Validate stop request body.
Similar to the start endpoint, the stop handler doesn't validate that stream_key exists and is a non-empty string.
Apply this diff:
if (req.method === "POST" && url.pathname === "/go-live/stop") {
const body = await parseJsonBody(req) as StopGoLiveRequest;
+ if (!body.stream_key || typeof body.stream_key !== "string") {
+ jsonResponse(res, 400, { ok: false, error: "Missing or invalid stream_key" });
+ return;
+ }
const session = sessions.get(body.stream_key);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (req.method === "POST" && url.pathname === "/go-live/stop") { | |
| const body = await parseJsonBody(req) as StopGoLiveRequest; | |
| const session = sessions.get(body.stream_key); | |
| if (!session) { | |
| jsonResponse(res, 404, { ok: false, error: "not found" }); | |
| return; | |
| } | |
| session.state = "stopping"; | |
| await session.stop(); | |
| sessions.delete(body.stream_key); | |
| jsonResponse(res, 200, { ok: true }); | |
| return; | |
| if (req.method === "POST" && url.pathname === "/go-live/stop") { | |
| const body = await parseJsonBody(req) as StopGoLiveRequest; | |
| if (!body.stream_key || typeof body.stream_key !== "string") { | |
| jsonResponse(res, 400, { ok: false, error: "Missing or invalid stream_key" }); | |
| return; | |
| } | |
| const session = sessions.get(body.stream_key); | |
| if (!session) { | |
| jsonResponse(res, 404, { ok: false, error: "not found" }); | |
| return; | |
| } | |
| session.state = "stopping"; | |
| await session.stop(); | |
| sessions.delete(body.stream_key); | |
| jsonResponse(res, 200, { ok: true }); | |
| return; |
🤖 Prompt for AI Agents
In src/controlServer.ts around lines 329 to 340, the POST /go-live/stop handler
currently trusts the parsed body; add validation to ensure body.stream_key
exists and is a non-empty string (same style as the start endpoint), returning a
400 JSON response for invalid/missing stream_key before looking up the session;
only proceed to get the session, set state, call stop(), delete from sessions
and respond 200 if validation passes.
| if (req.method === "GET" && url.pathname === "/go-live/status") { | ||
| const stream_key = url.searchParams.get("stream_key") ?? ""; | ||
| const session = sessions.get(stream_key); | ||
| if (!session) { | ||
| jsonResponse(res, 404, { ok: false, error: "not found" }); | ||
| return; | ||
| } | ||
| const out: StatusResponse = { | ||
| state: session.state, | ||
| lastError: session.lastError | ||
| }; | ||
| jsonResponse(res, 200, out); | ||
| return; |
There was a problem hiding this comment.
Minor: Reject empty stream_key in status endpoint.
The status endpoint allows an empty string for stream_key (line 343 with ?? ""), which will always result in a 404. It's clearer to reject this explicitly with a 400 error.
Apply this diff:
if (req.method === "GET" && url.pathname === "/go-live/status") {
- const stream_key = url.searchParams.get("stream_key") ?? "";
+ const stream_key = url.searchParams.get("stream_key");
+ if (!stream_key) {
+ jsonResponse(res, 400, { ok: false, error: "stream_key parameter required" });
+ return;
+ }
const session = sessions.get(stream_key);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (req.method === "GET" && url.pathname === "/go-live/status") { | |
| const stream_key = url.searchParams.get("stream_key") ?? ""; | |
| const session = sessions.get(stream_key); | |
| if (!session) { | |
| jsonResponse(res, 404, { ok: false, error: "not found" }); | |
| return; | |
| } | |
| const out: StatusResponse = { | |
| state: session.state, | |
| lastError: session.lastError | |
| }; | |
| jsonResponse(res, 200, out); | |
| return; | |
| if (req.method === "GET" && url.pathname === "/go-live/status") { | |
| const stream_key = url.searchParams.get("stream_key"); | |
| if (!stream_key) { | |
| jsonResponse(res, 400, { ok: false, error: "stream_key parameter required" }); | |
| return; | |
| } | |
| const session = sessions.get(stream_key); | |
| if (!session) { | |
| jsonResponse(res, 404, { ok: false, error: "not found" }); | |
| return; | |
| } | |
| const out: StatusResponse = { | |
| state: session.state, | |
| lastError: session.lastError | |
| }; | |
| jsonResponse(res, 200, out); | |
| return; |
🤖 Prompt for AI Agents
In src/controlServer.ts around lines 342 to 354, the status endpoint currently
treats an absent stream_key as an empty string and proceeds to lookup sessions
which leads to a 404; instead explicitly reject missing or empty stream_key and
return a 400 Bad Request with a clear error message (e.g., { ok: false, error:
"stream_key required" }) before attempting to get the session, so check if
url.searchParams.get("stream_key") is null or empty and respond 400, otherwise
continue with the existing session lookup and 200/404 handling.
| } catch (e) { | ||
| jsonResponse(res, 500, { ok: false, error: e instanceof Error ? e.message : String(e) }); | ||
| } |
There was a problem hiding this comment.
Major: Generic error responses may leak sensitive information.
The global catch handler returns the full error message (line 358), which could expose internal paths, tokens, or other sensitive details from stack traces.
Apply this diff to sanitize error responses:
} catch (e) {
- jsonResponse(res, 500, { ok: false, error: e instanceof Error ? e.message : String(e) });
+ console.error("Request error:", e);
+ jsonResponse(res, 500, { ok: false, error: "Internal server error" });
}🤖 Prompt for AI Agents
In src/controlServer.ts around lines 357 to 359, the catch handler currently
returns the raw error message to clients which can leak sensitive internals;
change it to log the full error server-side (use existing logger or
console.error with the caught error/stack) and return a sanitized generic
response to the client such as { ok: false, error: "Internal server error" }
(optionally include a non-sensitive error id or code for correlation), ensuring
you do not expose e.message or stack in the JSON response while keeping the HTTP
500 status.
…ssion.ffmpegCleanup, merge ffmpeg options, support encoder (nvenc) and pipe demux streams Co-authored-by: Genie <[email protected]>
cosineai
bot
force-pushed
the
cosine/docs/usage-workflow-issues
branch
from
6fddd93 to
255bc82
Compare
…trs, add ffmpeg cleanup, demux handling, and nvenc encoder support Co-authored-by: Genie <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/controlServer.ts (1)
251-251: Minor: Remove unsupported audio option (still present).The
-lfe_mix_level 1option was flagged in a previous review as unsupported for 2-channel Opus and marked as "✅ Addressed in commit 6fddd93", but it's still present in the code. This option can cause FFmpeg errors with stereo Opus output.Apply this diff:
if (merged.includeAudio) { command.addOutputOption("-map 0:a?"); command.audioChannels(2); - command.addOutputOption("-lfe_mix_level 1"); command.audioFrequency(48000);
🧹 Nitpick comments (1)
src/controlServer.ts (1)
10-10: Remove unused import.
prepareStreamis imported but never used in this file.Apply this diff:
-import { prepareStream } from "./media/newApi.js";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controlServer.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/controlServer.ts (6)
src/client/voice/StreamConnection.ts (1)
StreamConnection(4-33)src/client/Streamer.ts (2)
Streamer(26-279)opts(51-53)src/utils.ts (2)
isFiniteNonZero(29-31)SupportedVideoCodec(24-24)src/media/LibavDemuxer.ts (1)
demux(103-274)src/media/VideoStream.ts (1)
VideoStream(4-15)src/media/AudioStream.ts (1)
AudioStream(4-15)
🔇 Additional comments (2)
src/controlServer.ts (2)
241-245: LGTM: Encoder selection now respects request.The code now correctly uses
h264_nvencwhenmerged.encoder === "nvenc"and falls back tolibx264for software encoding, addressing the previous concern about hardcoded encoder selection.
366-371: LGTM: Auto-start block is well-structured.The auto-start logic correctly detects direct execution, handles errors gracefully, and exits with an appropriate error code.
Co-authored-by: Genie <[email protected]>
…iveRequest Co-authored-by: Genie <[email protected]>
Co-authored-by: Genie <[email protected]>
…fs/heads/cosine/docs/usage-workflow-issues
…/demux for ffmpeg, improve session lifecycle, UDP/WS wait and graceful shutdown Co-authored-by: Genie <[email protected]>
… include encoder in ffmpeg log Co-authored-by: Genie <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (12)
src/controlServer.ts (12)
78-93: [Duplicate] Harden JSON parsing: cap body size and validate content-type.This issue was raised in a previous review and remains unaddressed. The parser currently reads unlimited request bodies without content-type validation, exposing the server to DoS attacks and malformed input.
99-103: [Duplicate] Secure the control plane: require auth and bind to localhost.This critical security issue was raised in a previous review and remains unaddressed. The control server starts without authentication and binds to all network interfaces by default, exposing Discord tokens and streaming controls to unauthorized access.
113-119: [Duplicate] Add guard to prevent accidental self-bot misuse and document ToS risk.This critical compliance issue was raised in a previous review and remains unaddressed. Using discord.js-selfbot-v13 violates Discord's Terms of Service and Community Guidelines, risking account suspension. The code provides no safeguard against accidental deployment.
172-177: [Duplicate] Per-session encryption vs global mutable option.This major issue was raised in a previous review and remains unaddressed. Mutating
streamer.opts.forceChacha20Encryptionis a global change that affects all sessions. Concurrent sessions with differentencryptionPreferencevalues will conflict.
207-305: [Duplicate] Add handshake timeout for select_protocol_ack.This major issue was raised in a previous review and remains unaddressed. Without a timeout, sessions can hang in the "starting" state indefinitely if the SELECT_PROTOCOL_ACK never arrives, leaking resources.
221-223: [Duplicate] Reject negative/zero video dimensions and fps.This major issue was raised in a previous review and remains unaddressed.
isFiniteNonZeroallows negative values, which would produce invalid video configuration.
268-269: [Duplicate] Null check missing for conn.udp.This critical issue was raised in a previous review and remains unaddressed.
conn.udpis accessed without verifying it exists. If the connection hasn't fully initialized, this will throw a runtime error.
274-283: [Duplicate] Add error handlers for stream pipes.This major issue was raised in a previous review and remains unaddressed. The video and audio streams are piped without registering error handlers. If either pipe encounters an error, it will bubble up uncaught and crash the process.
311-316: [Duplicate] Validate request body structure before use.This major issue was raised in a previous review and remains unaddressed. The handler casts the parsed body to
StartGoLiveRequestwithout validating that required fields exist or have correct types. Invalid payloads will cause cryptic errors deeper instartGoLive.
317-329: [Duplicate] Validate stop request body.This major issue was raised in a previous review and remains unaddressed. The stop handler doesn't validate that
stream_keyexists and is a non-empty string before using it.
331-344: [Duplicate] Reject empty stream_key in status endpoint.This minor issue was raised in a previous review and remains unaddressed. The status endpoint allows an empty string for
stream_keyvia the?? ""fallback, which will always result in a 404. It's clearer to reject this explicitly with a 400 error.
346-350: [Duplicate] Generic error responses may leak sensitive information.This major issue was raised in a previous review and remains unaddressed. The global catch handler returns the full error message, which could expose internal paths, tokens, or other sensitive details from stack traces.
🧹 Nitpick comments (1)
src/controlServer.ts (1)
347-348: Reuse logger instance instead of creating new ones.Multiple
new Log("control")instances are created in the catch handler (line 347), listen callback (line 355), and auto-start block (line 367). This is inefficient and inconsistent with the logger declared at line 105.Apply this diff to reuse the outer-scope logger:
export async function startControlServer() { // ... token check ... const controlLog = new Log("control"); // ... other loggers ... // ... streamer setup ... async function startGoLive(payload: StartGoLiveRequest) { // ... uses controlLog from closure ... } const server = http.createServer(async (req, res) => { try { // ... routes ... } catch (e) { - const controlLog = new Log("control"); controlLog.error(e, "HTTP handler error"); jsonResponse(res, 500, { ok: false, error: e instanceof Error ? e.message : String(e) }); } }); const port = Number(process.env.PORT ?? 3000); server.listen(port, () => { - const controlLog = new Log("control"); controlLog.info({ port }, `Control server listening on http://localhost:${port}`); }); return { server, streamer }; } // Proto logger for protocol ACK steps const protoLog = new Log("control:proto"); // Auto-start if invoked directly if (import.meta.url === `file://${process.argv[1]}`) { - const controlLog = new Log("control"); + const autoStartLog = new Log("control"); startControlServer().catch((e) => { - controlLog.error(e, "Failed to start control server"); + autoStartLog.error(e, "Failed to start control server"); process.exit(1); }); }Also applies to: 355-356, 367-367
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controlServer.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/controlServer.ts (8)
src/client/voice/StreamConnection.ts (1)
StreamConnection(4-33)src/client/voice/BaseMediaConnection.ts (1)
start(142-181)src/client/Streamer.ts (1)
Streamer(26-279)src/utils.ts (2)
isFiniteNonZero(29-31)SupportedVideoCodec(24-24)src/media/newApi.ts (1)
prepareStream(107-372)src/media/LibavDemuxer.ts (1)
demux(103-274)src/media/VideoStream.ts (1)
VideoStream(4-15)src/media/AudioStream.ts (1)
AudioStream(4-15)
| const udpReady = await waitFor(() => conn.udp.ready, 5000, "udp.ready"); | ||
| udpLog.info({ ready: udpReady, ip: conn.udp.ip, port: conn.udp.port }, "UDP readiness"); |
There was a problem hiding this comment.
Guard against undefined conn.udp before accessing properties.
conn.udp is accessed without verifying it exists. If the UDP connection hasn't initialized, this will throw a runtime error that crashes the handler.
Apply this diff to add a null check:
+ if (!conn.udp) {
+ throw new Error("UDP connection not initialized");
+ }
const udpReady = await waitFor(() => conn.udp.ready, 5000, "udp.ready");
udpLog.info({ ready: udpReady, ip: conn.udp.ip, port: conn.udp.port }, "UDP readiness");🤖 Prompt for AI Agents
In src/controlServer.ts around lines 213-214, the code accesses conn.udp.ready,
conn.udp.ip and conn.udp.port without verifying conn.udp exists; add a
null/undefined guard before using those properties so the handler won't throw if
UDP hasn't initialized. Specifically, check if conn.udp is truthy before calling
waitFor(() => conn.udp.ready, ...); if conn.udp is undefined, skip the waitFor
and set udpReady to false (or an appropriate sentinel) and log ip/port as
undefined (or omit them) when calling udpLog.info. Ensure you don’t call
conn.udp.* when conn.udp is falsy so the handler remains safe.
…S log flags, request timing, and /go-live/debug endpoint Co-authored-by: Genie <[email protected]>
…lags and timing; parse bodies early and add /go-live/debug endpoint Co-authored-by: Genie <[email protected]>
…/WS body/message logging via env flags Co-authored-by: Genie <[email protected]>
…lags for HTTP/WS bodies; log HTTP requests/bodies, WS messages, and improve startup/error logging Co-authored-by: Genie <[email protected]>
…erver.ts Co-authored-by: Genie <[email protected]>
…fs/heads/cosine/docs/usage-workflow-issues
Co-authored-by: Genie <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (13)
src/controlServer.ts (13)
112-127: DoS protection still missing: body size limit and content-type validation.This issue was previously flagged. The parser still reads unbounded request bodies and doesn't validate content-type, leaving the server vulnerable to resource exhaustion attacks.
133-146: Authentication still missing.This issue was previously flagged. The control server starts without requiring any authentication token (e.g., CONTROL_AUTH_TOKEN), and the HTTP endpoints remain unauthenticated, exposing a critical attack surface.
198-203: Global encryption preference mutation still present.This issue was previously flagged. The code still mutates
streamer.opts.forceChacha20Encryption, which is a global setting that will affect all concurrent sessions. If two sessions request different encryption modes, they will conflict.
245-245: No timeout for select_protocol_ack handler.This issue was previously flagged. Without a timeout, if the ACK never arrives, the session will hang in "starting" state indefinitely. A timeout (e.g., 10 seconds) should be added to transition the session to an error state and clean up resources.
250-251: Critical: conn.udp accessed without null check.This issue was previously flagged. If the UDP connection hasn't initialized, accessing
conn.udp.ready,conn.udp.ip, orconn.udp.portwill throw a runtime error that crashes the handler.Add a guard before line 250:
+ if (!conn.udp) { + throw new Error("UDP connection not initialized"); + } const udpReady = await waitFor(() => conn.udp.ready, 5000, "udp.ready");
258-260: Video dimensions and fps allow negative values.This issue was previously flagged.
isFiniteNonZeropermits negative numbers, which are invalid for width, height, and fps. Use a positive-integer guard instead.Apply this diff:
- const cfgWidth = isFiniteNonZero(vidAttrs?.width) ? Math.round(vidAttrs!.width!) : undefined; - const cfgHeight = isFiniteNonZero(vidAttrs?.height) ? Math.round(vidAttrs!.height!) : undefined; - const cfgFps = isFiniteNonZero(vidAttrs?.fps) ? Math.round(vidAttrs!.fps!) : undefined; + const toPosInt = (n: unknown) => + typeof n === "number" && Number.isFinite(n) && n > 0 ? Math.round(n) : undefined; + const cfgWidth = toPosInt(vidAttrs?.width); + const cfgHeight = toPosInt(vidAttrs?.height); + const cfgFps = toPosInt(vidAttrs?.fps);
305-306: Critical: conn.udp accessed without null check.This issue was previously flagged. Accessing
conn.udpwithout verifying it exists will cause a runtime error if the UDP connection hasn't fully initialized.Apply this diff:
const udp = conn.udp; + if (!udp) { + throw new Error("UDP connection not ready"); + } udp.setPacketizer(codec);
311-320: Stream pipes missing error handlers.This issue was previously flagged. The video and audio streams are piped without error handlers. If either pipe encounters an error (network issues, codec mismatches, etc.), the error will bubble up uncaught.
Apply this diff:
+ const onPipeError = (e: unknown) => { + controlLog.error(e, "Stream pipe error"); + session.state = "error"; + session.lastError = e instanceof Error ? e.message : String(e); + }; + const vStream = new VideoStream(udp); session.cleanupFns.push(() => vStream.destroy()); - video.stream.pipe(vStream); + video.stream.on("error", onPipeError).pipe(vStream); + vStream.on("error", onPipeError); streamLog.info("Piping video stream to RTP"); if (audio) { const aStream = new AudioStream(udp); session.cleanupFns.push(() => aStream.destroy()); - audio.stream.pipe(aStream); + audio.stream.on("error", onPipeError).pipe(aStream); + aStream.on("error", onPipeError); vStream.syncStream = aStream; streamLog.info("Piping audio stream to RTP and enabling A/V sync"); }
349-356: Request body validation still missing.This issue was previously flagged. The handler casts the parsed body to
StartGoLiveRequestwithout validating that required fields exist or have correct types. Invalid payloads will cause cryptic errors instartGoLive.
358-373: Stop request validation still missing.This issue was previously flagged. The handler doesn't validate that
stream_keyexists and is a non-empty string before attempting to look up the session.
375-388: Status endpoint allows empty stream_key.This issue was previously flagged. The endpoint allows an empty string for
stream_key(line 376 with?? ""), which will always result in a 404. Explicitly rejecting with a 400 error would be clearer.
391-394: Generic error responses may leak sensitive information.This issue was previously flagged. The global catch handler returns the full error message, which could expose internal paths, tokens, or other sensitive details from stack traces.
397-398: Server binds to all interfaces without restriction.This issue was previously flagged. The server binds to
0.0.0.0(all network interfaces) by default, exposing the unauthenticated control endpoints to any network the host is connected to. It should bind to127.0.0.1or accept a configurableHOSTenvironment variable.Apply this diff:
const port = Number(process.env.PORT ?? 3000); - server.listen(port, () => { + const host = process.env.HOST ?? "127.0.0.1"; + server.listen(port, host, () => { - controlLog.info({ port }, `Control server listening on http://localhost:${port}`); + controlLog.info({ port, host }, `Control server listening on http://${host}:${port}`); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controlServer.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/controlServer.ts (8)
src/client/voice/StreamConnection.ts (1)
StreamConnection(4-33)src/client/voice/BaseMediaConnection.ts (1)
start(142-181)src/client/Streamer.ts (1)
Streamer(26-279)src/utils.ts (2)
isFiniteNonZero(29-31)SupportedVideoCodec(24-24)src/media/newApi.ts (1)
prepareStream(107-372)src/media/LibavDemuxer.ts (1)
demux(103-274)src/media/VideoStream.ts (1)
VideoStream(4-15)src/media/AudioStream.ts (1)
AudioStream(4-15)
| // Auto-start if invoked directly | ||
| if (import.meta.url === `file://${process.argv[1]}`) { | ||
| startControlServer().catch((e) => { | ||
| controlLog.error(e, "Failed to start control server"); | ||
| process.exit(1); | ||
| }); | ||
| }`) { | ||
| const controlLog = new Log("control"); | ||
| startControlServer().catch((e) => { | ||
| controlLog.error(e, "Failed to start control server"); | ||
| process.exit(1); | ||
| }); | ||
| } No newline at end of file |
There was a problem hiding this comment.
Critical: Syntax error and duplicate code in auto-start block.
Lines 405-417 contain malformed syntax that will prevent the file from compiling. Line 411 has }) {which is invalid, and lines 412-416 duplicate the error handling code. Additionally, line 412 re-declarescontrolLog` which is already declared at line 14. This appears to be a merge conflict or copy-paste error.
Apply this diff to fix:
// Auto-start if invoked directly
if (import.meta.url === `file://${process.argv[1]}`) {
startControlServer().catch((e) => {
controlLog.error(e, "Failed to start control server");
process.exit(1);
});
-}`) {
- const controlLog = new Log("control");
- startControlServer().catch((e) => {
- controlLog.error(e, "Failed to start control server");
- process.exit(1);
- });
}🤖 Prompt for AI Agents
In src/controlServer.ts around lines 405-417 there is a malformed and duplicated
auto-start block (the token "`}`) {`" at line 411 is invalid and lines 412-416
re-declare and duplicate the controlLog handling). Remove the
duplicated/malformed block and replace it with a single correct auto-start
check: keep only one if (import.meta.url === `file://${process.argv[1]}`) {
startControlServer().catch((e) => { controlLog.error(e, "Failed to start control
server"); process.exit(1); }); } ensuring you do not re-declare controlLog (use
the existing controlLog declared at line 14).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (12)
src/controlServer.ts (12)
112-127: Harden JSON parsing: cap body size and validate content-type.The parser reads unbounded request bodies without content-type validation, exposing the server to DoS attacks via oversized or malformed payloads.
133-138: Secure the control plane: require auth and bind to localhost.The HTTP endpoints accept unauthenticated requests on all interfaces (default 0.0.0.0), creating a critical attack surface for unauthorized stream hijacking and resource exhaustion.
Also applies to: 345-400
139-146: Add guard to prevent accidental self-bot misuse and document ToS risk.Using discord.js-selfbot-v13 violates Discord's Terms of Service and Community Guidelines, risking account termination. No startup guard prevents accidental deployment.
245-342: Add handshake timeout for select_protocol_ack.The handler waits indefinitely for
select_protocol_ack. If the event never fires (network issues, Discord server problems), the session remains in "starting" state forever, leaking resources.
250-251: Guard against undefined conn.udp before accessing properties.Accessing
conn.udp.ready,conn.udp.ip, andconn.udp.portwithout verifyingconn.udpexists will throw a runtime error if the UDP connection hasn't initialized.
258-260: Reject negative/zero video dimensions and fps.
isFiniteNonZeropermits negative values. Negative width, height, or fps will cause errors in video attribute negotiation or packetizer setup.
305-306: Null check missing for conn.udp.
conn.udpis accessed without verifying it exists. If the connection hasn't fully initialized, this will throw a runtime error.
311-311: Add error handlers for stream pipes.Video and audio streams are piped without error handlers. Errors in the pipe (codec mismatches, network issues) will bubble up uncaught and potentially crash the process.
Also applies to: 317-317
349-356: Validate request body structure before use.The handler casts the parsed body to
StartGoLiveRequestwithout validating required fields exist or have correct types. Invalid payloads cause cryptic errors deeper instartGoLive.
358-373: Validate stop request body.The stop handler doesn't validate that
stream_keyexists and is a non-empty string.
375-388: Reject empty stream_key in status endpoint.The status endpoint allows an empty string for
stream_key(line 376 with?? ""), which always results in a 404. Clearer to reject explicitly with a 400 error.
391-394: Generic error responses may leak sensitive information.The global catch handler returns the full error message (line 393), which could expose internal paths, tokens, or stack traces from the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controlServer.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/controlServer.ts (8)
src/client/voice/StreamConnection.ts (1)
StreamConnection(4-33)src/client/voice/BaseMediaConnection.ts (2)
streamer(116-118)start(142-181)src/client/Streamer.ts (1)
Streamer(26-279)src/utils.ts (2)
isFiniteNonZero(29-31)SupportedVideoCodec(24-24)src/media/newApi.ts (1)
prepareStream(107-372)src/media/LibavDemuxer.ts (1)
demux(103-274)src/media/VideoStream.ts (1)
VideoStream(4-15)src/media/AudioStream.ts (1)
AudioStream(4-15)
| // Encryption preference tweak | ||
| if (encryptionPreference === "XCHACHA20") { | ||
| streamer.opts.forceChacha20Encryption = true; | ||
| } else if (encryptionPreference === "AES256") { | ||
| streamer.opts.forceChacha20Encryption = false; | ||
| } |
There was a problem hiding this comment.
Per-session encryption preference still mutates global state.
Despite being marked as addressed in previous review (commit 0343aa7), the code still mutates streamer.opts.forceChacha20Encryption, which affects all concurrent sessions. If two sessions request different encryption preferences, they will conflict.
Consider passing encryption preference to the StreamConnection constructor or via a setter method that applies per-instance configuration rather than mutating the global Streamer.opts object.
🤖 Prompt for AI Agents
In src/controlServer.ts around lines 198 to 203, the code still mutates the
global streamer.opts.forceChacha20Encryption per request which causes
cross-session conflicts; instead, modify the code to stop touching streamer.opts
and pass the encryption preference into the StreamConnection instance
(preferably via a new constructor parameter or an instance setter) so each
StreamConnection stores its own forceChacha20Encryption flag; ensure the new
parameter/setter respects a default when not provided, update all
StreamConnection instantiations to supply the preference from the request
context, and remove the global mutation to preserve per-session isolation.
| const { video, audio } = await demux(prep.output, { format: "nut" }); | ||
| if (!video) throw new Error("No video stream"); | ||
| demuxLog.info({ | ||
| width: video.width, height: video.height, | ||
| framerate_num: video.framerate_num, framerate_den: video.framerate_den, | ||
| codec: video.codec | ||
| }, "Demuxed video stream"); |
There was a problem hiding this comment.
Demux error handling missing cleanup.
Despite being marked as addressed in previous review (commits 255bc82-5bad587), the demux call lacks error handling to cleanup the already-running FFmpeg process and PassThrough stream. If demux fails (e.g., invalid format, no video stream), resources leak.
Wrap the demux call and subsequent video stream validation in a try-catch block that calls prep.command.kill(), prep.output.destroy(), and abort.abort() before re-throwing.
🤖 Prompt for AI Agents
In src/controlServer.ts around lines 270 to 276, the demux call and subsequent
video existence check must be wrapped in a try-catch so failed demuxes don't
leak resources; move the await demux(...) and the if (!video) throw into a try
block, and in the catch call prep.command.kill(), prep.output.destroy(), and
abort.abort() before re-throwing the caught error so the FFmpeg process and
PassThrough stream are cleaned up on failure.
What this PR adds
High-level user workflow
Set environment variables and start the server
External controller (Python, etc.) obtains the handoff details from Discord (gateway/gateway-events or the code path in your controller that does the STREAM_CREATE handshake). The controller must provide a payload with keys below.
Start a Go Live session by POSTing JSON to /go-live/start with required fields (see shape below). The control server
Stop a stream via POST /go-live/stop with { "stream_key": "..." } or poll status via GET /go-live/status?stream_key=...
Important request/response schemas
POST /go-live/start (StartGoLiveRequest)
Required fields: guild_id, channel_id, user_id, session_id, stream_key, rtc_server_id, endpoint, token, ffmpeg.input
Optional: video: { width, height, fps }, encryptionPreference: "AES256" | "XCHACHA20"
ffmpeg.options supports: width, height, frameRate, bitrateVideo, bitrateVideoMax, bitrateAudio, includeAudio, encoder, hardwareAcceleratedDecoding, minimizeLatency, customHeaders, customFfmpegFlags
POST /go-live/stop
Body: { stream_key: string }
GET /go-live/status?stream_key=...
Returns state: "starting" | "running" | "stopping" | "stopped" | "error" and lastError
Example curl
Start (replace values):
curl -X POST http://localhost:3000/go-live/start -H 'Content-Type: application/json' -d '{ "guild_id":"G", "channel_id":"C", "user_id":"U", "session_id":"S", "stream_key":"KEY", "rtc_server_id":"RS", "endpoint":"https://...", "token":"VOICE_TOKEN", "ffmpeg": { "input": "http://stream-or-file.mp4", "options": { "bitrateVideo": 4000 } } }'
Stop:
curl -X POST http://localhost:3000/go-live/stop -H 'Content-Type: application/json' -d '{ "stream_key":"KEY" }'
Behavior details and things to be aware of
Authentication & Discord token:
Control server is unauthenticated over HTTP:
Handoff model:
ffmpeg pipeline and demuxing:
Codec handling and mapping:
Encryption preference:
Resource cleanup:
Notable implementation issues / potential bugs and recommended fixes
Variable name collision with ffmpeg
No validation/sanitization
Missing auth on HTTP API
select_protocol_ack assumption
FFmpeg flags and encoder selection
Error reporting & telemetry
Potential for leaked resources
Dependency chain / requirements
Security and policy notes
Operational tips & debugging
Suggested improvements before production
If you want, I can:
Summary
This adds a small but functional HTTP control server that accepts a Go Live handoff and runs an ffmpeg->demux->packetize->UDP streaming pipeline into the project's Streamer/StreamConnection stack. It is functional for local or trusted environments but needs security hardening, input validation, and a couple of bug fixes (ffmpeg naming collision and robust process cleanup) before production use.
Original Task: Discord-video-stream/y252nitjslhg
Author: Yui Koi
Summary by CodeRabbit
New Features
Chores