Repository navigation
feat: add external API runtime and request control plane - #810
bballdavis wants to merge 27 commits into
Conversation
4517a3e to
56bb87b
Compare
That was the plan! I just went ahead and started building out the PR's since i was confusing myself what was going where, but left them as draft. Once we get those first two I'll rebase and submit for review. |
c84bcf4 to
e7415ba
Compare
| const requests = () => requestService; | ||
|
|
||
| const sessionOnly = (req, res, next) => { | ||
| if (req.authType !== 'session') { |
There was a problem hiding this comment.
This is still the no-auth problem from the last round. When AUTH_ENABLED=false, verifyToken returns next() without setting req.authType, so this 403s on every call and the Requests page in #809 won't on Elfhosted and Cloudflare Access installs (like mine!). apikeys.js handles the same case by rejecting when x-api-key is present; I'd do the same here (deny api_key rather than require session).
dialmaster
left a comment
dialmaster
left a comment
There was a problem hiding this comment.
Heh, I know I left a lot of comments on this, but this is a big change and I want to make sure we get it right :)
Most of my inline comments come from one design decision: request state only changes inside an HTTP handler. There's no worker. A request moves forward only when a create, an idempotent retry, an admin approve, or a read that happens to reconcile touches it. That's why:
- provisioning runs yt-dlp under the apikeys row lock (externalRequestExecution.js:108) and unlink runs under row locks (:193)
- a stale processing row can't tell auto-approval from admin approval (externalRequestService.js:271)
- finished downloads don't show under
?status=completeduntil someone reads the right page (:435) - an approve POST waits on yt-dlp for up to two minutes and times out behind Cloudflare (externalRequestReviewService.js:462)
- a storage pause fails the request for good instead of holding it (externalRequestExecution.js:90)
I think the fix is contained: create and approve only flip the row to approved and charge quota; a small in-process executor (or the existing job queue) picks up approved rows outside any lock, re-checks authority, and does the slow work; and a jobModule.onJobEnded listener closes video requests (server.js:296 already wires one for the run tracker). Catalog, quotas, auth, and the review validation don't need to change.
Two more things:
- The 14 new
external*.jsfiles plusapiKeyChannelGrantModule.jsandsafeCommandLogging.jssit flat inserver/modules/. Features this size live in a subfolder here (channel/,mediaServers/,notifications/). Can these move toserver/modules/externalApi/? - A few changes alter behavior for everyone and aren't needed for the API: the path redaction in
safeCommandLogging, the TV-Y7 rating change, droppingx-request-id, and the removed downloads-directory log line.
The no-auth 403 on the review routes (externalRequests.js:12) is still open from the first round and is a blocker for me since it's my own setup.
| const shouldGrant = grantToRequestingKey ?? | ||
| (record.grant_to_requesting_key !== false); | ||
| try { | ||
| await workLimiter.run(() => sequelize.transaction(async (transaction) => { |
There was a problem hiding this comment.
assertExecutionCapacity takes the apikeys row FOR UPDATE on the next line, and then getChannelInfo runs inside the same transaction: the yt-dlp metadata fetch (up to 120s now), possibly a second yt-dlp call for the releases-tab check, the thumbnail and banner downloads, and the channel upsert. Every other request from that key goes through recordExternalApiUse -> markApiKeyUsed, which is an UPDATE on the same row, so they all block behind this.
On my dev instance: The UPDATE waits for the whole lock hold even though its WHERE clause is false. The practical effect is that a client that POSTs a channel request and then polls GET /requests/:id once a second pins one pool connection per poll. The pool is 10, so the whole app (UI included) runs out of connections in about 10 seconds, and then the polls start failing with 500s once the 50s lock wait expires.
The yt-dlp and network work needs to happen outside the transaction. Re-check authority and take the lock only around the final write.
| transaction | ||
| ); | ||
| const result = target.downloaded | ||
| ? await videoDeleter.deleteVideoById( |
There was a problem hiding this comment.
validateTarget has the Channel/ChannelVideo/Video rows locked and then deleteVideoById does the fs.unlink inside the transaction. On a NAS that can take a while.
Also, calling deleteVideoById directly skips what deleteVideos does after its loop: the per-channel .m3u regeneration (videoDeletionModule.js:262) and the storage guard refresh (line 333). So after an external delete the channel playlist still lists the file until the next rescan, and a download pause isn't re-evaluated.
| await attachRequestStatuses(key, [row]); | ||
|
|
||
| const metadataProvider = metadataService || require('./videoMetadataModule'); | ||
| const metadata = await metadataProvider.getVideoMetadata(youtubeId); |
There was a problem hiding this comment.
getVideoMetadata spawns yt-dlp on a cache miss (with the user's cookies, 60s timeout), writes jobs/info/.info.json, and updates channelvideos availability and published_at on the way.
That's a read-only endpoint letting an external key drive YouTube fetches and catalog writes.
The work limiter caps it at 2 at a time but not the total, and a client walking /videos and calling this per row will keep yt-dlp busy all day.
Probably better to serve from the cached .info.json only and return null metadata when there isn't one (another reason I don't clean up those .info.json files)
| 'embyApiKey', | ||
| 'youtubeApiKey', | ||
| 'apiKey', | ||
| 'key', |
There was a problem hiding this comment.
A bare key here removes the key field from existing logs that aren't about API keys: configModule.js:76, :758 and :779 log { key, previous, replacement } when a setting gets corrected on upgrade, and with this they no longer say which setting changed. key_hash, apiKey and req.body.key are fine; I'd drop this.
| ); | ||
| text = text.replace(/\bfile:\/\/[^\s"'<>]+/gi, '[REDACTED_PATH]'); | ||
| text = text.replace( | ||
| /(^|[\s"'=(])(?:\/(?:[^/\s"'<>]+\/)*[^/\s"'<>]+|[A-Za-z]:\\(?:[^\\\s"'<>]+\\)*[^\\\s"'<>]+)/g, |
There was a problem hiding this comment.
This replaces every absolute path in yt-dlp stderr, and redactSensitiveText is now applied in ytDlpRunner, channelYtdlpExecutor and the playlist module for everyone, not just the external API. "No space left on device: /tmp/..." and permission errors on the output folder are exactly what people paste into issues, and they'll now read [REDACTED_PATH]. Can we keep the path scrubbing out of the server log? The log download already scrubs secrets through logScrubber.
| })); | ||
|
|
||
| app.use(express.json()); | ||
| const defaultJsonParser = express.json(); |
There was a problem hiding this comment.
Express matches the /external-api/v1 mount case-insensitively, but this check (and the ones at :101, :429 and :859) is case-sensitive. So POST /EXTERNAL-API/v1/requests/videos gets parsed by the global 100 KB express.json() instead of the 16 KB one, and parse errors and the degraded-DB 503 come back in the legacy shape instead of the envelope. I tried it with a bare express app: the mount matches and req.path keeps the uppercase. Auth and the limiters still run inside the router, so nothing gets past them, it's just the contract. I'd lowercase req.path in these four checks, or mount the parser bypass and the two error handlers on app.use('/external-api', ...) so Express does the matching.
| requireCurrentAutoApproval(currentKey, 'channel'); | ||
| } | ||
| const provisioner = channelProvisioner || require('./channel/channelProvisioning'); | ||
| const result = await provisioner.getChannelInfo( |
There was a problem hiding this comment.
Two things here. With auto-approve on, this always ends in a grant for the requesting key (grant_to_requesting_key is set to true on every key-created request and no admin ever sees it), and enableChannel=true re-enables a channel I unsubscribed. That's the same thing my own add-channel does, so I'm fine with it, but it isn't written down anywhere: a key with auto-approve can basically resubscribe anything and then read it.
One actual bug: for a channel that's terminated and unsubscribed, getChannelInfo flips enabled back to true on its own connection, then the terminated check at :136 throws, so the request records as failed but the terminated row shows up in Subscriptions again. I'd look the URL up first and fail the request if the row is terminated, before calling getChannelInfo with enableChannel=true.
| ``` | ||
|
|
||
| Only canonical YouTube handle, `/channel/`, `/c/`, and `/user/` URLs are | ||
| accepted. Approval provisions/enables the channel. By default it also grants |
There was a problem hiding this comment.
This only describes the admin approval path. Worth saying that an auto-approved channel request grants the channel to the key with no admin decision, and that it re-enables a channel that was unsubscribed.
| try { return [normalizeUrlToVideoId(url).id]; } catch { return []; } | ||
| }); | ||
| const jobId = await jobModule.addOrUpdateJob( | ||
| const externalRequestId = this.getJobDataValue(jobData, 'externalRequestId'); |
There was a problem hiding this comment.
This reads externalRequestId from the job body, and routes/videos.js:1029 passes req straight through, so anyone with a session can put externalRequestId in the body and pick the job id (addJob takes any string there, not just a UUID). Session users are admins and job ids never end up in paths, so it isn't a security problem, but the comment in addJob says trusted internal callers only. I'd have the external execution pass it outside the body, or at least validate it as a UUID in addJob.
| * responses: | ||
| * 200: { description: Safe requester, target, and job metadata } | ||
| * 403: { description: Session authentication is required } | ||
| * 404: { description: Request not found } |
There was a problem hiding this comment.
A body sent as application/json; charset=utf-16 ends up here (body-parser throws charset.unsupported), goes to the app-level handler and comes back as a 500 "External API request failed". It's a client error; I'd map charset.unsupported to a 415 next to the other cases.
Restore the downloads-directory and cookie temp-dir log lines, the x-request-id correlation header outside the external API namespace, TV-Y7's text rating fallback, and unredacted yt-dlp stderr in server logs. Drop the bare 'key' redaction path so config upgrade logs keep naming the corrected setting. None of these were needed by the API.
…UTF-8 bodies With AUTH_ENABLED=false, verifyToken never sets req.authType, so the review routes' session check returned 403 for every call. Deny API keys instead, matching the key-management routes. External API POST bodies declaring a charset other than UTF-8 now get a 415 rather than a parse error or 500.
Move the external API services, their tests, and the channel grant module into a feature folder like channel/, mediaServers/, and notifications/. Give TV-Y7 its PG-level policy rank inside externalEligibility instead of changing ratingMapper, which also drives NFO output for every video.
Express routes /EXTERNAL-API/v1/... into the external router, but the app-level body parser bypass, error envelopes, degraded-database 503, and request id handling compared req.path case-sensitively. Share one isExternalApiPath check that matches the mount the way Express does.
Creating or approving a request now only records the decision and charges quota. A small in-process executor picks approved rows up outside any lock, re-checks the key's authority, and does the slow work, so no yt-dlp call, download queueing, or file unlink runs under a row lock or inside an HTTP handler: - Channel provisioning resolves the channel without enabling it and takes locks only for the final write, where a terminated channel now fails the request instead of being re-enabled. - Deletions go through deleteVideos, so the channel .m3u, playlists, and storage pause are refreshed like an in-app delete. - A storage pause or a busy video holds the request approved instead of failing it; the executor's sweep retries it. - A jobModule.onJobEnded listener closes video requests when their job ends, so ?status=completed no longer depends on who reads the list. Reads and quota checks no longer reconcile or lock anything. - A new approval_source column records whether a request was auto-approved, so the executor re-checks auto-approval only for those rows. - The download job id is passed as an option to doSpecificDownloads rather than read from the job data, and addJob rejects a preferred job id that is not a UUID.
Creating the request service in server.js eagerly required the channel, deletion and storage modules, which broke the server startup suites.
- Video detail reads metadata only from the cached .info.json and never runs yt-dlp or writes backfills. - Raw catalog SQL uses snake_case aliases and POSITION() instead of LOCATE(). - Video list ordering and keyset cursors (now v3) tie-break on channel video id. - Single-channel lookups use Channel.findOne with the grant include.
Document approval, background execution and held requests, cached-only video detail, auto-approved channel grants, delete and idempotency responses, the work queue env vars and the approval_source column. Bump the contract fixture to version 5 for the approved create response.
A background wake after approval could sweep before the test changed state (for example revoking a grant), which made the database job fail at random on either engine. The wake scheduler is now injectable and the database tests turn it off.
2a99a5f to
d698fb3
Compare
Hah no worries, I'm with you. Rather get it right the first time and no worry with it moving forward. I think I've caputred what you're looking for, but let me know if you have any other thoughts! |
Summary
Adds the opt-in external API runtime for #568 on the persistence foundation now merged by #750. This branch is rebased onto current
dev(ef996c10); the diff contains phase two only. The administrator UI remains separate in #809.AUTH_ENABLED=false, plus scopes, channel grants, media/rating policy, durable quotas, and bounded request/work queues. Legacy download keys cannot authenticate the external namespace.Persistence and query plans
The four migrations from #750 and upstream dependency corrections are preserved. The root lockfile updates only
proxy-addrfrom 2.0.7 to the compatible 2.0.8 security patch for GHSA-jqcg-44mw-7w3h; no global overrides were added. There are no obsolete migration markers, channel uniqueness changes, or channel deduplication.One runtime migration adds two
external_requestsindexes justified by the actual service SQL. Catalog request status is batched for the displayed page instead of using repeated correlated history scans. The reproducible local fixture contains 300,000 cached videos, 1,000 channels, 30,000 requests, and 10 grants; MariaDB 10.3 and MySQL 8.0 plans select the new indexes. Equivalent indexes under other names are preserved; rollback removes matching owned definitions only. CI now runs the runtime/database matrix and uploads the query-plan reports.Validation
All database and consumer data are synthetic. No real customer data, YouTube downloads, deployment, or merge was used for validation.
Verified final head:
9badb936aaec4a84a7403a9d75f03dd76785c4b7. CI run: all checks passed, including both downloadable database query-plan reports.