From 6d2bb7aeb06f134057abb73c9016e38f2a359fb8 Mon Sep 17 00:00:00 2001 From: Lucas Santana Date: Fri, 10 Apr 2026 12:34:18 -0300 Subject: [PATCH 1/5] fix(bot): replace play-dl youtube streaming with yt-dlp in bridge play-dl 1.9.7 youtube streaming is broken due to bot detection and the PO token requirement. the bridge was silently falling through to soundcloud which also failed for tracks not available there. replace the first bridge stage with a yt-dlp subprocess spawn. resolves with stdout once the first data chunk arrives; rejects on timeout (15s) or non-zero exit code. soundcloud full-query and title-only fallbacks remain unchanged. export streamViaYtDlp for unit testing. update bridge.spec.ts to mock child_process.spawn. all 23 bridge tests pass. --- .../player/playerFactory.bridge.spec.ts | 119 ++++++++++++++---- .../bot/src/handlers/player/playerFactory.ts | 71 +++++++++-- 2 files changed, 157 insertions(+), 33 deletions(-) diff --git a/packages/bot/src/handlers/player/playerFactory.bridge.spec.ts b/packages/bot/src/handlers/player/playerFactory.bridge.spec.ts index 7e7765063..9d53727ed 100644 --- a/packages/bot/src/handlers/player/playerFactory.bridge.spec.ts +++ b/packages/bot/src/handlers/player/playerFactory.bridge.spec.ts @@ -1,14 +1,16 @@ import { beforeEach, describe, expect, it, jest } from '@jest/globals' +import { EventEmitter } from 'events' +import { PassThrough } from 'stream' import type { Readable } from 'stream' const playdlSearchMock = jest.fn() const playdlStreamMock = jest.fn() +const spawnMock = jest.fn() + +jest.mock('child_process', () => ({ + spawn: (...args: unknown[]) => spawnMock(...args), +})) -// Stub `discord-player` and `@discord-player/extractor` so importing -// playerFactory.ts doesn't pull in their full dependency trees (one of -// which requires the native `file-type` module that isn't present in -// the test environment). We only exercise the bridge fallback chain — -// the Player class and DefaultExtractors aren't touched in these tests. jest.mock('discord-player', () => ({ Player: class { extractors = { @@ -37,10 +39,9 @@ jest.mock('@lucky/shared/utils', () => ({ debugLog: jest.fn(), })) -// Import AFTER the mocks so the real module wires against the mock. -// eslint-disable-next-line @typescript-eslint/no-var-requires import { createResilientStream, + streamViaYtDlp, streamViaSoundCloud, findMatchingSoundCloudResult, parseDurationString, @@ -48,6 +49,26 @@ import { const fakeStream = { on: jest.fn() } as unknown as Readable +function makeSpawnSuccess() { + const stdout = new PassThrough() + const proc = Object.assign(new EventEmitter(), { + stdout, + kill: jest.fn(), + }) + setImmediate(() => stdout.emit('data', Buffer.from('audio'))) + return proc +} + +function makeSpawnError(code = 1) { + const stdout = new PassThrough() + const proc = Object.assign(new EventEmitter(), { + stdout, + kill: jest.fn(), + }) + setImmediate(() => proc.emit('close', code)) + return proc +} + function makeTrack(overrides: Partial> = {}) { return { title: 'Bohemian Rhapsody', @@ -58,7 +79,7 @@ function makeTrack(overrides: Partial> = {}) { } } -describe('parseDurationString (real export)', () => { +describe('parseDurationString', () => { it('parses m:ss', () => { expect(parseDurationString('3:45')).toBe(225) }) @@ -81,7 +102,7 @@ describe('parseDurationString (real export)', () => { }) }) -describe('findMatchingSoundCloudResult (real export)', () => { +describe('findMatchingSoundCloudResult', () => { const results = [ { name: 'Bohemian Rhapsody', url: 'sc://1', durationInSec: 354 }, { name: 'Unrelated', url: 'sc://2', durationInSec: 180 }, @@ -166,25 +187,78 @@ describe('streamViaSoundCloud', () => { }) }) +describe('streamViaYtDlp', () => { + beforeEach(() => { + spawnMock.mockReset() + }) + + it('resolves with stdout when yt-dlp emits data', async () => { + const proc = makeSpawnSuccess() + spawnMock.mockReturnValue(proc) + + const stream = await streamViaYtDlp('https://youtube.com/watch?v=test') + expect(stream).toBe(proc.stdout) + expect(spawnMock).toHaveBeenCalledWith( + 'yt-dlp', + expect.arrayContaining([ + '--no-playlist', + '-o', + '-', + 'https://youtube.com/watch?v=test', + ]), + expect.objectContaining({ stdio: ['ignore', 'pipe', 'pipe'] }), + ) + }) + + it('rejects when yt-dlp closes with non-zero exit code', async () => { + const proc = makeSpawnError(1) + spawnMock.mockReturnValue(proc) + + await expect( + streamViaYtDlp('https://youtube.com/watch?v=test'), + ).rejects.toThrow(/exited with code 1/) + }) + + it('rejects when spawn emits error', async () => { + const stdout = new PassThrough() + const proc = Object.assign(new EventEmitter(), { + stdout, + kill: jest.fn(), + }) + spawnMock.mockReturnValue(proc) + setImmediate(() => proc.emit('error', new Error('ENOENT'))) + + await expect( + streamViaYtDlp('https://youtube.com/watch?v=test'), + ).rejects.toThrow('ENOENT') + }) +}) + describe('createResilientStream', () => { beforeEach(() => { + spawnMock.mockReset() playdlSearchMock.mockReset() playdlStreamMock.mockReset() }) - it('streams directly from source URL on first attempt', async () => { - playdlStreamMock.mockResolvedValueOnce({ stream: fakeStream }) + it('streams via yt-dlp from source URL on first attempt', async () => { + const proc = makeSpawnSuccess() + spawnMock.mockReturnValue(proc) const result = await createResilientStream(makeTrack()) - expect(result).toBe(fakeStream) - expect(playdlStreamMock).toHaveBeenCalledWith( - 'https://youtube.com/watch?v=fakeBohemian', + expect(result).toBe(proc.stdout) + expect(spawnMock).toHaveBeenCalledWith( + 'yt-dlp', + expect.arrayContaining([ + 'https://youtube.com/watch?v=fakeBohemian', + ]), + expect.anything(), ) expect(playdlSearchMock).not.toHaveBeenCalled() }) - it('falls back to SoundCloud primary search when direct stream fails', async () => { - playdlStreamMock.mockRejectedValueOnce(new Error('403')) + it('falls back to SoundCloud primary search when yt-dlp fails', async () => { + spawnMock.mockReturnValue(makeSpawnError(1)) playdlSearchMock.mockResolvedValueOnce([ { name: 'Bohemian Rhapsody - Queen', @@ -201,7 +275,7 @@ describe('createResilientStream', () => { }) it('falls back to SoundCloud title-only when primary returns no validated match', async () => { - playdlStreamMock.mockRejectedValueOnce(new Error('403')) + spawnMock.mockReturnValue(makeSpawnError(1)) playdlSearchMock .mockResolvedValueOnce([ { name: 'Unrelated', url: 'sc://miss', durationInSec: 180 }, @@ -221,8 +295,9 @@ describe('createResilientStream', () => { expect(playdlStreamMock).toHaveBeenCalledWith('sc://secondary') }) - it('streams directly from source URL even for spam uploader channels', async () => { - playdlStreamMock.mockResolvedValueOnce({ stream: fakeStream }) + it('streams via yt-dlp even for spam uploader channels', async () => { + const proc = makeSpawnSuccess() + spawnMock.mockReturnValue(proc) const result = await createResilientStream( makeTrack({ @@ -230,11 +305,11 @@ describe('createResilientStream', () => { author: 'Best Songs', }), ) - expect(result).toBe(fakeStream) + expect(result).toBe(proc.stdout) expect(playdlSearchMock).not.toHaveBeenCalled() }) - it('throws "Bridge exhausted" when a track has no URL and SoundCloud fails', async () => { + it('throws "Bridge exhausted" when track has no URL and SoundCloud fails', async () => { playdlSearchMock.mockResolvedValue([]) await expect( @@ -243,7 +318,7 @@ describe('createResilientStream', () => { }) it('throws "Bridge exhausted" when all stages fail', async () => { - playdlStreamMock.mockRejectedValueOnce(new Error('youtube 403')) + spawnMock.mockReturnValue(makeSpawnError(1)) playdlSearchMock.mockResolvedValue([]) await expect(createResilientStream(makeTrack())).rejects.toThrow( diff --git a/packages/bot/src/handlers/player/playerFactory.ts b/packages/bot/src/handlers/player/playerFactory.ts index 454e1ffaa..b41e80332 100644 --- a/packages/bot/src/handlers/player/playerFactory.ts +++ b/packages/bot/src/handlers/player/playerFactory.ts @@ -2,6 +2,7 @@ import { Player } from 'discord-player' import type { Track } from 'discord-player' import { DefaultExtractors } from '@discord-player/extractor' import * as playdl from 'play-dl' +import { spawn } from 'child_process' import type { Readable } from 'stream' import type { CustomClient } from '../../types' import { errorLog, infoLog, warnLog, debugLog } from '@lucky/shared/utils' @@ -101,14 +102,63 @@ const loadYoutubeExtractor = async (player: Player): Promise => { } } +/** + * Stream audio via yt-dlp subprocess. + * + * Resolves with the stdout Readable once yt-dlp begins writing data. + * Rejects with a timeout error if no data arrives within 15 seconds. + * Exported for testing. + */ +export function streamViaYtDlp(url: string): Promise { + return new Promise((resolve, reject) => { + const proc = spawn( + 'yt-dlp', + [ + '--no-playlist', + '-f', + 'bestaudio/best', + '-o', + '-', + '--quiet', + '--no-warnings', + '--no-progress', + url, + ], + { stdio: ['ignore', 'pipe', 'pipe'] }, + ) + + const timeout = setTimeout(() => { + proc.kill() + reject(new Error('yt-dlp: timed out waiting for stream start')) + }, 15_000) + + proc.stdout!.once('data', () => { + clearTimeout(timeout) + resolve(proc.stdout!) + }) + + proc.once('error', (err) => { + clearTimeout(timeout) + reject(err) + }) + + proc.once('close', (code) => { + clearTimeout(timeout) + if (code && code !== 0) { + reject(new Error(`yt-dlp exited with code ${code}`)) + } + }) + }) +} + /** * Bridge fallback chain (discord-player-youtubei v3 createStream signature). * - * Priority: YouTube direct → SoundCloud (full query) → SoundCloud (title only). + * Priority: yt-dlp direct → SoundCloud (full query) → SoundCloud (title only). * - * YouTube direct is attempted first since the track always carries its source - * URL and play-dl can stream it independently of the extractor. SoundCloud - * search is the fallback for tracks blocked by YouTube bot-detection. + * yt-dlp is tried first (play-dl YouTube streaming is broken due to bot + * detection). SoundCloud search is the fallback for tracks unavailable via + * yt-dlp. */ export async function createResilientStream( track: Pick, @@ -130,18 +180,17 @@ export async function createResilientStream( if (track.url) { try { - const direct = await playdl.stream(track.url) + const stream = await streamViaYtDlp(track.url) infoLog({ - message: 'Bridge: streamed directly from source URL', + message: 'Bridge: streamed via yt-dlp', data: { url: track.url, title: cleanedTitle || track.title }, }) - return direct.stream - } catch (directError) { + return stream + } catch (ytdlpError) { debugLog({ - message: - 'Bridge: direct stream failed, falling back to SoundCloud', + message: 'Bridge: yt-dlp failed, falling back to SoundCloud', data: { - error: (directError as Error).message, + error: (ytdlpError as Error).message, cleanedTitle, }, }) From 20ef7ee95f2dc428d764d3bb4e34e8fd5f8e9783 Mon Sep 17 00:00:00 2001 From: Lucas Santana Date: Fri, 10 Apr 2026 13:25:50 -0300 Subject: [PATCH 2/5] fix(bot): validate https scheme in streamviaytdlp before spawn prevents sonarcloud os command injection hotspot by rejecting any url that doesn't start with https:// before passing it to the yt-dlp subprocess. spawn with an args array is not actually vulnerable, but explicit validation satisfies the static analysis rule and makes intent clear. --- .../bot/src/handlers/player/playerFactory.bridge.spec.ts | 7 +++++++ packages/bot/src/handlers/player/playerFactory.ts | 5 +++++ 2 files changed, 12 insertions(+) diff --git a/packages/bot/src/handlers/player/playerFactory.bridge.spec.ts b/packages/bot/src/handlers/player/playerFactory.bridge.spec.ts index 9d53727ed..c81076171 100644 --- a/packages/bot/src/handlers/player/playerFactory.bridge.spec.ts +++ b/packages/bot/src/handlers/player/playerFactory.bridge.spec.ts @@ -210,6 +210,13 @@ describe('streamViaYtDlp', () => { ) }) + it('rejects immediately for non-https URLs without spawning', async () => { + await expect( + streamViaYtDlp('http://youtube.com/watch?v=test'), + ).rejects.toThrow(/invalid URL scheme/i) + expect(spawnMock).not.toHaveBeenCalled() + }) + it('rejects when yt-dlp closes with non-zero exit code', async () => { const proc = makeSpawnError(1) spawnMock.mockReturnValue(proc) diff --git a/packages/bot/src/handlers/player/playerFactory.ts b/packages/bot/src/handlers/player/playerFactory.ts index b41e80332..44cdaa676 100644 --- a/packages/bot/src/handlers/player/playerFactory.ts +++ b/packages/bot/src/handlers/player/playerFactory.ts @@ -110,6 +110,11 @@ const loadYoutubeExtractor = async (player: Player): Promise => { * Exported for testing. */ export function streamViaYtDlp(url: string): Promise { + if (!url.startsWith('https://')) { + return Promise.reject( + new Error(`yt-dlp: invalid URL scheme — only https:// is allowed`), + ) + } return new Promise((resolve, reject) => { const proc = spawn( 'yt-dlp', From 6be0de62202ce1840e6d4f385d7eaf6de479cf0f Mon Sep 17 00:00:00 2001 From: Lucas Santana Date: Fri, 10 Apr 2026 13:38:19 -0300 Subject: [PATCH 3/5] fix(bot): add nosonar annotation to suppress spawn hotspot the url is validated to be https:// before spawn is called. spawn with an args array does not use a shell, so command injection is not possible. nosonar annotation satisfies the sonarcloud security hotspot review requirement. --- packages/bot/src/handlers/player/playerFactory.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/bot/src/handlers/player/playerFactory.ts b/packages/bot/src/handlers/player/playerFactory.ts index 44cdaa676..0b80bd6b0 100644 --- a/packages/bot/src/handlers/player/playerFactory.ts +++ b/packages/bot/src/handlers/player/playerFactory.ts @@ -116,7 +116,9 @@ export function streamViaYtDlp(url: string): Promise { ) } return new Promise((resolve, reject) => { + // URL is validated above to be https:// — no shell injection possible with spawn array args. const proc = spawn( + // NOSONAR 'yt-dlp', [ '--no-playlist', From da960baaaa3cc3904186631a3cd2c2d06578ecc7 Mon Sep 17 00:00:00 2001 From: Lucas Santana Date: Fri, 10 Apr 2026 13:38:41 -0300 Subject: [PATCH 4/5] fix(bot): move nosonar annotation to comment line above spawn prettier formatted the inline comment to a new line, breaking the sonarcloud suppression. the annotation is now on the comment line above spawn() which sonarcloud also recognizes. --- packages/bot/src/handlers/player/playerFactory.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/bot/src/handlers/player/playerFactory.ts b/packages/bot/src/handlers/player/playerFactory.ts index 0b80bd6b0..4a8a9c03c 100644 --- a/packages/bot/src/handlers/player/playerFactory.ts +++ b/packages/bot/src/handlers/player/playerFactory.ts @@ -116,9 +116,8 @@ export function streamViaYtDlp(url: string): Promise { ) } return new Promise((resolve, reject) => { - // URL is validated above to be https:// — no shell injection possible with spawn array args. + // URL validated to https:// above; spawn array args have no shell injection risk. NOSONAR const proc = spawn( - // NOSONAR 'yt-dlp', [ '--no-playlist', From cc55b3e71388ec6675282e10cc4ebde4cadcbceb Mon Sep 17 00:00:00 2001 From: Lucas Santana Date: Fri, 10 Apr 2026 13:48:33 -0300 Subject: [PATCH 5/5] fix(bot): validate ytdlp urls against allowlist of known domains replaces the simple https:// prefix check with full url parsing using the URL constructor. validates both protocol (must be https) and hostname (must be in the known music service allowlist). this gives sonarcloud static analysis enough context to verify the input is sanitized before being passed to spawn. --- .../player/playerFactory.bridge.spec.ts | 28 +++++++++++++-- .../bot/src/handlers/player/playerFactory.ts | 34 ++++++++++++++++--- 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/packages/bot/src/handlers/player/playerFactory.bridge.spec.ts b/packages/bot/src/handlers/player/playerFactory.bridge.spec.ts index c81076171..542a5342e 100644 --- a/packages/bot/src/handlers/player/playerFactory.bridge.spec.ts +++ b/packages/bot/src/handlers/player/playerFactory.bridge.spec.ts @@ -210,13 +210,37 @@ describe('streamViaYtDlp', () => { ) }) - it('rejects immediately for non-https URLs without spawning', async () => { + it('rejects for non-https URLs without spawning', async () => { await expect( streamViaYtDlp('http://youtube.com/watch?v=test'), - ).rejects.toThrow(/invalid URL scheme/i) + ).rejects.toThrow(/only https/i) expect(spawnMock).not.toHaveBeenCalled() }) + it('rejects for domains not in the allowlist without spawning', async () => { + await expect( + streamViaYtDlp('https://evil.com/audio.mp3'), + ).rejects.toThrow(/not in allowlist/i) + expect(spawnMock).not.toHaveBeenCalled() + }) + + it('accepts all allowed domains', async () => { + const allowedUrls = [ + 'https://youtube.com/watch?v=test', + 'https://www.youtube.com/watch?v=test', + 'https://youtu.be/test', + 'https://music.youtube.com/watch?v=test', + 'https://soundcloud.com/artist/track', + 'https://open.spotify.com/track/test', + ] + for (const url of allowedUrls) { + const proc = makeSpawnSuccess() + spawnMock.mockReturnValue(proc) + await expect(streamViaYtDlp(url)).resolves.toBeDefined() + spawnMock.mockReset() + } + }) + it('rejects when yt-dlp closes with non-zero exit code', async () => { const proc = makeSpawnError(1) spawnMock.mockReturnValue(proc) diff --git a/packages/bot/src/handlers/player/playerFactory.ts b/packages/bot/src/handlers/player/playerFactory.ts index 4a8a9c03c..c2143665f 100644 --- a/packages/bot/src/handlers/player/playerFactory.ts +++ b/packages/bot/src/handlers/player/playerFactory.ts @@ -109,14 +109,38 @@ const loadYoutubeExtractor = async (player: Player): Promise => { * Rejects with a timeout error if no data arrives within 15 seconds. * Exported for testing. */ +const ALLOWED_YTDLP_DOMAINS = new Set([ + 'youtube.com', + 'www.youtube.com', + 'youtu.be', + 'music.youtube.com', + 'soundcloud.com', + 'www.soundcloud.com', + 'open.spotify.com', +]) + +function validateYtDlpUrl(url: string): void { + let parsed: URL + try { + parsed = new URL(url) + } catch { + throw new Error(`yt-dlp: invalid URL`) + } + if (parsed.protocol !== 'https:') { + throw new Error(`yt-dlp: only https URLs are allowed`) + } + if (!ALLOWED_YTDLP_DOMAINS.has(parsed.hostname.toLowerCase())) { + throw new Error(`yt-dlp: domain not in allowlist: ${parsed.hostname}`) + } +} + export function streamViaYtDlp(url: string): Promise { - if (!url.startsWith('https://')) { - return Promise.reject( - new Error(`yt-dlp: invalid URL scheme — only https:// is allowed`), - ) + try { + validateYtDlpUrl(url) + } catch (err) { + return Promise.reject(err) } return new Promise((resolve, reject) => { - // URL validated to https:// above; spawn array args have no shell injection risk. NOSONAR const proc = spawn( 'yt-dlp', [