fix(scraper): encode YouTube Data API query params through a shared builder - #24
Merged
Conversation
…uilder 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 <noreply@anthropic.com>
2 tasks
oratis
added a commit
that referenced
this pull request
Aug 9, 2026
…lake Review follow-up on this PR. The numbers went stale during review: #23 (+9 tests, +1 file) and #24 (+7, +1) merged while this branch sat open, so "656 / 69 files" was already wrong by the time it could land — the exact failure mode the PR exists to fix. Measured on main at 7df4183: 678 tests across 71 files. Corrected in all five places (CLAUDE.md ×2, memory.md §5.2 / §7.3 / Last-reviewed footer, which also still said "post #20"). Also documents why a clean checkout can show 2-5 red files: `npm test` is `node --test`, which runs files concurrently against the one shared influencex.db at the repo root, so writes collide and report `{ code: 'SQLITE_BUSY' }` on a rotating cast of files. It reproduces on main with no changes applied, and the triage step is a serialized re-run (--test-concurrency=1 → stable 678/678). Without this written down the next session reads the flake as its own regression — which is what the rest of this PR is trying to prevent. Verified: 678/678 serialized on main, and on main + #25 + this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merged
oratis
added a commit
that referenced
this pull request
Aug 9, 2026
) #24 introduced youtubeApiUrl() and migrated scraper.js and index.js, but youtube-discovery.js and content-metrics.js kept hand-rolling their request URLs. Both were safe — an earlier pass had added encodeURIComponent inline — but a builder that only covers some call sites is exactly the shape the original bug had: the injection survived because the construction was spread across ~13 places and each one had to be remembered separately. Their inputs are user-derived too (the search keyword, and a videoId parsed out of a submitted content URL). Adds a coverage guard that walks server/ and fails if any module builds a Data API URL directly again, so the next call site can't quietly opt out. youtube-api.js owns the one literal; publish/oauth.js is allowlisted because its OAuth userinfo URL is static (mine=true, nothing interpolated). Verified the guard fails and names the file when a raw URL is reintroduced. npm test 678/678. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
oratis
added a commit
that referenced
this pull request
Aug 9, 2026
* docs: 修正入门文档里已经失效的运维事实 排查一份 brand-voice embedding 的 bug 报告时发现问题本身已经在 #20 （`0c5323b`）修掉了，但顺手核对上下文的过程中撞上好几处文档与现状不符 —— 每一处都会让下一个会话走一段冤枉路，所以单独收一个 docs PR。 改了什么： - **prod DB 口令来源**：CLAUDE.md 和 memory.md 有 5 处说密码"在 .env 里"， 但这台机器上根本没有 .env（只有 .env.example）。全部改为从 Secret Manager 取：`gcloud secrets versions access latest --secret=DATABASE_URL`。 - **测试数字**：CLAUDE.md 同时写着 377 和 234，memory.md 写 234 —— 实际是 656 个服务端测试 / 69 个文件、~1 秒。前端测试也不再是"4 个组件测试"或 "还没有"，是 13 个 vitest 文件 + 5 条 Playwright（3 个 spec）。 - **Sentry / OTEL**：memory.md §5.4 还写着"没有（Sprint 1 待加）"，与同文件 §6 已关闭表和 CLAUDE.md 自相矛盾。两者早已接入，只是要配 DSN / OTLP 才上报。 - **brand_voices 生产现状**：借这次机会连 prod 只读查了一次 —— 表 0 行。 也就是说 embedding 写入路径坏了这么久没有造成数据损失，不需要 backfill。 这条结论写进 memory.md §6，省得以后有人再问一遍。 - 新增一条"仍然成立"：embedding 只在 `POST /api/brand-voices` 创建时写， 目前没有 update 路由所以无害，但以后加编辑接口必须重新 embed。 验证：纯文档改动，未动任何代码路径；`npm test` 656/656 通过。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: correct this PR's own test counts, and record the SQLITE_BUSY flake Review follow-up on this PR. The numbers went stale during review: #23 (+9 tests, +1 file) and #24 (+7, +1) merged while this branch sat open, so "656 / 69 files" was already wrong by the time it could land — the exact failure mode the PR exists to fix. Measured on main at 7df4183: 678 tests across 71 files. Corrected in all five places (CLAUDE.md ×2, memory.md §5.2 / §7.3 / Last-reviewed footer, which also still said "post #20"). Also documents why a clean checkout can show 2-5 red files: `npm test` is `node --test`, which runs files concurrently against the one shared influencex.db at the repo root, so writes collide and report `{ code: 'SQLITE_BUSY' }` on a rotating cast of files. It reproduces on main with no changes applied, and the triage step is a serialized re-run (--test-concurrency=1 → stable 678/678). Without this written down the next session reads the flake as its own regression — which is what the rest of this PR is trying to prevent. Verified: 678/678 serialized on main, and on main + #25 + this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
背景
server/scraper.js与server/index.js里的 YouTube Data API v3 请求 URL 是用模板字符串手工拼的。其中三处的值仍然没有编码就直接插进 query string：scraper.js:134videoIds.join(',')index.js:6327uploadsPlaylistIdindex.js:6426videoIds.join(',')host 是写死的，所以不是 SSRF；但这是往一个带鉴权、计配额的第三方 API 调用里注入参数，会产生错误结果和莫名其妙的配额消耗。
两种失效方式（已用旧字符串形式实测复现）：
#是更糟的一种 —— 它把&key=...挤进了 URL fragment，那部分根本没发给 Google。补充一个威胁模型上的细节：
handle/customName/channelId三条路径此前已经有encodeURIComponent，而且它们的提取正则是[^/?&]+，在到达 URL 之前就把&剥掉了 —— 这两条路的&注入是双重挡住的。但#不在那个字符类里。改动
手拼模板的写法本身会招来下一次裸插值，所以这里做的是结构性修复而不是逐点打补丁：
server/youtube-api.js——youtubeApiUrl(endpoint, params)，对每个值编码，并且把 nullish 值直接丢弃（而不是像旧模板那样序列化成字符串"undefined"）scraper.js6 个、index.js5 个。两个文件里已无残留的裸googleapis.com/youtube模板字符串一处与常规建议不同的取舍
没有用
new URLSearchParams({...})当编码器。因为它会把,编码成%2C，而逗号正是 Data API 在part/id上的列表分隔符。在一个"目的就是不要静默产生错误结果"的修复里去赌 Google 解析器接受%2C，不划算。所以 builder 的做法是：逐个元素编码 + 字面逗号拼接 —— 这正是index.js:6311已经在生产跑着的形状。已验证：对真实的 channel / playlist id，产出的 URL 与旧 URL 逐字节一致。唯一有意的线上格式变化是
publishedAfter，其中的冒号现在编码成%3A（标准行为，解码后完全相同）。测试
新增
server/__tests__/youtube-api-url.test.js，7 条：abc&part=contentDetails时产出的 URL 只有一个part=；另有 API key 被顶替、#截断、列表编码、nullish 丢弃、固定 hostscrapeYouTube（stub 掉./proxy-fetch），断言发出去的每一条 URL 都只有一个part=、一个key=、API key 完好、无 fragment。这条同时覆盖了没有任何正则过滤的 API 响应侧videoId路径这条集成测试第一次是失败的，而且失败得对 —— 我原本假设
&能穿过 handle 提取，实际穿不过。于是该路径的 payload 改成了#，&注入则在 builder 层和无过滤的 video-id 路径上验证。旧字符串形式已单独验证会在同样的断言上失败，确认测试有牙齿。
说明
client/node_modules，因此没跑vite buildserver/youtube-discovery.js（64、88 行）和server/content-metrics.js（71 行）同样手拼 YouTube URL，但当前都已正确编码，属于本次范围之外，未动。它们是下一个人会照抄的写法 —— 需要的话可以另开一个 PR 一并收进这个 builder🤖 Generated with Claude Code