From 85236134d9838624085029e7d2af4dde0d0e53fb Mon Sep 17 00:00:00 2001 From: oratis Date: Sun, 9 Aug 2026 23:03:42 +0800 Subject: [PATCH] fix(scraper): encode YouTube Data API query params through a shared builder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several YouTube Data API v3 request URLs were assembled by interpolating values straight into a template literal. Three were still raw: the video-id lists in scraper.js and index.js, and uploadsPlaylistId in the batch-discovery path. The host is fixed so this is not SSRF, but a value carrying `&` or `#` injects or truncates parameters in an authenticated, quota-metered third-party call. `#` was the worse case — it pushed `&key=...` into a URL fragment that never reached Google — and an injected `part=` silently doubles the parameter and changes the response shape. The handle / customName / channelId paths were already covered by encodeURIComponent (and their extraction regex strips `&` before it reaches the URL), but hand-rolled templates invite the next raw interpolation, so the fix is structural rather than site-by-site. - add server/youtube-api.js: youtubeApiUrl(endpoint, params), which encodes every value and drops nullish ones instead of serialising "undefined" - route all 11 call sites through it — scraper.js (6), index.js (5) - list params (part / id) keep the API's literal comma separator with each element encoded individually; URLSearchParams is deliberately not the encoder, since it emits that separator as %2C Wire format is byte-identical to the previous URLs for real channel and playlist ids. The only intentional change is publishedAfter, whose colons now encode as %3A. Verified: npm test 663/663 (was 656; +7 in server/__tests__/youtube-api-url.test.js covering the builder plus a scrapeYouTube run against a stubbed proxy-fetch). The old string form was checked to fail the same assertions — two `part=` parameters, and the API key lost to the fragment. Client is untouched. Co-Authored-By: Claude Opus 5 --- server/__tests__/youtube-api-url.test.js | 201 +++++++++++++++++++++++ server/index.js | 26 ++- server/scraper.js | 19 ++- server/youtube-api.js | 69 ++++++++ 4 files changed, 304 insertions(+), 11 deletions(-) create mode 100644 server/__tests__/youtube-api-url.test.js create mode 100644 server/youtube-api.js diff --git a/server/__tests__/youtube-api-url.test.js b/server/__tests__/youtube-api-url.test.js new file mode 100644 index 0000000..b6ce095 --- /dev/null +++ b/server/__tests__/youtube-api-url.test.js @@ -0,0 +1,201 @@ +/** + * YouTube Data API v3 URL construction. + * + * Regression guard for query-parameter injection: `handle`, `customName` and + * `channelId` are all parsed out of a user-submitted profile URL, so a handle + * like `abc&part=contentDetails` used to append a second `part=` parameter to + * an authenticated, quota-metered API call (and `abc#` truncated the URL, + * dropping our API key). See server/youtube-api.js. + */ + +const { test } = require('node:test'); +const assert = require('node:assert'); +const Module = require('node:module'); + +const { youtubeApiUrl } = require('../youtube-api'); + +// --- helpers --------------------------------------------------------------- + +function paramsOf(url) { + return new URL(url).searchParams; +} + +/** All values for a repeated key, so we can assert a param appears exactly once. */ +function countKey(url, key) { + return [...paramsOf(url).keys()].filter(k => k === key).length; +} + +// --- builder --------------------------------------------------------------- + +test('youtubeApiUrl: hostile handle cannot inject a second `part` parameter', () => { + const url = youtubeApiUrl('channels', { + part: 'id', + forHandle: 'abc&part=contentDetails', + key: 'test-key', + }); + + assert.strictEqual(countKey(url, 'part'), 1, 'exactly one part= parameter'); + assert.strictEqual(paramsOf(url).get('part'), 'id'); + // The whole hostile string survives intact as one opaque value. + assert.strictEqual(paramsOf(url).get('forHandle'), 'abc&part=contentDetails'); + assert.strictEqual(paramsOf(url).get('key'), 'test-key'); + assert.ok(url.includes('abc%26part%3DcontentDetails'), 'the & and = are percent-encoded'); +}); + +test('youtubeApiUrl: hostile handle cannot shadow the API key', () => { + const url = youtubeApiUrl('channels', { + part: 'id', + forHandle: 'abc&key=attacker-key', + key: 'real-key', + }); + + assert.strictEqual(countKey(url, 'key'), 1); + assert.strictEqual(paramsOf(url).get('key'), 'real-key'); +}); + +test('youtubeApiUrl: `#` cannot truncate the query string', () => { + const url = youtubeApiUrl('channels', { part: 'id', forHandle: 'abc#', key: 'real-key' }); + + // Raw interpolation put `key=` into the fragment, so it never reached Google. + assert.strictEqual(new URL(url).hash, '', 'no fragment'); + assert.strictEqual(paramsOf(url).get('key'), 'real-key'); + assert.strictEqual(paramsOf(url).get('forHandle'), 'abc#'); +}); + +test('youtubeApiUrl: array values become a comma list with each element encoded', () => { + const url = youtubeApiUrl('videos', { + part: ['snippet', 'statistics'], + id: ['vid1', 'vid2&part=contentDetails'], + key: 'test-key', + }); + + // Literal comma separators (the Data API's list syntax) survive... + assert.ok(url.includes('part=snippet,statistics'), 'part list uses literal commas'); + assert.strictEqual(countKey(url, 'part'), 1); + // ...but an element cannot smuggle in a separator or a new parameter. + assert.strictEqual(paramsOf(url).get('id'), 'vid1,vid2&part=contentDetails'); + assert.ok(url.includes('vid2%26part%3DcontentDetails')); +}); + +test('youtubeApiUrl: empty and nullish values are dropped, not stringified', () => { + const url = youtubeApiUrl('channels', { + part: 'id', + id: 'UC123', + key: undefined, // missing YOUTUBE_API_KEY used to serialise as "undefined" + forHandle: null, + order: '', + }); + + assert.strictEqual(paramsOf(url).has('key'), false); + assert.strictEqual(paramsOf(url).has('forHandle'), false); + assert.strictEqual(paramsOf(url).has('order'), false); + assert.strictEqual(paramsOf(url).get('id'), 'UC123'); +}); + +test('youtubeApiUrl: targets the fixed Data API v3 host', () => { + const url = youtubeApiUrl('search', { q: 'cats' }); + assert.strictEqual(new URL(url).origin, 'https://www.googleapis.com'); + assert.strictEqual(new URL(url).pathname, '/youtube/v3/search'); +}); + +// --- real call path -------------------------------------------------------- +// The builder being safe only matters if scrapeYouTube actually routes through +// it, so drive the real function with a stubbed ./proxy-fetch and inspect the +// URLs it puts on the wire. node --test runs each file in its own process, so +// poking require.cache here cannot leak into other test files. +// +// Note on the payload: the handle is extracted with /@([^/?&]+)/, so `&` is +// already stripped before it reaches the URL. `#` is *not* in that character +// class — and under raw interpolation `#` was the worse one, since it turned +// the rest of the query (including `&key=`) into a URL fragment that never +// reached Google. The video ids below have no such filter: they come straight +// off the API response and used to be joined in unencoded. + +test('scrapeYouTube: a hostile @handle cannot truncate or inject parameters', async () => { + const requested = []; + + const proxyFetchPath = require.resolve('../proxy-fetch'); + const stub = async (url) => { + requested.push(url); + const { pathname, searchParams } = new URL(url); + + if (pathname.endsWith('/channels') && searchParams.has('forHandle')) { + return { json: async () => ({ items: [{ id: 'UC_resolved' }] }) }; + } + if (pathname.endsWith('/channels')) { + return { + json: async () => ({ + items: [{ + // Empty description keeps discoverEmail from making network calls. + snippet: { title: 'Test Channel', description: '', thumbnails: {} }, + statistics: { subscriberCount: '1000', viewCount: '5000', videoCount: '10' }, + }], + }), + }; + } + if (pathname.endsWith('/search')) { + // A video id carrying an injection payload — it reaches the videos.list + // call below as an array element. + return { json: async () => ({ items: [{ id: { videoId: 'vid1&part=contentDetails' } }] }) }; + } + if (pathname.endsWith('/videos')) { + return { + json: async () => ({ + items: [{ statistics: { viewCount: '100', likeCount: '10', commentCount: '5' } }], + }), + }; + } + return { json: async () => ({ items: [] }) }; + }; + + const original = require.cache[proxyFetchPath]; + require.cache[proxyFetchPath] = new Module(proxyFetchPath, null); + require.cache[proxyFetchPath].filename = proxyFetchPath; + require.cache[proxyFetchPath].loaded = true; + require.cache[proxyFetchPath].exports = stub; + + const oldKey = process.env.YOUTUBE_API_KEY; + process.env.YOUTUBE_API_KEY = 'secret-api-key'; + + try { + // scraper.js reads YOUTUBE_API_KEY and requires ./proxy-fetch at load time, + // so require it only after both are in place. + const { scrapeYouTube } = require('../scraper'); + + const result = await scrapeYouTube( + 'https://www.youtube.com/@abc#part=contentDetails', + 'abc', + ); + + assert.strictEqual(result.success, true, `scrape failed: ${result.error}`); + assert.ok(requested.length >= 2, 'made at least the resolve + details calls'); + + for (const url of requested) { + assert.strictEqual(countKey(url, 'part'), 1, `exactly one part= in ${url}`); + assert.strictEqual(countKey(url, 'key'), 1, `exactly one key= in ${url}`); + assert.strictEqual(paramsOf(url).get('key'), 'secret-api-key', `API key intact in ${url}`); + assert.strictEqual(new URL(url).hash, '', `no fragment in ${url}`); + assert.strictEqual(new URL(url).origin, 'https://www.googleapis.com'); + } + + // The handle is carried as one opaque value, not as extra parameters, and + // the `#` no longer swallows `&key=...` into a fragment. + const resolveCall = requested.find(u => paramsOf(u).has('forHandle')); + assert.ok(resolveCall, 'used the cheap forHandle lookup'); + assert.strictEqual(paramsOf(resolveCall).get('forHandle'), 'abc#part=contentDetails'); + assert.strictEqual(paramsOf(resolveCall).get('part'), 'id'); + assert.ok(resolveCall.includes('abc%23part%3DcontentDetails'), 'the # and = are encoded'); + + // API-supplied video ids are encoded too — they flow into videos.list as a + // comma list and must not be able to add parameters either. + const videosCall = requested.find(u => new URL(u).pathname.endsWith('/videos')); + assert.ok(videosCall, 'fetched video statistics'); + assert.strictEqual(paramsOf(videosCall).get('id'), 'vid1&part=contentDetails'); + assert.strictEqual(paramsOf(videosCall).get('part'), 'statistics'); + } finally { + if (original) require.cache[proxyFetchPath] = original; + else delete require.cache[proxyFetchPath]; + if (oldKey === undefined) delete process.env.YOUTUBE_API_KEY; + else process.env.YOUTUBE_API_KEY = oldKey; + } +}); diff --git a/server/index.js b/server/index.js index 4c1510f..c7d2140 100644 --- a/server/index.js +++ b/server/index.js @@ -57,6 +57,7 @@ const xDiscovery = require('./x-discovery'); const redditDiscovery = require('./reddit-discovery'); const apifyQuota = require('./apify-quota'); const youtubeQuota = require('./youtube-quota'); +const { youtubeApiUrl } = require('./youtube-api'); const { runPendingMigrations, MULTITENANT_TABLES: ORPHAN_AUDIT_TABLES } = require('./migrations'); const emailTemplates = require('./email-templates'); const csvExport = require('./csv-export'); @@ -6298,7 +6299,9 @@ app.post(`${BASE_PATH}/api/discovery/batch-email`, rbac.requirePermission('email try { // Search channels const searchRes = await fetch( - `https://www.googleapis.com/youtube/v3/search?part=snippet&type=channel&q=${encodeURIComponent(kw)}&maxResults=50&key=${YOUTUBE_API_KEY}` + youtubeApiUrl('search', { + part: 'snippet', type: 'channel', q: kw, maxResults: 50, key: YOUTUBE_API_KEY, + }) ); const searchData = await searchRes.json(); if (searchData.error) { console.warn(`[BatchDiscovery] YouTube search error for "${kw}":`, searchData.error.message); continue; } @@ -6308,7 +6311,11 @@ app.post(`${BASE_PATH}/api/discovery/batch-email`, rbac.requirePermission('email // Get channel details in batch const statsRes = await fetch( - `https://www.googleapis.com/youtube/v3/channels?part=snippet,statistics,contentDetails&id=${channelIds.map(encodeURIComponent).join(',')}&key=${YOUTUBE_API_KEY}` + youtubeApiUrl('channels', { + part: ['snippet', 'statistics', 'contentDetails'], + id: channelIds, + key: YOUTUBE_API_KEY, + }) ); const statsData = await statsRes.json(); @@ -6324,7 +6331,9 @@ app.post(`${BASE_PATH}/api/discovery/batch-email`, rbac.requirePermission('email if (uploadsPlaylistId) { try { const plRes = await fetch( - `https://www.googleapis.com/youtube/v3/playlistItems?part=snippet&playlistId=${uploadsPlaylistId}&maxResults=1&key=${YOUTUBE_API_KEY}` + youtubeApiUrl('playlistItems', { + part: 'snippet', playlistId: uploadsPlaylistId, maxResults: 1, key: YOUTUBE_API_KEY, + }) ); const plData = await plRes.json(); const latestDate = plData.items?.[0]?.snippet?.publishedAt; @@ -6413,7 +6422,14 @@ app.post(`${BASE_PATH}/api/discovery/batch-email`, rbac.requirePermission('email // For now, search YouTube for "tiktok" + keyword to find cross-platform creators if (!YOUTUBE_API_KEY) continue; const searchRes = await fetch( - `https://www.googleapis.com/youtube/v3/search?part=snippet&type=video&q=${encodeURIComponent(kw)}&maxResults=20&key=${YOUTUBE_API_KEY}&publishedAfter=${new Date(Date.now() - 365 * 24 * 60 * 60 * 1000).toISOString()}` + youtubeApiUrl('search', { + part: 'snippet', + type: 'video', + q: kw, + maxResults: 20, + key: YOUTUBE_API_KEY, + publishedAfter: new Date(Date.now() - 365 * 24 * 60 * 60 * 1000).toISOString(), + }) ); const searchData = await searchRes.json(); if (searchData.error) continue; @@ -6423,7 +6439,7 @@ app.post(`${BASE_PATH}/api/discovery/batch-email`, rbac.requirePermission('email if (videoIds.length === 0) continue; const vidRes = await fetch( - `https://www.googleapis.com/youtube/v3/videos?part=snippet&id=${videoIds.join(',')}&key=${YOUTUBE_API_KEY}` + youtubeApiUrl('videos', { part: 'snippet', id: videoIds, key: YOUTUBE_API_KEY }) ); const vidData = await vidRes.json(); diff --git a/server/scraper.js b/server/scraper.js index 7073f85..21dc953 100644 --- a/server/scraper.js +++ b/server/scraper.js @@ -12,6 +12,7 @@ const fetch = require('./proxy-fetch'); const { safeFetchRaw } = require('./web/web-fetch'); +const { youtubeApiUrl } = require('./youtube-api'); const apify = require('./apify-client'); const apifyQuota = require('./apify-quota'); const profileCache = require('./kol-profile-cache'); @@ -68,7 +69,7 @@ async function scrapeYouTube(profileUrl, username) { if (handle) { // Try forHandle first (cheaper, 1 unit) const handleRes = await fetch( - `https://www.googleapis.com/youtube/v3/channels?part=id&forHandle=${encodeURIComponent(handle)}&key=${YOUTUBE_API_KEY}` + youtubeApiUrl('channels', { part: 'id', forHandle: handle, key: YOUTUBE_API_KEY }) ); const handleData = await handleRes.json(); if (handleData.items?.length > 0) { @@ -76,7 +77,7 @@ async function scrapeYouTube(profileUrl, username) { } else { // Fallback to search (100 units) const searchRes = await fetch( - `https://www.googleapis.com/youtube/v3/search?part=id&q=${encodeURIComponent(handle)}&type=channel&maxResults=1&key=${YOUTUBE_API_KEY}` + youtubeApiUrl('search', { part: 'id', q: handle, type: 'channel', maxResults: 1, key: YOUTUBE_API_KEY }) ); const searchData = await searchRes.json(); channelId = searchData.items?.[0]?.id?.channelId; @@ -87,7 +88,7 @@ async function scrapeYouTube(profileUrl, username) { const customName = profileUrl.match(/\/c\/([^/?&]+)/)?.[1]; if (customName) { const searchRes = await fetch( - `https://www.googleapis.com/youtube/v3/search?part=id&q=${encodeURIComponent(customName)}&type=channel&maxResults=1&key=${YOUTUBE_API_KEY}` + youtubeApiUrl('search', { part: 'id', q: customName, type: 'channel', maxResults: 1, key: YOUTUBE_API_KEY }) ); const searchData = await searchRes.json(); channelId = searchData.items?.[0]?.id?.channelId; @@ -100,7 +101,11 @@ async function scrapeYouTube(profileUrl, username) { // Step 2: Get channel details (1-3 quota units) const channelRes = await fetch( - `https://www.googleapis.com/youtube/v3/channels?part=snippet,statistics,brandingSettings&id=${encodeURIComponent(channelId)}&key=${YOUTUBE_API_KEY}` + youtubeApiUrl('channels', { + part: ['snippet', 'statistics', 'brandingSettings'], + id: channelId, + key: YOUTUBE_API_KEY, + }) ); const channelData = await channelRes.json(); @@ -124,14 +129,16 @@ async function scrapeYouTube(profileUrl, username) { let recentAvgViews = avgViews; try { const videosRes = await fetch( - `https://www.googleapis.com/youtube/v3/search?part=id&channelId=${encodeURIComponent(channelId)}&type=video&order=date&maxResults=10&key=${YOUTUBE_API_KEY}` + youtubeApiUrl('search', { + part: 'id', channelId, type: 'video', order: 'date', maxResults: 10, key: YOUTUBE_API_KEY, + }) ); const videosData = await videosRes.json(); const videoIds = (videosData.items || []).map(v => v.id.videoId).filter(Boolean); if (videoIds.length > 0) { const statsRes = await fetch( - `https://www.googleapis.com/youtube/v3/videos?part=statistics&id=${videoIds.join(',')}&key=${YOUTUBE_API_KEY}` + youtubeApiUrl('videos', { part: 'statistics', id: videoIds, key: YOUTUBE_API_KEY }) ); const statsData = await statsRes.json(); const videos = statsData.items || []; diff --git a/server/youtube-api.js b/server/youtube-api.js new file mode 100644 index 0000000..93b9de8 --- /dev/null +++ b/server/youtube-api.js @@ -0,0 +1,69 @@ +/** + * YouTube Data API v3 request-URL builder. + * + * Why this exists: the call sites used to interpolate values straight into a + * template literal — + * + * `https://www.googleapis.com/youtube/v3/channels?part=id&forHandle=${handle}&key=${KEY}` + * + * — and several of those values are derived from a user-submitted profile URL + * (the `@handle`, the `/c/` custom name, the `/channel/` segment). + * A handle containing `&` or `#` therefore injected or truncated query + * parameters in an authenticated, quota-metered third-party call: + * + * @abc&part=contentDetails → ...?part=id&forHandle=abc&part=contentDetails&key=... + * @abc&key= → shadows our API key parameter + * @abc# → truncates everything after it, key included + * + * The host is fixed, so this is not SSRF — but it produces wrong results and + * confusing quota charges, so every value now goes through encodeURIComponent + * here rather than being trusted at ~13 scattered call sites. + * + * Usage: + * youtubeApiUrl('channels', { part: ['snippet', 'statistics'], id, key }) + * + * Array values are joined on a literal comma — that is the Data API's list + * separator for `part` / `id`. Each element is encoded individually, so the + * separator stays structural and an element can never smuggle one in. We do + * not hand list params to URLSearchParams because it would emit the separator + * as `%2C`; per-element encoding + literal comma is the shape already proven + * in production by the batch-discovery path. + * + * Null / undefined / empty values are dropped rather than serialised as the + * string "undefined" (which is what the old templates did for a missing key). + */ + +const YOUTUBE_API_BASE = 'https://www.googleapis.com/youtube/v3'; + +function isEmpty(v) { + return v === undefined || v === null || v === ''; +} + +/** + * @param {string} endpoint Data API resource, e.g. 'channels' | 'search' | 'videos' | 'playlistItems' + * @param {Object} params Query parameters. Values may be scalars or arrays (arrays → comma list). + * @returns {string} Fully-encoded request URL. + */ +function youtubeApiUrl(endpoint, params = {}) { + const parts = []; + + for (const [key, value] of Object.entries(params)) { + if (isEmpty(value)) continue; + + let encoded; + if (Array.isArray(value)) { + const items = value.filter(v => !isEmpty(v)); + if (items.length === 0) continue; + encoded = items.map(v => encodeURIComponent(v)).join(','); + } else { + encoded = encodeURIComponent(value); + } + + parts.push(`${encodeURIComponent(key)}=${encoded}`); + } + + const query = parts.join('&'); + return `${YOUTUBE_API_BASE}/${encodeURIComponent(endpoint)}${query ? `?${query}` : ''}`; +} + +module.exports = { youtubeApiUrl, YOUTUBE_API_BASE };