fix(youtube): route the last two call sites through the URL builder - #28
Merged
Merged
Conversation
#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>
3 tasks
oratis
added a commit
that referenced
this pull request
Aug 9, 2026
…nded (#31) Two defects found while reviewing the #23–#30 batch, both now on main. **Wrong PR credited.** #29's .gitattributes header and memory.md §6 both say #27 fixed the usage-ledger NUL bytes. #27 was closed as a duplicate — #30 landed the identical fix (verified byte-for-byte identical trees). Anyone following the reference lands on a closed PR with an empty diff. **The count treadmill.** #26 corrected five hardcoded test counts from 234/377/656 to the then-accurate 678. #28 merged minutes later and made it 679, so a PR whose entire purpose was de-staling docs shipped a number that was stale on arrival. Five copies of a figure that changes on every test-bearing merge cannot stay right. Counts are now stated as a magnitude pinned to a commit ("~680 as of c7c7d5b") with an instruction to measure instead of cite, and the two places that only needed "all green" no longer carry a number at all. The prose that never had one ("Vitest files under client/src/{...}") aged fine through this whole batch, which is the argument. memory.md §5.2 records the pattern so the next doc pass doesn't reinstate it. The remaining 234/656 mentions are deliberate — they are the history being explained, not live facts. Verified: 679/679 serialized on this branch; grep confirms no stale hardcoded count or bare #27 reference survives. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Summary
Follow-on to #24. That PR introduced
youtubeApiUrl()and migratedscraper.jsandindex.js, butyoutube-discovery.jsandcontent-metrics.jskept hand-rolling their request URLs.Both were already safe — an earlier pass added
encodeURIComponentinline — but a builder that covers only some call sites is exactly the shape the original bug had: the injection survived because construction was spread across ~13 places, each needing to be remembered separately. Their inputs are user-derived too (the discovery search keyword; avideoIdparsed 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.jsowns the one literal;publish/oauth.jsis allowlisted because its OAuth userinfo URL is static (mine=true, nothing interpolated).Test plan
npm test678/678content-metrics.js:pass 7 / fail 1, listingcontent-metrics.js)🤖 Generated with Claude Code